-
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
6Dof mesh flip fix for LH scene #14907
Conversation
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
@@ -199,7 +199,7 @@ export class SixDofDragBehavior extends BaseSixDofDragBehavior { | |||
|
|||
if (pointerCount === 1) { | |||
this._targetPosition.copyFrom(this._ownerNode.absolutePosition); | |||
this._targetOrientation.copyFrom(this._ownerNode.absoluteRotationQuaternion); | |||
this._targetOrientation.copyFrom(this._ownerNode.rotationQuaternion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is position and scaling stay absolute, and only rotation uses local rotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's the reverse. the PR did change position to absoluteposition to fix a bug and did the replacement for rotation and scale as well to be consistent. But actually, it introduced a regression with rotation. Do you confirm @carolhmj ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multipointer was also changed to absolute, is it still an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for @carolhmj 's input on this fix and if it's ok then I'll do and test the multipoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to absolutePosition/Rotation/Scale because I removed this line https://github.com/BabylonJS/Babylon.js/pull/14708/files#diff-867020a1c839af098f39ec5a7d63e25c7fa37dfd2e507e2143f86bafe643e85aL202 which was using setPosition(null) to make the node's position be the absolute position. If you copy only the local rotation, wouldn't that ignore the parent's rotation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CedricGuillemet - i'll let you resolve this
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/14907/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/14907/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/14907/merge#BCU1XR#0 |
Visualization tests for WebGPU (Experimental) |
WebGL2 visualization test reporter: |
Weird enough, one of my open issues with WebXR dragging is the multipointer dragging which doesn't work as expected. I am debugging this now, but I don't want to cause any merge conflicts, so I might make the changes here or ask you to do them if possible. |
sure, np. let me know how I can help. if the mesh flips upside down with starting to drag, do the same for multipoint. |
just in case: flipping upside down is one of the artifacts this PR fixes. the other one I've seen is flying away (current tip of the branch 8ca757b does fix that too, but I just wanted to make sure you are aware of that): https://playground.babylonjs.com/#AZML8U#234 The model (2teaboxes.glb) that reproduces the flying away artifact has such a transform and geometry that the mesh is not near the origin. |
No, it is actually moving to a different position and sometimes scales incorrectly. still trying to find the best solution |
This pull request has been marked as stale because it has been inactive for more than 14 days. Please update to "unstale". |
@CedricGuillemet is this one still a thing? |
… gizmoMisc # Conflicts: # packages/dev/core/src/Physics/v2/physicsBody.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this PR still addressing sync physics, or just a gizmmo issue?
Oops, I forgot to change the PR name. Fixed now! |
I've tested this fix with LH/RH + rotations in parent transforms.
The base orientation that is
Babylon.js/packages/dev/core/src/Behaviors/Meshes/baseSixDofDragBehavior.ts
Line 353 in 36212ce
Used as a conjugate and then multiply by the delta. So, everything is kept local hence rotationQuaternion and not absoluteRotationQuaternion.