Custom Objects created from C# have the wrong Script resource attached.

[cgbeutler]

Godot version:
v3.3.1-stable_mono_win64

OS/device including version:
Windows 10 64-bit

Issue description:
Any Object declared in C#, then instantiated with new MyObject() has an invalid script attached.
ResourceLoader uses a resource's resource_path as a kind of "cache key" to avoid reloading things. Each Script resource that is saved to "res://" is usually cached by the ResourceLoader using the script's file path as its resource_path. This ensures that the script is only loaded once. The "Minimal reproduction project" below is fairly small and reveals all in the following snippet:

    public override void _Ready()
    {
        var custom = new CustomNode();
        AddChild(custom);
        var custom_node_script = custom.GetScript() as Script;
        GD.Print( "custom node script path was \"" + custom_node_script.ResourcePath + "\"" );

        var custom2 = GD.Load<CSharpScript>("res://CustomNode.cs").New() as CustomNode;
        AddChild(custom2);
        var custom2_node_script = custom2.GetScript() as Script;
        GD.Print( "Should have been \"" + custom2_node_script.ResourcePath + "\"" );
    }

The above will result in:

custom node script path was ""
Should have been "res://CustomNode.cs"

In other words, when any object is created using the form new CustomNode(), the script that gets attached is not the one cached by ResourceLoader. Instead it is some detached script object.
This has major implications when saving and loading. For example, were I to set both nodes' owners to the main scene node and then pack and save it to 'user://Main.tscn', we get the following tscn:

[gd_scene load_steps=4 format=2]

[ext_resource path="res://CustomNode.cs" type="Script" id=1]
[ext_resource path="res://Main.cs" type="Script" id=2]

[sub_resource type="CSharpScript" id=1]

[node name="Main" type="Node2D"]
script = ExtResource( 2 )

[node name="@@2" type="Node" parent="."]
script = SubResource( 1 )

[node name="@@3" type="Node" parent="."]
script = ExtResource( 1 )

The issue can be seen by comparing the first sub_resource to the first ext_resource. The desired save result is the ext_resource, which has a path that can be used to load the script and then the object. The first sub_resource fails as a "Sub Resource" as it should have below it all the information needed to re-create the object. Indeed, if we try to re-load this scene, we get an empty and invalid CSharpScript object that contains no real info.
Hopefully that clearly highlights the issue with not having the cached version of the script attached.

Steps to reproduce:

1. Run the main scene in the minimal project below

OR

1. Create a custom script in C# that inherits from an Object, like a Node2D or Resource.
2. Create a second script in C# and construct a new object of the type created in step 1.
3. Run the second script. The created object will not have the proper script attached.

Minimal reproduction project:
https://github.com/cgbeutler/godot_bug_csharp_missing_script_resource_path

[neikeq]

We handle things in a different way for scripts instances from C# rather than from the engine side. I'll have to think how to solve this.

[cgbeutler]

@neikeq, yeah, seems like a tricky one.
Also note that not having the version cached by the rest of the engine has other implications, too, since that script resource reference can have metadata and other stuff slapped on it at run-time.

[cgbeutler]

Oh, possible duplicate of #38191

[neikeq]

I think this is happening because we're not setting a path for the CSharpScript we create here:

godot/modules/mono/csharp_script.cpp

Lines 2949 to 2980 in 729461b

	Ref<CSharpScript> CSharpScript::create_for_managed_type(GDMonoClass *p_class, GDMonoClass *p_native) {
	// This method should not fail, only assertions allowed
	CRASH_COND(p_class == nullptr);
	// TODO OPTIMIZE: Cache the 'CSharpScript' associated with this 'p_class' instead of allocating a new one every time
	Ref<CSharpScript> script = memnew(CSharpScript);
	initialize_for_managed_type(script, p_class, p_native);
	return script;
	}
	void CSharpScript::initialize_for_managed_type(Ref<CSharpScript> p_script, GDMonoClass *p_class, GDMonoClass *p_native) {
	// This method should not fail, only assertions allowed
	CRASH_COND(p_class == nullptr);
	p_script->name = p_class->get_name();
	p_script->script_class = p_class;
	p_script->native = p_native;
	CRASH_COND(p_script->native == nullptr);
	p_script->valid = true;
	update_script_class_info(p_script);
	#ifdef TOOLS_ENABLED
	p_script->_update_member_info_no_exports();
	#endif
	}

Reason we're not setting is likely because back then we didn't have a way to get the path for a class, but now we do:

godot/modules/mono/csharp_script.h

Line 386 in 729461b

HashMap<String, DotNetScriptLookupInfo> dotnet_script_lookup_map;

godot/modules/mono/csharp_script.h

Lines 69 to 72 in 729461b

	struct DotNetScriptLookupInfo {
	String class_namespace;
	String class_name;
	GDMonoClass *script_class = nullptr;

Not sure if having two CSharpScript instances alive with the same path may cause an issue with Godot's resource system though or if it's fine.

[cgbeutler]

@neikeq Yeah, I realize that this issue is probably really to get the TODO here done:

godot/modules/mono/csharp_script.cpp

Lines 2954 to 2955 in 729461b

	// TODO OPTIMIZE: Cache the 'CSharpScript' associated with this 'p_class' instead of allocating a new one every time
	Ref<CSharpScript> script = memnew(CSharpScript);

The CSharpScript object is not free of state. Scripts are Godot Objects and can hold runtime data like with SetMeta and AddUserSignal. Because of that, using a cached version is the "correct" solution to ensure that all objects with that script share and hot-reload that state.

[cgbeutler]

I've been meaning to look into the pull request here: #44879 that adds a script server to see if that can be used to complete the TODO. Feel free to beat me to it.

[cgbeutler]

@neikeq Looking at this again, I realize it wasn't clear in my comment before, but I was responding to your remark here

Not sure if having two CSharpScript instances alive with the same path may cause an issue with Godot's resource system though or if it's fine.

When I said "The CSharpScript object is not free of state. Scripts are Godot Objects and can hold runtime data like with SetMeta and AddUserSignal."
If you did have two resources with the same path, they would also have unpredictable behavior with registering to the changed signal and stuff. All that said, it could maybe work as a temporary fix, if needed, as state stuff like the above on a script would be extremely rare.

I would also make a note that having the new script take over the path would just shift the problem. Issues like #38191 would suddenly only happen if the script loaded twice, making for a nasty bug that would trigger randomly and blow away saved resource data willy nillily.

[cgbeutler]

@neikeq After digging into the code a bit, it looks like ResourceFormatLoaderCSharpScript::load currently builds a new object every time and sets the path on it:

RES ResourceFormatLoaderCSharpScript::load(const String &p_path, const String &p_original_path, Error *r_error) {
...
	script->set_path(p_original_path);
...

As such, I think your temporary fix of setting script path would work fine. Loading scripts from ResourceLoader has worked fine for me. Eventually both places should have the cache TODO worked out, but this temp fix could get rid of a lot of issues. Pretty sure it would fix #38191 as well. Have you tried the fix yet?

[raulsntos]

Can't reproduce in Godot 4.0 RC 3, but can still reproduce in Godot 3.5.1 so moving to the 3.x milestone.