Various improvements to OpenXR extension wrappers

[BastiaanOlij]

This PR makes a few changes to the OpenXR extension wrapper. Some of these changes replace some of the work that @konczg did in #68259

• OpenXRExtensionWrapper no longer keeps a pointer to openxr_api, instead this is now accessed through OpenXRApi::get_singleton()
• OpenXRExtensionWrapper no longer keeps an array of requested extensions, instead get_request_extensions is now implemented on each wrapper to return this array.
• Requested extension wrappers in OpenXRAPI are now static so they can be registered even if OpenXRAPI isn't instantiated. This is required to make the extension wrappers accessible for editor purposes.
• Extension wrappers can now register meta data for supported controllers.
• All controllers handled by OpenXR extensions are now implemented as extension wrappers.

One additional change here that I'm thinking of doing but will do as a follow up PR is to also have extension wrappers create their part of the default action set. I'm delaying this simply because we have an ongoing discussion about the content of the default action set.

[BastiaanOlij]

cc @rsjtdrjgfuzkfg

You may wish to give your opinion on the controller changes. The idea here is to also redo your PR and move the logic into an openxr_pico_extension.h/cpp extension wrapper.

Here we can cheat and simply add a:

#ifndef XR_PICO_CONTROLLER_INTERACTION_EXTENSION_NAME
#define XR_PICO_CONTROLLER_INTERACTION_EXTENSION_NAME "XR_PICO_controller_interaction"
#endif

In the cpp file for now. We just need to find out what extension naming convention PICO is now following.

[rsjtdrjgfuzkfg]

@BastiaanOlij here are some comments on details that I would change / look into, but I think the overall plan seems solid and flexible.

Assuming Pico will at some point release an extension it will be easy to update the PR in the same style. Even if Pico ends up doing something else, we can still use the wrapper and just replace the mechanism for availability checks.

[rsjtdrjgfuzkfg]

I don't think you want to handle the singleton not existing defensively (affects multiple places). I'd remove the checks.

Otherwise: The macros below call get_singleton again, so they could get a different pointer. If you want to ensure the pointer is not null, store the result in a variable on the stack (could use the old name openxr_api) and use that with the macro? In addition, you should also add null checks to all other places that use get_singleton (which are a lot).

[BastiaanOlij]

In this case the checks are important though admittedly we're nitpicking here. OpenXRAPI will only exist when OpenXR is enabled and when we're in runtime. In the editor OpenXRAPI (unless the VR editor is enabled) won't be instantiated and can thus be NULL. But you are right, so far it's a bit hit and miss where we do and don't check it, and the chance of someone actually implementing things wrong are near zero because we don't instantiate the UI parts if OpenXR is turned off.

Still it's better to be safe and sorry with public APIS.

As for storing the pointer in a variable, I always use to do that but I'm on the fence on that one atm. This did remind me I needed to change the singleton call to be inlined, but even if not modern compilers will pick up on the function just returning a static value and optimizing it to just directly refer to that static variable.

I do admit things get a little more verbose this way...

[rsjtdrjgfuzkfg]

Imho defensive programming should always remain stable, even if things outside of the method change. get_singleton might theoretically change, so I'd argue it is better to not rely on compiler optimizations caused by its inline implementation.

But I do agree we're nitpicking, and it is not terribly important either way. It just felt to me as if you search/replaced openxr_api with OpenXRAPI::get_singleton() and the checks were unintentionally left over.

[BastiaanOlij]

Totally agree with you there, but I think that ship sailed a long time ago. Godot has many places where singletons are used in this manner.

And like I said, I'm on the fence about some of this. There really is no performance to gain here so code readability is an important factor too, and there really isn't much reason to think this code would ever be unintentionally called. I might still change it :)

[rsjtdrjgfuzkfg]

I did not see any places using the various non-virtual availability check methods (not only here). Are they left over?

[BastiaanOlij]

They are more there out of principle, there are various extensions that do require us to call is_available, I've added them out of habit mostly.

[rsjtdrjgfuzkfg]

The contract of on_register_metadata is not clear to me. The methods seem to register everything provided by the extension unconditionally (even if the extension is not available), but given the way it is called here (also unconditionally) it seems that it then always registers the metadata?

Imho it would be best to keep the call here unconditional, but specify that on_register_metadata only registers metadata for available extensions, not for all of them. That also simplifies the overall interface of the wrapper, as there is no need to ever check anything outside of the wrapper (so the availability checks that seem unused can be safely removed).

[BastiaanOlij]

Ok so this is pretty important. The metadata is always needed. Remember, I do not have a PICO, yet I may still want to add PICO bindings to my action map for people who want to play my game on a PICO.

So the action map editor needs the meta data regardless of whether the computer the developer is on supports each device. So we always register the metadata even if the extension is not available.

We also use the metadata to process the actionmap. Originally I only had logic that would filter out paths for a disabled extension. The problem here is that any path with a typo in it would be seen as valid. Many XR runtimes actually crash if invalid paths are supplied to it.
Having a complete set of meta data allows us to validate paths and filter out invalid ones and alert the user properly, while checking valid paths against enabled extensions and filter them out if an extension is not enabled.

[rsjtdrjgfuzkfg]

You're right, of course. I was confused as I thought this PR is also supposed to move the filtering into the extension wrapper, but of course your change does not touch the old filters that are already in place. Sorry for my confusion!

I still think that might be a good idea for more flexibility (let each extension wrapper decide whether or not their actions should be passed on to OpenXR), but that is for a different PR.

[BastiaanOlij]

Well like I said, I kinda had it working like that originally and just found things became unstable because the items will be in the users action map. We loose the ability to know whether a path is invalid, or whether an extension filters out the path because it's not enabled. Though I guess that could be solved in other ways as well.

