godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.16k stars 97 forks source link

Still call the lifecycle functions implicitly unless `super()` is used explicitly #4461

Open h0lley opened 2 years ago

h0lley commented 2 years ago

Describe the project you are working on

Any project utilizing inheritance.

Describe the problem or limitation you are having in your project

When I say "implicit", I am referring to the lifecycle functions being called automatically by the engine.


In Godot 3.x, the lifecycle functions - _ready(), _process(), etc. - were called implicitly including in the scenario where they were also declared by an inheriting class.

I think the implicit call made sense as things like setup, process, and event/callback logic of a node should not depend on the logic of an inheriting node class by default. Now, it feels like an inconsistency to me that lifecycle functions sometimes are and sometimes aren't called implicitly. For new users, too, I think it's easiest & most intuitive to learn about them as "functions the engine takes care of calling for you - always".

As it is now in Godot 4, I am certain it is much more of a source of potential bugs than it was in 3. For no good reason, we have to litter our code with super() calls that need to be maintained. When adding a lifecycle function declaration in a superclass, we have to think of checking all inheriting classes to see if they have a lifecycle function that needs a super() call added. Recently I spent many hours looking for a bug of which the cause turned out to be an early return in _process that skipped a super() call.

It's not just me either; I've already seen several cases of people on Discord #godot-4 being perplexed about a bug they struggle to locate the origin of, where it eventually turned out that they've had unknowingly overwritten a lifecycle function (the poorly documented _enter_world() to name a specific example).

Furthermore, currently @onready is considered a declaration of _ready() and thus prevents the implicit calls of inherited _ready() functions, so people have to write rather silly code:

@onready var my_var = "hey"

func _ready():
    super() # nothing else in here

Generally the required super() just seems like a lot of unnecessary boilerplate as in the vast majority of cases the exact point at which the superclass function is called isn't relevant (or a default point would do just fine). Of course as a reminder, I am exclusively referring to the lifecycle i.e. engine called functions here, which generally belong to nodes.

The only scenario where always requiring super() makes sense is when an user would sometimes need the lifecycle function in the superclass to never be called at all, but when that's the case, then I reckon it's an issue of code architecture and the user should rearrange their class inheritance and/or logic instead.

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

While how Godot 3 did things worked just fine for me, I understand that the new approach of explicitly calling super() gives more control over exactly at which point in the logic the superclass function is invoked.

So why not have the best of both worlds: Still call inherited lifecycle functions implicitly like usual, but when the user puts an explicit super() in the corresponding function of the inheriting class, then the implicit call is skipped.

Implementation example: When a _process function concludes or returns, then the engine should do a check if super() has already been called, and when it hasn't, it should be called by the engine. This would result in the behavior we know from Godot 3 with the added functionality of being able to specify a "custom call location" via super(). For functions like _ready, the behavior would slightly differ from Godot 3 as unlike with _process, Godot 3 calls the superclass first. However, this doesn't really matter since in the scenario where the user needs that behavior, they can put the explicit super() now.


This existing proposal addresses the same issue, however I'd like to open a new one as I frankly don't see the need for the additional annotation it proposes. I don't think there's a valid use-case for overwriting a lifecycle function and then never calling into the superclass.

I have read through this discussion of the past where people seem to be overwhelmingly in favor of not calling superclass lifecycle functions automatically, though I still wanted to start a new discussion as

  1. of course mostly people having an issue with the original behavior would end up looking for and participating in that thread.
  2. those people seem to be mostly complaining about how this is unintuitive or inconsistent behavior to them, but the functions in question are engine-called Godot-specific outliers in the first place so I don't see the need for them to conform with their expectations - their behavior just needs to be documented properly.
  3. people were complaining about the inflexibility of not being able to call into the superclass function at a specific point in the corresponding function of the inheriting class, which would still remain solved under this proposal.
  4. I've been rather active on the Discord help channels the past couple of years and do not remember ever seeing someone having an issue with the Godot 3 behavior of calling lifecycle functions. I think the people who've complained about it in 2016 were mainly confused as it seemed to conflict with their "OOP mindset", but did not actually produce any practical example as to why this is an issue (except the thing with the inflexibility mentioned in 3., but again, that remains solved).

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

