From a91a3d01f849ba3a7c1615003f8f11ca824d3d58 Mon Sep 17 00:00:00 2001 From: Raul Santos Date: Fri, 2 Sep 2022 22:31:56 +0200 Subject: C#: Create script instance of reloaded scripts even if they're not tools Scripts that are instantiated at some point will always be recreated if they ever become placeholders to prevent non-tool scripts instantiated manually by users to become placeholders, if they do become placeholders due to errors that prevent instantiation (such as a missing parameterless constructor) these scripts will also be recreated replacing the temporary placeholder. If a script is marked as a tool but becomes a non-tool script in a rebuild, the script will become a placeholder and will no longer be considered applicable to be replaced by an instance since the user explicitly removed the Tool attribute. --- modules/mono/csharp_script.cpp | 44 +++++++++++++++++++--- modules/mono/csharp_script.h | 3 ++ .../GodotSharp/Core/Bridge/ScriptManagerBridge.cs | 3 +- 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/modules/mono/csharp_script.cpp b/modules/mono/csharp_script.cpp index 2e59987fe6..7e59c43663 100644 --- a/modules/mono/csharp_script.cpp +++ b/modules/mono/csharp_script.cpp @@ -784,6 +784,13 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) { for (Object *obj : script->instances) { script->pending_reload_instances.insert(obj->get_instance_id()); + // Since this script instance wasn't a placeholder, add it to the list of placeholders + // that will have to be eventually replaced with a script instance in case it turns into one. + // This list is not cleared after the reload and the collected instances only leave + // the list if the script is instantiated or if it was a tool script but becomes a + // non-tool script in a rebuild. + script->pending_replace_placeholders.insert(obj->get_instance_id()); + RefCounted *rc = Object::cast_to(obj); if (rc) { rc_instances.push_back(Ref(rc)); @@ -836,6 +843,7 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) { obj->set_script(Ref()); // Remove script and existing script instances (placeholder are not removed before domain reload) } + script->was_tool_before_reload = script->tool; script->_clear(); } @@ -924,24 +932,34 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) { ScriptInstance *si = obj->get_script_instance(); + // Check if the script must be instantiated or kept as a placeholder + // when the script may not be a tool (see #65266) + bool replace_placeholder = script->pending_replace_placeholders.has(obj->get_instance_id()); + if (!script->is_tool() && script->was_tool_before_reload) { + // The script was a tool before the rebuild so the removal was intentional. + replace_placeholder = false; + script->pending_replace_placeholders.erase(obj->get_instance_id()); + } + #ifdef TOOLS_ENABLED if (si) { // If the script instance is not null, then it must be a placeholder. // Non-placeholder script instances are removed in godot_icall_Object_Disposed. CRASH_COND(!si->is_placeholder()); - if (script->is_tool() || ScriptServer::is_scripting_enabled()) { - // Replace placeholder with a script instance + if (replace_placeholder || script->is_tool() || ScriptServer::is_scripting_enabled()) { + // Replace placeholder with a script instance. CSharpScript::StateBackup &state_backup = script->pending_reload_state[obj_id]; - // Backup placeholder script instance state before replacing it with a script instance + // Backup placeholder script instance state before replacing it with a script instance. si->get_property_state(state_backup.properties); ScriptInstance *script_instance = script->instance_create(obj); if (script_instance) { script->placeholders.erase(static_cast(si)); + script->pending_replace_placeholders.erase(obj->get_instance_id()); obj->set_script_instance(script_instance); } } @@ -951,8 +969,24 @@ void CSharpLanguage::reload_assemblies(bool p_soft_reload) { #else CRASH_COND(si != nullptr); #endif - // Re-create script instance - obj->set_script(script); // will create the script instance as well + + // Re-create the script instance. + if (replace_placeholder || script->is_tool() || ScriptServer::is_scripting_enabled()) { + // Create script instance or replace placeholder with a script instance. + ScriptInstance *script_instance = script->instance_create(obj); + + if (script_instance) { + script->pending_replace_placeholders.erase(obj->get_instance_id()); + obj->set_script_instance(script_instance); + continue; + } + } + // The script instance could not be instantiated or wasn't in the list of placeholders to replace. + obj->set_script(script); +#if DEBUG_ENABLED + // If we reached here, the instantiated script must be a placeholder. + CRASH_COND(!obj->get_script_instance()->is_placeholder()); +#endif } } diff --git a/modules/mono/csharp_script.h b/modules/mono/csharp_script.h index 3509a5c87d..f2844a051d 100644 --- a/modules/mono/csharp_script.h +++ b/modules/mono/csharp_script.h @@ -90,6 +90,9 @@ class CSharpScript : public Script { HashSet pending_reload_instances; RBMap pending_reload_state; + + bool was_tool_before_reload = false; + HashSet pending_replace_placeholders; #endif String source; diff --git a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs b/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs index 3884781988..092724a6b1 100644 --- a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs +++ b/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs @@ -130,7 +130,6 @@ namespace Godot.Bridge { // Performance is not critical here as this will be replaced with source generators. Type scriptType = _scriptTypeBiMap.GetScriptType(scriptPtr); - var obj = (Object)FormatterServices.GetUninitializedObject(scriptType); var ctor = scriptType .GetConstructors(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance) @@ -151,6 +150,8 @@ namespace Godot.Bridge } } + var obj = (Object)FormatterServices.GetUninitializedObject(scriptType); + var parameters = ctor.GetParameters(); int paramCount = parameters.Length; -- cgit v1.2.3