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

Add support for OpenXR hand interaction extension #81533

Merged
merged 1 commit into from
May 2, 2024

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Sep 11, 2023

This PR adds support for OpenXRs hand interaction extension that was introduced in 1.0.28.

This is a long awaited interaction that allows us to use the action system when optical hand tracking is used. This makes it much easier to create a portable application that works with hand tracking and controller tracking.

The extension introduces two new poses:

  • Pinch pose is a location centered between thumb and index finger pointing forward
  • Poke pose is a location at the tip of the index finger

It also adds 3 gesture based inputs:

  • pinch, triggered when the user pinches thumb and index finger together. The primary use case is to use this together with a raycast childed to the pinch pose and trigger interaction with an element the raycast touches
  • aim activation, is triggered when the index finger is fully extended. The primary use case is to activate a poke function when active. As the value based input is based on the curl of the index finger, the inverse can also be used to detect a trigger action.
  • graps, triggered when the user makes a fist. The primary use case is to detect grabbing an object.

When both a hand interaction profile and a controller interaction profile is supplied, the XR runtime will switch between the profiles depending on whether the user is holding a controller or whether optical hand tracking is active.

However if you only supply a hand interaction profile in your action map, the OpenXR specification states that any XR runtime that supports this extension should use the hand interaction profile even if a controller is being held.
This is ideal for projects that are designed toward hand tracking but want a controller fall back, especially in AR/Passthrough applications.

See: https://registry.khronos.org/OpenXR/specs/1.0/html/xrspec.html#XR_EXT_hand_interaction

A WIP demo project with basic functionality can be found here: godotengine/godot-demo-projects#973

Note: This PR is basically done, I need to do some testing and create a nice example project.

@BastiaanOlij
Copy link
Contributor Author

I've made this optional so there is now a tickbox that needs to be ticked:
image

I'm currently stuck with testing as I'm not sure if something is not working correctly yet, or if the extension hasn't been rolled out on Quest yet. Right now the logging isn't working properly.

