Add GDExtension support for OpenXR extension wrappers

[konczg]

This commit adds the classes OpenXRExtensionWrapperExtension and OpenXRAPIExtension that can be used in GDExtensions to define OpenXR extension wrappers. It modifies extension wrapper registration so that they can be registered before OpenXRAPI instantiation (e.g. in core level initialization of GDExtensions).

As OpenXRAPI requires all extension wrappers to be registered upon initialization, the construction order of OpenXRAPI and the extension wrappers has been swapped, and extension wrappers now do not acquire the OpenXRAPI instance as a constructor parameter, but it is passed to them upon OpenXRAPI initialization through the OpenXRExtensionWrapper::initialize() method. This - along with making registered_extension_wrappers a static member of OpenXRAPI - allows us to register extension wrappers before OpenXRAPI construction.

Regarding the timing of extension wrapper registration, in the current design this needs to happen in core level extension initialization. The reason for this is that OpenXRAPI is currently instantiated and initialized at server level module initialization, which precedes server level extension initialization, so unless the Godot core is extended with an additional initialization level for OpenXR extension wrappers, they need to be initialized at core level.

OpenXR types are mapped to Godot's type system in the following way: XR handles (e.g. XrSession) are mapped to uint64_t. The same is true for enums such as XrResult. For structs such as XrEventDataBuffer, these are passed as pointers with GDNativePtr/GDNativeConstPtr instances. Currently there is an issue with the binding generator that results in a build error when GDNativePtr/GDNativeConstPtr is used as return type, so for these cases (e.g. get_instance_proc_addr) the current workaround is to pass the pointer as a 64-bit unsigned integer.

An issue that came up with GDExtensions in Godot 4.x is that library resources do not get properly globalized on Android: GDExtension libraries can only be specified with resource paths, and the libraries get properly placed in the lib folder of the apk, but when loading the library, Godot tries to find it relative to the given resource path. To solve this issue, I modified ProjectSettings::globalize_path() so that on Android, libraries are loaded without their path specified. This fixes the issue of loading GDExtension libraries, although this might be considered a workaround, and further discussion might be needed on better ways to solve this issue.

+#ifdef ANDROID_ENABLED
+               if (p_path.ends_with(".so")) {
+                       return p_path.get_file();
+               }
+#endif

The PR also adds a new "xr_features/use_anchor_api" export setting, which is required for the implementation of extension wrappers for FB spatial anchors and scene capture.

Developed by Migeran

[m4gr3d]

Instead of making this change (which could have further unintended ramifications), let's update https://github.com/godotengine/godot/blob/master/editor/plugins/gdextension_export_plugin.h#L76 so that on Android (and iOS?), it's not restricted to the res:// directory.

[BastiaanOlij]

@konczg and a reminder to have a look at this.

[konczg]

@m4gr3d @BastiaanOlij @kisg A possible issue with this approach could be that the exporting of these libraries is based on the paths specified in the gdextension configuration, which is the same path used for loading the library, so we cannot specify it as a path relative to the apk's lib folder, or else the exporting will be unsuccessful.

I modified the initial approach to resolve the library path in the GDExtensionResourceLoader to eliminate such unintended ramifications: https://github.com/godotengine/godot/pull/68259/files#diff-990645d1851b240a11471d20d4fda50e90b5d72a333565a5036b54807720b7a2R540
Could you please take a look at it?

[m4gr3d]

@konczg The update looks good!

[m4gr3d]

Do we have any means of cleaning the extension wrappers?

[BastiaanOlij]

This needs to move into register_types.cpp/uninitialize_openxr_module IMHO

[BastiaanOlij]

@konczg just a reminder, this would be nice to add into the uninitialize_openxr_module function

[konczg]

This has been added with @BastiaanOlij 's changes: https://github.com/godotengine/godot/pull/68259/files#diff-29befd060761763bf49d5349defccbc36170ce85ce687d92743b312acc330cf4R191

[m4gr3d]

