C#: Add global class support

[raulsntos]

Port of #63126 originally implemented by @willnationsdev to the current master.

Adds support for C# to be able to define global classes using the [GlobalClass] attribute and export global script classes using the [Export] attribute.

Differences from the original PR

• The additional ExportAttribute constructor is not included.
  • It seems this did not make it in the GDScript version either and I think it may be better as a separate attribute anyway.
• The [Global] attribute was renamed to [GlobalClass].
  • I think the original name was a bit too vague, but to be honest I'm also not very happy with the new name either. I'll gladly take suggestions.
• The [GlobalClass] attribute always uses the name of the C# class.
  • The original implementation allowed specifying a different name but I don't think that's useful in C# where classes already have names.
• The [GlobalClass] attribute does not contain the icon path, a separate [Icon] attribute is provided.
  • The [Icon] attribute allows specifying the path to the icon of a C# class similar to the @icon annotation in GDScript.
  • This allows specifying an icon even when the class is not registered as a global class.
• The EditorFileSystem::update_file call was moved from CSharpLanguage::reload_assemblies to ScriptManagerBridge.LookupScriptsInAssembly.
  • The original implementation wasn't working properly, global script classes weren't getting registered when they should.
  • @neikeq helped me figure out how to make it work.

Related issues and PRs

• Fixes Add first-class custom resource support godot-proposals#18 for C#.
• Closes Add C# resource export. #63126 as superseded.
• Fixes [C#] Godot can't find C# mainloop  #69795.
• Fixes Add support for registering global script classes (class_name) using C# #26875.
• Closes Registering Global Script Classes under Mono #48056 as superseded.

[willnationsdev]

The [GlobalClass] attribute always uses the name of the C# class.

The original implementation allowed specifying a different name but I don't think that's useful in C# where classes already have names.

Haven't had time to actually review it yet, but I will mention now that the reason I had made the mapped Godot name customizable (but defaulting to the C# class name) was because Reduz mentioned having no intention of introducing namespacing into Godot's centralized user types. If people start namespacing their types using underscored prefixes and the like (MyLib_MyNode), you don't want that forcing C# users to have to adopt an identical pattern for whatever reason just to create a C# class that follows the same convention as some other library to preserve consistency in the Godot user interface. Hence, the ability for users to manually set the name to whatever they really want it to be should the need arise.

[raulsntos]

I haven't thought about that, but assuming the user's class is inside a namespace we could probably combine the class name with its namespace to register it as a global class and that'd be less likely to conflict. We could also add the ability to customize the name in the future without breaking compatibility so I'd prefer not yo add it for the first implementation.

[RedworkDE]

Some actual testing later:

1. Things generally seem to work fine.
2. ResourceLoader.Load<GlobalResource>() throws a InvalidCastException when loading a resource:

[GlobalClass]
public partial class ResCS : Resource
{
	[Export]
	public string Name;
}

try
{
	OS.Alert(ResourceSaver.Save(new ResCS() { Name = "string" }, "res://rcs.tres").ToString());
	OS.Alert(ResourceLoader.Load<ResCS>("res://rcs.tres").Name);
}
catch (Exception ex)
{
	OS.Alert(ex.ToString());
}

3. When using custom resource loaders and savers, they will sometimes not get reregistered, but I have not narrowed down what exactly the issue is. It is somewhat related to

   godot/editor/editor_file_system.cpp

   Lines 1582 to 1588 in 604493e

   	// Rescan custom loaders and savers.
   	// Doing the following here because the `filesystem_changed` signal fires multiple times and isn't always followed by script classes update.
   	// So I thought it's better to do this when script classes really get updated
   	ResourceLoader::remove_custom_loaders();
   	ResourceLoader::add_custom_loaders();
   	ResourceSaver::remove_custom_savers();
   	ResourceSaver::add_custom_savers();

   because without global classes they always get removed there and never readded, so the build I normally use has those lines commented out. With this PR the loaders (marked as GlobalClass) usually survive the script update, but not always.

[raulsntos]

I have been unable to reproduce 2. Can you share the res://rcs.tres file that was created by the ResourceSaver?

While testing 2, I think I've run into 3 but I'm not sure of the exact steps to reproduce it.

[RedworkDE]

OutputTest.zip
Also includes the rest of the project and a GDScript version that (if my understanding of typed GDScript is correct) should be equivalent and works fine.

[raulsntos]

OK, I think I understand what you meant now. Your maincs.cs script is [Tool] so the script is executed in the editor, when opening the main scene that has the script attached the _Ready method is invoked. However, because the RelCS class is not marked as [Tool] it's not recognized by the editor, non-Tool scripts are loaded as Resource. This is an existing limitation and not related to this PR. If you want your resource types to be available in the editor you need to annotate them with the [Tool] attribute as well.

[RedworkDE]

Point 3 also boils down to user error / missing [Tool] annotations. For some reason at startup, ResourceFormatLoader / Saver without tool are registered and used; but after a file system reload loaders without tool are not reregistered and their files disappear.

While debugging this I found one additional oddity: The global class list (in the debugger and the new method added on master) contains types not marked as [GlobalClass] one derived from EditorPlugin and one from ScriptLanguageExtension, tho I am unsure if this is an issue or expected behavior.

[raulsntos]

Point 3 also boils down to user error / missing [Tool] annotations.

I think global classes should be registered regardless of having the [Tool] annotation, I think that's how it works in GDScript, scripts with class_name don't need to have the @tool annotation to be registered as a global class.

Also, in my testing classes were sometimes unregistered even if they had the [Tool] annotation, so it doesn't seem related to me.

While debugging this I found one additional oddity: The global class list (in the debugger and the new method added on master) contains types not marked as [GlobalClass]

That's odd, while testing I have also been able to see some classes without the [GlobalClass] attribute added to the global classes list, I have looked at the current GDScript implementation of get_global_class_name and I have changed the C# implementation to follow it more closely. It seems to have fixed this issue.

I also realized I wasn't assigning some out parameters in ScriptManagerBridge.UpdateScriptClassInfo in the catch block. I don't think this is related to the issues mentioned above since I didn't see any exception logged in the console so the catch block wasn't reached.

[dotlogix]

Does this PR also allow us now to export custom node types as classes or is it an unrelated issue?
It would be really nice to finally be able to do that especially for theming :)

[raulsntos]

This PR should allow you to use any C# class derived from Node or Resource registered with the [GlobalClass] attribute in any of the contexts where you can use a GDScript class named with the class_name syntax. This means it should show up in the Add Node and Create Resource dialogs. Exporting custom resources (i.e.: a type that derives from Resource with the [GlobalClass] attribute) should also restrict the types of resource that can be assigned in the inspector and will let you create and load instances of that resource type more easily.

I don't have screenshots from this PR, but these screenshots from the related GDScript PR and the GDScript documentation for named classes should give you an idea of where these C# global classes would show up:

By the way, this should allow us to finally get rid of add_custom_type which AFAIK is currently only used as a workaround for the lack of global class support in C#, but we may be too late for such a breaking change.

[bleuthoot-sven]

I have been testing this build for a short while and noticed this bug:

• When creating a resource and adding the [GlobalClass] attribute to it, doesn't make it available for use in a node [Export] variable unless I restart Godot.

Godot_CSharp_bug.mp4

Project I used:
CSharp Test.zip

[raulsntos]

The EditorFileSystem->update_file method was being called on LookupScriptsInAssembly which is called when the assembly is loaded, but it seems this may be too soon and the script is still not valid and not added to the ScriptTypeBiMap so the base ended up empty. I have now added EditorFileSystem->update_file calls to the CSharpScript::reload and CSharpScript::reload_registered_script methods after the script becomes valid.

@bleuthoot-sven Thanks for testing, I think this issue may be fixed now, can you try the latest version of this PR? It's in commit 5f73edd5d94cdf659fd842410f3b605cab73cbc5, also make sure to clear the GodotNuGetFallbackFolder directory.

[bleuthoot-sven]

The EditorFileSystem->update_file method was being called on LookupScriptsInAssembly which is called when the assembly is loaded, but it seems this may be too soon and the script is still not valid and not added to the ScriptTypeBiMap so the base ended up empty. I have now added EditorFileSystem->update_file calls to the CSharpScript::reload and CSharpScript::reload_registered_script methods after the script becomes valid.

@bleuthoot-sven Thanks for testing, I think this issue may be fixed now, can you try the latest version of this PR? It's in commit 5f73edd, also make sure to clear the GodotNuGetFallbackFolder directory.

Yup, seems to work fine now.

---

I have one additional question that is very likely not related to this PR, but discovered it while playing around with this PR.

When adding a resource to a node [Export] variable, the field will return to <empty> when I re-select the node. This not only happens in C#, but also with GDScript.

Is this intentional or a (known) issue?

[raulsntos]

When adding a resource to a node [Export] variable, the field will return to <empty> when I re-select the node. This not only happens in C#, but also with GDScript.

If it also happens in GDScript, then it seems like the issue is in the inspector code. So it's out of scope for this PR. Not sure if it's a known issue though.

[RedworkDE]

Fixes #69795

Edit: Went looking though the rest of the issues on the 4.0 milestone and found a few more.

Fixes #26875
Probably Fixes #38191
Probably Supersedes #48056

[RedworkDE]

I briefly tested the current state and didn't manage to immediately crash it again (or find any glaring issues), I'll take a deeper look later.

[markdibarry]

Tested a few scenarios in a new project and an existing one. No problems. Looks good!

[paulloz]

Can confirm the issue I mentioned above seem to be fixed now.

[RedworkDE]

Generally LGTM otherwise, is probably in a state where it could be merged and improved in followups when issues crop up.

I tested with custom resources, nodes, resource loader and main loops.

I encountered two minor somewhat related issues:

ERROR: Script does not inherit a CustomResourceLoader: res://res_loader.cs.
   at: (core\io\resource_loader.cpp:1104)

Appears a few times when (re-)loading assemblies, but the resource loaded worked just fine anyways.
Loader in question:

using Godot;
using System;

[GlobalClass]
[Tool]
public partial class res_loader : ResourceFormatLoader
{
	public override string[] _GetRecognizedExtensions() => new[] { "test_res" };
	public override Variant _Load(string path, string originalPath, bool useSubThreads, int cacheMode) => new res3();
}

And one times when exiting the editor I got the error as some scripts apparently survived the language, tho not sure if that is actually related to this PR:

ERROR: FATAL: DEV_ASSERT failed  "_first == nullptr" is false.
   at: SelfList<class CSharpScript>::List::~List (C:\dir\godot_3\core/templates/self_list.h:114)

================================================================
CrashHandlerException: Program crashed
Engine version: Godot Engine v4.1.dev.mono.custom_build (e46806eb86f3bc0ad65ff303a57456a2185d6c4a)
Dumping the backtrace. Please include this when reporting the bug to the project developer.
[0] SelfList<CSharpScript>::List::~List (C:\dir\godot_3\core\templates\self_list.h:114)
[1] CSharpLanguage::~CSharpLanguage (C:\dir\godot_3\modules\mono\csharp_script.cpp:1235)
[2] CSharpLanguage::`scalar deleting destructor'
[3] uninitialize_mono_module (C:\dir\godot_3\modules\mono\register_types.cpp:74)
[4] uninitialize_modules (C:\dir\godot_4.0.1\modules\register_module_types.gen.cpp:279)
[5] Main::cleanup (C:\dir\godot_3\main\main.cpp:3566)
[6] widechar_main (C:\dir\godot_3\platform\windows\godot_windows.cpp:183)
[7] _main (C:\dir\godot_3\platform\windows\godot_windows.cpp:205)
[8] main (C:\dir\godot_3\platform\windows\godot_windows.cpp:217)
[9] __scrt_common_main_seh (D:\a\_work\1\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
[10] <couldn't map PC to fn name>
-- END OF BACKTRACE --
================================================================

[GeorgeS2019]

@raulsntos
@paulloz

We are trying to create a c# binding to GDExtension Google Mediapipe.
If I understand correctly, this is the PR

[akien-mga]

We could add a DEV_ASSERT, but for production user templates is it really worth crashing instead of just erroring out? Or is it impossible to recover once we're here?

[RedworkDE]

It is only called behind a IsEditorHint check, so it should be unreachable unless someone messes around with reflection (except that the check would have crashed again, so good that you commented and I double checked)

Thb calling this outside the editor means just that there is a logic error in the caller, so a DEV_ASSERT or just a normal ERR_FAIL would be enough here.

[raulsntos]

It is as RedworkDE says, this code path should be unreachable so I used a crash because a user should never see it anyway. But it makes sense to me to use a DEV_ASSERT too so I changed it. Let me know if the check is backwards.

[akien-mga]

I'm no expert on the C# integration, but the scope of the changes looks sensible to me, and this was well tested and reviewed by @RedworkDE.

Would love to get @neikeq's eyes on it too to give more confidence that this is the right approach, but I think we can benefit a lot from merging this this week to include in 4.1 dev 4 for wider testing, so we can polish in the beta phase.

[RedworkDE]

I double checked all the interop methods again and did not find any other issues or discrepancies with them.

[YuriSizov]

Congratulations everyone and big thanks to Raul and Will for the work they've put into giving our C# users a powerful feature and parity with GDScript users! Let's catch all the bugs during the bug-fixing phase and send this to everyone in one month with the stable release 🎉

[cpuccino]

Bit late to the party but is there any chance we can change the name to something like RegisteredType instead - which is what this repo's doing - https://github.com/Atlinx/Godot-Mono-CustomResourceRegistry - GlobalClass just sounds a bit weird imo

Either way, really really awesome job everyone :D 🎉 🎉

[Shadowblitz16]

Why was C# not compiled directly to a new type?
It's still treated as a script instead of actually using gd-extension