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: Add support for binding modifiers #97140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Sep 18, 2024

Adds support for OpenXR binding modifiers to the action map.

This allows you to add modifiers to inputs such as applying thresholds or offsetting values, etc.
Binding modifiers can add additional input paths or change the type of an input path.

Todos:

  • Create infrastructure for binding extension system to the action map
  • Make UI changes to the action map UI
  • Implement support for haptic triggers
  • Implement analog threshold extension (may move this into vendor plugin)
    • implement resource class
    • implement UI
  • Implement dpad binding extension
    • implement resource class
    • implement UI

Depends on:

There is now a demo for this project that can be found here:
godotengine/godot-demo-projects#1137

Some improvements that can be done (possibly in follow up PRs):

  • Should show our modifier button icon in a different color if there are modifiers defined.
  • Should find a way to limit the modifiers you can create to those that are applicable.

Contributed by Khronos Group through the Godot Integration Project

@BastiaanOlij BastiaanOlij added this to the 4.x milestone Sep 18, 2024
@BastiaanOlij BastiaanOlij requested a review from a team September 18, 2024 10:15
@BastiaanOlij BastiaanOlij self-assigned this Sep 18, 2024
@AThousandShips AThousandShips changed the title OpenXR: Adding support for binding modifiers OpenXR: Add support for binding modifiers Sep 18, 2024
@BastiaanOlij BastiaanOlij force-pushed the xr_action_map_enhancements branch 3 times, most recently from 45f85cf to 17a009e Compare September 26, 2024 04:57
@BastiaanOlij
Copy link
Contributor Author

Back working on this after dealing with a bout of the flu. Starting to get an interface together but need to change direction slightly.

@BastiaanOlij BastiaanOlij force-pushed the xr_action_map_enhancements branch 2 times, most recently from e3aeb3b to a419e71 Compare October 14, 2024 03:29
@BastiaanOlij BastiaanOlij force-pushed the xr_action_map_enhancements branch 2 times, most recently from 0b77271 to 577c7ef Compare October 14, 2024 23:06
@BastiaanOlij
Copy link
Contributor Author

While some of this is a bit of an unknown (but likely) future, I changed our action map structure slightly so we can record binding modifiers both on interaction profiles and on individual bindings.

