-
-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for abstract classes in GDScript #5641
Comments
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. |
With the way this works, could it not be called |
@aaronfranke
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
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: |
I updated the proposal and PR to use |
I figured out a way to do it: 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);
}
} |
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: |
In you original post, who are the "users"? Is this a game where players program in GDScript? |
@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 However, the rest of the feature is basically free. Setting the
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. |
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 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. |
Cool fix to @jabcross's point about runtime cost.
Clever workaround. Still prefer the annotation though. |
Plus this is another interface to the script API that external script plugin maintainers would have to keep in mind. |
This could be made a lint, or a feature of the language server, though. |
A 9 letter annotation would cost more than your if get_type and throw to parse? |
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. |
Not really, it checks for any annotations (anything that starts with |
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 @)
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:
|
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? |
@jabcross Multiple inheritance will not be added, it's an infamously bad feature.
It's already checking for the
The method in |
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
That doesn't change that it'll have to check for it one additional time.
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? |
By the way, notes on terminology:
A "method that needs to be defined or overridden by the inheritor" is basically the definition of a trait or interface. |
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.
Yep. That's why many said to change the annotation to abstract instead. |
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. |
@bitbrain Inheritance and composition solve different problems. I would not limit one in favor of the other. |
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 #4872) |
I agree with @bitbrain, I also prefer interfaces with composition over abstract classes, it's better design and closer to SOLID. |
Abstract classes already exist in Godot. I see no reason why the functionality shouldn't be accessible in GDScript. 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. |
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.
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. |
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;
|
I make plugins that have classes which shouldn't be instantiated directly instead the user should |
I think we should keep GDScript simple; easy to learn and use. We do not need to imitate other complicated OOP languages. func abstract_method():
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. |
Here is modular_camera.gdextends Node3D
func _ready():
pass FirstPersonCharacter.gdclass_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. As you can see, its almost "there". @Kemeros, The simplest solution as I see - just update editor and do not show |
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. |
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? |
@DasGandlaf I agree with you, but note that interfaces would be a different proposal from abstract classes. |
@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:
|
@aaronfranke oh yeah for sure. |
@cloewen8 I'd be fine with that as long as you can implement multiple abstract classes like in typescript. |
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).
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 |
@Shidoengie Generally I agree about inheritance, although it's up to the developers skill, but this is a separate discussion. |
Just imagine Sprite2D would have to have 15 exports for all possible texture resource types instead of using one with Texture2D... |
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 I support this proposal. |
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:
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: godotengine/godot#67777
Here is a picture of the "Create New Node" dialog:
In the image
MyAbstract
is@abstract
, while the other is not. Note that ifExtendsMyAbstract
was made abstract then both would be hidden.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 useextends "my_script.gd"
to extend them, and if you want to use theis
operator then you have to useconst 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.
The text was updated successfully, but these errors were encountered: