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: change bindings to 'flatten' source paths #98163

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

BastiaanOlij
Copy link
Contributor

Our OpenXR action map currently stores multiple input/output paths (now called source paths) per action in an interaction profile.

This causes an issue with upcoming binding modifiers (see #97140).

This PR will "flatten" this structure so one binding records a single binding between an action and a source path.
If multiple paths should be bound, multiple binding entries are needed.

This is technically a breaking change in that the action map will be altered in such a way that it can't be loaded by older versions of Godot, but it is required to implement future modifiers.

Contributed by Khronos Group through the Godot Integration Project

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.

Overall, this restructuring of the action map data makes sense to me!

This is also a breaking change with regard to code that programmatically creates an action map with multiple paths per binding. However, I think it's pretty rare to programmatically create an action map, and even so, it would only affect bindings with multiple paths, which even further reduces the affected code. I personally think this is acceptable.

In 99.9% of cases, developers will create action maps in the editor, and this should correctly load and convert existing action maps (although, I haven't tested it - only reviewed the code).

TL;DR LGTM :-)

Only a handful of minor notes.

modules/openxr/action_map/openxr_interaction_profile.h Outdated Show resolved Hide resolved
modules/openxr/action_map/openxr_interaction_profile.h 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
@BastiaanOlij
Copy link
Contributor Author

I'm also renaming source_path to binding_path after an internal discussion that source path misleads developers in thinking the bindings are absolute. This is a common misperception around the action map where bindings are suggestions that the XR runtime can override either due to user preferences or due to other types of hardware being used.

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.

Overall, this looks great to me!

I personally think we could go a little further with removing the code for old action maps in deprecated=no builds, but that shouldn't necessarily hold this up.

@BastiaanOlij
Copy link
Contributor Author

Everything should be cleared up now, should be ready to be merged.

@Repiteo Repiteo merged commit 8b6c7bf into godotengine:master Oct 24, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Oct 24, 2024

Thanks!

@BastiaanOlij BastiaanOlij deleted the openxr_flatten_bindings branch October 27, 2024 23:23
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