Right now the dpad modifier is a bit of the odd duckling being the only modifier that is linked to an action set/source path combination and is thus recorded on the interaction profile level.
The analog threshold modifier is linked to an action/source path combo which is recorded as a binding (hence the changes in #98163) and it is very likely that this is going to be true for future modifiers as well.

Right now they are all added to the next chain or XrInteractionProfileSuggestedBinding when calling xrSuggestInteractionProfileBindings however this may change/be enhanced in the future.

@BastiaanOlij
Copy link
Contributor Author

UI is finally coming together. Binding modifiers are accessible either on the interaction profile (1) or for individual bindings (2):
image

Either button opens up a popup that allows for creating applicable binding modifiers on that level:
image

I still need to change the dialog for selecting a new binding modifier as it doesn't filter by type, and we only have the two modifiers right now but that will change in the near future.

@BastiaanOlij
Copy link
Contributor Author

Some interesting feedback concerning the DPad modifier extension that we need to ensure gets documented well.

The extension actually does two things when enabled.

  1. It adds new binding paths you can use for each thumbstick and thumbpad input. These paths are immediately usable even if you do not add a dpad binding modifier
  2. And it adds the dpad binding modifier, which lets you customise the dpad behaviour

@BastiaanOlij
Copy link
Contributor Author

All the new paths have been added to the meta data database. In doing this I've cleaned up a lot of the code to make it easier to manage. Still planning on adding a bit of extra logic in the UI when editing the action map so we only show bindings that are available based on which extensions have been selected in project settings so we don't get a whole bunch the user isn't using.

@BastiaanOlij BastiaanOlij force-pushed the xr_action_map_enhancements branch 2 times, most recently from 4fb4d9a to 00c39d2 Compare October 21, 2024 07:44
@BastiaanOlij
Copy link
Contributor Author

Other then needing to do some more testing, this should now all work.

My only remaining issue is that I had to use the EditorInspector class to make the property editors work properly but these introduce the resource section into the UI and I haven't found a way to supress this:

image

These sections make no sense in this context so I hope there is a way to remove them.

@BastiaanOlij BastiaanOlij marked this pull request as ready for review October 21, 2024 07:50
@BastiaanOlij BastiaanOlij requested review from a team as code owners October 21, 2024 07:50
@BastiaanOlij BastiaanOlij requested review from a team as code owners October 21, 2024 07:50
@BastiaanOlij BastiaanOlij force-pushed the xr_action_map_enhancements branch 2 times, most recently from cb7dd7a to 2129833 Compare October 21, 2024 08:02
@BastiaanOlij
Copy link
Contributor Author

One more extra thing, this PR also introduces the OpenXRHapticBase base class and OpenXRHapticVibration class, currently only used for the new features in the action map. There are a number of vendor specific implementations of the haptic feedback OpenXR struct that forms the base of this implementation that are also used for triggering haptic feedback pulses from code.

I'm thinking of replacing OpenXRAPI::trigger_haptic_pulse with a version that takes an OpenXRHapticBase object as a parameter and we'll be able to perform different haptic effects as they are added to the OpenXR spec generically.
This is something we'll do in a separate PR however.

@fire
Copy link
Member

fire commented Oct 21, 2024

Maybe you can figure out why the tests fail.


editor/editor_inspector.cpp:4098:57: runtime error: member access within null pointer of type 'struct EditorFeatureProfileManager'
editor/editor_inspector.cpp:4098:154: runtime error: member access within null pointer of type 'struct Node'

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.4.dev.gh-97140 (b26897c0a0f76a2c8ee1f48419e975911b95d691)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] ./bin/godot.linuxbsd.editor.dev.double.x86_64.san(+0x380b26f7) [0x55bd781cc6f7] (??:?)
[2] /lib/x86_64-linux-gnu/libc.so.6(+0x43090) [0x7fc58e828090] (??:0)
[3] EditorInspector::_notification(int) (??:?)
[4] EditorInspector::_notificationv(int, bool) (??:?)
[5] Object::notification(int, bool) (??:?)
[6] Node::_propagate_ready() (??:?)
[7] Node::_propagate_ready() (??:?)
[8] Node::_propagate_ready() (??:?)
[9] Node::_set_tree(SceneTree*) (??:?)
[10] Node::_add_child_nocheck(Node*, StringName const&, Node::InternalMode) (??:?)
[11] Node::add_child(Node*, bool, Node::InternalMode) (??:?)
[12] void call_with_variant_args_helper<__UnexistingClass, Node*, bool, Node::InternalMode, 0ul, 1ul, 2ul>(__UnexistingClass*, void (__UnexistingClass::*)(Node*, bool, Node::InternalMode), Variant const**, Callable::CallError&, IndexSequence<0ul, 1ul, 2ul>) (??:?)
[13] void call_with_variant_args_dv<__UnexistingClass, Node*, bool, Node::InternalMode>(__UnexistingClass*, void (__UnexistingClass::*)(Node*, bool, Node::InternalMode), Variant const**, int, Callable::CallError&, Vector<Variant> const&) (??:?)
[14] MethodBindT<Node*, bool, Node::InternalMode>::call(Object*, Variant const**, int, Callable::CallError&) const (??:?)
[15] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (??:?)
[16] GDScriptInstance::callp(StringName const&, Variant const**, int, Callable::CallError&) (??:?)
[17] Object::callp(StringName const&, Variant const**, int, Callable::CallError&) (??:?)
[18] Variant::callp(StringName const&, Variant const**, int, Variant&, Callable::CallError&) (??:?)
[19] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (??:?)
[20] GDScriptInstance::callp(StringName const&, Variant const**, int, Callable::CallError&) (??:?)
[21] Node::_gdvirtual__ready_call() (??:?)
[22] Node::_notification(int) (??:?)
[23] Node::_notificationv(int, bool) (??:?)
[24] Object::notification(int, bool) (??:?)
[25] Node::_propagate_ready() (??:?)
[26] Node::_set_tree(SceneTree*) (??:?)
[27] Node::_add_child_nocheck(Node*, StringName const&, Node::InternalMode) (??:?)
[28] Node::add_child(Node*, bool, Node::InternalMode) (??:?)
[29] void call_with_variant_args_helper<__UnexistingClass, Node*, bool, Node::InternalMode, 0ul, 1ul, 2ul>(__UnexistingClass*, void (__UnexistingClass::*)(Node*, bool, Node::InternalMode), Variant const**, Callable::CallError&, IndexSequence<0ul, 1ul, 2ul>) (??:?)
[30] void call_with_variant_args_dv<__UnexistingClass, Node*, bool, Node::InternalMode>(__UnexistingClass*, void (__UnexistingClass::*)(Node*, bool, Node::InternalMode), Variant const**, int, Callable::CallError&, Vector<Variant> const&) (??:?)
[31] MethodBindT<Node*, bool, Node::InternalMode>::call(Object*, Variant const**, int, Callable::CallError&) const (??:?)
[32] GDScriptFunction::call(GDScriptInstance*, Variant const**, int, Callable::CallError&, GDScriptFunction::CallState*) (??:?)
[33] GDScriptInstance::callp(StringName const&, Variant const**, int, Callable::CallError&) (??:?)
[34] Node::_gdvirtual__process_call(double) (??:?)
[35] Node::_notification(int) (??:?)
[36] Node::_notificationv(int, bool) (??:?)
[37] CanvasItem::_notificationv(int, bool) (??:?)
[38] Control::_notificationv(int, bool) (??:?)
[39] Object::notification(int, bool) (??:?)
[40] SceneTree::_process_group(SceneTree::ProcessGroup*, bool) (??:?)
[41] SceneTree::_process(bool) (??:?)
[42] SceneTree::process(double) (??:?)
[43] Main::iteration() (??:?)
[44] OS_LinuxBSD::run() (??:?)
[45] ./bin/godot.linuxbsd.editor.dev.double.x86_64.san(main+0x4a3) [0x55bd781cbf9c] (??:?)
[46] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3) [0x7fc58e809083] (??:0)
[47] ./bin/godot.linuxbsd.editor.dev.double.x86_64.san(_start+0x2e) [0x55bd781cba3e] (??:?)
-- END OF BACKTRACE --

@BastiaanOlij
Copy link
Contributor Author

Maybe you can figure out why the tests fail.

Looks like a failure to check for NULL parameters with standard unit tests. Sadly on a part of the API that is completely unrelated to this work, so I don't know why it's suddenly failing.

@BastiaanOlij
Copy link
Contributor Author

Hmm I wonder, the line of code that fails is EditorFeatureProfileManager::get_singleton()->connect("current_feature_profile_changed", callable_mp(this, &EditorInspector::_feature_profile_changed));
I'm guessing the testing doesn't setup the editor classes so possibly me using the EditorInspector in classes that are exposed as they are designed to be subclassed in the vendors plugin.

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.

Thanks!

This PR is epic in size, but it looks great! All my code review comments are very nitpick-y things.

I tested the new editor stuff, and it all seemed to work fine. I didn't attempt to test it functionally - I see that SteamVR and Monado reportedly support it, so I may attempt to give it a try later.

Since PR #98163 has been merged, this should be rebased. That'll make reviewing a tiny bit easier.

modules/openxr/action_map/openxr_action_map.cpp Outdated Show resolved Hide resolved
modules/openxr/action_map/openxr_action_map.cpp Outdated Show resolved Hide resolved
modules/openxr/action_map/openxr_action_map.cpp Outdated Show resolved Hide resolved
modules/openxr/action_map/openxr_interaction_profile.cpp Outdated Show resolved Hide resolved
modules/openxr/action_map/openxr_interaction_profile.cpp Outdated Show resolved Hide resolved
modules/openxr/action_map/openxr_interaction_profile.h Outdated Show resolved Hide resolved
modules/openxr/doc_classes/OpenXRDpadBindingModifier.xml Outdated Show resolved Hide resolved
modules/openxr/editor/openxr_binding_modifier_editor.cpp Outdated Show resolved Hide resolved
modules/openxr/editor/openxr_binding_modifier_editor.cpp Outdated Show resolved Hide resolved
@BastiaanOlij BastiaanOlij force-pushed the xr_action_map_enhancements branch 4 times, most recently from 5654653 to 7d8bef8 Compare October 30, 2024 21:52
@@ -4095,6 +4095,7 @@ void EditorInspector::_notification(int p_what) {
} break;

