Wanted to take a quick look through this. Looks cool and makes sense to me! Would be curious to try this out with a relevant API sometime but in the spec it looks like it's only used by Magic Leap currently?

It's used by a number of APIs still in development, but it will be some time before those are public.

As part of the change to have OpenXRExtensionWrapper be an Object, there's a couple other changes that I personally think we should make:

1. Merge the methods from OpenXRCompositionLayerProvider up into OpenXRExtensionWrapper. While using OpenXRCompositionLayerProvider like a C#/Java/PHP/etc interface makes sense, multiple inheritance in C++ can cause weird issues especially in conjunction with Godot's Object and ClassDB system. I forget the issue and PR numbers, but in the past we had a really hard to debug crash that was the result of this, and I personally think it would be better to avoid the problem entirely by just not using multiple inheritance. (Also, we had some later work that attempted to follow the example of OpenXRCompositionLayerProvider and my review was "let's not do that anymore" because of these issues, but I worry that future contributors may still see OpenXRCompositionLayerProvider and again try to follow its example, so let's just get it out of there)
2. Deprecated OpenXRExtensionWrapperExtension in favor of just using the now-exposed OpenXRExtensionWrapper. We can do this without breaking compatibility by just making the default implementations of the virtual methods on OpenXRExtensionWrapper use the GDExtension implementations from OpenXRExtensionWrapperExtension, and emptying out OpenXRExtensionWrapperExtension but not removing it. Old GDExtensions would still extend OpenXRExtensionWrapperExtension and continue working fine, but newer GDExtensions could directly extend OpenXRExtensionWrapper. This will eliminate the annoying extension-extension we have in OpenXRExtensionWrapperExtension :-)

Maybe it makes sense to spin the "OpenXRExtensionWrapper becomes an Object" part of this into its own PR to focus on that? Or, if we think this one is ready to merge when the Godot 4.5 dev cycle opens up, I could do the other changes I describe above as a follow-up PR?

UPDATE: The crash related to multiple inheritance that I was referring to in point nr 1 was issue #88613 fixed by PR #88689

@dsnopek the crash issues were caused by a moving pointer between the different classes because of late introduction of a v-table.
This is fixed by making sure the primary base class has a virtual destructor. We dealt with that.

My main worry with point 1 is that we'll end up looping through all wrappers just to find out only one or two actually have compositor logic.

For point 2, it's definitely worth doing a follow up to clean up OpenXRExtensionWrapperExtension however it has become the norm to always implement a *Extension class for exposure to GDExtension and keep the base class abstract.

My main worry with point 1 is that we'll end up looping through all wrappers just to find out only one or two actually have compositor logic.

I'm not saying to get rid of the registration for the extensions that affect compositor layers. Just that it would take OpenXRExtensionWrapper * rather than OpenXRCompositionLayerProvider *. Take a look at register_projection_views_extension() - for that one we're also doing a registration, but we didn't make a new base class for it.

however it has become the norm to always implement a *Extension class for exposure to GDExtension and keep the base class abstract.

We do it both ways for different things in Godot.

The only advantage of keeping it abstract (as opposed to virtual) is that users could try to extend OpenXRExtensionWrapper in GDScript, which won't work because of all the pointer arguments. But we already have the risk, because they could try to do the same with OpenXRExtensionWrapperExtension, and I've never heard of anyone trying to do that. It's really only changing which class name carries the risk.

Ok, I've made this ready for proper review. Been testing this for the past few weeks and it is working fine. Sadly can't share the project I'm working on just yet.

I did change things so it properly uses XrFutureEXT internally and the class methods used by extension in the core can use this type, but the functions exposed to GDExtension and GDScript use uint64_t. Much clearer implementation.

@dsnopek I've left the classes as they are for now, I do think we need to look into this further but changing those would be serious breaking changes, especially the *extension change. I do hear what you're saying here though so far in all my discussions with the core team I get two answers, some always wanting the specific extension classes and others saying its find to just make the base class virtual. I'm not bothered one way or the other.

But changing it now will break our existing vendor extension and I think that's not worth it. Also it will require a bunch of other cleanup that I'd rather do in a separate PR if we decide to go down this route.

@BastiaanOlij:

I do think we need to look into this further but changing those would be serious breaking changes, especially the *extension change.
[...]
But changing it now will break our existing vendor extension and I think that's not worth it.

I'm happy to make a follow-up PR! However, the changes I described above would not be breaking changes - they should be fully backwards compatible. The vendor extension should be able to keep working without any changes

@dsnopek I still have to test it but have a look at the future result implementation I did and see if this matches your expectation.
I did it as a separate commit so you can rebase your changes ontop of it.

Squashed the commits into one. I think this is ok to merge.

Thanks!