-
Notifications
You must be signed in to change notification settings - Fork 544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #337 : multiple fixes in the code and modernize the style #345
base: master
Are you sure you want to change the base?
Conversation
969169d
to
6f94dd7
Compare
run `npx vite` into www folder to not need rebundle for now
I need to make some fixes first. Don't merge anything yet. I'll comment all the changes to justify them |
Will do, thank you |
This code is not working and does not seem to be related to webgazer as is now. Contains executable code for a server in python to collect training data and store in a sqlite db file (?)
I commented the changes, but I would still like to go a bit further with the www folder. I will see if I have the time. |
@jeffhuang I finished the complete refacto of the project. I'd be very happy to discuss any change as intensively as needed, but I'm 100% convinced that this structure will be way easier to maintain and evolve in the future. You can test it. I made some small visual changes in the test project and added new buttons, but I can go back to how it was before. I simplified the API a lot for new users of the project. Each function details the available parameters. I had to switch the src to typescript because of the test project. Importing the types there was too hard otherwise, and securing the calls would have been too hard without any type checks. |
Amazing, looking into this and testing... |
Sorry for the delay. I'll review this at the end of the week. |
Hey @Sharcoux! I saw that MR of yours, checked the code but I'm still wondering if you have fixed the threadRidge bug, did you? Im asking it cause if you did I'll not even open a MR with the fix I did 😄 |
I don't know what bug you're talking about, but you can test my branch. From what I tested, it worked fine... |
I'll check if there might be a problem with the build. How did you run it? Anyway, you need to build the webgazer package before building the www part |
i use |
Do you have errors during the |
Try with this new commit maybe |
this |
Ouch... They are not supposed to be part of the files. Let me fix |
Done. Sorry for the mess! |
It's right now, thanks. |
Suggestion for WebGazer: I created a function that sets the FPS of WebGazer to improve performance, allowing the user to choose what suits them best.
use in loop:
|
I've been going through these changes for a whlie, and I really appreciate the work @Sharcoux but it's honestly pretty hard to review thoroughly. There's changes to the build process, linting, and small fixes. I really wish each of these could be separate. Is there a way to pull out the pure linting changes into a separate PR? Some aren't really necessary, like linting the d3. Maybe an alternative is if the www stuff goes into a separate repository at this point, so the library is one repo, and the www is a repo. It's less important that the www is reviewed as thoroughly, as long as it works and showcases the library. I really want to merge this. It's clearly an improvement. And @xanderkoo is helping out as well. But there's just too many lines changed to go through in a useful way. Thoughts? |
Another option that's not very satisfying is if there are any bug fixes, we can review those separately and incorporate them into the main branch. And the rest, we can point to your branch as the more modern style. |
This is not 100% complete, as I don't understand very well the build process. I'd need some enlightenment about the www part in order to finish this MR. However, it solves numerous issues reported here along with other ones I noticed on the fly.