OpenXR: Clean-up OpenXRExtensionWrapper by removing multiple inheritance and deprecating OpenXRExtensionWrapperExtension#104087
Conversation
76917d0 to
fc6dbf3
Compare
fc6dbf3 to
58b54d2
Compare
7dcf56f to
ac12b80
Compare
OpenXRExtensionWrapper by removing multiple inheritance and deprecating OpenXRExtensionWrapperExtensionOpenXRExtensionWrapper by removing multiple inheritance and deprecating OpenXRExtensionWrapperExtension
|
Now that all the dependent PRs have been merged, this one is ready for review! |
BastiaanOlij
left a comment
There was a problem hiding this comment.
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.
|
@BastiaanOlij Thanks for the review!
This PR already wraps |
…tance and deprecating `OpenXRExtensionWrapperExtension`
ac12b80 to
9d3c950
Compare
Actually, it looks like I missed a couple spots - sorry about that! I've verified that it's not present in the |
devloglogan
left a comment
There was a problem hiding this comment.
LGTM, tested with some of the XR samples and no issues arose. :)
akien-mga
left a comment
There was a problem hiding this comment.
Looks good to me, happy to see multiple inheritance go away 😬
|
Thanks! |
This builds on PR #101951 so marking as DRAFT until that one is mergedRefactors as I described in a comment on that PR:
OpenXRCompositionLayerProviderup intoOpenXRExtensionWrapper. While usingOpenXRCompositionLayerProviderlike a C#/Java/PHP/etcinterfacemakes sense, multiple inheritance in C++ can cause weird issues especially in conjunction with Godot'sObjectandClassDBsystem. 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 ofOpenXRCompositionLayerProviderand my review was "let's not do that anymore" because of these issues, but I worry that future contributors may still seeOpenXRCompositionLayerProviderand again try to follow its example, so let's just get it out of there)OpenXRExtensionWrapperExtensionin favor of just using the now-exposedOpenXRExtensionWrapper. We can do this without breaking compatibility by just making the default implementations of the virtual methods onOpenXRExtensionWrapperuse the GDExtension implementations fromOpenXRExtensionWrapperExtension, and emptying outOpenXRExtensionWrapperExtensionbut not removing it. Old GDExtensions would still extendOpenXRExtensionWrapperExtensionand continue working fine, but newer GDExtensions could directly extendOpenXRExtensionWrapper. This will eliminate the annoying extension-extension we have inOpenXRExtensionWrapperExtension:-)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 :-))