Open Sharcoux opened 6 months ago
Thanks @Sharcoux we'll take a look at this soon!
I really appreciate you running the lint and investigating these. Most of them seem to be Kalman filter code so I wonder if @xanderkoo would take a look. #4 and #6 are perplexing, and I'll look into them more. #7 is probably old code that was meant for the offline video analysis.
Hi there. @jeffhuang we came to understand the www part. We're finishing testing all the changes and fixing all potential errors.
Fantastic, thank you so much! If you could scope down the PR to individual fixing, that would be much appreciated. It's harder for us to review lengthier PRs as we'd have to test each fix separately.
Ok, we finished fixing everything. We had trouble fixing the webpack bundle but solved it easily with vite that is much faster for building anyway and is probably going to be the future of webpack.
About doing individual fixes, it's a bit hard. We didn't implement new features that we could separate from the rest. We just removed duplicated code, add typing and modern code style, and the rest of the evolutions were just consequences of these changes. When the types didn't match, we noticed some errors that we fixed. The original code was also using mjs files, but not importing them as module, but polluting the global environment with a "webgazer" variable instead, which is generally considered a bad practice.
Our next steps will be to simplify the API for users of the lib. I find the current version a bit harsh to use for newcomers.
Ok, here are the list of problems I noticed in the code:
I'm trying to solve all of that, but I need some help with the following issues I'm not sure about how to solve:
1st problem
https://github.com/brownhci/WebGazer/blob/d309e076f5953e7184aa5838f1a93184b04fea44/src/worker_scripts/util.js#L389
y[i] = [y[i]]
: this will transform anumber[][]
array into a number[][][] array. I would be surprised if that was intentional, provided the comment above. My guess is that the intended code was:2nd problem
https://github.com/brownhci/WebGazer/blob/d309e076f5953e7184aa5838f1a93184b04fea44/src/worker_scripts/util.js#L381
z
is supposed to be of typenumber[]
while thesub
method expects a parameter of typenumber[][]
. In another file, I noticed the exact same code with that single difference: there was that extra line before:I guess that this is the correct version?
3rd problem
https://github.com/brownhci/WebGazer/blob/d309e076f5953e7184aa5838f1a93184b04fea44/src/util_regression.mjs#L157
mCoefficients[i] = solution[i]
:mCoefficients
is of typenumber[]
butsolution
is of typenumber[][]
. I have no idea how this is supposed to be solved. Should I just takesolution[i][i]
?4th problem
https://github.com/brownhci/WebGazer/blob/d309e076f5953e7184aa5838f1a93184b04fea44/src/util_regression.mjs#L151
if (mCoefficients.length * n !== mCoefficients.length)
: This can only be true ifn === 1
. Is it what we want to test instead?Anyway, the line above that define the value of
n
is completely stupid:const n = (mCoefficients.length !== 0 ? mCoefficients.length / mCoefficients.length : 0)
This is equivalent to
mCoefficients.length ? 1 : 0
. In short, those 2 lines could be simplified withif(mCoefficients.length)
. However, there is absolutely no relation with Array length being a multiple of m...5th problem
https://github.com/brownhci/WebGazer/blob/d309e076f5953e7184aa5838f1a93184b04fea44/src/util_regression.mjs#L50
H
is being declared twice. Which one is the correct value?6th problem
https://github.com/brownhci/WebGazer/blob/d309e076f5953e7184aa5838f1a93184b04fea44/src/index.mjs#L311
d
is of type string. This will lead to really problematic consequences in theget
andgetTrueIndex
methods7th problem
https://github.com/brownhci/WebGazer/blob/d309e076f5953e7184aa5838f1a93184b04fea44/src/index.mjs#L639
debugVideoLoc
has to be a MediaStream, because we callgetTracks()
on it. But it is a string, here...