godot icon indicating copy to clipboard operation
godot copied to clipboard

OpenXR: Add support for binding modifiers

Open BastiaanOlij opened this issue 1 year ago • 10 comments
trafficstars

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:

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

Depends on:

  • #97392
  • #98163

Contributed by Khronos Group through the Godot Integration Project

BastiaanOlij avatar Sep 18 '24 10:09 BastiaanOlij

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 avatar Oct 14 '24 03:10 BastiaanOlij

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 avatar Oct 14 '24 23:10 BastiaanOlij

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 avatar Oct 15 '24 07:10 BastiaanOlij

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 avatar Oct 18 '24 05:10 BastiaanOlij

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 avatar Oct 20 '24 11:10 BastiaanOlij

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 avatar Oct 21 '24 07:10 BastiaanOlij

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.

BastiaanOlij avatar Oct 21 '24 08:10 BastiaanOlij

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 --

fire avatar Oct 21 '24 08:10 fire

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 avatar Oct 21 '24 23:10 BastiaanOlij

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.

BastiaanOlij avatar Oct 21 '24 23:10 BastiaanOlij

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 avatar Nov 22 '24 08:11 BastiaanOlij

@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.

BastiaanOlij avatar Nov 22 '24 09:11 BastiaanOlij

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

DPad modifiers aren't added to individual bindings, they are added to the interaction profile with the binding button on the right. I need to figure out if I can add additional filters to the standard popup. I didn't want to create two separate base classes but maybe i need to go down that route.

BastiaanOlij avatar Dec 05 '24 21:12 BastiaanOlij

@BastiaanOlij:

DPad modifiers aren't added to individual bindings, they are added to the interaction profile with the binding button on the right. I need to figure out if I can add additional filters to the standard popup. I didn't want to create two separate base classes but maybe i need to go down that route.

I think even just having a more user-friendly error message would be OK. Right now, I'm getting a very programmer-y error and the modifier just fails to be added.

Maybe we just need a popup with an explanation?

dsnopek avatar Dec 06 '24 15:12 dsnopek

@BastiaanOlij:

DPad modifiers aren't added to individual bindings, they are added to the interaction profile with the binding button on the right. I need to figure out if I can add additional filters to the standard popup. I didn't want to create two separate base classes but maybe i need to go down that route.

I think even just having a more user-friendly error message would be OK. Right now, I'm getting a very programmer-y error and the modifier just fails to be added.

Maybe we just need a popup with an explanation?

I decided to go down the subclass path after all. So there is now an OpenXRIPBindingModifier class for binding modifiers that are recorded on interaction profile level, and a OpenXRActionBindingModifier class for binding modifiers that are recorded on individual actions/bindings.

Right now it seems a little overkill but once more binding modifiers become available it will start to make a whole lot more sense I recon.

BastiaanOlij avatar Dec 08 '24 08:12 BastiaanOlij

@dsnopek Thanks for checking it again though, I do think it's time we merge this and any further things we find we can deal with later. It's a big PR but for people who don't use binding modifiers it's fairly low risk.

@AThousandShips dang :) That's a lot of cleanup of existing code. Worth doing I guess, I'll go through it later today :)

BastiaanOlij avatar Dec 11 '24 20:12 BastiaanOlij

Limited it largely to new code 😅

AThousandShips avatar Dec 12 '24 10:12 AThousandShips

Thanks!

akien-mga avatar Dec 12 '24 13:12 akien-mga