godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.52k stars 21.26k forks source link

4.0 GDExtension: global_get_singleton not returning expected singletons. #64975

Open Splizard opened 2 years ago

Splizard commented 2 years ago

Godot version

v4.0.alpha.custom_build.0c5f25495

System information

Ubuntu 22.04 Vulkan

Issue description

I'm writing a set of GDExtension/GDNative Go bindings and attempting to load all of the global singletons. I have tried loading them both at level 3 initialization time (GDNATIVE_INITIALIZATION_EDITOR) and/or when _ready is called on a extension class. A select number of them fail to load and while I expected that Javascript and JavaClassWrapper fail because they require certain environments. I did not expect the Server types to fail.

ERROR: Failed to retrieve non-existent singleton 'JavaClassWrapper'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'JavaScript'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'DisplayServer'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'RenderingServer'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'AudioServer'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'PhysicsServer2D'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'PhysicsServer3D'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'NavigationServer2D'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'NavigationServer3D'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'XRServer'.
   at: get_singleton_object (core/config/engine.cpp:251)
ERROR: Failed to retrieve non-existent singleton 'CameraServer'.

I might add that calling Engine.get_singleton("DisplayServer") for example, does work from GDScript. Is there something special about these 'Server' singletons? How is the initialization for these singletons meant to work from GDExtension?

Steps to reproduce

Call global_get_singleton on a GDNativeInterface with any of the failing cases shown in the Issue Description.

Example

# include "gdnative_interface.h"

const GDNativeInterface *api;

void deinitialize(void *userdata, GDNativeInitializationLevel p_level) {}

void initialize(void *userdata, GDNativeInitializationLevel p_level) {
    if (p_level == GDNATIVE_INITIALIZATION_EDITOR) {
        api->global_get_singleton("DisplayServer");
    }
}

GDNativeBool loadExtension(const GDNativeInterface *p_interface, const GDNativeExtensionClassLibraryPtr p_library, GDNativeInitialization *r_initialization) {
    r_initialization->minimum_initialization_level = GDNATIVE_INITIALIZATION_EDITOR;
    r_initialization->initialize = initialize;
    r_initialization->deinitialize = deinitialize;
    api = p_interface;
    return 1;
}
Calinou commented 2 years ago

@Splizard Please upload a minimal reproduction project to make this easier to troubleshoot.

Splizard commented 2 years ago

Minimal reproduction project 64975.zip

Only a linux.64 shared library file is included, so you will need to build the minimal main.c file and add it to example.gdextension in order to debug this on other platforms.

BastiaanOlij commented 1 year ago

I'm running into this too with XR interfaces implemented in GDExternal. I've updated my reference implementation today so it can be reproduced with that.

This line fails:

XRServer *xr_server = XRServer::get_singleton();

https://github.com/BastiaanOlij/godot_xr_reference/blob/update_godot_4_stable/src/register_type.cpp#L19

Bit of a showstopper, shame we mist it for the stable release.

BastiaanOlij commented 1 year ago

Ok, looking into this some more, the problem is due to this: https://github.com/godotengine/godot/blob/master/main/main.cpp#L2315

    MAIN_PRINT("Main: Load Physics");

    initialize_physics();
    initialize_navigation_server();
    register_server_singletons();

register_server_singletons is where the singletons are registered for access by scripts.

This is after: https://github.com/godotengine/godot/blob/master/main/main.cpp#L2309

    initialize_modules(MODULE_INITIALIZATION_LEVEL_EDITOR);

Which is the last time we call into the initialization of GDExternals meaning that GDExternals has no way to interact with the server singletons, even though some of them have long since been created.

The solution in @Splizard PR was to introduce a new initialization level, which we definitely don't want to do.

The problem that we have in this is that there have been many discussions over time about the timing of registering our singletons, and reasons for why the timing was changed. These reasons are poorly documented so changing the timing once again may cause other problems to surface.

For my use case, I definitely need access to the XRServer singleton when we are calling into initialize_modules(MODULE_INITIALIZATION_LEVEL_SERVERS); as XRInterfaces need to potentially be registered with the XRServer before our graphics pipeline is setup.

The question here to answer is: What is the reasons all the servers need to be registered together? This is delaying the moment of registering because we don't create, say they physics server until much later, and this is correct. And there is an argument that only registering some servers, will lead to questions by users why the server they need, isn't available. There is something to say for consistency.

The flipside of the argument is that not registering servers as soon as possible after they have been instantiated, makes our initialization levels pretty much useless.

This will need further discussion with the results documented properly once and for all, but seeing the core team is currently preoccupied with GDC, this will likely happen afterwards.