Skip to content

Improve numerical stability of projection #23

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

Merged
merged 1 commit into from
May 15, 2024

Conversation

hborchardt
Copy link
Contributor

I was investigating this behaviour in plotly: plotly/plotly.js#3306
and found that, even looking at the first "good" provided example https://jsfiddle.net/Lv4qpoyu/3/ it is not well usable, because the axis labels jump around a lot while rotating the scene. When decreasing the visible time range in that fiddle even more, the jumping even gets chaotic, making the plot unusable.

I then kept looking for the cause and it appears that when we are dealing with large position values (like the UTC timestamps) that are offset by the model matrix, the current implementation is numerically unstable because the project operation is done left to right, multiplying the matrices before applying them to the position.

For good measure, I also added another pair of braces to replace the projection*view matrix-matrix-multiplication with matrix-vector multiplications, which should be faster and more stable, even though I did not see an effect.

I tested this change by patching the dist/plotly.js file and applying my changes to the shader strings, which greatly improved the positioning of the axis labels.

If you accept this change, I can also create similar pull requests in the different gl-vis repositories where similar constructs are used.

@positron96
Copy link

Hi, how is this going with this issue? I am interested as I am also stumbling onto plotly problems with time-based 3d graphs

@archmoj
Copy link
Contributor

archmoj commented May 8, 2024

Thanks very much for the investigation and apologies for the wait.
It looks like we should also use your fix in lib/shaders/backgroundVert.glsl.

I noticed some baseline changes when testing on plotly.js possibly highlighting the improvements but we should be careful about those.
Are you interested in opening a pull request on plotly.js and link to your commits?
If so I could help you get started on that.

@hborchardt
Copy link
Contributor Author

Hi, great that this issue may be moved forward.

I agree that the backgroundVert.glsl should be adjusted as well, I can do that.

Regarding plotly, sure! Do you mean in the PR switch over to my fork of the lib, so that it can be tested? When I created this PR I thought the way would be to get improvements merged here, and then just bump the version in the plotly repo in a PR.

@archmoj
Copy link
Contributor

archmoj commented May 9, 2024

These modules are better tested on plotly.js repository including image test.
So yes I mean linking to your fork(s).
I general (and in future PRs for other forks) I suggest you have an up to date mirror of master of the repository and propose the changes from a different branch other than master. That way you could simply sync you master branch if there are other changes to upstream.

For this PR you could do the same (sync with upstream & rename your branch).

After that on your plotly.js fork you need to update dependencies of stack_modules and then rebuild stack_modules/index.js.

cd cd stackgl_modules/
npm i

Using node v16 (or possibly higher) as well as your commit hash on your fork e.g.

npm install https://github.com/hborchardt/gl-axes3d#62b7f1aa027a562649ea2425bd379c9a1dd775f8

To rebuild stack_modules/index.js

npm run bundle-stackgl

Alternatively you may tweak stack_modules/index.js directly to test your ideas without the above.
But that's just for a quick prototype.

Then commit the changes and open a PR to plotly.js which runs all the tests.

@hborchardt
Copy link
Contributor Author

I created the PR in plotly.js, fixing all 9 gl-vis repositories that exhibit the problematic pattern and are used in plotly.

Fingers crossed

@archmoj
Copy link
Contributor

archmoj commented May 15, 2024

This PR is closed. Please open a new PR form your new branch master...hborchardt:gl-axes3d:hborchardt/3d_improve_numerical_stability

@archmoj
Copy link
Contributor

archmoj commented May 15, 2024

Alternatively (since you've already referenced this PR) you could simply merge your changes to into your master branch and push to your fork.

When the position vector has large values that are cancelled out
by large values in the model matrix, it is more numerically stable
to first multiply the position by the model matrix instead of multiplying
the matrices together first.
@hborchardt hborchardt reopened this May 15, 2024
@hborchardt
Copy link
Contributor Author

Yep, sorry. I got confused while managing 10 repos at the same time 😄
Updated my master branch so this PR is open again and remains the correct one.

@archmoj
Copy link
Contributor

archmoj commented May 15, 2024

Yep, sorry. I got confused while managing 10 repos at the same time 😄 Updated my master branch so this PR is open again and remains the correct one.

@hborchardt You are doing great!
We are almost there and @dy have already accepted your changes in few repositories.
See gl-vis/gl-streamtube3d#13 (comment)

@archmoj
Copy link
Contributor

archmoj commented May 15, 2024

Excellent PR.
💃

@archmoj archmoj merged commit cef3a23 into gl-vis:master May 15, 2024
@archmoj
Copy link
Contributor

archmoj commented May 15, 2024

Published [email protected] to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants