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

Make Settings for the individual extensions #234

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kimau
Copy link

@Kimau Kimau commented Dec 16, 2024

You often don't want to use all the extensions, so this adds project settings to control which extensions are enabled for the addon.

I've found it useful for my current game.

You often don't want to use all the extensions, so this adds project
settings to control which extensions are enabled for the addon.
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

At a high-level, this is a really great idea! I'll try to test this when I have a moment.

Which Godot versions have you tried it with? I suspect this won't work with Godot 4.3 and earlier, because the ProjectSettings singleton isn't available early enough. PR godotengine/godot#98862 was recently merged to Godot master (and I personally tested it with a use case like your PR :-)), so I think this may need to be Godot 4.4+

UPDATE: Yeah, the CI, which uses Godot 4.3, is spitting out this error message:

ERROR: Failed to retrieve non-existent singleton 'ProjectSettings'.
   at: (core/config/engine.cpp:294)

So, this change will be Godot 4.4+ - which is fine, but it's something we need to consider when merging it. The current master here is still compatible with Godot 4.3, but we'll need to switch to 4.4 at some point for some of the new features that are in the works


ClassDB::register_class<OpenXRFbHandTrackingAimExtensionWrapper>();
OpenXRFbHandTrackingAimExtensionWrapper::get_singleton()->register_extension_wrapper();
void add_plugin_core_settings() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we probably want all these internal functions to be static?

Suggested change
void add_plugin_core_settings() {
void static add_plugin_core_settings() {


ClassDB::register_class<OpenXRFbCompositionLayerSecureContentExtensionWrapper>();
OpenXRFbCompositionLayerSecureContentExtensionWrapper::get_singleton()->register_extension_wrapper();
void add_plugin_project_settings() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
void add_plugin_project_settings() {
static void add_plugin_project_settings() {

@dsnopek dsnopek added this to the 4.x milestone Dec 16, 2024
@dsnopek dsnopek added the enhancement New feature or request label Dec 16, 2024
Comment on lines +267 to +271
if (global_bool_get("xr/openxr/extensions/fb_passthrough")) {
Engine::get_singleton()->register_singleton("OpenXRFbPassthroughExtensionWrapper", OpenXRFbPassthroughExtensionWrapper::get_singleton());
ClassDB::register_class<OpenXRFbPassthroughGeometry>();
ClassDB::register_class<OpenXRMetaPassthroughColorLut>();
}
Copy link
Collaborator

@dsnopek dsnopek Dec 16, 2024

Choose a reason for hiding this comment

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

For the node types, I don't think we should make registering them conditional on the project setting, otherwise you have to restart the Godot editor for the node types to become available to use. So, I think this one, and all the following shouldn't be checking project settings

There may be an argument for always registering the classes for the extension wrappers also (just not calling register_extension_wrapper()), so that the editor is aware that the classes exist, and what their methods are, because a couple of those singletons have public methods.

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