C#: Create script instance of reloaded scripts even if they're not tools

[raulsntos]

Fixes #64381 and the exception part of #65032

As discussed in Rocket Chat with @neikeq this is a proof of concept that instantiates the reloaded scripts even if they don't have the [Tool] attribute for cases where a non-Tool class was instantiated by the user from a Tool class.

Without this PR these classes are replaced by a PlaceHolderScriptInstance when rebuilding the assemblies and they never turn back into a CSharpInstance which means their methods are no longer called by the editor for example in the case of EditorInspectorPlugin::_CanHandle (see #64381).

Now, if a class is ever instantiated in the editor we avoid creating a PlaceHolderScriptInstance but if it ever becomes one, we will always try to replace it with a CSharpInstance unless the class had the [Tool] attribute before the reload but was removed so after the reload the class is no longer a Tool, in this case because the user explicitly removed the attribute so the intention is clear that the class must no longer be instantiated and therefore we replace the instance with a placeholder and no longer consider it when replacing placeholders with instances.

[neikeq]

One thing to keep in mind is that with this change, if a script was instantiated because it was marked [Tool] and that attribute is removed, after rebuilding it would be instantiated again. That may not be what the user intended.
If we want to avoid that, we would need to keep a flag like was_tool before unloading to handle this scenario. However, if we're fine with that behavior, then this is fine.

[raulsntos]

Also, I don't really understand what case this is but we are only re-creating the script when it's marked with [Tool] so should it be changed too?

godot/modules/mono/csharp_script.cpp

Lines 928 to 950 in daec089

	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
	CSharpScript::StateBackup &state_backup = script->pending_reload_state[obj_id];
	// 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<PlaceHolderScriptInstance *>(si));
	obj->set_script_instance(script_instance);
	}
	}
	continue;
	}

[neikeq]

No, those are placeholder instances. If you were to re-create a C# script instance for them, that would be like turning every single script into a tool script after reloading.
The scripts we want to reload shouldn't have placeholder instances any way, as they are either tool scripts or scripts instantiated manually.

[raulsntos]

If we want to avoid that, we would need to keep a flag like was_tool before unloading to handle this scenario. However, if we're fine with that behavior, then this is fine.

I don't consider this behavior acceptable, since an user could have a node script with the [Tool] attribute running in the scene and then when removing the attribute the node's script will still be running in the scene with no way to stop it other than re-opening the editor. This will surely result in more confused users than the ones confused by the issue this PR is intending to solve.

So I have implemented the was_tool flag you describe to try and avoid the regression described, let me know if you think it's too hacky.

[raulsntos]

I think it should be good now, it no longer crashes and always reloads scripts if they were ever instantiated unless the user explicitly removes the [Tool] attribute.

[akien-mga]

Thanks!