case NOTIFICATION_READY: {
ERR_FAIL_NULL(EditorFeatureProfileManager::get_singleton());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that CI testing runs through this, and we're not fully setting up the editor, we need to exit here as the EditorFeatureProfileManager singleton does not get constructed.

doc/classes/ProjectSettings.xml Outdated Show resolved Hide resolved
@@ -156,6 +175,10 @@ void OpenXRActionMap::remove_interaction_profile(Ref<OpenXRInteractionProfile> p
int idx = interaction_profiles.find(p_interaction_profile);
if (idx != -1) {
interaction_profiles.remove_at(idx);

ERR_FAIL_COND_MSG(p_interaction_profile->action_map != this, "Removing interaction profile that belongs to this action map but had incorrect action map pointer."); // this should never happen!
p_interaction_profile->action_map = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to call p_interaction_profile->action_map->remove_interaction_profile(p_interaction_profile) before you reset the action_map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the design is that an interaction profile always belongs to a single action map. The action_map member is only null momentarily in 2 conditions:

  1. when we're creating a new interaction profile right before it's added to an action map
  2. when we're destructing an interaction profile where it's remove from the action map just before.

The check to see if the actionmap pointer is different is just to catch errors in usage, someone who later on adds code that would attempt to add an interaction profile to multiple action maps, and these checks just erroring out.

Comment on lines +124 to +130
Array OpenXRIPBinding::get_binding_modifiers() const {
Array ret;
for (const Ref<OpenXRBindingModifier> &binding_modifier : binding_modifiers) {
ret.push_back(binding_modifier);
}
return ret;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not directly returning the binding_modifiers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we're converting from a Vector<Ref<OpenXRBindingModifier>> to an Array. I don't think there is an implicit conversion or was that added at some time?

binding_modifiers.remove_at(idx);

ERR_FAIL_COND_MSG(p_binding_modifier->ip_binding != this, "Removing binding modifier that belongs to this binding but had incorrect binding pointer."); // this should never happen!
p_binding_modifier->ip_binding = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar comment; should p_binding_modifier be removed from ip_binding before resetting it?

Comment on lines +28 to +29
Return [code]false[/code] if binding modifiers of this type are recorded on an interaction profile.
Return [code]true[/code] if binding modifiers of this type are recorded on bindings within an interaction profile.
Copy link
Contributor

Choose a reason for hiding this comment

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

The description is a bit confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically there are two types of binding modifiers, those recorded on the interaction profile, and those recorded against a specific binding on the interaction profile. This flag simply indicates which type we're dealing with.
This really won't get a lot cleared until more binding modifiers become public.

Comment on lines +24 to +25
If [code]false[/code], when the joystick enters a new dpad zone this becomes true.
If [code]true[/code], when the joystick remains in active dpad zone, this remains true even if we overlap with another zone.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also hard to understand what this field is for at first read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had real difficulty describing this in text, you really need to look at the diagrams in the API spec and even there the description is overly technical.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are those diagrams publicly available? Can you link to them in this description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you scroll to the top of the help page you'll notice that on both modifiers, I link to the specification for the modifiers.

For the DPad modifier its this one: https://registry.khronos.org/OpenXR/specs/1.1/html/xrspec.html#XR_EXT_dpad_binding

Totally open to suggestions on how to work that more user friendly and embed that into our documentation.

modules/openxr/editor/openxr_action_map_editor.cpp Outdated Show resolved Hide resolved
modules/openxr/editor/openxr_binding_modifier_editor.cpp Outdated Show resolved Hide resolved
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.

Logic looks good, although I'd like us to iterate on the documentation as in their current state, they're hard to parse.

@BastiaanOlij Is there a test project that can be used to help validate those changes?

@BastiaanOlij
Copy link
Contributor Author

Did some more testing with this while addressing some of the feedback, found out that when both modifier extensions are enabled in the settings, one wouldn't work because both extensions require the base modifier extension. Fixed that up so Godot will react properly if multiple wrappers ask for the same extension to be enabled.

@BastiaanOlij
Copy link
Contributor Author

@BastiaanOlij Is there a test project that can be used to help validate those changes?

@m4gr3d, I added a demo project on godot-demo-projects, see link in OP.

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 skimmed the latest code, and mostly just found more little nitpicky stuff.

I haven't tested the binding modifiers at runtime, but I did re-test the editor stuff again. I'm able to add an OpenXRAnalogThresholdModifier, and that seems to be working fine. However, when I try to add an OpenXRDpadBindingModifier I get this error:

ERROR: Condition "!new_binding_modifier->record_on_binding()" is true.
   at: _on_dialog_created (modules/openxr/editor/openxr_binding_modifiers_dialog.cpp:134)

Is that expected? If so, how should dpad modifiers be added?

// Append the extensions requested by the registered extension wrappers.
HashMap<String, bool *> requested_extensions;
// We can request an extension multiple times if there are dependencies
struct request_extension {
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though this is a very local struct, I think we always use camel case for struct names - using snake case feels a little weird

Comment on lines +53 to +55
GDVIRTUAL0RC(bool, _record_on_binding)
GDVIRTUAL0RC(String, _get_description)
GDVIRTUAL0R(PackedByteArray, _get_ip_modification)
Copy link
Contributor

Choose a reason for hiding this comment

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

These feel like they should be required? So:

Suggested change
GDVIRTUAL0RC(bool, _record_on_binding)
GDVIRTUAL0RC(String, _get_description)
GDVIRTUAL0R(PackedByteArray, _get_ip_modification)
GDVIRTUAL0RC_REQUIRED(bool, _record_on_binding)
GDVIRTUAL0RC_REQUIRED(String, _get_description)
GDVIRTUAL0R_REQUIRED(PackedByteArray, _get_ip_modification)

This will make it an error if GDVIRTUAL_CALL() can't find the method, and mark them as required in the extension_api.json.

This won't have any effect on the sub-classes within Godot that override these methods; only for GDExtension classes that do.

class OpenXRBindingModifier : public Resource {
GDCLASS(OpenXRBindingModifier, Resource);

private:
Copy link
Contributor

Choose a reason for hiding this comment

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

This superfluous private: could be removed:

Suggested change
private:

#include "../action_map/openxr_action_set.h"
#include "../action_map/openxr_binding_modifier.h"
#include "editor/editor_inspector.h"
// #include "editor/editor_properties.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this?

Suggested change
// #include "editor/editor_properties.h"

void set_off_threshold(float p_threshold);
float get_off_threshold() const;

void set_on_haptic(const Ref<OpenXRHapticBase> p_haptic);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might have meant to be a const reference? So:

Suggested change
void set_on_haptic(const Ref<OpenXRHapticBase> p_haptic);
void set_on_haptic(const Ref<OpenXRHapticBase> &p_haptic);

void set_on_haptic(const Ref<OpenXRHapticBase> p_haptic);
Ref<OpenXRHapticBase> get_on_haptic() const;

void set_off_haptic(const Ref<OpenXRHapticBase> p_haptic);
Copy link
Contributor

Choose a reason for hiding this comment

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

This one too:

Suggested change
void set_off_haptic(const Ref<OpenXRHapticBase> p_haptic);
void set_off_haptic(const Ref<OpenXRHapticBase> &p_haptic);

void set_is_sticky(bool p_sticky);
bool get_is_sticky() const;

void set_on_haptic(const Ref<OpenXRHapticBase> p_haptic);
Copy link
Contributor

Choose a reason for hiding this comment

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

And this:

Suggested change
void set_on_haptic(const Ref<OpenXRHapticBase> p_haptic);
void set_on_haptic(const Ref<OpenXRHapticBase> &p_haptic);

void set_on_haptic(const Ref<OpenXRHapticBase> p_haptic);
Ref<OpenXRHapticBase> get_on_haptic() const;

void set_off_haptic(const Ref<OpenXRHapticBase> p_haptic);
Copy link
Contributor

Choose a reason for hiding this comment

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

And here:

Suggested change
void set_off_haptic(const Ref<OpenXRHapticBase> p_haptic);
void set_off_haptic(const Ref<OpenXRHapticBase> &p_haptic);

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.

4 participants