-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Improved quaternion application #14075
Conversation
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/14075/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/14075/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/14075/merge#BCU1XR#0 |
Looks great, and it does pass the tests. Just wondering - why is it better? is it just the few less multiplications? |
Thanks :) Yes exactly. It needs less arithmetic operations to compute the same result. Since rotating stuff is the sole purpose of quaternions in the 3D world, this should be as fast as possible. |
Is it possible, at this stage @infusion , to see the benefit in a side by side comparison of two Babylon Playgrounds that push quaternion rotation computations to the limit? |
I ran a few manual tests using this simple playground: https://playground.babylonjs.com/#23GPBX#1 I am not seeing any regression. I don't see a big difference in performance, with slight improvement in the new PR. if anyone else wants to test it - would be great :-) |
I'm not seeing any measurable difference in the two PGs (javascript optimization can be frustrating...), but the new method is not slower and we say in the comment of the method that the quaternion must be a unit quaternion, so I think we can approve the new calculation? |
I am totally fine with that. Merging! |
I am the author of Quaternion.js ( https://github.com/infusion/Quaternion.js ) and thought it might be useful for a wider audience to benefit from an improvement to rotate vectors using quaternions I've derived here https://raw.org/proof/vector-rotation-using-quaternions/
I discussed this already with the fine folks from Three.js where I also proposed to add this improvement mrdoob/three.js#26456