Skip to content

OpenXR: Clean-up OpenXRExtensionWrapper by removing multiple inheritance and deprecating OpenXRExtensionWrapperExtension#104087

Merged
akien-mga merged 1 commit intogodotengine:masterfrom
dsnopek:death-to-multiple-inheritance-openxr-edition
Mar 21, 2025
Merged

OpenXR: Clean-up OpenXRExtensionWrapper by removing multiple inheritance and deprecating OpenXRExtensionWrapperExtension#104087
akien-mga merged 1 commit intogodotengine:masterfrom
dsnopek:death-to-multiple-inheritance-openxr-edition

Conversation

@dsnopek
Copy link
Copy Markdown
Contributor

@dsnopek dsnopek commented Mar 13, 2025

This builds on PR #101951 so marking as DRAFT until that one is merged

Refactors as I described in a comment on that PR:

  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. In the past we had a really hard to debug crash (see Crash on exit when using OpenXRExtensionWrapperExtension #88613) 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 :-)

NOTE: This is 100% backward compatible! I tested it with the "Meta Composition Layers XR Sample" from the asset library, which includes the binaries for version 3.1.2 of the godot_openxr_vendors extension.

(Although, CI is going to complain about missing methods, but they moved up to a parent class which is fine - I'll copy the messages into the ignore file after CI gives them to me :-))

@dsnopek dsnopek added this to the 4.x milestone Mar 13, 2025
@dsnopek dsnopek requested review from a team as code owners March 13, 2025 21:45
@dsnopek dsnopek marked this pull request as draft March 13, 2025 21:48
@dsnopek dsnopek force-pushed the death-to-multiple-inheritance-openxr-edition branch 5 times, most recently from 76917d0 to fc6dbf3 Compare March 14, 2025 00:29
@dsnopek dsnopek force-pushed the death-to-multiple-inheritance-openxr-edition branch from fc6dbf3 to 58b54d2 Compare March 14, 2025 19:06
@dsnopek dsnopek force-pushed the death-to-multiple-inheritance-openxr-edition branch 4 times, most recently from 7dcf56f to ac12b80 Compare March 19, 2025 13:18
@dsnopek dsnopek changed the title [DRAFT] OpenXR: Clean-up OpenXRExtensionWrapper by removing multiple inheritance and deprecating OpenXRExtensionWrapperExtension OpenXR: Clean-up OpenXRExtensionWrapper by removing multiple inheritance and deprecating OpenXRExtensionWrapperExtension Mar 19, 2025
@dsnopek dsnopek marked this pull request as ready for review March 19, 2025 13:34
@dsnopek dsnopek requested review from a team as code owners March 19, 2025 13:34
@dsnopek
Copy link
Copy Markdown
Contributor Author

dsnopek commented Mar 19, 2025

Now that all the dependent PRs have been merged, this one is ready for review!

Copy link
Copy Markdown
Contributor

@BastiaanOlij BastiaanOlij left a comment

Choose a reason for hiding this comment

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

Really nice cleaning this up, it makes me wonder if I've overused *Extension classes in a few other places.

My only nitpick is that I think we need some #ifndef DISABLE_DEPRECATED checks around some of the deprecated classes.
This will mean that people who compile without deprecated classes and functions must compile a new vendor plugin but that is acceptable.

@dsnopek
Copy link
Copy Markdown
Contributor Author

dsnopek commented Mar 20, 2025

@BastiaanOlij Thanks for the review!

My only nitpick is that I think we need some #ifndef DISABLE_DEPRECATED checks around some of the deprecated classes.

This PR already wraps OpenXRExtensionWrapperExtension (the one class this PR deprecates) in #ifndef DISABLE_DEPRECATED.

…tance and deprecating `OpenXRExtensionWrapperExtension`
@dsnopek dsnopek force-pushed the death-to-multiple-inheritance-openxr-edition branch from ac12b80 to 9d3c950 Compare March 20, 2025 12:20
@dsnopek
Copy link
Copy Markdown
Contributor Author

dsnopek commented Mar 20, 2025

This PR already wraps OpenXRExtensionWrapperExtension (the one class this PR deprecates) in #ifndef DISABLE_DEPRECATED.

Actually, it looks like I missed a couple spots - sorry about that! I've verified that it's not present in the extension_api.json from a scons deprecated=no build with my latest push

Copy link
Copy Markdown
Contributor

@devloglogan devloglogan left a comment

Choose a reason for hiding this comment

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

LGTM, tested with some of the XR samples and no issues arose. :)

Copy link
Copy Markdown
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.

Looks good to me, happy to see multiple inheritance go away 😬

@akien-mga akien-mga merged commit 2715017 into godotengine:master Mar 21, 2025
20 checks passed
@akien-mga
Copy link
Copy Markdown
Member

Thanks!

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.

5 participants