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

Various fixes for OpenXR action map meta data and editing #68528

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Nov 11, 2022

This PR makes a number of changes around the OpenXR action map

Earlier creation of the default action map (done)

If a VR project did not yet have an action map we didn't create it until the project was first run. This causes issues in visibility and may result in a missing action map when deployed on mobile devices.

The action map is now created by the initialization of the action map editor, as this only runs if OpenXR is enabled for a project, it seemed like a good place.

Marking the resource as edited (done)

The editor has been enhanced so all actions create undo/redo entries and mark the resources as edited.

Should solve #68041

Make Action map meta data extendable (done)

The class OpenXRDefs contains meta data that details out all possible paths we can use for building our action map. OpenXR does not have a mechanism for enumerating this data.

As extensions are now being added that introduce additional paths, having this data be static is no longer enough.

To better define what this class does, I've renamed it to OpenXRInteractionProfileMetaData, turned it into a singleton and made it possible to register new paths. It is also registered with ClassDB so it can be accessed for GDExternals.

Use action map meta data for filtering (done)

Action maps need to be as complete as possible to allow games to be deployed to multiple devices, but not all devices will support the extensions required for certain entries in the action map. For this we introduced OpenXRExtensionWrapper::is_path_supported.

By completing the action map meta data and added extension information to this we can use that data to filter out unsupported entries. This will also allow us to generate warnings/errors when there are misspelled entries in the action map.

Currently OpenXR runtimes have a tendency to crash when unsupported paths are supplied.

Move registering of extension paths into extensions (future)

Note that currently extensions are only registered when OpenXRAPI is instantiated, and this class is not guaranteed to be in the editor. We need #68259 to be merged, but there is a mutual dependency here.
This PR should be merged first, then 68259, and then we can do an additional PR to clean this up.

Add ability for extensions to add entries to the default action map (future)

Again this will be dependent on the changes in 68259 to be merged so this will be in a followup PR, however currently we add default entries to our default action map including entries related to extensions.

It would be nicer if extensions can add their default entries but we need the extensions registered regardless of whether OpenXRAPI is instantiated.

This will be important for instance in our PICO extension so default PICO controllers can be setup, see #68023

@BastiaanOlij BastiaanOlij added this to the 4.0 milestone Nov 11, 2022
@BastiaanOlij BastiaanOlij self-assigned this Nov 11, 2022
@BastiaanOlij
Copy link
Contributor Author

One question I have is whether OpenXRActionMapMetaData is the right name, it is meta data to construct our action map with, but it really is meta data describing our interaction profiles. This info says nothing about the actions a user may or may not add to their action map. I'm contemplating renaming it OpenXRIPMetaData

@BastiaanOlij
Copy link
Contributor Author

@akien-mga @Calinou do you guys have any idea about how to solve the resource marked as edit issue? I can't find anything good about how we track this.

@BastiaanOlij BastiaanOlij force-pushed the openxr_actionmap_changes branch 3 times, most recently from 628f2a3 to 44d58bd Compare November 18, 2022 09:49
@BastiaanOlij
Copy link
Contributor Author

I'm still looking for a solution to the save thing but the other bits are all done.

@BastiaanOlij
Copy link
Contributor Author

Currently working on implementing undo/redo logic for the action map editor together with properly flagging resources are edited. After that this PR can be reviewed.

@BastiaanOlij BastiaanOlij force-pushed the openxr_actionmap_changes branch 3 times, most recently from 6351249 to 70b0403 Compare November 25, 2022 08:44
@BastiaanOlij
Copy link
Contributor Author

Took way more time than I would have liked and I'm not 100% sure whether the approach I took was entirely right, it felt a bit round peg square hole implementing undo/redo and I might come back to this at some point as I get more experience with this system.

That said, I think this works good enough. Could defo use some feedback from the editor team.

@BastiaanOlij BastiaanOlij marked this pull request as ready for review November 25, 2022 09:03
@akien-mga akien-mga requested a review from a team November 25, 2022 09:16
@BastiaanOlij BastiaanOlij force-pushed the openxr_actionmap_changes branch from 70b0403 to 96bbdf7 Compare November 25, 2022 09:26
@BastiaanOlij BastiaanOlij mentioned this pull request Dec 6, 2022
24 tasks
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Not sure if the emit_changed() is the right way to update things, or if it should be notify_property_list_changed() or be expected to be picked up automatically. This has always been and remains confusing to me, we really need clear documentation on what to use and when.

But this shouldn't be blocking, let's merge.

@akien-mga akien-mga merged commit cd855f6 into godotengine:master Dec 13, 2022
@akien-mga
Copy link
Member

Thanks!

@KoBeWi
Copy link
Member

KoBeWi commented Dec 14, 2022

Not sure if the emit_changed() is the right way to update things, or if it should be notify_property_list_changed() or be expected to be picked up automatically.

emit_changed() should be emitted whenever a Resource changes. Right now it's not done everywhere to avoid bloat, but nothing is preventing it. However, I think it should only emit when the value actually changes, not always when calling setter.

This signal is supposed to notify other resources or editors, so they can react and update. Only the inspector updates automatically.

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.

3 participants