godot

godot copied to clipboard

Fixes https://github.com/godotengine/godot/issues/74267.

The crash happens because scripting languages are not ready, but we are trying to load their scripts and it fails for one reason or another. Changing the order like that seems safe to me, and the project now loads without issues both in editor and when running scenes.

Right, so right now it crashes on generating C# glue, because that happens during the ScriptServer::init_languages() step. Possibly it's missing some information from the default theme needed for generation. Not sure what to do about it, but it feels like we should not be handling the glue generation during the language initialization step, should we? If we could postpone it until after setup2() is done, this issue should be resolved.

cc @raulsntos @neikeq

Edit: According to the description of the design for the main.cpp file (found in its comments), command-line tools should run in start(). Maybe we should add a hook for ScriptServer languages that is triggered during start()?

I've added a second commit to show what I mean. Tested it locally, and it seems to work as expected. But let's see what CI thinks (and what other maintainers think, of course 🙃 ).

I will have to squash commits before merging, if this is an acceptable solution.

Right, so right now it crashes on generating C# glue, because that happens during the ScriptServer::init_languages() step. Possibly it's missing some information from the default theme needed for generation. Not sure what to do about it, but it feels like we should not be handling the glue generation during the language initialization step, should we? If we could postpone it until after setup2() is done, this issue should be resolved.

cc @raulsntos @neikeq

Edit: According to the description of the design for the main.cpp file (found in its comments), command-line tools should run in start(). Maybe we should add a hook for ScriptServer languages that is triggered during start()?

As long as it's called after all classes have been registered, it should be fine. Although I do wonder what that crash was about.

There was a PR that reworks command line handling (#26213, superseded by #44594), so the idea was to use that for the bindings generation command if it was ever merged.

Since this init() code seems to be a workaround for not having a proper facility for handling command line options from a module, I'm not sure we should add a new start() core method in ScriptLanguage just for this. If there are other use cases for separate init and start in ScriptLanguages I'm not against it, but this specific issue doesn't make a strong use case IMO.

This also seems to somewhat replicate what MODULE_INITIALIZATION_LEVEL_* is used for. Though I've attempted using MODULE_INITIALIZATION_LEVEL_EDITOR (the last one to be called AFAIK) but I got a crash (likely similar to the one caused by this PR originally):

================================================================
handle_crash: Program crashed with signal 11
Engine version: Godot Engine v4.1.dev.mono.custom_build (8b1568af70eea62e34267ee4949170d7e2c4faa0)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] /lib64/libc.so.6(+0x36940) [0x7f373b4d3940] (??:0)
[2] Ref<Theme>::ref(Ref<Theme> const&) (/home/akien/Projects/godot/godot.git/./core/object/ref_counted.h:61)
[3] Ref<Theme>::Ref(Ref<Theme> const&) (/home/akien/Projects/godot/godot.git/./core/object/ref_counted.h:178)
[4] ThemeDB::get_default_theme() (/home/akien/Projects/godot/godot.git/scene/theme/theme_db.cpp:102)
[5] DocTools::generate(bool) (/home/akien/Projects/godot/godot.git/editor/doc_tools.cpp:588 (discriminator 4))
[6] EditorHelp::generate_doc() (/home/akien/Projects/godot/godot.git/editor/editor_help.cpp:2192)
[7] BindingsGenerator::_initialize() (/home/akien/Projects/godot/godot.git/modules/mono/editor/bindings_generator.cpp:3883)
[8] BindingsGenerator::BindingsGenerator() (/home/akien/Projects/godot/godot.git/modules/mono/editor/bindings_generator.h:799)
[9] ./bin/godot.linuxbsd.editor.dev.x86_64.mono() [0x649304e] (/home/akien/Projects/godot/godot.git/modules/mono/editor/bindings_generator.cpp:3909)
[10] BindingsGenerator::handle_cmdline_args(List<String, DefaultAllocator> const&) (/home/akien/Projects/godot/godot.git/modules/mono/editor/bindings_generator.cpp:3954 (discriminator 4))
[11] initialize_mono_module(ModuleInitializationLevel) (/home/akien/Projects/godot/godot.git/modules/mono/register_types.cpp:75)
[12] initialize_modules(ModuleInitializationLevel) (/home/akien/Projects/godot/godot.git/modules/register_module_types.gen.cpp:131)
[13] Main::setup2(unsigned long) (/home/akien/Projects/godot/godot.git/main/main.cpp:2300)
[14] Main::setup(char const*, int, char**, bool) (/home/akien/Projects/godot/godot.git/main/main.cpp:1874)
[15] ./bin/godot.linuxbsd.editor.dev.x86_64.mono(main+0xff) [0x53311a5] (/home/akien/Projects/godot/godot.git/platform/linuxbsd/godot_linuxbsd.cpp:61)
[16] /lib64/libc.so.6(+0x23677) [0x7f373b4c0677] (??:0)
[17] /lib64/libc.so.6(__libc_start_main+0x85) [0x7f373b4c0735] (??:0)
[18] ./bin/godot.linuxbsd.editor.dev.x86_64.mono(_start+0x21) [0x5330fe1] (??:?)
-- END OF BACKTRACE --
================================================================

I'm a bit wary of adding more core init callbacks to workaround the clash between two very specific use cases which shouldn't need to clash: scripting user themes, and generating the C# documentation for the core API.