n/a

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

n/a

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

n/a

Mickeon commented 2 years ago

As it is now in Godot 4, I am certain it is much more of a source of potential bugs than it was in 3. For no good reason, we have to litter our code with super() calls that need to be maintained.

I feel like calling super() implicitly as 3.x does it is one of those things you don't realise you love until they take it away from you. I feel like a lot of users rightfully like the idea of consistency (by needing to repeatedly write super()) only in theory, because in practice, as the proposal suggests, it leads to a lot of littering of the same word, and confusing errors.

Mickeon commented 2 years ago

their behavior just needs to be documented properly.

Indeed! Part of me is baffled that such an important feature to know about is nested and explained in one whole page, instead of being described and specified under every function that makes use of it.

KoBeWi commented 2 years ago

I feel like most users have problem with that only because they are used to the old behavior. I've seen users who don't know about multilevel calls and get confused about them, and it even led to bugs in my projects.

Now, it feels like an inconsistency to me that virtual functions sometimes are and sometimes aren't called implicitly.

That's how it was in 3.x. _ready(), _process() etc. were exceptions to how usually virtual functions are called.

Furthermore, currently @onready is considered a declaration of _ready() and thus prevents the implicit calls of inherited _ready() functions, so people have to write rather silly code:

This is a bug.

h0lley commented 2 years ago

I feel like most users have problem with that only because they are used to the old behavior. I've seen users who don't know about multilevel calls and get confused about them, and it even led to bugs in my projects.

true, I can see how both behaviors can lead to bugs, but am still making the argument that the current Godot 4 behavior is the bigger can of worms.

in Godot 3, if you have the wrong expectation you end up with a bug. in Godot 4, if you have the wrong expectation OR if you forget to place / accidentally skip (change in branched logic, early return, etc.) a super() call you end up with a bug.

I see no good reason for putting the burden of having to think of placing the super() calls for life cycle functions on the user. life cycle functions do not need to strictly abide by OOP expectations as they already are a Godot specific thing that needs to be documented anyways.

That's how it was in 3.x. _ready(), _process() etc. were exceptions to how usually virtual functions are called.

I'm not sure what you mean. what life cycle functions in Godot 3 behave differently from _ready(), _process()? all I am aware of is the case of certain functions - i.e. _ready() - being called in the superclass first, and others - e.g. _process() - are called in the inheriting class first. but that is addressed by this proposal.

This is a bug.

yea, a bug that illustrates my point, that is. why is it ok for the superclass _ready() to be called implicitly when the inheriting class declares an @onready, but not when it declares a _ready()? in practice it is ok in both cases, except in a few rare situations for which the user has the option to put the explicit super().

KoBeWi commented 2 years ago

if you forget to place / accidentally skip a super() call

Just put your super() calls always at the very top, unless you need to change the call order.

Same happens with other methods that aren't called implicitly, so not sure how is this an argument. Unless your proposal is about calling implicitly any virtual method.

I'm not sure what you mean. what virtual functions in Godot 3 behave differently from _ready(), _process()?

EMBYRDEV commented 2 years ago

I personally disagree with this. Implicit behaviour can be a baffling source of bugs at times and calling super() in most cases in return for not having to do horrible things to have the option to ignore the base class function is a fair trade.

This also keeps it in line with other popular languages used in game development so I don't think many people will be too bothered by this.

h0lley commented 2 years ago

Implicit behaviour can be a baffling source of bugs at times

the life cycle functions are already automatically called by the engine and that isn't baffling anyone. "special functions the engine takes care of calling for you" is very easy to understand.

in return for not having to do horrible things to have the option to ignore the base class function is a fair trade.

I take it you are referring to an abomination akin to this:

func _on_process(delta):
    # this can now be overwritten in an inheriting class
    pass

func _process(delta):
    _on_process(delta)

