godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.17k stars 98 forks source link

Add support for abstract classes in GDScript #5641

Open aaronfranke opened 2 years ago

aaronfranke commented 2 years ago

Describe the project you are working on

A game where I have several scripts which I do not want to instance, but other scripts extending those types can be instanced.

Describe the problem or limitation you are having in your project

There is no way to indicate that a named class type should not be instanced in a way that will make the editor show them as disabled in the Create New Node dialog and also the resource picker dialog.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The proposal is to add an annotation that marks a class as abstract. For example:

@abstract
class_name MyAbstract extends Node

Bikeshedding: Should we call this feature @abstract (❤️) or @virtual (🚀)? The engine has both abstract and virtual. In GDScript such a feature would be closer to the engine's virtual classes (which can be instanced in C++, but they are not intended to be) rather than the engine's abstract classes (which cannot be instanced at all in the C++ code). But abstract would likely be a more friendly term for users.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

I have implemented it here: https://github.com/godotengine/godot/pull/67777

Here is a picture of the "Create New Node" dialog:

Screen Shot 2022-10-23 at 1 41 22 PM
@abstract
class_name MyAbstract extends Node
class_name ExtendsMyAbstract extends MyAbstract

In the image MyAbstract is @abstract, while the other is not. Note that if ExtendsMyAbstract was made abstract then both would be hidden.

Screen Shot 2022-10-23 at 3 12 40 PM

In my implementation so far it currently only affects the editor dialogs and will prevent being instanced from code via .new(), it could be extended to show up in the in-editor documentation.

If this enhancement will not be used often, can it be worked around with a few lines of script?

There are two work-arounds. One is to ignore the problem. Another is to not give your abstract classes class_name, but this requires you to use extends "my_script.gd" to extend them, and if you want to use the is operator then you have to use const MyScript = preload("my_script.gd") in every file where you refer to that type.

Is there a reason why this should be core and not an add-on in the asset library?

It can't, this directly affects the editor code and the GDScript language.

Mickeon commented 2 years ago

~At that point, I feel like it would make sense to extend the annotation to have it work with individual methods...~ Scratch that, it wouldn't exactly be useful.

Although, I'm not sure about neither virtual, nor abstract, when it comes to the name. Something about it... Sure, it's pretty much standard terminology nowadays, but it doesn't feel... "Godot". I would not like to exaggerate, because at the end of the day these are probably only going to be used by extensions, but that terminology may risk polluting the language.

Mickeon commented 2 years ago

In this image MyVirtual is @virtual, while the other two are not (I wanted to show that a virtual script can both extend from another script and be extended by another script). Note that if ExtendsMyVirtual was made virtual then both would be hidden, because virtual classes are only shown if they have non-virtual child classes.

With the way this works, could it not be called @hidden? Simpler language, and still describes what it does. Doesn't mean the class can't be extended. Perhaps, someday, the annotation could be expanded to be applicable to properties and methods (Godot's private keyword friendly equivalent?)...?

Kemeros commented 2 years ago

@aaronfranke virtual and abstract are two different concepts so i don't think the naming is quite right if i understand the goal correctly. You want to prevent scripts/classes instantiation, yes?

Also in my implementation so far it currently only affects the editor dialogs, it could be extended to show up in the in-editor documentation, and maybe have checks to prevent users from instancing such scripts directly.

If the goal is to prevent instantiation, it should be prevented both in editor dialogs and also in code. Code should only permit to extend abstract scripts/classes.

If i am understanding the goal correctly, the annotation name should be called @abstract.

@Mickeon

Sure, it's pretty much standard terminology nowadays, but it doesn't feel... "Godot".

With the way this works, could it not be called @hidden? Simpler language, and still describes what it does. Doesn't mean the class can't be extended.

@hidden would only make sense if the goal is to hide scripts/classes from the editor dialogs without preventing instantiation through code. Is that a good idea though? It feels like it would only be half a feature to me.

Perhaps, someday, the annotation could be expanded to be applicable to properties and methods (Godot's private keyword friendly equivalent?)...?

Having private methods and properties would be cool but shouldn't that be a different proposal? I'm not sure it's related here?

Edit: More thoughts: @virtual could be confusing as virtual base classes exist and are used to prevent the infamous diamond shaped inheritance that cause ambiguity. Maybe i'm overthinking this?

aaronfranke commented 2 years ago

I updated the proposal and PR to use @abstract instead of @virtual. The feedback has been overwhelmingly in favor of @abstract from many users.

aaronfranke commented 2 years ago

If the goal is to prevent instantiation, it should be prevented both in editor dialogs and also in code. Code should only permit to extend abstract scripts/classes.

I figured out a way to do it:

Screen Shot 2022-10-23 at 3 12 40 PM
if (is_constructor && base_type.is_meta_type) {
    Ref<GDScript> scr = ResourceLoader::load(base_type.script_path, "GDScript");
    if (scr.is_valid() && scr->is_abstract()) {
        push_error(vformat(R"*(Cannot call constructor on abstract class "%s".)*", scr->name), p_call);
    }
}
jabcross commented 2 years ago

Not a fan. Abstract classes make more sense in fully compiled languages.

GDScript is interpreted, adding syntax features has a runtime cost for every script. And there's nothing keeping you from just using normal classes and treating them as abstract.

You can always add a check to the constructor: if get_type() == "ClassName" : // throw error. This should throw an error if you call the constructor but not if you call a derived class's constructor.

jabcross commented 2 years ago

In you original post, who are the "users"? Is this a game where players program in GDScript?

aaronfranke commented 2 years ago

@jabcross That's a good point, in fact the check in my most recent comment really only exists as a tool to help users write stricter code, and it can be completely skipped on release builds. I updated the PR to add #ifdef DEBUG_ENABLED around that code so that it's skipped on target=template_release builds (production builds).

However, the rest of the feature is basically free. Setting the is_abstract flag only happens if the script contains @abstract, and reading this value only happens when calling ClassDB::is_virtual, which is only used in the editor.

In you original post, who are the "users"? Is this a game where players program in GDScript?

Sorry, I don't know how that word slipped in there. I meant that I don't want other devs of the game to instance them. I may have wrote that since I was simultaneously thinking of the use case of shipping GDScript addons, but that's unrelated to the game that I was mentioning in that sentence.

jabcross commented 2 years ago

You misunderstand. The runtime cost is when parsing the GDScript file, not necessarily in the execution. Even if a script doesn't have any abstract classes, it still has to check for the abstract token. This will make the editor just a bit slower, especially for larger projects with many scripts. It also bloats the Script class. And it can't be removed in the future, for backwards compatibility reasons.

My opinion is that abstract classes only make sense in strictly object-oriented programming languages, which GDScript isn't. Interfaces or Traits would be a better solution for this problem.

Kemeros commented 2 years ago

I updated the PR to add #ifdef DEBUG_ENABLED around that code so that it's skipped on target=template_release builds (production builds).

Cool fix to @jabcross's point about runtime cost.

You can always add a check to the constructor: if get_type() == "ClassName" : // throw error.

Clever workaround. Still prefer the annotation though.

jabcross commented 2 years ago

Plus this is another interface to the script API that external script plugin maintainers would have to keep in mind.

jabcross commented 2 years ago

This could be made a lint, or a feature of the language server, though.

Kemeros commented 2 years ago

A 9 letter annotation would cost more than your if get_type and throw to parse?

jabcross commented 2 years ago

Yes! Because the token check needs to be done regardless of if it's used. If you have a project with 100 scripts, it's going to happen in all of them (actually, every time it could appear). The get_type() gets parsed normally, so only on files where it's used. It doesn't change the parser.

aaronfranke commented 2 years ago

Even if a script doesn't have any abstract classes, it still has to check for the abstract token.

Not really, it checks for any annotations (anything that starts with @), if it is an annotation then it parses it with GDScriptParser::parse_annotation, grabs it from valid_annotations[annotation->name]; and then runs annotation->apply(this, head); using that annotation's apply method. Having an extra possibility in valid_annotations could potentially make it take longer, but we're talking almost certainly less than a nanosecond.

jabcross commented 2 years ago

If it's just an annotation, that's not as bad. But it's bad if you're adding class/class_name/extends annotations just for this feature (one more place to check for the @)

but we're talking almost certainly less than a nanosecond.

It builds up. I know that the change seems small, but it's this kind of tiny, seemingly-innocuous-at-the-time addition that bloats a language. There's a reason C++ got the way it is right now.

There are other problems too:

Kemeros commented 2 years ago
  • Interfaces/Traits/multiple inheritance are related proposals that would be affected or could subsume this one. Since you can't roll back changes without breaking everyone's projects, a change like this would get in the way of one of those features.

Judging by previous proposals, these are not comming anytime soon. Multiple inheritance was a no from Vnen no?

jabcross commented 2 years ago

Judging by previous proposals, these are not comming anytime soon. Multiple inheritance was a no from Vnen no?

Yeah. But this feature would stay in the language forever, including when they eventually come. Or at least until the next major Godot release, and who knows if we'll be alive for that.

@vnen opinions?

aaronfranke commented 2 years ago

@jabcross Multiple inheritance will not be added, it's an infamously bad feature.

if you're adding class/class_name/extends annotations just for this feature (one more place to check for the @)

It's already checking for the @ at the top for class_name/extends with the @tool and @icon annotations.

If you're adding a field and method to the Script class, you're not just changing GDScript but also adding a new constraint to every other plugin script.

The method in Script has a default implementation of { return false; }.

jabcross commented 2 years ago

@jabcross Multiple inheritance will not be added, it's an infamously bad feature.

Just an example of proposal. Traits and interfaces pose the same issue. I agree it's not great, but OOP isn't great in general

It's already checking for the @ at the top for class_name/extends with the @tool and @icon annotations.

That doesn't change that it'll have to check for it one additional time.

The method in Script has a default implementation of { return false; }.

I'm talking about the API. Is "abstract" something we want just for GDScript, or for every possible script language, even non-object-oriented ones? Does it make sense to ask that question of plugin maintainers?

jabcross commented 2 years ago

By the way, notes on terminology:

virtual means a virtual dispatch table. It's a table where pointers to the implementations of some methods are stored in fully compiled languages, it doesn't just mean that "this method needs to be defined or overridden by the inheritor". You could say every method is virtual in GDScript. I've noticed in your PR that you still use the word in some of the code.

A "method that needs to be defined or overridden by the inheritor" is basically the definition of a trait or interface.

Kemeros commented 2 years ago

I agree it's not great, but OOP isn't great in general

I mean... Bit of a large statement. OOP has some pitfalls if you're not careful, true. Multiple inheritance is one of them. Godot is built to help avoid pitfalls i find. It's great.

virtual means a virtual dispatch table. It's a table where pointers to the implementations of some methods are stored in fully compiled languages, it doesn't just mean that "this method needs to be defined or overridden by the inheritor".

Yep. That's why many said to change the annotation to abstract instead.

bitbrain commented 2 years ago

Is there any use case for abstract classes that could not be solved instead through node composition? Abstract classes promote Inheritance over Composition and I'd prefer the concept of Interfaces in GDScript over abstract classes. Just my 2 cents.

aaronfranke commented 2 years ago

@bitbrain Inheritance and composition solve different problems. I would not limit one in favor of the other.

bitbrain commented 2 years ago

@bitbrain Inheritance and composition solve different problems

Could you perhaps explain what each of these solves that the other one cannot?

EDIT I do not see any problem that cannot be solved through composition that only gets solved through inheritance. (requires the presence of interfaces, though https://github.com/godotengine/godot-proposals/issues/4872)

DasGandlaf commented 1 year ago

I agree with @bitbrain, I also prefer interfaces with composition over abstract classes, it's better design and closer to SOLID.

Changryy commented 1 year ago

Abstract classes already exist in Godot. I see no reason why the functionality shouldn't be accessible in GDScript.

image image image

It would be very useful for teams with dedicated programmers and designers, as it would be visually clear what the designers shouldn't instantiate. It might seem insignificant since you can just tell them, but with tons of classes (especially if there are subclasses that are also abstract) things can get confusing quickly, and it doesn't help that humans are forgetful.

I would also like to add that if a subclass of Resource is abstract it shouldn't show in the "new resource" menu.

image

DasGandlaf commented 1 year ago

While IMO I like interfaces more in various reasons, I would be completely fine with abstract classes, as long as, you can implement from multiple abstract classes (like in typescript f.e.). This would satisfy users asking for interfaces, as abstract classes are basically interfaces with more functionality, and users that are asking for abstract classes. This feature is really important, I want to do TDD in gdscript, but as long as this is not implemented in some sort of way, with TDD I will lose the typing features.

Abstract classes already exist in Godot. I see no reason why the functionality shouldn't be accessible in GDScript.

What do you mean by that?

Also, I may want to implement this feature myself but I'm not promising anything, so if anyone would be so nice and point me into the right direction, that'd be very helpful. Maybe a plugin, or whatever they're called, if that's possible. Thanks.

Changryy commented 1 year ago

What do you mean by that?

There are already built-in abstract classes (CanvasItem, Viewport, etc), so it would make sense for users to be able to define their own classes with identical behaviour;

me2beats commented 1 year ago

I make plugins that have classes which shouldn't be instantiated directly instead the user should extend them. I want the user to see the error like the class is abstract etc. So I`m for this feature

heppocogne commented 1 year ago

I think we should keep GDScript simple; easy to learn and use. We do not need to imitate other complicated OOP languages. When I need abstract method, I just write push_error("unimplemented").

func abstract_method():
    push_error("unimplemented")
Kemeros commented 1 year ago

When I need abstract method, I just write push_error("unimplemented").

The subject is abstract classes, not just methods. The goal is to prevent instancing classes/nodes. This doesn't complexify GDScript as you are not required to use the feature.

Working around this feature is more complex than having it.

plandem commented 1 year ago

Here is abstract class that cant be added via editor

modular_camera.gd

extends Node3D

func _ready(): 
   pass

FirstPersonCharacter.gd

class_name FirstPersonCharacter extends "res://addons/camera3d_toolkit/scripts/modular_camera.gd"

func _ready():
  pass

It is the only way that i know right now. But it still will show you ugly path to "base" class in editor when adding node, but yeah - cant add "base" from editor.

Screenshot 2023-06-19 at 09 04 01

As you can see, its almost "there".

@Kemeros, The simplest solution as I see - just update editor and do not show base classes without class_names there as well or show something not so ugly like full path to it.

cloewen8 commented 1 year ago

I'd prefer the concept of Interfaces in GDScript over abstract classes.

My only concern with interfaces in the strictest sense is they are intended to define what is exposed rather than behaviour and would require multiple inheritance. The benefits of an interface are already achieved using duck-typing.

Abstract classes are great to avoid developer errors, but it is not useful on its own. This really would need to be added alongside abstract functions (if not having the abstract functions make the class abstract). This is really only ever needed while editing, I would imagine it can be stripped from releases without consequences.

It is the only method I am aware of achieving a single source of truth without exposing incomplete classes or just bloating a main class (without multiple inheritance). Mixins would be great as well, but this again is just multiple inheritance and a different proposal.

DasGandlaf commented 1 year ago

Duck typing does the same as interfaces except that it isn't statically typed, error prone, hard to refactor, 0 code completion, etc.

I think you get my point. So no, the benefits of an interface do not exist with duck typing.

Defining what's exposed and not behavior only happens when a developer doesn't know how to design good architecture. Still, if that were the case, static typing, code completion, compile time errors are better than not having them, when using duck typing.

The benefits still outweigh the harms, but if that's your only problem, do you have a good alternative?

aaronfranke commented 1 year ago

@DasGandlaf I agree with you, but note that interfaces would be a different proposal from abstract classes.

cloewen8 commented 1 year ago

@DasGandlaf I still think abstract classes (with abstract functions) are the best solution at this time.

Interfaces are great as well, but Godot already chose a different paradigm, choosing ease-of-use (duck-typing) over strictness (static type checking). If strict type checking is needed, C# is a far better suited.

Abstract classes on the other hand are useful with both paradigms:

achieving a single source of truth without exposing incomplete classes or just bloating a main class

DasGandlaf commented 1 year ago

@aaronfranke oh yeah for sure. Just to clarify, I was actually responding to @cloewen8. Also, reading my message now, I hope I didn't come across as rude, I didn't mean it that way 😅

DasGandlaf commented 1 year ago

@cloewen8 I'd be fine with that as long as you can implement multiple abstract classes like in typescript.

cloewen8 commented 1 year ago

@DasGandlaf

I hope I didn't come across as rude

I always try to assume a good intent and answer as logically as possible with these types of discussions.

I definitely see your point and do feel interfaces have their place. GDScript absolutely could be more strict (TypeScript exists for Javascript developers for a good reason after all). My only concern is developers using interfaces as traits, instead of having the far more flexible mixins pattern (like with Java).

implement multiple abstract classes

This is a case where mixins would be better suited to the task. I'm fine with either personally but it sounds like multiple inheritance is a no-go right now.

Shidoengie commented 1 year ago

@DasGandlaf

I hope I didn't come across as rude

I always try to assume a good intent and answer as logically as possible with these types of discussions.

I definitely see your point and do feel interfaces have their place. GDScript absolutely could be more strict (TypeScript exists for Javascript developers for a good reason after all). My only concern is developers using interfaces as traits, instead of having the far more flexible mixins pattern (like with Java).

implement multiple abstract classes

This is a case where mixins would be better suited to the task. I'm fine with either personally but it sounds like multiple inheritance is a no-go right now.

Inheritance in general is flawed and this would allow for even more messier inheritance hierarchies

DasGandlaf commented 1 year ago

@Shidoengie Generally I agree about inheritance, although it's up to the developers skill, but this is a separate discussion. Essentially in typescript you can implement or extend abstract classes, extending (inherit) you can only inherit one, implementing (interface like) you can multiple. That's what I would prefer.

HenryLoenwind commented 1 year ago

DIT I do not see any problem that cannot be solved through composition that only gets solved through inheritance.

class_name bar extends Ressource
...
class_name baz extends Ressource
...
@export foo : either_bar_or_baz_but_no_other_Ressource

Just imagine Sprite2D would have to have 15 exports for all possible texture resource types instead of using one with Texture2D...

esklarski commented 9 months ago

I just ran into this same issue myself. I'm creating an effect system where a designer can add effects to an event.

To enable varied logic I have a base class EventEffect, and then derived types "ResourceEffect, WeatherEffect, etc" that implement the logic. It would be really nice to prevent EventEffect form being instantiated as it does nothing and only confuses things by appearing in the drop down list.

Screenshot from 2024-02-25 11-55-35

I support this proposal.