Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved extension checking logic #87

Merged
merged 1 commit into from
Sep 12, 2021

Conversation

BastiaanOlij
Copy link
Member

This changes the logic that checks and enabled extensions in our plugin.
It works with a map to which we add the extensions we want.
The entry then points to a boolean that the logic either sets or clears depending on whether the extension is available.
If this pointer is a nullptr we deem the extension as a requirement and it not being available will fail initialization of the plugin.
Finally we enabled all extensions that were requested and found.

The boolean flags allow further logic to actually implement the usage of the extension.

I've also moved most of the FB extensions out of the Android only sphere, I suspect a number of these will become extensions also implemented by other vendors as has happened with a number of Microsoft and Valve extensions that still bear their names (especially around refresh rates and such, really weird those aren't part of the core spec already).

@BastiaanOlij BastiaanOlij self-assigned this Sep 8, 2021
@BastiaanOlij BastiaanOlij added the enhancement New feature or request label Sep 8, 2021
@BastiaanOlij BastiaanOlij added this to the 1.1.0 milestone Sep 8, 2021
@BastiaanOlij
Copy link
Member Author

I also snuck in setting the view configuration type...

Copy link
Collaborator

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

The overall structure looks good! I've added some feedback on some implementation details.

src/ARVRInterface.cpp Outdated Show resolved Hide resolved
src/openxr/OpenXRApi.cpp Show resolved Hide resolved
src/openxr/OpenXRConfig.cpp Outdated Show resolved Hide resolved
src/openxr/OpenXRConfig.cpp Outdated Show resolved Hide resolved
}; break;
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we just return the XR view configuration types values as is? What's the advantage for defining another set of values that map to them especially since the plan is to include as much of OpenXR as possible in Godot 4.x?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed this comment which seems to make the case for it: so our dropdown in the UI works properly.

Seems fine by me if that's the only available option.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's just that for a enum dropdown in the UI you define the property like so:

	register_property<OpenXRConfig, int>("view_configuration", ... , GODOT_PROPERTY_HINT_ENUM, "Mono,Stereo");

So 0 = Mono, 1 = Stereo, etc.

Also at some point we will have to think of how we'll unify some of this in Godot 4. We'll likely end up with our own enumerations in the core of Godot for this that map to whatever systems and capabilities that are in place in the runtime. While I'd like to think everyone will switch to OpenXR and we don't have to worry about any other runtimes, I think that is wishful thinking.

@BastiaanOlij BastiaanOlij merged commit 2c003d6 into GodotVR:master Sep 12, 2021
@BastiaanOlij BastiaanOlij deleted the improve_enable_extensions branch September 12, 2021 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants