Open cgbeutler opened 3 years ago
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.
@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.
Oh, possible duplicate of #38191
I think this is happening because we're not setting a path for the CSharpScript
we create here:
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:
https://github.com/godotengine/godot/blob/729461b2a455bd3b4afb26bb362863a68e98a9ee/modules/mono/csharp_script.h#L386 https://github.com/godotengine/godot/blob/729461b2a455bd3b4afb26bb362863a68e98a9ee/modules/mono/csharp_script.h#L69-L72
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.
@neikeq Yeah, I realize that this issue is probably really to get the TODO here done:
https://github.com/godotengine/godot/blob/729461b2a455bd3b4afb26bb362863a68e98a9ee/modules/mono/csharp_script.cpp#L2954-L2955
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.
I've been meaning to look into the pull request here: https://github.com/godotengine/godot/pull/44879 that adds a script server to see if that can be used to complete the TODO. Feel free to beat me to it.
@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.
@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 https://github.com/godotengine/godot/issues/38191 as well. Have you tried the fix yet?
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.
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'sresource_path
as a kind of "cache key" to avoid reloading things. EachScript
resource that is saved to "res://" is usually cached by the ResourceLoader using the script's file path as itsresource_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:The above will result in:
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:The issue can be seen by comparing the first
sub_resource
to the firstext_resource
. The desired save result is theext_resource
, which has a path that can be used to load the script and then the object. The firstsub_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 invalidCSharpScript
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:
OR
Object
, like aNode2D
orResource
.Minimal reproduction project: https://github.com/cgbeutler/godot_bug_csharp_missing_script_resource_path