[rsjtdrjgfuzkfg]

Couldn't we directly pass an available or enabled parameter to register_interaction_profile and similar methods instead of an extension name?

If registration of metadata happens before the extensions are checked, that parameter would be a boolean reference, else a plain boolean. All filtering logic can then happen in the extension wrapper.

(But as I said, this is not necessary for this PR)

[BastiaanOlij]

Indeed, we're registering everything before the extensions are checked, in fact the extensions are only checked in runtime (or we're in the VR editor).

Pushing pointers around is a problem as well, actually get_requested_extensions is a problem here as well as we take a boolean pointer here. This was fine in the Godot 3 plugin but poses a problem in 4 when implementing things in GDExtensions.
In the Vulkan drivers I've applied the same registration approach but we check if an extension was enabled by it's name.
I may do the same here.

The way Khronos nowadays handles things, a lot of stuff is checked by name to make it as flexible as possible and as performance isn't really a consideration here.

That said, I get your point that we may want more flexibility in checking if a path is usable, we may at some point want to have the ability to set a callable function or something. But it all falls very much outside of the scope of this PR so as you say, food for thought for a follow up PR.

[BastiaanOlij]

Assuming Pico will at some point release an extension it will be easy to update the PR in the same style. Even if Pico ends up doing something else, we can still use the wrapper and just replace the mechanism for availability checks.

The important bit is getting the extension name right, we need to make sure we use the name that PICO actually currently uses, else the entries for PICO will be filtered out of the action map when submitted to OpenXR as we verify the extension exists and is enabled.

[rsjtdrjgfuzkfg]

Regarding the Pico extension, I did not see any documentation officially stating it, but XR_PICO_controller_interaction is likely correct (judging from extension listings I get from the Pico hardware with all SDK versions I used so far).

[BastiaanOlij]

If PICO already has the update in their firmware it's a matter of just running Godot with --verbose so it prints out all the extensions available on the platform.

[rsjtdrjgfuzkfg]

@BastiaanOlij I did check that already, and for both firmware and SDK versions I ever used XR_PICO_controller_interaction was present. So I think that is correct for the pico paths we currently use and thus sufficient for my PR, even though there will be a new extension and new paths in the future.

No idea how we handle merging though. Given that the paths that work now will likely never become official part of the OpenXR spec, it seems reasonable to keep my PR as draft for the next months. But on the other hand, that would likely mean that everyone interested in Pico devices will need to build from source and apply my PR. So merging with a clear "experimental" label (visible from within the engine when adding the controller to the action map) might be reasonable.

[m4gr3d]

@BastiaanOlij These improvements look great, but they bring up the topic of how much support we're planning to provide to the Godot 3 branch and its OpenXR plugin.

If we're planning to fully support it until Godot 4 XR support is stable, then these improvements need to be backported to Godot 3 in order to simplify the act of adding extensions / capabilities to both Godot 3 & Godot 4.
In that scenario, it'll also be useful to discuss on the set of XR features we plan to add throughout the year so we can ensure there is consistent support and functionality between Godot 3 & Godot 4.

[BastiaanOlij]

@BastiaanOlij These improvements look great, but they bring up the topic of how much support we're planning to provide to the Godot 3 branch and its OpenXR plugin.

Well most of these changes don't impact Godot 3 as this is mostly about cleaning up the wrapper structure to fit the fact that the classes are now in the core and need to follow certain guidelines.
Structurally the wrappers still work the same.

As for what is implemented in them I think we can divide things into two groups:

1. fixes that can be back ported should be back ported.
2. new features should be evaluated at a case by case basis, some things aren't worth the effort imho especially considering we have limited resources. I myself rather spend my time getting Godot 4 to a point where it can replace Godot 3 which it currently isn't (though with the OpenGL driver in Godot 4 its potentially already better then Godot 3 with GLES2, it's on the Vulkan side where most of the issues lie).

One case in point for me is back porting PICO and Khronos loader support into Godot 3. Something that is highly requested but which has a number of roadblocks that aren't worth our time if Godot 4 suffices.

[BastiaanOlij]

Ok all changes have been done. I also renamed get_request_extensions to get_requested_extensions, just seemed better spelling wise but still not sure if it fits the best. Anyway it's really nitpicking at this point.

I think we're pretty much ready to merge this one.

[m4gr3d]

Structurally the wrappers still work the same.

At the moment they do, but if we don't have a policy of backporting architectural changes, then we'll quickly reach a point where the improvements accumulate to the point they don't work the same.

That by itself is fine so long as we define our policy going forward to be as mentioned:

1. fixes that can be back ported should be back ported.
2. new features should be evaluated at a case by case basis, some things aren't worth the effort imho especially considering we have limited resources. I myself rather spend my time getting Godot 4 to a point where it can replace Godot 3 which it currently isn't (though with the OpenGL driver in Godot 4 its potentially already better then Godot 3 with GLES2, it's on the Vulkan side where most of the issues lie).

I think that 2 can be scratched out because all new features will require medium to large effort. E.g:

• Implementing the OpenXR apis for the feature
• Designing and implementing the Godot wrappers api to expose the functionality
• Updating the samples to demo the functionality
• Updating the documentation to document the functionality
• Api iteration / bug fixes once released to public

[BastiaanOlij]

@m4gr3d agree on number 2, I can't think of any recent new features that are worth back porting. To be exact, I did one or two and never merged them because they are breaking changes and feel it's just not worth it.

In the end any back porting of bug fixes or small features that are possible will require extra work, the difference in how classes are organized in GDNative v.s. in core and all the renames of nodes in Godot 4 already make that a mostly "do it a second time" type of thing.

So to be honest, I'm not too concerned here.

[akien-mga]

Thanks!