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

@BastiaanOlij BastiaanOlij added this to the 4.4 milestone Oct 14, 2024
@BastiaanOlij BastiaanOlij force-pushed the openxr_flatten_bindings branch from 214a59c to df53941 Compare October 14, 2024 06:57
@BastiaanOlij BastiaanOlij marked this pull request as ready for review October 14, 2024 06:57
@BastiaanOlij BastiaanOlij requested review from a team as code owners October 14, 2024 06:57
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.

@BastiaanOlij BastiaanOlij force-pushed the openxr_flatten_bindings branch from df53941 to f84bbb3 Compare October 15, 2024 23:05
@BastiaanOlij BastiaanOlij requested review from AThousandShips, dsnopek and a team October 15, 2024 23:06
@BastiaanOlij BastiaanOlij force-pushed the openxr_flatten_bindings branch from f84bbb3 to b3f53db Compare October 15, 2024 23:28
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 BastiaanOlij force-pushed the openxr_flatten_bindings branch from b3f53db to 2af0e58 Compare October 21, 2024 00:12
@BastiaanOlij
Copy link
Contributor Author

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

@BastiaanOlij BastiaanOlij force-pushed the openxr_flatten_bindings branch from 2af0e58 to 3e36f52 Compare October 21, 2024 23:31
@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.

6 participants