I fully agree that nobody should need to do this, but also as mentioned already I do not think this use-case really exists. when you find yourself needing to prevent a superclass life cycle function from being called at all something is wrong with your logic and/or inheritance arrangement. when a worm enemy enters the scene tree it does so both as worm and as enemy - it never makes sense for it to do so only as worm. by default, the enemy logic should be able take care of itself without depending on the worm logic. the way Godot 3 did things made sense. the only problem was that there was no flexibility in when the superclass function would be called.

Spartan322 commented 2 years ago

Wouldn't this eliminate superclass call omission for only particular methods? Lacking a call to the superclass is a legitimate use case. I don't recall any commonly used language that implements this behavior and its not deterministically predictable without documentation.

h0lley commented 2 years ago

Lacking a call to the superclass is a legitimate use case.

not for the particular Godot specific methods in question.

I don't recall any commonly used language that implements this behavior

see it not as a language but as an engine feature - just like it always has been the case with Godot 3 where it never was a technical issue; only one where a couple of people not having had their expectations met.

the life cycle functions - _ready, _process, etc. - are already special methods that are invoked automatically and exclusively by the engine. this is already not predictable without documentation.

from the user point of view, this doesn't need to be about "implicitly invoking super calls" (that's really just an implementation detail), but about automatically calling all the life cycle functions of which the user learns as functions automatically called by the engine - no matter the level of inheritance.

Spartan322 commented 2 years ago

not for the particular Godot specific methods in question.