@BastiaanOlij We should also determine the criteria for an extension for 'graduating' from a GDExtension to being a core.
The simplest approach would be to follow the OpenXR process, i.e: when a vendor extension moves to the core OpenXR spec, so should its extension wrapper.

[BastiaanOlij]

@BastiaanOlij We should also determine the criteria for an extension for 'graduating' from a GDExtension to being a core. The simplest approach would be to follow the OpenXR process, i.e: when a vendor extension moves to the core OpenXR spec, so should its extension wrapper.

Agreed, if it's in the OpenXR official headers and it doesn't clash with an existing core API, it can be in Godot itself. If it requires APIs that still have an unclear IP, are not open sourced or require a vendors SDK, it should be an external.

[BastiaanOlij]

Finally had time to give this a proper look at. For the most part I fully agree with the changes done here, this seems like a sound approach.

The only thing I have serious questions about is OpenXRAPIExtension and how this is designed to work, I probably need to look at an actual extension implementation but at first glance, I think this isn't right.

There are a lot of things exposed that are already accessible by simply using OpenXRInterface that can be obtained by calling XRServer::find_interfaceor XRServer::get_primary_interface.
I also feel that some of these functions (like 'get_instance' and 'get_system_id') should be moved into OpenXRExtensionWrapperExtension and just added as public functions of this class. This class is already provided with a pointer to the OpenXRAPI class and it keeps things nicely contained instead of introducing a new class.

All the action stuff should be removed, this makes no sense, this is not how OpenXR is supposed to work. The actions and interaction profiles come from the action map the user define, it is the developer that determines which actions they want and how they want them setup, plugins should not mess with this.
The only two things that we may need to add here are:

• registering things into the metadata database (openxr_defs.h/cpp) which admittedly will need to be enhanced so plugins can register possible paths that can be used.
• allowing plugins to add entries into the default action map that is created, though I'm not sure about this one as it potentially pollutes the action map with unnecessary actions.

Simply put, a developer should be able to trash the entire action map and build their own, as detailed or empty as they want, in fact this is recommended. The default action map is only there to create an example and a base and I even wonder if we haven't already put too much in there.

Remember, the actions the developer creates are the actions that are exposed to any rebinding UI and they need to make sense to the end user. If there are suddenly half a dozen entries that plugins start adding that overlap or duplicate entries that actually apply to the game, that is not a good user experience.

For instance, the VR editor currently runs on 4 actions mapped to all available devices. It has none of the extra entries that do not apply to the editor. Possibly a few more actions will be added, but they will be named and mapped appropriately. I expect, and indeed will teach, developers to be as succinct in creating their action maps.

I'm fully aware that there currently aren't any rebinding UIs (which is causing a lot of confusion in developers and a lot of game engines coming up with creative ways to work around this shortcoming) and that on Quest there is far less need for rebinding as there is only one input device, but we've got a UI that allows us to do the right thing.

[BastiaanOlij]

Actually, thinking this through we have a bigger issue with the action map though the changes here around how we register extensions is going to be vital.

In the normal editor we never create an instance of the OpenXRAPI. We only do this when XR is enabled. This means we don't currently have a mechanism for registering ActionMap meta data with OpenXRDefs (which i plan to rename OpenXRActionMapMetaData which is far more fitting).

[konczg]

@BastiaanOlij @kisg The current strategy was to expose to OpenXRExtensionWrapperExtension all parts of OpenXRAPI that OpenXRExtensionWrapper has access to, which is also the reason for putting them in a distinct OpenXRAPIExtension class, as I did not want to pollute OpenXRExtensionWrapperExtension with this many additional methods. After looking at it more thoroughly, it is clear that most of these should not be accessed by extension wrappers. However, isn't the problem here that the same OpenXRAPI is accessible both by OpenXRInterface and the extension wrappers, so even for the core extensions there is no separation of the interfaces? I think we should define more clearly which methods should be accessed by OpenXRInterface, and which by the extension wrappers.
Anyways, I listed the methods that I think should be kept accessible by OpenXRExtensionWrapperExtensions, are there any methods that we might not require here?

uint64_t get_instance();
uint64_t get_system_id();
uint64_t get_session();

Transform3D transform_from_pose(GDNativeConstPtr<const void> p_pose);
bool xr_result(uint64_t result, String format, Array args = Array());

static bool openxr_is_enabled(bool p_check_run_in_editor = true);
uint64_t get_instance_proc_addr(String p_name);
String get_error_string(uint64_t result);
String get_swapchain_format_name(int64_t p_swapchain_format);
bool is_initialized();
bool is_running();
uint64_t get_play_space();
int64_t get_next_frame_time();
bool can_render();

[BastiaanOlij]

@BastiaanOlij @kisg The current strategy was to expose to OpenXRExtensionWrapperExtension all parts of OpenXRAPI that OpenXRExtensionWrapper has access to, which is also the reason for putting them in a distinct OpenXRAPIExtension class, as I did not want to pollute OpenXRExtensionWrapperExtension with this many additional methods. After looking at it more thoroughly, it is clear that most of these should not be accessed by extension wrappers. However, isn't the problem here that the same OpenXRAPI is accessible both by OpenXRInterface and the extension wrappers, so even for the core extensions there is no separation of the interfaces? I think we should define more clearly which methods should be accessed by OpenXRInterface, and which by the extension wrappers. Anyways, I listed the methods that I think should be kept accessible by OpenXRExtensionWrapperExtensions, are there any methods that we might not require here?

uint64_t get_instance();
uint64_t get_system_id();
uint64_t get_session();

Transform3D transform_from_pose(GDNativeConstPtr<const void> p_pose);
bool xr_result(uint64_t result, String format, Array args = Array());

static bool openxr_is_enabled(bool p_check_run_in_editor = true);
uint64_t get_instance_proc_addr(String p_name);
String get_error_string(uint64_t result);
String get_swapchain_format_name(int64_t p_swapchain_format);
bool is_initialized();
bool is_running();
uint64_t get_play_space();
int64_t get_next_frame_time();
bool can_render();

Ah that makes a lot more sense now that I understand to context.

The problem that we have is that the OpenXRAPI class contains a large part of the API to OpenXR, only some of it should be outward facing or accessible to extensions, most of it is consumed by OpenXRInterface only.

It's really where GDExtension fails a bit imho, for it to access stuff, we need to expose them to parts of the system where we don't really want it exposed.

That said, this makes a good argument for not introducing OpenXRAPIExtension but elevating OpenXRAPI to a proper Godot object type and then just being conservative with what we expose through _bind_methods.
That will probably have some consequences on how we instance and manage it, a singleton pattern might not be the best approach in this case, I'll have to think about that some more.

[konczg]

@BastiaanOlij @kisg

The only thing I have serious questions about is OpenXRAPIExtension and how this is designed to work, I probably need to look at an actual extension implementation but at first glance, I think this isn't right.

We've published an example of implementing OpenXR extension wrappers through GDExtensions, it demonstrates how a wrapper for XR_FB_scene_capture can be added as an addon:
https://github.com/migeran/godot_openxr_meta

[BastiaanOlij]

Sorry @konczg , took me awhile before I could look at this again. I think this is very close to what it should be. I'm still going back and forth whether we should just change OpenXRApi to a proper object but it makes a lot of sense to have the extension object to just expose those bits that are safe to expose. With it now cleaned up it's pretty straight forward.

@Faless if you haven't looked at this yet, can you have a look as well? Especially seeing konczg is still having issues with pointers.

Also needs a rebase :)

[BastiaanOlij]

Isn't this something we can simply have in the plugin? Assuming it is build as an AAR it's able to add to the manifest if I'm not mistaken, and the inclusion of the plugin would require this to be true.

[m4gr3d]

@konczg Take a look at #78958; in 4.2, we are expanding the capabilites of the android plugin to allow moving these type of configuration out of core.
You can see what the refactor looks like in GodotVR/godot_openxr_vendors#38

So you should move these configs, as well as the anchor gdextension implementation in the Godot Openxr loader plugin instead of including them in core.

