Godot: [C#] Overriding _Process() in a derived class will always call the base version

[redxdev]

Godot version:
3.0 beta2 (mono)

OS/device including version:
Windows 10 1709

Issue description:
Deriving from a custom C# class and overriding _Process() (and possibly other godot lifecycle methods - I haven't had a chance to test all of them) will always call the base class version of _Process() first - even if a call to base._Process() isn't specified. This is not consistent with normal C# behavior (and indeed, methods other than ones called by godot work as expected).

Example:

MyBaseClass.cs:

class MyBaseClass : Node2D
{
    public override void _Process(float delta)
    {
        GD.Print("MyBaseClass");
    }
}

MyDerivedClass.cs:

class MyDerivedClass : MyBaseClass
{
    public override void _Process(float delta)
    {
        GD.Print("MyDerivedClass")
    }
}

Apply MyDerivedClass to a node.

Expected:

MyDerivedClass

Printed to console every frame.

Actual:

MyBaseClass
MyDerivedClass

Printed to console every frame. This behavior should normally only happen if there was a call to base._Process(delta) inside MyDerivedClass.

Example of correct behavior in C#: https://ideone.com/ZhtnTj

[redxdev]

Just checked and _Ready() also displays this behavior. I assume most (if not all) godot-defined methods will act similarly.

I'm not sure if this is an intentional decision to mirror GDScript behavior, but it does create an inconsistency with how C# normally works - especially since this _only_ applies to godot methods.

[vnen]

Those functions are on a call_multilevel, so they call all the functions in the inheritance tree. It could be acceptable in GDScript, but it breaks the expectation (even more) on other languages. I think this should be revised.

[vnen]

Related discussion: #6500

[reduz]

@vnen I'm not really sure if it's worth implementing multilevel call in C#. In GDScript it does make sense though.

[neikeq]

Keep in mind that GDScript has the equivalent of base._Process() (just ._process()), so I suppose this was done to avoid writing that every time.
I am not against making it manual, if people prefer that. Is there any method that would cause trouble if not called in the correct order?

[redxdev]

Throwing my reasoning behind this bug, for whatever that's worth:

There are two main issues (in my opinion) with the current way _Process() works in C#. The first is that those who are familiar with C# are going to be very confused - the language does not call base functions automatically _ever_ (destructors/finalizers might be an exception to this, but I can't remember off the top of my head). This is also the case with languages in the same family (Java, C++, etc).

Even ignoring those already familiar with C#, there's another problem. Keeping _Process() as it is creates an inconsistency with the language - even if godot functions call base implementations automatically, the rest of the language does not. Having your own function work differently than overridden godot functions is, in my mind, a pretty annoying issue. Especially since there's no indicator in the code you write that this is the case - when overriding _Process() you use the exact same syntax as with any other function, except inexplicably it works differently. Documentation will (I assume) help this along a bit, but then you'd have to explicitly know to look for that in the documentation.

Those are my main reasons for bringing this issue up.

Now, I'm not completely familiar with godot's internals (I'm fairly new to godot, and my perspective is mostly from an outsider to the community) but a question I would like to bring up is this - if a change is made to go with C#'s usual behavior (rather than the current behavior), which of the following would end up being required if, say, I wanted to create a script derived from AnimatedSprite:

class MySprite : AnimatedSprite
{
    public override void _Process(float delta)
    {
      // my code, etc...
    }
}

or

class MySprite : AnimatedSprite
{
    public override void _Process(float delta)
    {
      base._Process(delta);
      // my code, etc...
    }
}

The idea here being whether or not we'd have to call the base implementation for native classes. Ideally, I think, the first option would be the best (and it doesn't break C#'s expected behavior - we can assume that the actual implementation of the native _Process() is a different un-overridable function, ie _Internal_Process() that calls the custom _Process() function). That said, I'm not sure how the C# integration is written so I don't know which is possible/desired/reasonable/etc. Just my 2 cents.

[reduz]

Godot no longer uses _process internally, so the risk of overriding the
godot function is low. _gui_event is another story. I think making
multilevel not work only at C# script side (but still call internal godot
functions) should be the way to go.

On Dec 25, 2017 9:58 PM, "Sam Bloomberg" [e-mail] wrote:

Throwing my reasoning behind this bug, for whatever that's worth:

There are two main issues (in my opinion) with the current way _Process()
works in C#. The first is that those who are familiar with C# are going to
be very confused - the language does not call base functions automatically
ever (destructors/finalizers might be an exception to this, but I can't
remember off the top of my head). This is also the case with languages in
the same family (Java, C++, etc).

Even ignoring those already familiar with C#, there's another problem.
Keeping _Process() as it is creates an inconsistency with the language -
even if godot functions call base implementations automatically, the rest
of the language does not. Having your own function work differently than
overridden godot functions is, in my mind, a pretty annoying issue.
Especially since there's no indicator in the code you write that this is
the case - when overriding _Process() you use the exact same syntax as
with any other function, except inexplicably it works differently.
Documentation will (I assume) help this along a bit, but then you'd have to
explicitly know to look for that in the documentation.

Those are my main reasons for bringing this issue up.

Now, I'm not completely familiar with godot's internals (I'm fairly new to
godot, and my perspective is mostly from an outsider to the community) but
a question I would like to bring up is this - if a change is made to go
with C#'s usual behavior (rather than the current behavior), which of the
following would end up being required if, say, I wanted to create a script
derived from AnimatedSprite:

class MySprite : AnimatedSprite
{
public override void _Process(float delta)
{
// my code, etc...
}
}

or

class MySprite : AnimatedSprite
{
public override void _Process(float delta)
{
base._Process(delta);
// my code, etc...
}
}

The idea here being whether or not we'd have to call the base
implementation for native classes. Ideally, I think, the first option would
be the best (and it doesn't break C#'s expected behavior - we can assume
that the actual implementation of the native _Process() is a different
un-overridable function, ie _Internal_Process()). That said, I'm not sure
how the C# integration is written so I don't know which is
possible/desired/reasonable/etc. Just my 2 cents.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/godotengine/godot/issues/15053#issuecomment-353899301,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF-Z23tWfWDrKUntISIWHG-DgldZZUd-ks5tEETNgaJpZM4RMY8H
.

[neikeq]

@redxdev Godot virtual methods in C# are just empty methods you can override, so you don't need base._Process(delta) from classes that inherit Godot types directly. If you write another class that inherits from MySprite, then that class would need the base call.

[akien-mga]

Not critical for the upcoming 3.0 release, so moving to the next milestone. A fix can still be cherry-picked for 3.0.x maintenance releases once available in the master branch.

[neikeq]

This is a very simple patch, so if the concensus is to change it, I would prefer to do it before 3.0 stable.

[mysticfall]

I'd like to see this to be fixed before 3.0, if it can be done by a simple patch.

Although I agree that it wouldn't affect too many people critically in practice, it's an exception to the rule that diverges from how the language is supposed work in a fundamental way.

Those who are familiar with C# will most definitely invoke base methods selectively depending on the circumstances and expect them to work just like in any other C# program.

And it is a quite common scenario to extend a user node type (i.e. Player -> Actor -> Node) to override the parent's functionality completely (not invoking base.SomeFunction()), it has a chance to confuse them if user defined methods and those defined by Godot behave differently, when they should act in the same way, as it is defined by C#'s language specification.

[NathanWarden]

I second the motion to change it before 3.0 :)

