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

Implement XR_FB_hand_tracking_aim extension #81

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

devloglogan
Copy link
Collaborator

This PR depends on Godot PR 87546

Adds extension wrapper for the XR_FB_hand_tracking_aim extension. The added singleton has methods to determine gesture states / pinch strength while hand tracking, and also provides a number of signals indicating when certain gestures begin/end.

Since this extension wrapper will only be supported by 4.3 onward, as well as the others listed in the above Godot PR, maybe it would make since to merge this into a 4.3 branch instead of master?

Also, one thing that might be worth discussing is how we should implement demos of these extension wrappers moving forward. Right now they're being dropped in main.tscn, but these are only meta extensions, so it doesn't really make sense to have them always present. Maybe we want to start thinking of implementing a menu to navigate implemented extensions and load corresponding demos?

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 31, 2024

Note: This will fail CI until the Godot PR is merged, and we're using godot-cpp with an updated extension_api.json for it

@BastiaanOlij
Copy link
Member

BastiaanOlij commented Feb 4, 2024

XR_FB_hand_tracking_aim has the dubious honour of being one of the FB extensions that isn't following the design of OpenXR very well.
It's basically being replaced by XR_EXT_hand_interaction (which Meta sadly isn't supporting yet but for which we have an open PR that can be merged once we have a runtime on which we can test it). Together with the additions to the touch profiles Meta recently did, that means that there is a clean switch in a player controlling the game with a controller, or switching over to handtracking, with the action map.

I'm not against support for hand tracking aim, but it should come with a big warning that it's an older solution that is not portable, and that soon better alternatives are possible through the hand interaction API.

For getting this PR over the line I'd like to see two changes:

  1. The feature needs to be gated behind a project setting. People need to opt in to this extension being activated. Its very likely that in the long term, many people will leave it off
  2. I wouldn't go for direct access to the extension. Instead I would prefer if the extension manages two new positional trackers so normal XRController3D nodes can be used but with hardcoded actions that bypass the actionmap (similar to what we do with Skeleton). So we simply create two new paths, say /user/fb_hand_aim/left and /user/fb_hand_aim/right that only has a default pause (which is the provided aim pose) and various actions like pinch_index, pinch_middle, etc.

@BastiaanOlij
Copy link
Member

Note that for point 2, we probably need to make an amendment to core so OpenXRInterface::get_suggested_tracker_names calls into extension wrappers to ask for additional entries to return. We can then just have the 3 OpenXR paths returned, move the HTC entries into the HTC extension, and have this extension return the two new trackers. This will ensure they are selectable from the dropdown.

Similarly, but this should be for later as its a much bigger job and a more structural change, get_suggested_pose_names should be implemented to actually query the action map for poses mapped in the action map and call into extensions to return hardcoded action names.

@devloglogan devloglogan force-pushed the fb-handtracking-ext branch 2 times, most recently from d637884 to c94823d Compare February 9, 2024 20:32
Comment on lines 143 to 164
XRPose::TrackingConfidence confidence = XRPose::TrackingConfidence::XR_TRACKING_CONFIDENCE_LOW;

if (!(aim_state[i].status & XR_HAND_TRACKING_AIM_VALID_BIT_FB)) {
confidence = XRPose::TrackingConfidence::XR_TRACKING_CONFIDENCE_NONE;
} else if (aim_state[i].status & XR_HAND_TRACKING_AIM_COMPUTED_BIT_FB) {
confidence = XRPose::TrackingConfidence::XR_TRACKING_CONFIDENCE_HIGH;
}
Copy link
Collaborator Author

@devloglogan devloglogan Feb 9, 2024

Choose a reason for hiding this comment

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

Still more work to be done on updating this PR but I rebased and swapped over to positional trackers as suggested. Not sure if in this context there is a point to checking anything for tracking confidence, but for the moment I threw this in. I don't understand what XR_HAND_TRACKING_AIM_COMPUTED_BIT_FB really means yet, from the spec on what the flag means:

Aiming data is computed from additional sources beyond the hand data in the base structure

Copy link
Member

Choose a reason for hiding this comment

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

I think that means that if full optical tracking isn't available (hand is not visible) that the current position is based on indirect measurements. I believe Quest for instance just added a way to determine hand position based on the cameras still seeing the arm.

So my gut feeling says that if XR_HAND_TRACKING_AIM_COMPUTED_BIT_FB is set, our confidence would be low, not high.

Could be wrong though and I don't think many users check beyond confidence not being NONE.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd discussed with @dsnopek and he thought the same. At the moment I have it setting the confidence always to LOW, unless the flag XR_HAND_TRACKING_AIM_VALID_BIT_FB is not set, in which case I set confidence to NONE. So I'm not actually using the XR_HAND_TRACKING_AIM_COMPUTED_BIT_FB bit anywhere now.

@m4gr3d
Copy link
Collaborator

m4gr3d commented Feb 12, 2024

Since this extension wrapper will only be supported by 4.3 onward, as well as the others listed in the above Godot PR, maybe it would make since to merge this into a 4.3 branch instead of master?

We follow the approach in Godot core where master tracks the tip of the development branch, so once approved this change can be merged into the master branch.
We'll need to update the godot-cpp submodule accordingly.

@devloglogan
Copy link
Collaborator Author

Updated and rebased! Also see godotengine/godot#88311 for changes to get_suggested_tracker_names()

@m4gr3d
Copy link
Collaborator

m4gr3d commented Feb 16, 2024

@devloglogan Can you bump the compatibility_minimum value in plugin.gdextension to 4.3.

@m4gr3d m4gr3d added this to the 3.0.0 milestone Feb 16, 2024
@m4gr3d m4gr3d added the enhancement New feature or request label Feb 16, 2024
@devloglogan devloglogan force-pushed the fb-handtracking-ext branch 2 times, most recently from d681806 to 69be122 Compare February 19, 2024 20:40
@devloglogan
Copy link
Collaborator Author

Rebased and bumped the compatibility_minimum value.

@devloglogan
Copy link
Collaborator Author

Rebased! I believe all requested changes have been addressed?

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

I just retested this, and it's working wonderfully!

The latest code is looking good to me as well.

Great work!

@m4gr3d m4gr3d merged commit a944df2 into GodotVR:master Mar 6, 2024
7 checks passed
@devloglogan devloglogan deleted the fb-handtracking-ext branch April 24, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants