Refactor module initialization #60723

reduz · 2022-05-03T09:58:31Z

Changed to use the same stages as extensions.
Makes the initialization more coherent, helping solve problems due to lack of stages.
Makes it easier to port between module and extension.
removed the DRIVER initialization level (no longer needed).

bruvzg · 2022-05-03T10:16:44Z

modules/text_server_adv/register_types.cpp

@@ -32,7 +32,11 @@

 #include "text_server_adv.h"

-void preregister_text_server_adv_types() {
+void initialize_text_server_adv_module(ModuleInitializationLevel p_level) {


There are still references to the old functions for the GDExtension build init few lines later:

init_obj.register_server_initializer(&preregister_text_server_adv_types); init_obj.register_server_terminator(&unregister_text_server_adv_types);

I guess GDExtension initializer/terminator registration probably should be changed to work in the same way as new initialize_*_module, so it should be fixed in godot-cpp first.

I am not entirely sure how to fix those, maybe in a subsequent PR ?

With the godotengine/godot-cpp#750 and following patch, it should work:

diff --git a/modules/text_server_adv/register_types.cpp b/modules/text_server_adv/register_types.cpp index b4fd87e1e7..6a26584506 100644 --- a/modules/text_server_adv/register_types.cpp +++ b/modules/text_server_adv/register_types.cpp @@ -47,7 +47,7 @@ void initialize_text_server_adv_module(ModuleInitializationLevel p_level) { } void uninitialize_text_server_adv_module(ModuleInitializationLevel p_level) { - if (p_level != MODULE_INITIALIZATION_LEVEL_SCENE) { + if (p_level != MODULE_INITIALIZATION_LEVEL_SERVERS) { return; } } @@ -65,8 +65,9 @@ extern "C" { GDNativeBool GDN_EXPORT textserver_advanced_init(const GDNativeInterface *p_interface, const GDNativeExtensionClassLibraryPtr p_library, GDNativeInitialization *r_initialization) { GDExtensionBinding::InitObject init_obj(p_interface, p_library, r_initialization); - init_obj.register_server_initializer(&preregister_text_server_adv_types); - init_obj.register_server_terminator(&unregister_text_server_adv_types); + init_obj.register_initializer(&initialize_text_server_adv_module); + init_obj.register_terminator(&uninitialize_text_server_adv_module); + init_obj.set_minimum_library_initialization_level(MODULE_INITIALIZATION_LEVEL_SERVERS); return init_obj.init(); } diff --git a/modules/text_server_adv/register_types.h b/modules/text_server_adv/register_types.h index 0028ca99db..dfe20c860c 100644 --- a/modules/text_server_adv/register_types.h +++ b/modules/text_server_adv/register_types.h @@ -31,9 +31,12 @@ #ifndef TEXT_SERVER_ADV_REGISTER_TYPES_H #define TEXT_SERVER_ADV_REGISTER_TYPES_H -#define MODULE_TEXT_SERVER_ADV_HAS_PREREGISTER - +#ifdef GDEXTENSION +#include <godot_cpp/core/class_db.hpp> +using namespace godot; +#else #include "modules/register_module_types.h" +#endif void initialize_text_server_adv_module(ModuleInitializationLevel p_level); void uninitialize_text_server_adv_module(ModuleInitializationLevel p_level); diff --git a/modules/text_server_fb/register_types.cpp b/modules/text_server_fb/register_types.cpp index d69bd996ee..fa7b87fc17 100644 --- a/modules/text_server_fb/register_types.cpp +++ b/modules/text_server_fb/register_types.cpp @@ -47,7 +47,7 @@ void initialize_text_server_fb_module(ModuleInitializationLevel p_level) { } void uninitialize_text_server_fb_module(ModuleInitializationLevel p_level) { - if (p_level != MODULE_INITIALIZATION_LEVEL_SCENE) { + if (p_level != MODULE_INITIALIZATION_LEVEL_SERVERS) { return; } } @@ -65,8 +65,9 @@ extern "C" { GDNativeBool GDN_EXPORT textserver_fallback_init(const GDNativeInterface *p_interface, const GDNativeExtensionClassLibraryPtr p_library, GDNativeInitialization *r_initialization) { GDExtensionBinding::InitObject init_obj(p_interface, p_library, r_initialization); - init_obj.register_server_initializer(&preregister_text_server_fb_types); - init_obj.register_server_terminator(&unregister_text_server_fb_types); + init_obj.register_initializer(&initialize_text_server_fb_module); + init_obj.register_terminator(&uninitialize_text_server_fb_module); + init_obj.set_minimum_library_initialization_level(MODULE_INITIALIZATION_LEVEL_SERVERS); return init_obj.init(); } diff --git a/modules/text_server_fb/register_types.h b/modules/text_server_fb/register_types.h index 4c804cc028..229aec2266 100644 --- a/modules/text_server_fb/register_types.h +++ b/modules/text_server_fb/register_types.h @@ -31,9 +31,12 @@ #ifndef TEXT_SERVER_FB_REGISTER_TYPES_H #define TEXT_SERVER_FB_REGISTER_TYPES_H -#define MODULE_TEXT_SERVER_FB_HAS_PREREGISTER - +#ifdef GDEXTENSION +#include <godot_cpp/core/class_db.hpp> +using namespace godot; +#else #include "modules/register_module_types.h" +#endif void initialize_text_server_fb_module(ModuleInitializationLevel p_level); void uninitialize_text_server_fb_module(ModuleInitializationLevel p_level);

So we can wait for godotengine/godot-cpp#750 to be merged, then @reduz amends this PR with the above diff + re-enables godot-cpp CI?

Thanks hugely! Will apply this today.

neikeq · 2022-05-03T16:09:26Z

main/main.cpp

+	NativeExtensionManager::get_singleton()->deinitialize_extensions(NativeExtension::INITIALIZATION_LEVEL_EDITOR);
+	uninitialize_modules(MODULE_INITIALIZATION_LEVEL_EDITOR);


Inconsistency: deinitialize / uninitialize. I think the latter is correct, but not sure.

I feel we should rename stuff in GDExtension from deinitialize to uninitialize, but it likely should be done in a subsequent PR.

According to Google:

"deinitialize" 51700 results

"uninitialize" 64000 results
Kinda seems the later is a bit more popular.

For fun, but I guess we should probably ensure a single term for the whole engine - https://twitter.com/reduzio/status/1521782429526200321

I think "finalize" as proposed in your poll might be a better term. "uninitialize" might be a bit more common than "deinitialize" but it's also used for uninitialized variable, which means something which has not been initialized yet. While here things have been initialized, used, and now need to be cleaned up (which is usually different from just de-initializing variables).

Actually as I write this, "deinitialize" sounds a bit better to avoid the confusion with not yet initialized "uninitialized". That is if we want to keep "initialize" as part of the name since the enums are about INITIALIZATION_LEVEL. Otherwise I think another term like "finalize" can be clearer and less prone to typos (it's always super difficult to spell uninitialize IMO with all those ni's :P).

Alternatively, I like "cleanup" too for this kind of task.

We can discuss this with other contributors I guess, but anything we decide we should probably make it consistent across the codebase.

neikeq · 2022-05-03T16:15:38Z

@AndreaCatania Does this fully supersede #50179? It's not quite the same, do you miss anything in this one?

BastiaanOlij · 2022-05-04T00:18:28Z

I think I was using DRIVER in actual extensions seeing the OpenXR extension was originally a driver, not that it matters anymore now that it is core :)

We do need to make sure that people are aware of the impact, while I don't think many people are building editor extensions yet, the enum changing from 4 to 3 will break their plugin unless they recompile.

BastiaanOlij

Looks good to me

AndreaCatania · 2022-05-04T05:14:07Z

This is a good feature!

I think that the only missing thing is to call initialize_modules(MODULE_INITIALIZATION_LEVEL_BEGIN); at the really start of Error Main::setup and initialize_modules(MODULE_INITIALIZATION_LEVEL_DONE); at the really end of Main::setup2

The first one is useful so you can hook the initialization even before the core & platform are initialized, indeed initialize_modules(MODULE_INITIALIZATION_LEVEL_CORE); is called only after the core is fully initialized.
The former is useful so you can do stuff when everything is initialized, and the engine is ready to be used as normal.

Other than that, it looks really nice.

reduz · 2022-05-04T08:07:00Z

@AndreaCatania There is not much you can do before CORE is initialized so IMO that is a bit pointless. If you want to override some core things like filesystem drivers or other stuff, you can still do it after it is initialized.

AndreaCatania · 2022-05-04T08:35:08Z

@reduz It would be handy in case you want to configure some core functionality during the engine initialization, so you can execute the configurator before the core is started.
Also, since there is no initialization order at the moment, you may override a core functionality making it dependent from another functionality: which should be initialized before.

Add another event should not make things bad anyway, so probably is worth add that to make these two --^ possible.

reduz · 2022-05-04T13:14:47Z

@AndreaCatania I understand the concern, but for the time being I will not add another level unless there is a specific need for it. Its easy to add if the need arises.

AndreaCatania · 2022-05-04T13:36:59Z

Ok, thanks!

AndreaCatania

LGTM

Faless · 2022-05-04T13:39:34Z

This looks mostly okay for me, few notes:

I'm getting ERROR: Condition "int32_t(p_level) != level" is true. on exit due to leftover extension_deinitialization in unregister_server_types and unregister_editor_types (trying to deinitialize the same level twice), they should likely be removed from register_[servers/editor]_type.cpp, along with the extensionmanager include I guess.

This solves my issue with webrtc (thanks!) so I'm closing #60454 , but the contributor in #60317 requires additional changes (namely register_server_singletons to happen before the last init level.

I think this is because the need to call functions in servers singletons (e.g. XRServer::add_interface/XRServer::set_primary_interface) which right now happens on singletons.
Maybe, having static method support, we could refactor those methods in case, but there might be more cases where you want your extension to interact with server singletons before scene instantiation begins.

Something on the line of (untested):

diff --git a/main/main.cpp b/main/main.cpp
index 5ddce818ca..07daf583d0 100644
--- a/main/main.cpp
+++ b/main/main.cpp
@@ -1947,6 +1947,19 @@ Error Main::setup2(Thread::ID p_main_tid_override) {
 	initialize_modules(MODULE_INITIALIZATION_LEVEL_SCENE);
 	NativeExtensionManager::get_singleton()->initialize_extensions(NativeExtension::INITIALIZATION_LEVEL_SCENE);
 
+	register_platform_apis();
+
+	MAIN_PRINT("Main: Initialize server singletons.");
+
+	camera_server = CameraServer::create();
+
+	initialize_physics();
+	initialize_navigation_server();
+	register_server_singletons();
+
+	initialize_modules(MODULE_INITIALIZATION_LEVEL_READY);
+	NativeExtensionManager::get_singleton()->initialize_extensions(NativeExtension::INITIALIZATION_LEVEL_READY);
+
 #ifdef TOOLS_ENABLED
 	ClassDB::set_current_api(ClassDB::API_EDITOR);
 	EditorNode::register_editor_types();
@@ -1957,10 +1970,6 @@ Error Main::setup2(Thread::ID p_main_tid_override) {
 
 #endif
 
-	MAIN_PRINT("Main: Load Modules");
-
-	register_platform_apis();
-
 	// Theme needs modules to be initialized so that sub-resources can be loaded.
 	initialize_theme();
 
@@ -1981,14 +1990,6 @@ Error Main::setup2(Thread::ID p_main_tid_override) {
 		}
 	}
 
-	camera_server = CameraServer::create();
-
-	MAIN_PRINT("Main: Load Physics");
-
-	initialize_physics();
-	initialize_navigation_server();
-	register_server_singletons();
-
 	// This loads global classes, so it must happen before custom loaders and savers are registered
 	ScriptServer::init_languages();

reduz · 2022-05-04T14:13:03Z

@Faless Just fixed it, make sure to test any way but I no longer see the errors.

BastiaanOlij · 2022-05-04T14:13:56Z

Thats strange Fales, I've only done a read through the code, haven't tested anything specific, but the init of XR related classes is mostly done in scene initialisation, which happens after the XRServer is available.

The exception is OpenXR which has two points of initialisation, setting up the OpenXR "driver" part, which happens as early as possible, and the initialisation of the OpenXR "interface" which happens in scene initialisation and calls the add_interface method on the XRServer singleton.

I got the impression from looking at the changes in the OpenXR module that this division was retained as it is checking the mode nicely. If this is giving errors the placement of initialisation has changed.

BastiaanOlij · 2022-05-04T14:19:11Z

modules/openxr/register_types.cpp

-void preregister_openxr_types() {
-	// For now we create our openxr device here. If we merge it with openxr_interface we'll create that here soon.
+void initialize_openxr_module(ModuleInitializationLevel p_level) {
+	if (p_level == MODULE_INITIALIZATION_LEVEL_SERVERS) {


I need to check but I think we've moved this too far down, this part needs to run really early and I think was run during the pre-register functions. Now it's run later if I read the code change in main.cpp correctly.

That said, the only option earlier is MODULE_INITIALIZATION_LEVEL_CORE but this is run before core extensions are created. Might still be correct for OpenXR but not sure.

I'll do some testing tomorrow, it's bed time for me.

Faless · 2022-05-04T14:25:11Z

@BastiaanOlij I'm not sure, it's more like a guess, but basically right now extensions cannot call Engine::get_singleton("ServerSingleton") (XRServer, AudioServer, etc) during initialization phase.
I didn't understand which method exactly the OP need to call in #60317 so maybe @kidrigger can clarify on that.

Faless · 2022-05-04T14:32:18Z

@reduz looks good to me now, just one nit-pick, we should also remove the line:

#include "core/extension/native_extension_manager.h"

in both editor/editor_node.cpp and servers/register_server_types.cpp which is no longer needed.

reduz · 2022-05-04T14:43:37Z

Alright, done with the changes and suggestions. If no further changes are desired, it should be ready to merge once it passes CI.

* Changed to use the same stages as extensions. * Makes the initialization more coherent, helping solve problems due to lack of stages. * Makes it easier to port between module and extension. * removed the DRIVER initialization level (no longer needed).

akien-mga

Looks good to me.

akien-mga · 2022-05-04T17:08:46Z

Thanks!

See godot commit 3d2dd79ecd2c8456ba9401f6b12333d01f61e13e godotengine/godot-proposals#3371 godotengine/godot#60723

reduz requested review from a team as code owners May 3, 2022 09:58

akien-mga added enhancement topic:core labels May 3, 2022

akien-mga added this to the 4.0 milestone May 3, 2022

bruvzg reviewed May 3, 2022

View reviewed changes

reduz force-pushed the refactor-module-initialization branch from 8b6ac14 to beaa149 Compare May 3, 2022 12:21

reduz requested a review from a team as a code owner May 3, 2022 12:21

reduz force-pushed the refactor-module-initialization branch from beaa149 to 974b273 Compare May 3, 2022 14:04

neikeq reviewed May 3, 2022

View reviewed changes

BastiaanOlij approved these changes May 4, 2022

View reviewed changes

bruvzg mentioned this pull request May 4, 2022

Change registered initializers / terminators to the single function with level argument. godotengine/godot-cpp#750

Merged

reduz force-pushed the refactor-module-initialization branch from 974b273 to 74964b3 Compare May 4, 2022 13:29

AndreaCatania approved these changes May 4, 2022

View reviewed changes

Faless mentioned this pull request May 4, 2022

[Core] Register platform/modules before editor. Add MODULES extension level. #60454

Closed

reduz force-pushed the refactor-module-initialization branch from 74964b3 to d8a8a65 Compare May 4, 2022 14:11

BastiaanOlij reviewed May 4, 2022

View reviewed changes

Faless approved these changes May 4, 2022

View reviewed changes

reduz force-pushed the refactor-module-initialization branch from d8a8a65 to ac8d28e Compare May 4, 2022 14:43

akien-mga mentioned this pull request May 4, 2022

Added INITIALIZATION_LEVEL_READY for extension setup. #60317

Closed

Refactor module initialization

de0ca3b

* Changed to use the same stages as extensions. * Makes the initialization more coherent, helping solve problems due to lack of stages. * Makes it easier to port between module and extension. * removed the DRIVER initialization level (no longer needed).

reduz force-pushed the refactor-module-initialization branch from ac8d28e to de0ca3b Compare May 4, 2022 15:35

akien-mga approved these changes May 4, 2022

View reviewed changes

AndreaCatania mentioned this pull request May 4, 2022

Add godot app lifecycle callbacks to modules. #50179

Closed

akien-mga merged commit 84f64dd into godotengine:master May 4, 2022

BastiaanOlij mentioned this pull request May 13, 2022

Fix OpenXR initialization - introduce pre server initialization level #60996

Closed

lufog mentioned this pull request May 25, 2022

Sync with Godot master GDRETools/gdsdecomp#53

Merged

8 tasks

akien-mga mentioned this pull request Jul 18, 2022

Add ready callback after godot setup. godotengine/godot-cpp#739

Closed

Faless mentioned this pull request Oct 12, 2022

Fix #64975 by introducing a new initialization level. #65018

Closed

Meptl added a commit to Meptl/goost that referenced this pull request May 9, 2023

Fix up build scripts

3572f13

See godot commit 3d2dd79ecd2c8456ba9401f6b12333d01f61e13e godotengine/godot-proposals#3371 godotengine/godot#60723

clayjohn mentioned this pull request Sep 25, 2023

Register RenderingServer class and singleton with scene types #82323

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor module initialization #60723

Refactor module initialization #60723

reduz commented May 3, 2022

bruvzg May 3, 2022 •

edited

Loading

reduz May 3, 2022

bruvzg May 4, 2022

akien-mga May 4, 2022

bruvzg May 4, 2022

reduz May 4, 2022

neikeq May 3, 2022

reduz May 3, 2022

reduz May 4, 2022

reduz May 4, 2022

akien-mga May 4, 2022 •

edited

Loading

reduz May 4, 2022

neikeq commented May 3, 2022

BastiaanOlij commented May 4, 2022 •

edited

Loading

BastiaanOlij left a comment

AndreaCatania commented May 4, 2022

reduz commented May 4, 2022

AndreaCatania commented May 4, 2022

reduz commented May 4, 2022

AndreaCatania commented May 4, 2022

AndreaCatania left a comment

Faless commented May 4, 2022

reduz commented May 4, 2022

BastiaanOlij commented May 4, 2022

BastiaanOlij May 4, 2022

Faless commented May 4, 2022

Faless commented May 4, 2022

reduz commented May 4, 2022

akien-mga left a comment

akien-mga commented May 4, 2022

		NativeExtensionManager::get_singleton()->deinitialize_extensions(NativeExtension::INITIALIZATION_LEVEL_EDITOR);
		uninitialize_modules(MODULE_INITIALIZATION_LEVEL_EDITOR);

Refactor module initialization #60723

Refactor module initialization #60723

Conversation

reduz commented May 3, 2022

bruvzg May 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neikeq commented May 3, 2022

BastiaanOlij commented May 4, 2022 • edited Loading

BastiaanOlij left a comment

Choose a reason for hiding this comment

AndreaCatania commented May 4, 2022

reduz commented May 4, 2022

AndreaCatania commented May 4, 2022

reduz commented May 4, 2022

AndreaCatania commented May 4, 2022

AndreaCatania left a comment

Choose a reason for hiding this comment

Faless commented May 4, 2022

reduz commented May 4, 2022

BastiaanOlij commented May 4, 2022

Choose a reason for hiding this comment

Faless commented May 4, 2022

Faless commented May 4, 2022

reduz commented May 4, 2022

akien-mga left a comment

Choose a reason for hiding this comment

akien-mga commented May 4, 2022

bruvzg May 3, 2022 •

edited

Loading

akien-mga May 4, 2022 •

edited

Loading

BastiaanOlij commented May 4, 2022 •

edited

Loading