brownhci / WebGazer

WebGazer.js: Scalable Webcam EyeTracking Using User Interactions
https://webgazer.cs.brown.edu
Other
3.54k stars 536 forks source link

More testing #164

Closed RobinReborn closed 3 years ago

RobinReborn commented 4 years ago

I've done more testing, I tested the regression functions and the util functions. I've also restructured the code, there was some redundancy in the regression functions. Additionally I've modified the webpack.config.js to create a separate file for our tensorflow dependency. This also allows us to compute code coverage, right now we're at 80%.

I've noted a few functions in the code which aren't being used which have been marked with //not used !?, I'm not sure if they should be removed or if they're part of something to be added later. Additionally, due to the way the code is structured we cannot directly test some of the more basic functions (such as ridge) this is because they are private - it's not clear to me if they could be made public without negative consequences, whether we need to find a way of testing them while keeping them private or whether they're not sufficiently important to test.

I've also updated the code in trainingData to be compatible with Python3, but it's not clear to me how to run that code.

Skylion007 commented 4 years ago

Can you clarify what is wrong with the Python code? Why is it unclear how to run it? We can improve the documentation of it.

jeffhuang commented 4 years ago

Thanks Robin for your contributions so far. You're a legend

RobinReborn commented 4 years ago

The python code fixes are trivial - just changing an import and adding parenthesis to print statements. I wasn't sure if there was some external data source needed but now that I look at it more closely it will need some javascript updates as it still refers to clmTracker. But I'm not sure where the documentation for it is, so it could probably use a simple readme.

Do either of you have a resolution for the unused code issue? The easiest thing to do would be to delete it, then I could get to 100% code coverage.

Skylion007 commented 4 years ago

I tested everything works fine on OSX. Not entirely sure about splitting of tf.js into dist like how it is. Wouldn't it make more sense to reference directly from node modules?

RobinReborn commented 4 years ago

The primary benefit of splitting build files in webpack is to calculate code coverage.

Tensorflow is still referenced directly from node modules, but when it's built it's put in a separate file.

dmikushin commented 4 years ago

Thanks for working on this @RobinReborn ! I came across the following issues:

1) When I do npm run serve, the dist/webgazer.var.js is not found during the calibration, because it's in the parent folder 2) The floating red dot cursor is not displayed during the calibration 3) The calibration is unable to pass through the last center-circle step; glancing at the center circle for the infinite time, nothing happens (the reference website gives the accuracy report window)

RobinReborn commented 4 years ago

We're not using dist/webgazer.var.js, in www/ there should be a symlink to ../dist/webgazer.js as webgazer.js (this could be a problem if you are using Windows).

I cannot regenerate these errors, I see the red dot and I get an accuracy report after glancing at the center circle.

So I think there's an issue with where calibration is accessing webgazer.js, if you run npm run build in the root directory it will generate the appropriate files, then you may need to fix the symlink.

dmikushin commented 4 years ago

@RobinReborn I'm using MacOS. I've ran into the issues mentioned above while following the README instructions. Could you please kindly take a look at the following video proof: https://youtu.be/1E1u2xhFFrs

jeffhuang commented 4 years ago

I believe @Skylion007 is in the midst of redoing how the www/ folder works.

RobinReborn commented 4 years ago

@dmikushin This PR is for the branch more_testing , not master.

dmikushin commented 4 years ago

@dmikushin This PR is for the branch more_testing , not master.

Sorry, I'm not sure what do you mean. I've just cloned the branch of this PR as is.

RobinReborn commented 4 years ago

your video shows

git clone git@github.com:RobinReborn/WebGazer.git

Which is a clone of my repository which by default puts you on the master branch.

You need to run

git checkout more_testing

dmikushin commented 4 years ago

Ah! I'm sorry for this silly mistake :(

Skylion007 commented 4 years ago

168 Should finally make it so we don't have to commit things into dist to not break the website.

Skylion007 commented 4 years ago

@RobinReborn Should I just go ahead and resolve the merge conflicts then?

RobinReborn commented 4 years ago

Sure

RobinReborn commented 4 years ago

Is there anything needed before this PR is merged?

RobinReborn commented 3 years ago

@jeffhuang @Skylion007 any interest in getting this merged sometime?

jeffhuang commented 3 years ago

We certainly should, and I'm sorry about the delay. I don't know if @Skylion007 has any more time right now, but I'll tag @alexpapster as well just so she's aware of this pending PR.

alexpapster commented 3 years ago

It's my order to tag @xanderkoo . Thanks, @RobinReborn , we'll put it in our list of the things to do

RobinReborn commented 3 years ago

@xanderkoo @alexpapster @jeffhuang @Skylion007 Does anyone have the bandwidth to merge my PR? I'm planning to do a lot more with this codebase (there's an upgrade to tensorflow facemesh which allows for iris detection https://google.github.io/mediapipe/solutions/iris.html), if you're all too busy with other stuff I will just make these updates on my own fork.

xanderkoo commented 3 years ago

@RobinReborn I'll try and get this done within the next week or so -- could you give a recap of the things that have been added/modified?

Also, what were you planning on doing with iris detection? I believe we tried it out towards when it was initially released in hopes that we might be able to come up with a geometric model, but the predictions per second left much to be desired.

RobinReborn commented 3 years ago

@xanderkoo thanks for the quick response.

I've removed a lot of unnecessary and unused files. I've noted several functions that appear not to be used with the comment //not used !? I've consolidated repeated code in the regression files so they all import from util_regression.mjs. I've added two more test files, regression_test.js and util_test.js. And I've updated the trainingData python files to work with python3

RobinReborn commented 3 years ago

I've only started looking at iris detection, but it looks like you can get more precise data. The existing code performs regressions based on a 6*10 rectangle that contains the eye (as well as skin around the eye, eyelashes etc). I figure you could do a lot more if you only got the iris - but I'd be interested in hearing more about the limitations you found.

xanderkoo commented 3 years ago

@RobinReborn Thanks again! I'm looking through this now and I think it all looks good so far. Quick question: it looks like you removed the githooks from package.json -- is there a reason why? @Skylion007 @jeffhuang Do we want to keep them?

xanderkoo commented 3 years ago

Also, is there a reason that you changed the params.applyKalmanFilter references to window.applyKalmanFilter?

xanderkoo commented 3 years ago

Could you also explain what you modified in test/webgazer_test.js ? It looks like you moved some stuff into the new test files you created, but I'm not sure what else you changed.

RobinReborn commented 3 years ago

The hooks were removed for my personal preference - I found it annoying that everything was rebuilt when I committed because a lot of the time my changes weren't part of what is rebuilt and usually I would build and test before committing.

I removed some code in test/webgazer_test.js that calculated times as that's already done by the existing framework and fixed the code coverage calculation to only use this codebase (and not tensorflow.js). A lot of the rest is indentation - I've put the tests into another describe block

The change to window.applyKalmanFilter isn't something I totally remember at this point, but it's in ridgeRegThreaded.mjs which currently isn't working https://github.com/brownhci/WebGazer/issues/163 it might have something to do with the params not being accessible from a thread.

xanderkoo commented 3 years ago

Sorry for the delay -- I just made some small tweaks, but I'll be merging it now.

PR:

Tweaks:

Notes: