Improve handling of generic C# types

[raulsntos]

Improve handling of generic C# types

• Create CSharpScript for generic C# types.
  • ScriptPathAttributeGenerator registers the path for the generic type definition.
  • ScriptManagerBridge lookup uses the generic type definition that was registered by the generator.
  • Constructed generic types use a virtual csharp:// path so they can be registered in the map and loaded as if there was a different file for each constructed type, even though they all share the same real path.
    • Thanks so much to @Delsin-Yu for the idea.
  • This allows getting the base type for a C# type that derives from a generic type.
• Shows base scripts in the Add Node and Create Resource dialogs even when they are generic types.
  • get_global_class_name implementation was moved to C# and now always returns the base type even if the script is not a global class (this behavior matches GDScript).
• Create CSharpScript::TypeInfo struct to hold all the type information about the C# type that corresponds to the CSharpScript, and use it as the parameter in UpdateScriptClassInfo to avoid adding more parameters.
  • The fields of the struct, as well as other CSharpScript fields, are also now documented 🙂.
• Fixes C# generaic usage can cause Duplicate Key Exception when performing Assembly Reloading #87877

There's also a little bit of background on these previously merged PRs:

• Fix C# Godot.SourceGenerators for generic classes #55597.
• C#: Skip getting class info for unbound generics #70511.

[zaevi]

I met the exception when selecting a node with generic type inherited:

  C:/Users/zaevi/source/Godot/godot-source/modules/mono/glue/GodotSharp/GodotSharp/Core/NativeInterop/ExceptionUtils.cs:113 - System.IO.IOException: 文件名、目录名或卷标语法不正确。 : 'C:\Users\zaevi\source\Godot\temp\crash_test\csharp:\generic\Base.cs:Base`1[System.Int32].cs'
     at System.IO.FileSystem.GetAttributeData(String fullPath, Boolean returnErrorOnNotFound)
     at System.IO.File.GetLastWriteTime(String path)
     at GodotTools.Utils.File.GetLastWriteTime(String path) in C:\Users\zaevi\source\Godot\godot-source\modules\mono\editor\GodotTools\GodotTools\Utils\File.cs:line 25
     at GodotTools.Inspector.InspectorPlugin._ParseBegin(GodotObject godotObject) in C:\Users\zaevi\source\Godot\godot-source\modules\mono\editor\GodotTools\GodotTools\Inspector\InspectorPlugin.cs:line 27
     at Godot.EditorInspectorPlugin.InvokeGodotClassMethod(godot_string_name& method, NativeVariantPtrArgs args, godot_variant& ret) in C:\Users\zaevi\source\Godot\godot-source\modules\mono\glue\GodotSharp\GodotSharpEditor\Generated\GodotObjects\EditorInspectorPlugin.cs:line 156
     at GodotTools.Inspector.InspectorPlugin.InvokeGodotClassMethod(godot_string_name& method, NativeVariantPtrArgs args, godot_variant& ret) in C:\Users\zaevi\source\Godot\godot-source\modules\mono\editor\GodotTools\GodotTools\Godot.SourceGenerators\Godot.SourceGenerators.ScriptMethodsGenerator\GodotTools.Inspector.InspectorPlugin_ScriptMethods.generated.cs:line 50
     at Godot.Bridge.CSharpInstanceBridge.Call(IntPtr godotObjectGCHandle, godot_string_name* method, godot_variant** args, Int32 argCount, godot_variant_call_error* refCallError, godot_variant* ret) in C:\Users\zaevi\source\Godot\godot-source\modules\mono\glue\GodotSharp\GodotSharp\Core\Bridge\CSharpInstanceBridge.cs:line 24

Seems it tries to check LastWriteTime using that virtual path.

[paulloz]

At that point, if this condition is true, it is possible that scriptPath already contains a csharp:// path, right? If that's the case, GetVirtualConstructedGenericTypeScriptPath will throw.

[raulsntos]

That's a really good point, but I don't think it actually ever happens in practice. We only reach this code when GetPathOtherwiseGetOrCreateScript returns true, and that only happens when there is no existing script registered in _scriptTypeBiMap.

The first time this code executes, when the script hasn't been created yet and thus it's not in _scriptTypeBiMap, the virtual path (csharp://) will also not have been added to _pathTypeBiMap. At this time, the script is created and added with the virtual path to both bimaps, so this code should not be reachable again after that.

[paulloz]

I've seen it raised a few times when I was testing earlier. But can't reproduce reliably. I'm wondering if that might have been a compounding error from the one @zaevi reported above (that I witnessed too).

[raulsntos]

I was wrong, if the script is freed then it can happen because the path will still be in _pathTypeBiMap but the script will have been removed from _scriptTypeBiMap. I guess I will also check here if the path starts with csharp:// and avoid entering the if.

[raulsntos]

@zaevi Interesting, it doesn't throw in Linux so I didn't notice. Should be fixed now.

[paulloz]

I'm having weird behaviours with export updates + the error I'm talking about above. See video attached.

godot.windows.editor.dev.x86_64.mono_6ur3hBmru1.mp4

[zaevi]

I'm having weird behaviours with export updates + the error I'm talking about above. See video attached.

godot.windows.editor.dev.x86_64.mono_6ur3hBmru1.mp4

Could be caused by the cache. It's ok after I delete the .godot folder (and rebuild), but updating the exported property and reloading the project throws the exception again.

[raulsntos]

@paulloz Thanks, I can reproduce following the steps in the video. I'm having the same behavior in 4.2.1-stable. The inspector plugin does not seem to be called at all when modifying generic scripts, but it doesn't seem to be introduced by this PR.

Oh I see, for generic scripts like res://Foo.cs we end up creating an empty CSharpScript so it's always valid = false, and since it's invalid it never refreshes the inspector:

godot/modules/mono/csharp_script.cpp

Line 2223 in d335281

if (!valid) {

We would not be able to update the exports anyway, since it's a generic type definition and we can't call C# methods on an unbound generic. Not sure how we could fix this so I guess I'll leave it for the future since it's a pre-existing issue.

I'm also able to reproduce the other error you mentioned, I don't know why I didn't encounter it before.

[paulloz]

The inspector plugin does not seem to be called at all when modifying generic scripts, but it doesn't seem to be introduced by this PR.

You are right about the inspector plugin, but the exports themselves should appear after a build. They currently do on master:

godot.windows.editor.dev.x86_64.mono_9zcHO52g8q.mp4

We would not be able to update the exports anyway, since it's a generic type definition and we can't call C# methods on an unbound generic.

But at that point the generic type is fully bound, since it's the once in the class hierarchy of a valid node.

[raulsntos]

You are right about the inspector plugin, but the exports themselves should appear after a build. They currently do on master

Oh, you are right. I feel like I tested this and it wasn't working. Thank you for testing.

But at that point the generic type is fully bound, since it's the one in the class hierarchy of a valid node.

I mean the empty CSharpScript with the path res://Foo.cs, not the CSharpScript with the virtual path csharp://Foo.cs:Foo`1[System.String].cs that is part of the Bar hierarchy.

I have checked and it seems _update_exports is never called on Foo<string> because AddScriptBridge returns false since it can't find the virtual path in the _pathTypeBiMap at this point of the reloading. This is probably because of the reload order, Foo<string> comes first since it's the base script but the virtual path won't be added until we construct it when reloading Bar which comes afterwards. I'm starting to think the complexity of these virtual paths may be too high.

[Delsin-Yu]

Back to the reloading order again... Is it possible to somehow refractor the reloading logic to ensure this editor reserialization call comes after a complete iteration of to_reloads? This will remove the need to bother with script reloading order any more.

[raulsntos]

Is it possible to somehow refactor the reloading logic to ensure this editor reserialization call comes after a complete iteration of to_reloads?

That sounds like it require drastic changes to the reload logic which risk introducing bugs, I'm not sure it's worth doing such a big change since we'll be moving away from scripts in the future.

---

the exports themselves should appear after a build. They currently do on master

I was curious why it was working in master since we also don't have a path to the script there and found out that scripts without paths actually go through a different path when reloading assemblies.

So I made scripts with csharp:// paths be reloaded in the same way (as if they had no path) and it seems the exports update now.

Edit: Also rebased to fix the conflict with InspectorPlugin.

[paulloz]

Seems to work fine from my tests. It's still possible we'll have some weird behaviours since it's a large-ish change, but I'm fairly confident 👌

[akien-mga]

Thanks!

[EduardoLemos567]

That sounds like it require drastic changes to the reload logic which risk introducing bugs, I'm not sure it's worth doing such a big change since we'll be moving away from scripts in the future.

@raulsntos

Could you, please, elaborate a bit on "we'll be moving away from scripts in the future" ? :)
I'm very interested on the future of C# in Godot.

Also if there's any roadmap for C# features in the future, could you or anyone point me the link(s) ?

Thank you.