In a Turing Complete language you can't really claim that and the limitation doesn't open up extra avenues of functionality (and currently would cost more to close, and that's excluding conceptual cost for developers)

see it not as a language but as an engine feature

Its not respected in C# and was already deemed a problematic bug in C# when it happened so in the least it is a GDScript feature, it reasonably has to be called a language specific feature.

only one where a couple of people not having had their expectations met.

If a language builds expectations and violates those expectations, even under special circumstance, that's still poor design, again we had this problem in C# and it was deemed a problem and was removed.

are already special methods that are invoked automatically and exclusively by the engine.

And? That doesn't justify the behavior, you don't override default engine node behavior by excluding the base/super call in most of those methods (unless I'm misremembering, but I'm pretty certain default behavior is implemented in the notification method anyway) and you should have capacity to override them if only because there are legitimate uses cases that could be presented by doing that.

this is already not predictable without documentation.

Those methods are documented though and where they're called is documented. Like just because you don't make a specific call to a method doesn't mean anything here, that's a completely unrelated thing, those methods are integrally called to the engine and much of their behavior is actually fairly decently documented because its one of the most important things you need to know, _ready is super well documented as its important for initializing anything in a node that enters the tree. Them being virtual in gdscript isn't exactly that relevant to the issue here.

but about automatically calling all the virtual functions of which the user learns as functions automatically called by the engine - no matter the level of inheritance.

Which violates the functionality of virtual methods in every language I know of, and the next most used language (C#) disagrees with this, the result of which creates a disagreement on what virtual should mean in any language (virtual just refers to the capacity for a method to be overriden by derived classes instead of calling the base class method) and would only confuse anyone that has to pay attention to C# and GDScript. (and to which caused problems and was explicitly deemed a problem to be removed when it happened in C#, so its not being implemented in the least for C# and likely nothing outside of GDScript in particular if at all)

Calinou commented 2 years ago

Removal of implicit multi-level calls was one of the most requested features for GDScript 2.0, so I wouldn't revert it for 4.0. This may also improve performance slightly when you don't need multi-level calls, although I haven't benchmarked it.

h0lley commented 2 years ago

Removal of implicit multi-level calls was one of the most requested features for GDScript 2.0

care to backup that claim? the thread from 2016 I linked in OP was pretty much the only sizable public discussion on it I was able to find, and that's far from being ground for calling it "most requested".

to the contrary, my experience from porting a medium sized project to 4 and following a lot of discussion on Discord makes me firmly believe that this is now a far bigger issue than it was in 3 due to the reasons outlined above.

Its not respected in C#

there's no multi-level calls in C#? if so, shame.

otherwise, none of your arguments really make sense to me.

If a language builds expectations and violates those expectations that's still poor design

sure, but I don't see how that is the case with the Godot 3 multi-level calls. even with a firm OOP mindset, I understood intuitively how _ready, _process, etc. are called implicitly on all levels and never got hung up on it. in fact I never questioned it - it was obvious behavior to me since these functions need to always be called anyways.

_ready is super well documented

I never said it is not.

as its important for initializing anything in a node that enters the tree.

exactly, which is why it must always be called, and skipping the call unless explicitly invoked by the user is nothing but a source of bugs.

dalexeev commented 2 years ago

https://godotengine.org/article/gdscript-progress-report-new-gdscript-now-merged

Remove multi-level calls

Another common source of confusion was removed. If you create an override of some lifecycle functions (such as _process or _ready) it still called the superclass implementation implicitly. Worse yet: some functions called the superclass before the subclass, and some went on the other direction.

This behavior is now completely removed. If you need to call the parent implementation, you can use the super keyword as mentioned above. This is common in OOP languages, so it should be more aligned to what user expects (which is evidenced by multiple issues reporting this behavior in the past). It also gives users control of when the super implementation should be called.

Note that methods defined in the C++ code are still called. This is needed to make sure engine behavior is correct (like the button _gui_input which is needed to execute the pressing behavior).

h0lley commented 2 years ago

some functions called the superclass before the subclass, and some went on the other direction.

It also gives users control of when the super implementation should be called.

yes, those were the technical issues. both still remain resolved under this proposal.

any critique that remains is anecdotes about expectations, which I still don't see how that applies to these special lifecycle functions and feel like it should be easily overruled by the concerns pointed out in OP.

I'll edit OP to rename virtual functions into lifecycle functions now as that seems to be more fitting terminology.

dalexeev commented 2 years ago

In my opinion, here, as in most places, explicit is better than implicit, and uniformity is better than exceptions. If I read the code and see super(), it means that not only the function code in the current class will be executed, but also in the parent class. And I also see the moment at which it will happen. It's not clear to me why there should be exceptions for "lifecycle functions".

Also, it is not obvious which functions are lifecycle and which are not. All starting with _? All virtual? All virtual voids?

I don't want to make this kind of argument, but it seems to me that those who support this proposal are simply used to the old behavior. But I can say to myself that I got used to super() very quickly.

Spartan322 commented 2 years ago

to the contrary, my experience from porting a medium sized project to 4 and following a lot of discussion on Discord makes me firmly believe that this is now a far bigger issue than it was in 3 due to the reasons outlined above.

Well the first case is anecdotal with familiarity with old behavior, doesn't contribute to your intended point of it being a wider issue when everything in 4.0 is a breaking change anyway, even someone familiar with the old behavior that wanted the new one will make mistakes because old habits do die hard no matter how desirable the new habit is. As for discussion on discord, none of that was shown as opposition that appeared before the proposed changes were merged and it could just as much be people adapting to the change or it could even be a select group of people that dislike the change because they're unfamiliar with it. If this is really a issue worth reverting, people will support this issue as much if not more then the previous change of behavior.

there's no multi-level calls in C#? if so, shame.

Its not a shame, its consistent, anyone who uses C# is used to calling the base method because its a requirement for virtual methods in C# if you wish to refer to base method behavior. It was deemed a problem and was removed because it violated C# and there was no way to stop it. (and it requires unnecessary bloat work to prevent double calls anytime you call the base method in C#)

They conceptually don't exist in any common language (as far I have experience in and I know a lot of languages) because uncontrollable implicit behavior is pretty universally deemed bad design. Even Python, a gold standard for user-friendly design, requires you to always call the base methods. (I suppose you can make decorators that do it, but that's still not virtual since all Python methods are inherently virtual) There isn't a good reason to violate Python design paradigms in this case either. It pretty widely violates common standard programming behaviors, also without reprocessing the built C# binary, there is no way to specifically detect if the base method was called in the method so unless Godot was to do that you can never replicate GDScript behavior in C#.

sure, but I don't see how that is the case with the Godot 3 multi-level calls. even with a firm OOP mindset, I understood intuitively how _ready, _process, etc. are called implicitly on all levels and never got hung up on it. in fact I never questioned it - it was obvious behavior to me since these functions need to always be called anyways.

How does this address expectation consistency? Just because you are aware of an exception to standard rules of the language does not validate the exception. Nothing about this explain why this should be considered as a valid point. Also its obvious to you? Again that doesn't validate your point, that just means you either got used to it or are aware of it, being aware or habitualized to an exception doesn't validate the exception. Also I don't see how a "firm OOP mindet" is relevant here, this applies just as much in any language that's trying not to be esoteric.

I never said it is not.

the virtual functions - _ready, _process, etc. - are already special methods that are invoked automatically and exclusively by the engine. this is already not predictable without documentation.

The documentation here was irrelevant which is why I said that, its completely unrelated. Also the methods aren't special because they're virtual, all method made in GDScript are virtual, they're special because the engine calls them.

exactly, which is why it must always be called, and skipping the call unless explicitly invoked by the user is nothing but a source of bugs.

The closest case I can think of where this comes up is that in C# a default base constructor will be called if it exists on the base classes, but the thing is one of the base constructors must be called by derived classes anyway so it just allows you to implicitly call the base default constructors if you don't. (and unless you have explicit constructors, default constructors are made in all classes anyway) In any case this is behavior you have total control over and is a requirement with constructors to define anyhow. There is no requirement that _ready, _process, or any of the other special engine methods need to be called in derived classes, it is entirely possible to build objects that don't care about all the base behavior just like how in C# you can override virtual methods that never call the base classes. The only one you can maybe argue is _init, but that's because it can be called as the constructor method anyway. (and if we're gonna do that I'd say the language should enforce a requirement of such behavior on it then, making it implicit behavior you can't control would be problematic)

h0lley commented 2 years ago

if it's truly impossible to make consistent with C#, I've no qualms conceding. I also would prioritize consistent behavior between the language options over this mostly QoL feature.

none of the other arguments hold in my mind though.

again, this is not to be seen as a part of the language, but as how Godot as a game engine decides to invoke those lifecycle methods. it's an engine feature that makes particular sense for scripting game entities. there's a good reason the original author went with this behavior. now people have to write more boilerplate code resulting in increased proneness to bugs as explained above.

Spartan322 commented 2 years ago

if it's truly impossible to make consistent with C#, I've no qualms conceding

This is so far been considered a problem, I can't see a reason it should be acceptable.

again, this is not to be seen as a part of the language,

Except it will be literally shared by none of the other languages used on Godot and there is not one binding that will be capable to do this, it can only be intrinsic to GDScript because of the control we have over GDScript, no other language would respect this.

but as how Godot as a game engine decides to invoke those lifecycle methods.

It requires intrinsic behavior implemented into GDScript to accomplish and would require processing or modifying the bytecode otherwise to replicate anywhere else, it would be required to be a language feature by specific implementation and it still violates the meaning of virtual.

it's an engine feature that makes particular sense for scripting game entities

That you don't have control over, there is a legitimate use case you are as a result forbidden from using.

there's a good reason the original author went with this behavior.

Unless explicitly described, this is a bad assumption.

now people have to write more boilerplate code resulting in increased proneness to bugs as explained above.

Never been a problem in any other language and its not been a problem in Unity or Unreal, its never been considered boilerplate because its describing necessary behavior. And if you really don't feel like calling super in a bunch of derived classes you could just write another method to make the call to the original method and leave it alone. I'm also pretty sure as well this could be implemented with a plugin or extension as it currently stands whereas reverting this change would prohibit users from ever being capable to have the current behavior.

h0lley commented 2 years ago

you've already written most of that in previous comments - there's no need to repeat yourself as it just makes me reply the same things as well.

if this is indeed as problematic to implement as you say - then again - I've no problems accepting it. if this were the only argument left standing, I'd stop replying. with this level of controversy there's no shot of this being implemented anyways.

you are looking at this mostly from an implementation perspective whereas I do so from an user's perspective. the user doesn't care at all about whether something is implemented on language or engine level - if it works it works.

the engine invoked lifecycle functions are already a Godot specific thing in the first place. for the user to learn about those as "always invoked by the engine - no matter the level of inheritance" is a trivial expectation, and it makes intuitive sense as those particular functions need be always invoked anyways. I wouldn't be surprised if most users don't even think of lifecycle function declaration in a subclass as function overwriting. they just want to hook in additional game entity logic.

there's a good reason the original author went with this behavior.

Unless explicitly described, this is a bad assumption.

it makes it so that the lower level lifecycle logic of a game entity is not dependent on the higher level logic by default - this problem is what I assume the original author saw as well.

That you don't have control over, there is a legitimate use case you are as a result forbidden from using.

there's no use-case for never calling into lifecycle functions - e.g. _ready, _process - of the superclass when declared. when you find yourself with such a need, rearranging your class inheritance and/or logic is always the better solution.

there is nothing useful made impossible under this proposal. it is also not a reversal but an alternative solution. the actual issues which remain addressed are quoted here.

dalexeev commented 2 years ago

I still don't see a problem with the need for an explicit super() when overriding a method, especially only for some particular methods. The "you can forget to do this" argument is not very convincing.

The child class specializes the parent class, so it is the responsibility of the child class not to break the expectations of external code and call the parent method if necessary. Implicit multicalls are a "rebellion" of the base class against changes made by the inheritor.

I agree to discuss the possibility of requiring @override to be written when overriding a method to avoid unintentional errors (after all, we cannot override vars). But this should happen for all methods, with no weird exceptions like lifecycle functions.

Therefore, #3936 seems more acceptable to me than this proposal. (However, I don't agree with the default multicalls, and I think that then they should also have an annotation. And since it's boring to write annotations all the time, the current behavior is the most compromise option.)

nanoquack commented 1 year ago

While I find the new behaviour much more consistent from a technical standpoint, this is a thing that really has to be documented in the Upgrading to Godot 4 Guide in the docs (Currently it isn't). Most people upgrading to Godot 4 will probably stumble upon this in some way or another as I did.

AnidemDex commented 11 months ago

You can get a godot 3 similar behavior using Object._notification :thinking: That is called for all levels according the documentation (and some tests with custom values, haven't tried with built-in notifications)

dalexeev commented 11 months ago

You can get a godot 3 similar behavior using Object._notification

See godotengine/godot#81139.

h0lley commented 11 months ago

You can get a godot 3 similar behavior using Object._notification 🤔 That is called for all levels according the documentation (and some tests with custom values, haven't tried with built-in notifications)

although I am still plagued to this day by hard to track down bugs due to life cycle function calls in complex inheritance trees being skipped for no good reason, I would have to be conscious about using _notification() just as much and it's not really realistic to consistently replace all life cycle hooks with _notification(). still, good to know that there are some exceptions after all, and really funny to read that core maintainers are saying that those multilevel calls "make sense", almost as if brutal consistency is not always the best choice. I guess all this change did is that now users have to really dig through the docs to learn about some super obscure outliers instead of simply understanding all life cycle functions to be invoked for you. now they sometimes are and sometimes are not automatically called in the name of consistency.

Mickeon commented 11 months ago

The very least that could be done is for autocompletion to fill out a super() anytime an inherited method is being overridden, or at least send out a warning.

The lack of consistency is whimsical and leads to very nasty bugs if you don't know what to look for. It's surprising to me that https://github.com/godotengine/godot/pull/81139 has not been merged with an agreed solution yet.

As a small note, this compilation error has been added. image In 4.1 the script would fail at run-time. It's a step in the right direction, at least.

KoBeWi commented 11 months ago

tbh some warning when the base method is not called could be helpful.

I'd still keep the current behavior though. I had some use-cases where I intentionally didn't call the base method and they'd be impossible otherwise.