metadata->register_io_path("/interaction_profiles/ext/hand_interaction_ext", "Pinch pose", "/user/hand/right", "/user/hand/right/input/pinch_ext/pose", "", OpenXRAction::OPENXR_ACTION_POSE);
metadata->register_io_path("/interaction_profiles/ext/hand_interaction_ext", "Poke pose", "/user/hand/left", "/user/hand/left/input/poke_ext/pose", "", OpenXRAction::OPENXR_ACTION_POSE);
metadata->register_io_path("/interaction_profiles/ext/hand_interaction_ext", "Poke pose", "/user/hand/right", "/user/hand/right/input/poke_ext/pose", "", OpenXRAction::OPENXR_ACTION_POSE);
metadata->register_io_path("/interaction_profiles/ext/hand_interaction_ext", "Palm pose", "/user/hand/left", "/user/hand/left/input/palm_ext/pose", XR_EXT_PALM_POSE_EXTENSION_NAME, OpenXRAction::OPENXR_ACTION_POSE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the 'palm pose' require the XR_EXT_palm_pose to be enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, though there was talk at some point that this extension would require palm pose to also be available. I'm really hoping palm pose becomes a mandatory thing at some point, it's pretty useless as an optional extension.

Anyway, we support the palm pose extension so if its enable, these paths become active if the extension is supported by the device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah it has remained linked but optional:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

We could wrap this in a check for the palm extension?

Suggested change
metadata->register_io_path("/interaction_profiles/ext/hand_interaction_ext", "Palm pose", "/user/hand/left", "/user/hand/left/input/palm_ext/pose", XR_EXT_PALM_POSE_EXTENSION_NAME, OpenXRAction::OPENXR_ACTION_POSE);
if (OpenXRPalmPoseExtension::get_singleton()->is_available()) {
metadata->register_io_path("/interaction_profiles/ext/hand_interaction_ext", "Palm pose", "/user/hand/left", "/user/hand/left/input/palm_ext/pose", XR_EXT_PALM_POSE_EXTENSION_NAME, OpenXRAction::OPENXR_ACTION_POSE);
}

profile->add_new_binding(primary, "/user/hand/left/input/pinch_ext/value,/user/hand/right/input/pinch_ext/value");
profile->add_new_binding(primary_click, "/user/hand/left/input/pinch_ext/ready_ext,/user/hand/right/input/pinch_ext/ready_ext");

// Use activation as secondary
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec is unclear, what's the role of aim_activate_ext?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean the OpenXR spec?

The "aim_activate" gesture is runtime defined, and it should be chosen so that the "aim" pose tracking is stable and usable for pointing at a distant target while the gesture is being performed.

That is indeed extremely vague, so the runtime defines the situation it is activated by detecting you're pointing at something. How it does that? 🤷

So far I was under the impression this basically returns 1.0/true when you extend your finger, but I could be wrong about that.

Personally this is the only element of this new profile I'm unlikely to use, until someone shows me a good use case :)

@m4gr3d
Copy link
Contributor

m4gr3d commented Oct 4, 2023

I'm currently stuck with testing as I'm not sure if something is not working correctly yet, or if the extension hasn't been rolled out on Quest yet. Right now the logging isn't working properly.

Just double-checking that you're including the hand tracking permissions when testing this extension?

@BastiaanOlij BastiaanOlij force-pushed the openxr_hand_interaction branch 2 times, most recently from a2e76ae to 4ef64f0 Compare October 5, 2023 06:04
@BastiaanOlij
Copy link
Contributor Author

I'm currently stuck with testing as I'm not sure if something is not working correctly yet, or if the extension hasn't been rolled out on Quest yet. Right now the logging isn't working properly.

Just double-checking that you're including the hand tracking permissions when testing this extension?

As far as I'm aware yes, I've combined the logic with the normal hand tracking demo and the hand tracking works, but the interaction doesn't.

This could simply be because its gated behind the OpenXR version or because it's not yet enabled on the current Quest OS.

Anyway, I've added some basic logic around this here: godotengine/godot-demo-projects#973

@BastiaanOlij BastiaanOlij force-pushed the openxr_hand_interaction branch 2 times, most recently from 6a87026 to d882031 Compare October 6, 2023 13:53
@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
@BastiaanOlij BastiaanOlij force-pushed the openxr_hand_interaction branch from d882031 to e18ed03 Compare November 3, 2023 01:44
@BastiaanOlij
Copy link
Contributor Author

Just a bit of extra info on the progress with this PR. Basically the functionality is finished but we're currently waiting for XR Vendors to take the implementation of the feature out of beta.

@BastiaanOlij BastiaanOlij force-pushed the openxr_hand_interaction branch from e18ed03 to 9007a32 Compare December 18, 2023 04:18
@BastiaanOlij BastiaanOlij force-pushed the openxr_hand_interaction branch from 9007a32 to 7dd0939 Compare January 27, 2024 08:10
@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jan 31, 2024

Note regarding CI failure, is_hand_interaction_supported and is_eye_gaze_interaction_supported have been changed to const as these should have been const to begin with. These functions have never had mutable behaviour so not sure if we can just add those to the ignore file, or if this really breaks things?

edit I've undone this change as not to upset GDExtensions etc. It's not directly related to this PR. we can always make a separate PR if this is important.

@dsnopek
Copy link
Contributor

dsnopek commented Jan 31, 2024

Because the hash changes, it would really break compatibility. However, adding compatibility functions for them should be pretty easy!

There's a number of examples of them in the code base to look at, for example, TileMap::get_used_rect() had this exact same situation where it was non-const and was changed to const. Take a look at the TileMap::_get_used_rect_bind_compat_78328() method (the number is the PR), and how it's wrapped in #ifndef DISABLE_DEPRECATED and implemented in tile_map.compat.inc.

Or, if you can't work it out, just let me know and I can make a patch :-)

@BastiaanOlij
Copy link
Contributor Author

Because the hash changes, it would really break compatibility. However, adding compatibility functions for them should be pretty easy!

There's a number of examples of them in the code base to look at, for example, TileMap::get_used_rect() had this exact same situation where it was non-const and was changed to const. Take a look at the TileMap::_get_used_rect_bind_compat_78328() method (the number is the PR), and how it's wrapped in #ifndef DISABLE_DEPRECATED and implemented in tile_map.compat.inc.

Or, if you can't work it out, just let me know and I can make a patch :-)

Was this in relation to this PR David? I think you might have meant to leave this as a comment on one of Giles PRs?

@dsnopek
Copy link
Contributor

dsnopek commented Apr 1, 2024

Was this in relation to this PR David? I think you might have meant to leave this as a comment on one of Giles PRs?

Yes, my comment was meant for this PR!

I was just pointing to Gilles' changes as an example for how to handle this situation, since he also had a method change to const and added the compatibility methods.

This PR will need to add some compatibility methods because adding/removing const changes the method hash, which would cause any existing GDExtension calling these methods to break.

@BastiaanOlij BastiaanOlij force-pushed the openxr_hand_interaction branch 3 times, most recently from 6408a83 to f04bde9 Compare April 26, 2024 09:41
@BastiaanOlij
Copy link
Contributor Author

This PR will need to add some compatibility methods because adding/removing const changes the method hash, which would cause any existing GDExtension calling these methods to break.

I ended up undoing the change, it's not really related to this PR, just something I noticed for existing calls that we normally make consts. I think that if its important, we can make a separate PR.

@BastiaanOlij BastiaanOlij marked this pull request as ready for review April 29, 2024 06:25
@BastiaanOlij BastiaanOlij requested review from a team as code owners April 29, 2024 06:25
@BastiaanOlij
Copy link
Contributor Author

I'm changing this back to ready for review. Right now the only HMD that implements this extension is the Magic Leap 2, but I'm hoping that other headsets follow suit. I think it's worth merging this for 4.3

@akien-mga akien-mga requested a review from a team April 29, 2024 08:11
Copy link
Contributor

@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 don't have the hardware to test this, but overall the code looks good to me!

@BastiaanOlij BastiaanOlij force-pushed the openxr_hand_interaction branch from f04bde9 to 527c30c Compare May 2, 2024 09:15
@akien-mga akien-mga merged commit 3dca334 into godotengine:master May 2, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@BastiaanOlij BastiaanOlij deleted the openxr_hand_interaction branch May 3, 2024 02:05
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.

5 participants