Skip to content
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

OpenXR: Fix small hand tracking issues #82722

Merged

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Oct 3, 2023

This PR fixes a number of long outstanding issues with hand tracking:

  • xr/openxr/extensions/hand_tracking is a new project setting that enables the hand tracking extension. As existing projects assume this is already enabled, the default is true. But this allows the user to disable this extension if they do not want to use it. Some platforms require the user to accept hand tracking when used.
  • Removal of the XR_FB_HAND_TRACKING_AIM_EXTENSION_NAME extension, this was never fully implemented in Godot 4 as this was being superseded by XR_EXT_HAND_INTERACTION_EXTENSION_NAME. We are adding the new OpenXR standard functionality in Add support for OpenXR hand interaction extension #81533
  • Small fix for head tracking confidence, we were capturing this but not setting it on the tracker
  • The "skeleton" pose on our XRController3D node was never properly implemented, we add support here. Note that OpenXR - add access to hand joint validity flags #82715 should be merged so we can add confidence here correctly. I suggest we merge this and fix this in 82715 before merging that.
  • Skeleton update in OpenXRHands now also updates the position, not just the orientation, of the bones.

There is a demo project (WIP) that can be found here that tests the functionality:
godotengine/godot-demo-projects#973

@BastiaanOlij BastiaanOlij added this to the 4.2 milestone Oct 3, 2023
@BastiaanOlij BastiaanOlij self-assigned this Oct 3, 2023
Comment on lines -165 to -168
hand_trackers[i].aimState.pinchStrengthIndex = 0.0;
hand_trackers[i].aimState.pinchStrengthMiddle = 0.0;
hand_trackers[i].aimState.pinchStrengthRing = 0.0;
hand_trackers[i].aimState.pinchStrengthLittle = 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the alternative for retrieving this data now?

Copy link
Contributor Author

@BastiaanOlij BastiaanOlij Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new hand interaction extension (#81533) replaces this functionality in a generic cross platform way.

We never actually implemented the old FB only extension in Godot 4, this code was copied over from Godot 3 without adding all the additional logic.

(do note that the middle/ring/little pinch functionality was not added in the generic solution, if anyone actually uses this, I suggest we move this functionality into a GDExtension as it is very unlikely to be adopted more broadly)

OpenXRHandTrackingExtension *hand_tracking_ext = OpenXRHandTrackingExtension::get_singleton();
if (hand_tracking_ext && hand_tracking_ext->get_active()) {
for (int i = 0; i < MAX_OPENXR_TRACKED_HANDS; i++) {
OpenXRInterface::Tracker *tracker = find_tracker(i == 0 ? "/user/hand/left" : "/user/hand/right");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fragile. A better approach would be to move the logic below into a function that takes in the XrPath:

handle_hand_tracking("/user/hand/left");
handle_hand_tracking("/user/hand/right");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but it'll still remain index based as it's hardcoded in the spec that we just access the two hands in hand tracking.

It is us in this case that map hand tracking to the xrPaths which is something I never got why when the hand tracking API was added to OpenXR, wasn't how it worked in the first place.

I'm guessing because while the OpenXR spec focussed on the left hand and right hand, and any controller held would be bound, to a lot of XR runtime implementations treating /user/hand/left not as the left hand, but as the left controller. This likely because it was the approach common before OpenXR changed it. This has led to a lot of issues that were only corrected since the 1.0.28 OpenXR spec, and still has a few left over edge cases in certain XR runtimes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, let me know what you think of how I changed it.

@BastiaanOlij
Copy link
Contributor Author

Ok, did some more testing with this and it resolves pretty much all the issues I was encountering. I even had the Valve hand meshes working properly with hand tracking natively on the Quest Pro, which is something that never worked before (did all sorts of weird mesh deformation).

@BastiaanOlij BastiaanOlij force-pushed the openxr_fix_hand_tracking_issues branch from 9cc30e0 to c60ef33 Compare October 4, 2023 02:18
@BastiaanOlij BastiaanOlij marked this pull request as ready for review October 4, 2023 02:19
@BastiaanOlij BastiaanOlij requested review from a team as code owners October 4, 2023 02:19
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@akien-mga akien-mga changed the title OpenXR Fix small hand tracking issues OpenXR: Fix small hand tracking issues Oct 4, 2023
@akien-mga akien-mga merged commit d7bca20 into godotengine:master Oct 4, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij BastiaanOlij deleted the openxr_fix_hand_tracking_issues branch October 5, 2023 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants