C#: Add abstract class support

[398utubzyt]

At the moment, Godot internally does not support abstract scripts. This poses an issue when trying to create abstract classes in languages such as C#, because it has no way of knowing that trying to instantiate an abstract class is an invalid operation.

Abstract scripts appearing in the create dialog can no longer be instantiated as is the case with built-in classes such as CanvasItem. This also allows global abstract classes to remain untouched and functional. Furthermore, non-global C# classes no longer throw an error about an empty inheriting script path.

• Supersedes C#: Error when registering abstract class as global #80304.
• Fixes C#: CSharpScript.CanInstantiate() returns true for abstract classes but .New() throws at runtime #78244.

[raulsntos]

Thanks a lot! I've been meaning to add proper support for abstract classes to C#.

This is a review of the code only, I haven't had a chance to build and test it yet.

[raulsntos]

I have built and tested the PR and looks great, but the inspector's resource picker still lets you choose abstract classes. You'll have to modify EditorResourcePicker to ignore abstract classes so they don't show up:

godot/editor/editor_resource_picker.cpp

Line 512 in bc88dca

if (!is_custom_resource && !(ScriptServer::is_global_class(t) || ClassDB::can_instantiate(t))) {

However, it has crossed my mind that maybe we should change ClassDB::can_instantiate instead, so it considers abstract global classes non-instantiable. The related PR #67777 makes similar changes but it changes ClassDB::is_virtual.

[398utubzyt]

it has crossed my mind that maybe we should change ClassDB::can_instantiate instead

#67777 seems to make a claim about ClassDB virtual classes being the same as abstract classes. Should there be a similar change to ClassDB::is_virtual as well?

If that ends up being desirable, I wonder if it would be beneficial to move these kinds of checks on global classes to a new ScriptServer function. That would be out of the scope of this PR, but is something I can look into in the future.

[raulsntos]

#67777 seems to make a claim about ClassDB virtual classes being the same as abstract classes. Should there be a similar change to ClassDB::is_virtual as well?

I think this may be because GDScript @abstract classes aren't really abstract, they behave more like virtual classes and can still be instantiated although the editor will prevent you from doing so. In fact the annotation was originally @virtual. This PR is still unmerged though, so it could change.

I'm not sure what would be the correct changes to make to ClassDB. At least for C#, since abstract classes are really abstract and can't be instantiated, I feel like it makes more sense to add the check to ClassDB::can_instantiate. However, we already return false in this case in CSharpScript::can_instantiate.

So, maybe CreateDialog should be checking if a script is instantiable instead of checking if the script is abstract. But there may be undesirable consequences to doing this change that I can't think of right now.

Adding a similar change to ClassDB::is_virtual sounds reasonable to me. It seems the editor doesn't allow instantiating these classes either. From what I can see it always checks both (!can_instantiate and is_virtual).

I wonder if it would be beneficial to move these kinds of checks on global classes to a new ScriptServer function. That would be out of the scope of this PR, but is something I can look into in the future.

That sounds reasonable to me if it can simplify some of these checks. I'd like to know the opinion of others as well, but yeah it's out of scope for this PR and can be done in a follow-up.

[398utubzyt]

Sounds good. I'll write those ideas down for future PRs. I went ahead and added the check to ClassDB::is_virtual in my previous commit, so it should be good for review as is.

[398utubzyt]

Forgot to expose Script::is_abstract through ClassDB.

[raulsntos]

Looks good, I tested it and works as expected. Needs a rebase after #80184.

[aaronfranke]

So, maybe CreateDialog should be checking if a script is instantiable instead of checking if the script is abstract. But there may be undesirable consequences to doing this change that I can't think of right now.

It's better the way it is now, since we don't want to present creating virtual/abstract classes to the user. This is the entire purpose of marking it as virtual/abstract. Whether it can be technically instantiated on the C++ or C# side is a secondary concern, we do of course want those to be forbidden too, but CreateDialog needs to consider more than that.

Since this PR has overlap with #67777, and the core changes in this PR are a superset of #67777, I wonder if #67777 should be merged first. But I haven't yet investigated why it fails CI (all I know is that it did not fail CI when I first opened the PR, something changed in the engine after I made the PR and I rebased it). EDIT: I have updated #67777, the core changes are now different, instead I localized the logic into CreateDialog::_is_type_abstract.

[398utubzyt]

Since this PR has overlap with #67777, I wonder if #67777 should be merged first.

Since this PR goes a bit further than just CreateDialog, I'm not sure if merging #67777 first would be desirable. Instead, I believe this PR would provide a good base for #67777 simply because it is focused on providing internal logic for abstract classes in order to fix pre-existing bugs with C# rather than implementing a new GDScript feature.

Plus, there are some changes I've made regarding the GDExtension implementation and a few ClassDB methods after @raulsntos' review that I think would also need to be made in #67777 in order to make it work in other places (such as EditorResourcePicker) and also not break GDExtension compat.

I would certainly love to see what others think, though.

So, maybe CreateDialog should be checking if a script is instantiable instead of checking if the script is abstract. But there may be undesirable consequences to doing this change that I can't think of right now.

To add onto what @aaronfranke mentioned, Script::can_instantiate returns false while inside the editor unless it's called on a tool script (in which case it always returns true prior to this PR) which can lead to undesirable results when used as a method for checking abstractness.

[dsnopek]

I didn't test, but the GDExtension code in this PR looks good to me!

[reduz]

Looks good on my side, but make sure both the GDScript and Mono teams approve.

[398utubzyt]

Rebased after #81487.

[398utubzyt]

I realized it may be beneficial to add an error message to Object::set_script since it doesn't seem like there is a way to filter abstract scripts out of the file dialog. See changes here.

[398utubzyt]

make sure both the GDScript and Mono teams approve.

Just want to clarify, is GDScript approval necessary here? Afaik the only change I made there is a basic one-line override of Script::is_abstract which only returns false due to GDScript not supporting abstraction yet.

@reduz

[akien-mga]

Thanks!