-
-
Notifications
You must be signed in to change notification settings - Fork 569
fix(tracking): 🐛 Correct quaternion multiplication order in motion prediction #2947
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
base: master
Are you sure you want to change the base?
Conversation
OpenXR uses velocities in global coordinates. This is what we would like to adhere to. SteamVR instead uses local coordinate system. We are doing the conversion from global to local in Sorry for not working on this before but I'm quite busy and I was unable to test. I'll leave this to you in the meantime, let me know how it goes. |
…ger to alvr_client_openvr post-prediction
Changes committed. |
alvr/server_core/src/tracking/mod.rs
Outdated
motion.linear_velocity += motion | ||
.angular_velocity | ||
.cross(motion.pose.orientation * config.pose_offset.position); | ||
motion.angular_velocity = | ||
motion.pose.orientation.conjugate() * motion.angular_velocity; | ||
|
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.
There seems to be a mismatch, are you sure the linear velocity doesn't need changing?
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.
The angular velocity here is in global coordinates, hasn’t been converted, it matches the global linear velocity, so I think it is no need to change.
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 suspect this needs some more thought. Both the recentering and the controller offset parameters would also shift the frame of reference for the linear and angular velocity. I think this wasn't correct before and still isn't correct now. But if it works good enough now i think i can merge
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.
If it works alright for you, we can merge
… recentered coordinates.
The problem seems to be in the prediction phase. Maybe we should perform the prediction first and then apply the offset, otherwise the controller rotation after applying the offset is not as expected. |
I messed it, this has nothing todo with predict, it should be an issue with the linear velocity processing. Let me check again. |
need a new setting option to set the rotation center for controller and hand tracking, now I use the default offset value as alternative, requires refactoring |
Oh yeah. Well on the client side we do apply per-platform pose corrections for controllers. But we don't correct the origin of the linear and angular velocity. So that would have to be done on the client side first |
I think everything is okay now, it can be merged |
I don't understand why you need a pivot offset. Isn't this something that should be inferred by the controller offset? Does this mean that SteamVR always needs a pivot offset even when the controller offset is 0,0,0 (either for translation or rotation)? |
For some device, such as my Pico4, the controller pivot position in the default state is at the tail rather than at the center. Although this can be fixed by modifying the pose corrections in the client, I observed that only some devices have applied it, which may cause others to have incorrect pivot position issues. Adding a pivot offset allows users to manually correct this issue. If you insist that it should be entirely determined by the controller offset, I will remove this setting. |
Actually I dont think pivot has sth todo with controller offset, pivot is just a relative position of the controller. |
I think i will experiment more myself. It's hard to agree with certainty until i test more |
This PR corrects the multiplication order to
old * delta
, ensuring that the rotational delta from angular velocity is correctly applied in the local coordinate system rather than world coordinate system of the tracked device.The wrong multiplication was made apparent after the motion prediction logic was moved from the client to the server in #2904 , which make the movement of the controller become strange under high-speed movement.