C# Script Registration that use Generics can error out in the ScriptManagerBridge

[taylorhadden]

Godot version

4.1.stable.mono

System information

Godot v4.1.stable.mono - Windows 10.0.22621 - Vulkan (Forward+) - dedicated NVIDIA GeForce RTX 3080 (NVIDIA; 31.0.15.3640) - AMD Ryzen 7 5800X 8-Core Processor (16 Threads)

Issue description

This issue occurs when two scripts are derived from a generic script that uses the same type.

Say you have a script, GenericScript<T>:

using Godot;
using System.Collections.Generic;

public partial class GenericScript<T> : Node3D
{
  protected List<T> someList = new List<T>();
}

And we have two scripts that derive from that class with the same type parameter:

public partial class TestScript : GenericScript<float>
{
	// Called when the node enters the scene tree for the first time.
	public override void _Ready()
	{
		someList.Add(1);
	}
}

public partial class OtherTestScript : GenericScript<float>
{

}

Building the mono project while a scene is loaded will present an error. See the reproduction steps below.

Steps to reproduce

Step 1

Create a scene that has two nodes. One node has TestScript attached, the other node has OtherTestScript attached (see above for definition).

Step 2

Rebuild Mono from the MSBuild tab in the bottom right.

You will see the following error in the console:

modules/mono/glue/runtime_interop.cpp:1324 - System.ArgumentException: An item with the same key has already been added. Key: GenericScript`1[System.Single]
     at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
     at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
     at Godot.Bridge.ScriptManagerBridge.ScriptTypeBiMap.Add(IntPtr scriptPtr, Type scriptType) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs:line 23
     at Godot.Bridge.ScriptManagerBridge.TryReloadRegisteredScriptWithClass(IntPtr scriptPtr) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 579

Step 3A

If you Rebuild Mono again, you will see the following error and Godot will no longer be in a good state.

modules/mono/mono_gd/gd_mono.cpp:513 - .NET: Failed to unload assemblies. Please check https://github.com/godotengine/godot/issues/78513 for more information. (User)

Step 3B

If you instead close the scene so that no scenes are loaded and Rebuild Mono, the rebuild will appear successful and no errors will be reported.

Minimal reproduction project

A repro project can be found in this repository. At the time of writing this ticket, the latest version was 5f6983c.

The repro contains the scripts defined above, a test scene, and a README.md file with steps essentially the same as the reproduction steps above.

[taylorhadden]

If it ends up being that using generic scripts is simply not supported, I would suggest that this is added to the documentation and a warning error is produced upon contact with such a script.

[dannymate]

This is a good write up. I created a similar repro here (though I didn't make the connection between matching types) in a related issue but this should be its own thing or even supersede the previous issue if there's no other causes.

If it ends up being that using generic scripts is simply not supported, I would suggest that this is added to the documentation and a warning error is produced upon contact with such a script.

I really hope this isn't the case, I would despair.

Edit: I think the issue lies in why the classnames aren't being used in the dictionary instead of the base class. Some kind of compiler reload weirdness I would expect.

[ricaun]

Have the same problem,

I am going to try to recompile the Bridge and implement something to ignore repeated Type.

Like this code:

godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs

Lines 68 to 70 in bd6af8e

	// Due to partial classes, more than one file can point to the same type, so
	// there could be duplicate keys in this case. We only add a type as key once.
	_typePathMap.TryAdd(scriptType, scriptPath);

[ricaun]

Hello,

I rebuilt the GodotSharp and basically changed Add to TryAdd.
https://github.com/ricaun/godot/blob/09191b1bed27adc574cb444adf2c8781a7585fd0/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs#L24-L26

The message Missing class qualified name for reloading script happens sometimes, still need to figure out this part, but is normal because I'm ignoring some pointers that have the same type.

In the end, the reload looks like is working fine.

Here is my custom build using the 4.1.1-stable: https://github.com/ricaun/godot/releases/tag/4.1.1-stable-fix-generic-class

[dannymate]

Hello,

I rebuilt the GodotSharp and basically changed Add to TryAdd. ricaun/godot@09191b1/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs#L24-L26

The message Missing class qualified name for reloading script happens sometimes, still need to figure out this part, but is normal because I'm ignoring some pointers that have the same type.

In the end, the reload looks like is working fine.

Here is my custom build using the 4.1.1-stable: ricaun/godot@4.1.1-stable-fix-generic-class (release)

I have tested with the repro provided and I can't seem to get the issue to trigger. I used a Windows 10 machine. I'm on Linux though so I'm unable to test it thoroughly for any edge cases.

[ricaun]

I have tested with the repro provided and I can't seem to get the issue to trigger. I used a Windows 10 machine. I'm on Linux though so I'm unable to test it thoroughly for any edge cases.

Probably you could copy and replace the folder GodotSharp in Linux version and should work.
I only modified the GodotSharp.dll file, swapping should do the trick.

[CantyCanadian]

Hello,

I rebuilt the GodotSharp and basically changed Add to TryAdd. https://github.com/ricaun/godot/blob/09191b1bed27adc574cb444adf2c8781a7585fd0/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.types.cs#L24-L26

The message Missing class qualified name for reloading script happens sometimes, still need to figure out this part, but is normal because I'm ignoring some pointers that have the same type.

In the end, the reload looks like is working fine.

Here is my custom build using the 4.1.1-stable: https://github.com/ricaun/godot/releases/tag/4.1.1-stable-fix-generic-class

I cannot say if this comes with any side-effect, but I prefer Godot remaining in a stable state with it complaining of missing class qualified name than it breaking down and requiring a restart every 2 build. So, for the moment, you are my hero.

I've been having the same issue for the past few days and stumbled onto this thread from my investigations. Your build seem to do the trick for now. Hopefully a more stable solution will be found.

[dannymate]

I have tested with the repro provided and I can't seem to get the issue to trigger. I used a Windows 10 machine. I'm on Linux though so I'm unable to test it thoroughly for any edge cases.

Probably you could copy and replace the folder GodotSharp in Linux version and should work. I only modified the GodotSharp.dll file, swapping should do the trick.

Sorry it took me so long to get back to you. I actually had to replace the Windows exe with the linux executable. I was surprised it booted and that your changes were reflected.

Booted this up on my project that has a ton of generic classes that triggers this error a lot. I only got modules/mono/glue/runtime_interop.cpp:1324 - Missing class qualified name for reloading script (Happens when a generic class is used.) to trigger the one time on build.

The issues I listed here don't occur anymore. As far as I could tell there wasn't any regressions.

Do you think you could create a PR for this change? That way more people can test and maybe we can get some eyes on that last error that occurs.

The error seems to come from

godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs

Line 558 in a2f90d5

GD.PushError("Missing class qualified name for reloading script");

Edit:
// Due to generic classes, more than one class can point to the same type, so
// there could be duplicate keys in this case. We only add a type as key once.

This is what's triggering it I believe. If you look at the line I sent you, can see just after that, if it matches, it removes the matching <key, value> from the dictionary. So when the next generic class comes along with the same <key, value> it triggers the error because it doesn't return anything.

[ricaun]

godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs

Line 558 in a2f90d5

GD.PushError("Missing class qualified name for reloading script");

Edit: // Due to generic classes, more than one class can point to the same type, so // there could be duplicate keys in this case. We only add a type as key once.

This is what's triggering it I believe. If you look at the line I sent you, can see just after that, if it matches, it removes the matching <key, value> from the dictionary. So when the next generic class comes along with the same <key, value> it triggers the error because it doesn't return anything.

Yep I was thinking in create a PR but I didn't look the guidelines to do so. And I didn't figure out why this error happens.

I tried remove every thing or remove each Type e didn't works. The strange part is the error message only happens one time, after you rebuild the solution.

[dannymate]

I think it's caused by a mismatch in "_scriptTypeMap" & "_typeScriptMap" within the "ScriptTypeBiMap" class. As for why it only triggers once I'm not sure. What possible effect it causes I am also not sure.

It could also be being caused by this line https://github.com/godotengine/godot/blob/a2f90d565ad29edcb3bdab77bc7df51cdde8514a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs#L552C1-L553C1. I'm not sure of the contents of this method so it might make more sense as the error message is slightly different.

Edit: The only alternative I can think of to tryadd is to make Type unique somehow or provide a way to get a unique value for Type that can be converted back and forth.

[dannymate]

What if we tryadd "_typeScriptMap" first and if it fails we don't add to "_scriptTypeMap"? I wonder if that would cause any issues. Would be nice to have someone experienced with the C# glue to help.

I'm also confused about how two different Types can be equal to one another.

Edit: https://bradwilson.typepad.com/blog/2009/02/when-is-a-type-not-a-type.html
As possible guess I have.
I think Dictionary key checks use the Equals operator on Type. This equals check uses the Type.UnderlyingSystemType property to check. So if we used "type == key" it would return "false" but "type.Equals(key)" or in other words "type.UnderlyingSystemType == key.UnderlyingSystemType" is "true".

Edit 2: I tested to check if two generic types with the same arguments are always different and regardless of equals or == they both resolve to false so I guess it's not that.

[ricaun]

I guess there is some problem in here:
https://github.com/godotengine/godot/blob/a2f90d565ad29edcb3bdab77bc7df51cdde8514a/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs#L34C1-L36C95

At least the last time I looked in that place. Tried to add each type but didn't know.
Probably gonna add a lot of GD.Warning to understand how the unload works. And why when generic is used create repeated Type.

[dannymate]

It does look like the ScriptTypeBiMap is built from the _pathTypeBiMap.

This method seems to build the _pathTypeBiMap and does seem to get the Type.

[ricaun]

Hello, I tested this weekend some approaches and in the end didn't find a way to remove the warning (Missing class qualified name for reloading script).

I still didn't find out where all the class type is generated and request by Godot...

[dannymate]

It's probably just worth creating a PR then and getting more eyes on it then I'm unsure myself.

[Delsin-Yu]

It turns out the issue is related to Generic Derivatives holding empty Script Path on the native side, which requires further investigation, the approach I took seems like a workaround that just happens to suppress this issue: It makes ReflectionUtils.FindTypeInLoadedAssemblies returns null forever, and causing early returns on the caller site which prevents the new type to entering BiMap at all, and in our cause, only those duplicated generic derivatives are ever went down here.

The issue appears to be ReflectionUtils.FindTypeInLoadedAssemblies, current implementation returns a different instance of Type when the typeFullName is a generic type, weird.

AppDomain.CurrentDomain.GetAssemblies()
            .FirstOrDefault(a => a.GetName().Name == assemblyName)?
            .GetType(typeFullName)

This new approach appears to return the same instance, so it should fix related issues.

Type.GetType($"{assemblyName}.{typeFullName}")

Fix validated using This Repo, and our in-house project (which is extra complex and deeply suffers from this issue).

[Delsin-Yu]

#87550 should fix this issue. After deeper debugging, we believe this issue is caused by an incorrect script reloading order, that is and I quote from the PR:

After successfully performing an Assembly Unloading, it is crucial for the Native side to reload the list of unloaded scripts in a specific order.

For example: a ChildClass must have its BaseClass reloaded before getting passed into the C# reloading method, otherwise, since after unloading, there is no way for the C# side to recognize this new BaseClass, it will create and tie a new instance of BaseClass back to the Native side, this will eventually causing Duplicate Key Exception when the actual pending BaseClass is reloading.

CSharpScriptDepSort contains the algorithm for sorting these reload-pending scripts, this method relies on CSharpScript::get_base_script() for obtaining the base script of a CSharp script.
Unfortunately, for generic inheritances, the base type for a GenericChildClass : GenericBaseClass<int> is not GenericBaseClass<T> which we have a path for, instead the actual base type is GenericBaseClass<int> that never has a script_path, so the get_base_script() method always returns null for GenericChildClass in this case, hence it breaks the sorting method, causing unpredictable script reloading order, and we eventually get Duplicate Key Exception on the Managed side if that random order happens to be incorrect.

The PR adds a new get_base_script dedicated to this sorting algorithm, which has everything the original algorithm does, except it replaces the script_path empty checking with class_name empty checking, this should fix the script ordering issue.

However, GenericBaseClass<int> is a valid type, so the public API get_base_script should return the actual CSharpScript instance, instead of a null value, but assigning path to these in-between types proves challenging, we cannot duplicate the script_path property of the original script GenericBaseClass<T> since Godot resources don't allow multiple resources sharing the same path, so a separate issue and/or pr is required on implementing support for some sorts of virtual paths, such as res://path/to/GenericBaseClass.cs: Int32, but that is out of the scope of the PR.

[FeralPug]

Is there a solution for this problem I am currently running into this issue in v4.2.1.stable.mono.official.b09f793f5

[Delsin-Yu]

Is there a solution for this problem I am currently running into this issue in v4.2.1.stable.mono.official.b09f793f5

This issue should be fixed in 4.3, you can try 4.3 dev4 out and see if it is fixed on your use case.

[ultrasuperpingu]

I had this problem. I tried 4.3 dev5 and it seems gone (I had a error message at first but it disappeared and I didn't save it and wasn't able to make it appears again, sorry).

But on 4.2.1 stable, I had a pretty big and anoying issue, that I guess, was caused by this:

• Scripts exported values that was default in the scene was periodically changed to null or zeros by there own
• Connections to a signal on my scripts was lost periodically too

It seems to be ok with 4.3 dev5 but I thought it could be an interesting information for you :)

[maxloy]

I had this problem. I tried 4.3 dev5 and it seems gone (I had a error message at first but it disappeared and I didn't save it and wasn't able to make it appears again, sorry).

I just pulled down 4.3 dev5 in an attempt to fix this issue and got this error on the first load (haven't had errors since). Is this the same one you saw?

Assertion failed: Script path can't be empty.
    Details: 
     at Godot.GodotTraceListener.Fail(String message, String detailMessage) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/GodotTraceListener.cs:line 24
     at System.Diagnostics.TraceInternal.Fail(String message, String detailMessage)
     at System.Diagnostics.Debug.Fail(String message, String detailMessage)
     at Godot.Bridge.ScriptManagerBridge.GetGlobalClassName(godot_string* scriptPath, godot_string* outBaseType, godot_string* outIconPath, godot_string* outClassName) in /root/godot/modules/mono/glue/GodotSharp/GodotSharp/Core/Bridge/ScriptManagerBridge.cs:line 232

[ultrasuperpingu]

I just pulled down 4.3 dev5 in an attempt to fix this issue and got this error on the first load (haven't had errors since). Is this the same one you saw?

I can't tell it for sure, but I'd say probably, conditions seams similar enough :)
Thanks for the sharing