-
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
Remove side effect on axis parameter in Quaternion.RotationAxisToRef(…) #15381
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://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/refs/pull/15381/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/15381/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/15381/merge#BCU1XR#0 |
WebGL2 visualization test reporter: |
Visualization tests for WebGPU (Experimental) |
Visualization tests for WebGL 1 have failed. If some tests failed because the snapshots do not match, the report can be found at If tests were successful afterwards, this report might not be available anymore. |
This is a follow-up to PR BabylonJS#15381 (BabylonJS#15381). While identifying that bug I detected three more similar issues, two of which should be fixed here. See also https://forum.babylonjs.com/t/unexpected-side-effect-of-quaternion-rotationaxis/52622 Notice that - I just detected the issues fixed here by looking for issues similar to the one fixed in my earlier PR. - I have no test case for the affected functions. - There seem to be no unit tests for the affected functions either. So you might want to review this change particularly thoroughly.
* Remove side effects on more method parameters This is a follow-up to PR #15381 (#15381). While identifying that bug I detected three more similar issues, two of which should be fixed here. See also https://forum.babylonjs.com/t/unexpected-side-effect-of-quaternion-rotationaxis/52622 Notice that - I just detected the issues fixed here by looking for issues similar to the one fixed in my earlier PR. - I have no test case for the affected functions. - There seem to be no unit tests for the affected functions either. So you might want to review this change particularly thoroughly. * make eslint happy
See https://forum.babylonjs.com/t/unexpected-side-effect-of-quaternion-rotationaxis/52622
This PR just fixes
Quaternion.RotationAxis(...)
andQuaternion.RotationAxisToRef(...)
.The other 3 suspect lines in the file mentioned in my initial post are not handled here.