Can't we split the default theme init into two methods, one called early on to set up the core API's default theme, and one called later on to do updates based on user scripts?

Yeah, I don't like that I had to add and expose a new method to ScriptServer either (and the name is not great). But this is probably why the glue generation was bolted on like that originally. There is currently no suitable hook for post-setup/startup handlers, as there was initially no need for them. But that means that glue generator does not wait for the initialization of the engine to complete, unlike other CLI tools, which leads to this conflict.

Splitting theme into two is not impossible, but it would be a very verbose hack to work around another hack for the sake of not adding a dedicated hook into the initialization logic. If that's the case, I would just keep the current issue without a fix until we either improve CLI handling logic, or I move theme definitions to the scene component level, doing essentially what you ask but in a more idiomatic way.

Scripted styleboxes are not supported in 3.x anyway, so this is all new and uncharted, and there would probably be a very small number of users who are affected.

Edit: Hmm, on a second thought, maybe I can fix it on the theme level.

Since it's already pretty hacky, how about we go all in and make the hack clearer and cleaner? Ripe for future refactoring ;)

diff --git a/main/main.cpp b/main/main.cpp
index b15588e700..a6e42c3c6b 100644
--- a/main/main.cpp
+++ b/main/main.cpp
@@ -108,6 +108,10 @@
 
 #include "modules/modules_enabled.gen.h" // For mono.
 
+#ifdef MODULE_MONO_ENABLED
+#include "modules/mono/editor/bindings_generator.h"
+#endif
+
 /* Static members */
 
 // Singletons
@@ -312,7 +316,6 @@ void finalize_navigation_server() {
 
 void initialize_theme_db() {
 	theme_db = memnew(ThemeDB);
-	theme_db->initialize_theme();
 }
 
 void finalize_theme_db() {
@@ -532,6 +535,7 @@ Error Main::test_setup() {
 
 	// Theme needs modules to be initialized so that sub-resources can be loaded.
 	initialize_theme_db();
+	theme_db->initialize_theme();
 	register_scene_singletons();
 
 	ERR_FAIL_COND_V(TextServerManager::get_singleton()->get_interface_count() == 0, ERR_CANT_CREATE);
@@ -2314,6 +2318,7 @@ Error Main::setup2(Thread::ID p_main_tid_override) {
 	register_platform_apis();
 
 	// Theme needs modules to be initialized so that sub-resources can be loaded.
+	// Default theme is initialized later, after ScriptServer is ready.
 	initialize_theme_db();
 	register_scene_singletons();
 
@@ -2341,8 +2346,18 @@ Error Main::setup2(Thread::ID p_main_tid_override) {
 	// This loads global classes, so it must happen before custom loaders and savers are registered
 	ScriptServer::init_languages();
 
+	theme_db->initialize_theme();
 	audio_server->load_default_bus_layout();
 
+#if defined(MODULE_MONO_ENABLED) && defined(TOOLS_ENABLED)
+	// Hacky to have it here, but we don't have good facility yet to let modules
+	// register command line options to call at the right time. This needs to happen
+	// after init'ing the ScriptServer, but also after init'ing the ThemeDB,
+	// for the C# docs generation in the bindings.
+	List<String> cmdline_args = OS::get_singleton()->get_cmdline_args();
+	BindingsGenerator::handle_cmdline_args(cmdline_args);
+#endif
+
 	if (use_debug_profiler && EngineDebugger::is_active()) {
 		// Start the "scripts" profiler, used in local debugging.
 		// We could add more, and make the CLI arg require a comma-separated list of profilers.
diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp
index 932e97c46b..1dbf6b3471 100644
--- a/modules/mono/csharp_script.cpp
+++ b/modules/mono/csharp_script.cpp
@@ -42,7 +42,6 @@
 
 #ifdef TOOLS_ENABLED
 #include "core/os/keyboard.h"
-#include "editor/bindings_generator.h"
 #include "editor/editor_internal_calls.h"
 #include "editor/editor_node.h"
 #include "editor/editor_settings.h"
@@ -103,13 +102,6 @@ void CSharpLanguage::init() {
 	}
 #endif
 
-#if defined(TOOLS_ENABLED) && defined(DEBUG_METHODS_ENABLED)
-	// Generate the bindings here, before loading assemblies. The Godot assemblies
-	// may be missing if the glue wasn't generated yet in order to build them.
-	List<String> cmdline_args = OS::get_singleton()->get_cmdline_args();
-	BindingsGenerator::handle_cmdline_args(cmdline_args);
-#endif
-
 	GLOBAL_DEF("dotnet/project/assembly_name", "");
 #ifdef TOOLS_ENABLED
 	GLOBAL_DEF("dotnet/project/solution_directory", "");

Could also be refactored further to move the actual command line argument handling to the same place as other core arguments within #ifdef MODULE_MONO_ENABLED, but that requires some more work. I just went for the simplest option.

BTW, both your current PR and might change make this comment no longer applicable:

// Generate the bindings here, before loading assemblies. The Godot assemblies
// may be missing if the glue wasn't generated yet in order to build them.

But I'm not sure how important it was. It seems to work fine to generate the glue.

Okay, did exactly that. Wasn't going to go this way initially due to the hacky nature and this being main.cpp, but if it's fine then this is the cleanest option. I looked more into splitting the theme initialization step, but it was all starting to get messy quickly (and could negatively impact load times unless done extensively).

Thanks!

Cherry-picked for 4.0.1.