[redxdev]

I also support changing this before 3.0 if the patch is simple.

[mysticfall]

It seems that the problem is more severe than I initially thought. Not only you can't prevent the base method from being invoked when you override _Ready(), it can even get invoked multiple times if you have a deep type hierarchy where the mehod is overriden by more than one children.

Maybe we should fix it sooner than later before more people start to use it for serious projects? It's possible their project might behave differently once this issue gets fixed, so I'm beginning to think that it can be a more serious problem than what it appears.

[NathanWarden]

Maybe we should fix it sooner than later before more people start to use it for serious projects?

I might go ahead and take a stab at it today unless @neikeq is already on this?

[neikeq]

Damn, I wanted to do this before 3.0 stable but completely forgot about it...
@mysticfall you mean it can get invoked multiple time if you also call the base method manually?

[mysticfall]

@neikeq, Yes, that's what I just found. When I added a class that extends from another class that includes base._Ready(), the method got invoked twice, so I had to apply a temporary workaround in my project.

[akien-mga]

For the reference, even though 3.0 is now stable and supposed not to get API breakages, we made it clear in the release that C# support is alpha, so if you need to break compat for C#, go ahead, users will understand that it's for the greater good.

[NathanWarden]

An example of what @mysticfall is talking about would be:

SubClass.cs

``` C#
using Godot;
using System;

public class SubClass : BaseClass
{
public override void _Ready()
{
GD.Print("I should be called");
}
}

BaseClass.cs

``` C#
using Godot;
using System;

public abstract class BaseClass : Spatial
{
    public override void _Ready()
    {
        GD.Print("I should not be called");
    }
}

The output is:
I should not be called
I should be called

So, it gets even worse when someone calls base._Ready() since it now prints what's in the base class twice. It shouldn't print anything from the base class at all unless base._Ready() is called :)

[mysticfall]

I also made a simple test case that reproduces the problem:

```c#
public class GrandParent : Node
{
public override void _Ready()
{
base._Ready();

    GD.Print($"### _Ready() was invoked on '{Name}'.");
}

}

public class Parent : GrandParent
{
public override void _Ready()
{
base._Ready();
}
}

public class Child : Parent
{
public override void _Ready()
{
base._Ready();
}
}

When adding only `Child` to a node and running it, the outcome was:

_Ready() was invoked on 'Main Camera'.

_Ready() was invoked on 'Main Camera'.

_Ready() was invoked on 'Main Camera'.

`` So, it seems that everybase._Ready()` call results in an additional invocation, which is quite a serious deviation from how the language is supposed to work.

[neikeq]

Alright, gonna change this right now.