[lyuma]

I'm concerned about the use of Multiple Inheritance with class OpenXRExtensionWrapperExtension : public Object, OpenXRExtensionWrapper given that OpenXRExtensionWrapper has member variables

one of the principles I've usually followed is to avoid MI in c++ unless all but the first parent have no members
In this case, OpenXRExtensionWrapper defines

        OpenXRAPI *openxr_api = nullptr;
        HashMap<String, bool *> request_extensions;

it's such a small amount of data that I think these belong in each of the child classes, and those should be the ones to implement the get_request_extensions() function and the constructor
That way, OpenXRExtensionWrapper can be a class with only virtual functions. perhaps get_request_extensions() can be declared pure virtual so implementers are forced to store the request_extensions

[BastiaanOlij]

Hmm weird @lyuma , I thought I had reacted to this but I guess I either didn't post my reaction or lost it somehow.

This approach kinda grew out of how things were in the plugin so I think we can clean this up nicely.

We can get rid of the openxr_api member variable all together. We can just use OpenXRAPI::get_singleton(). This also works nicer with when GDExtension is used as the wrapper is then used.

I think we can also get rid of request_extensions. We can simply implement HashMap<String, bool *> get_request_extensions() on each extension and return the map, there is no need to keep this as a member variable.

I'm on the fence about whether we should stick with MI, I don't really see a point to make all the internal wrapper objects proper Godot objects because it just adds a bunch of objects not intended for end users to pickup. I guess we can only register the virtual classes and GDExtension wrapper class, but just using MI on the GDExtension wrapper seems fine to me.

[BastiaanOlij]

@konczg note that I've put some overlapping changes into #70694 including applying the changes requested by @lyuma .
Once merged you can add your two GDExtension classes on top of that work.

[konczg]

@BastiaanOlij @m4gr3d @kisg I've rebased this to master.

[BastiaanOlij]

The team wants to do a 4.1.1 patch at the end of the week before we start merging, hopefully soon after this can be one of the first PRs merged for 4.2-dev :)

[YuriSizov]

@konczg Could you please squash your commits into one? Make sure that the final commit has a short but descriptive message (the title of this PR is a good option). See this documentation, if you need help with squashing.

As mentioned during the review, it would be great to add at least some descriptions to the newly created documentation. At the very least we need class descriptions, but it would also be amazing if you could fill in method and property description as well.

[YuriSizov]

@konczg I'm not sure if this was done on purpose or not, but currently your commit is attributed to Bastiaan, while you are only tagged as a committer. You should probably amend the author information, or at least add yourself as a co-author using this construct at the end of your commit message body

Co-authored-by: Your Name <your_email>

[konczg]

@YuriSizov Sorry, this was done by accident, I've fixed the commit.

[m4gr3d]

Thanks for the quick update!

Can you clean up the anchor options as mentioned in the comments.

[m4gr3d]

@konczg Can you move these configs to the Godot OpenXR loader plugin as mentioned in https://github.com/godotengine/godot/pull/68259/files#r1265943701

[m4gr3d]

Thanks for the update! Let me know if you need assistance with updating the Godot OpenXR loader plugin with the anchor configs.

[m4gr3d]

Looks great!

[BastiaanOlij]

@YuriSizov is there any input the production team would like to give here? It would be nice to get this merged :)

[YuriSizov]

is there any input the production team would like to give here?

Not sure yet, but we have it marked for dev2, so the aim would be to finalize and merge it before that. 👍

[YuriSizov]

Some code style changes and documentation clarifications are required (as well as a few formatting and grammar issues).

[YuriSizov]

@konczg Thanks for the updates! There were a few more notes that you've probably missed because GitHub collapsed them. :) By the way, you don't need to reply to every one of them, you can just hit the "Resolve conversation" button to mark it as resolved.

Regarding OpenXRAPI, if it's not some object that we can refer users to, then we should probably just write it as plain text, "OpenXR API", with no formatting.

[YuriSizov]

I think docs look good now, thanks!

[YuriSizov]

Thanks! This is amazing work!