Skip to content
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

Allow access to the "actual" node a script is attached to. #23164

Closed
ghost opened this issue Oct 20, 2018 · 7 comments
Closed

Allow access to the "actual" node a script is attached to. #23164

ghost opened this issue Oct 20, 2018 · 7 comments

Comments

@ghost
Copy link

ghost commented Oct 20, 2018

Feature Request

Lets say we have a base class called Entity, subclasses of which are attached to various types of nodes in our game.

public abstract class Entity : Node { ... }

We want to attach a subclass of Entity it to a KinematicBody2D node.

Once we do this, we have (almost) no way of accessing the properties and methods of the real KinematicBody2D node from Entity outside of using the Get(), Set() and Call() methods because Entity only inherits from Node.

Because of this, I propose adding a public method or property to Node that returns a single instance of the script's underlying real node. Let's call this method GetRealNode() for now.


public class Node : Object 
{
    public Node GetRealNode() 
    { 
        ... 
    }
}

General Concept

If we have an Entity attached to a node of type Node2D, the following code will not compile because Entity has no position property.

var entity = GetNode<Entity>("Entity");
var position = entity.Position; // ==> Error

But if we can grab an instance of the entity's actual node, we can access the position and all of the functionality of the underlying Node2D.

var node = (Node2D) entity.GetRealNode();
var position = node.Position; // ==> OK

Details

Calling GetRealNode() on a user created script class should return an instance of the actual node the script is attached to. Calling GetRealNode() on a built-in, engine defined node such as Spacial should cause the node to simply return itself.

For this section, the code snippets refer to calls from within the Entity class.

The following expression equals true as both the C# script instance, and the instance returned by GetRealNode() should share the same native node instance. In terms of what the engine sees, they are the same node.

NativeInstance == GetRealNode().NativeInstance // ==> True

However, this is false, as they are not the same C# object...

this == GetRealNode() // ==> False

Example

public class KinematicEntity : Entity 
{
    // Shortcut to access the underlying KinematicBody2D.
    KinematicBody2D Body => (KinematicBody2D) GetRealNode();

    public override void _Ready() 
    {
        // This should print true, as the same C# instance should be returned every time.
        GD.Print(GetRealNode() == GetRealNode());
        
        // This should also print true. 
        GD.Print(GetRealNode().GetRealNode() == GetRealNode());

        // Let's say the node this script is present on is named "Todd".
        // These should both print "Todd".
        GD.Print(Name);
        GD.Print(GetRealNode().Name);

        // If we change the script's name variable...
        Name = "Susan";

        // These should both print "Susan", as all native data (such as the name of the node) is shared in the native node instance.
        GD.Print(Name);
        GD.Print(GetRealNode().Name);

        // And vice versa...
        GetRealNode().Name = "Steve";

        // These should both print "Steve".
        GD.Print(Name);
        GD.Print(GetRealNode().Name);
    }

    public override void _PhysicsProcess(float delta) 
    {
        // Make use of the underlying KinematicBody2D.
        Body.MoveAndSlide(new Vector2(0, 100));
        GD.Print(Body.GlobalPosition);
    }
}

If we do not have the above access to the underlying node instance (like we do now) we would have to do this to call MoveAndSlide() and then print the position...

Call("move_and_slide", new Vector2(0, 100));
GD.Print(Get("global_position"));

These calls lose static typing, are slower, and can incur per-frame heap allocations due to boxing or params object[] creation.


Having a robust way to access the real node type would make it possible to write more reusable, generalized code, potentially shareable between 2D and 3D, while still maintaining static typing, performance and avoiding per-frame heap allocations.

@ghost
Copy link
Author

ghost commented Oct 20, 2018

Related issue: #11980

@ghost ghost changed the title [Mono] [Feature Proposal] Allow access to the real node of a C# script. [Mono] [Feature Proposal] Allow access to the "actual" node a script is attached to. Oct 20, 2018
@ghost ghost changed the title [Mono] [Feature Proposal] Allow access to the "actual" node a script is attached to. [Mono] [Feature Proposal] [Discussion] Allow access to the "actual" node a script is attached to. Oct 20, 2018
@NathanWarden
Copy link
Contributor

It would be extremely useful since writing scripts where multiple types of nodes share common properties/methods is now a bit more difficult since you can't just automatically have the properties and methods via inheritance and now have to have a class reference in each class that you want to have the same properties/methods. Any "common base" you want means you have to do it through an interface instead of inheritance. So, it's not impossible, but does make it a little bit more difficult.

If this is a problem that can be solved it would be great if it were implemented, but in the meantime I don't personally mind since I'm more interested in seeing C# in as stable of a state as possible :)

@akien-mga akien-mga changed the title [Mono] [Feature Proposal] [Discussion] Allow access to the "actual" node a script is attached to. Allow access to the "actual" node a script is attached to. Oct 20, 2018
@Zylann
Copy link
Contributor

Zylann commented Oct 21, 2018

Just wanted to raise this:
It looks like the same problem raised some time ago in GDScript, where devs also wanted several scripts of their game to inherit a common base, but as a result they could not use any derived function of the nodes they put it on, because it would break OOP and required downcasting or using get/set/call.
To me that highlighted a weird situation for a dynamically typed language in which that was technically possible, but as we introduced strongly typed inheritance it made it "conceptually" impossible.
In C# it's worse because the language itself disallows this already, you can't inherit two things at the same time, so we end up with downcasting...
So when looking at this situation, from an OOP point of view it looks like a wrong design, and the way to solve it would be to move away from inheritance and compose using child nodes rather than inheriting, there are plenty other designs doing variations of that in order to escape those limitations. Otherwise, it seems people not wanting to go that route will have to downcast anyways using some C#-specific utility.

@ghost
Copy link
Author

ghost commented Oct 21, 2018

Thanks for the input!

it would be great if it were implemented, but in the meantime I don't personally mind since I'm more interested in seeing C# in as stable of a state as possible

Yeah, I agree with this. This feature could easily wait until after 3.1 is released. Stable C# should be the number one priority. I would, however, love to hear if there would be any major technical issues with having this feature implemented. The ignorant part of me thinks this would be relatively quick for the devs to set up, but I'd like to know more.

Any "common base" you want means you have to do it through an interface instead of inheritance. So, it's not impossible, but does make it a little bit more difficult.

Yep! I'm currently working on a small Godot game framework for personal use that needs to be very generic, ideally supporting both 2D and 3D. I would love to have a common base class for all "entities", but at the moment I've had to go with a common interface and an internal, non-node class that each implementation delegates its properties and methods to.

The major problem with this a whole heck of a lot of boilerplate delegation. Each script that implements the common interface has around 100+ lines of nothing but delegated properties and methods! Some of these scripts are exactly the same other than the base node type. Some IDE's can generate this delegation for you (such as Jetbrains Rider), but it's certainly ideal.

It should be noted that once C# gets default interface method and property implementations in C# 8, using a common interface would get alot easier. With default implementations we can delegate to an internal class inside the interface rather than in each implementation. This doesn't get rid of all delegation bloat, but a good amount of it.

But yes, interfaces and delegation do make the design possible, just a bit wonky.

@ghost
Copy link
Author

ghost commented Oct 21, 2018

So when looking at this situation, from an OOP point of view it looks like a wrong design, and the way to solve it would be to move away from inheritance and compose using child nodes rather than inheriting, there are plenty other designs doing variations of that in order to escape those limitations.

Yeah, I can attest that composition is generally the way to go. Godot and Unity were obviously designed with it in mind, just in different ways! However, one point of friction in Godot, for me at least, is what I'm going to call "The Battle for the Root Node".

Composition in Godot works well in a downward direction, ie parent nodes use their direct or indirect children to control how they work. We (usually) would like to use the scene's root node as a sort of facade that provides functionality that is ultimately implemented in its children. In most cases this works great, but some nodes like KinematicBody2D, really need to be the root node of the scene to function.

As such, we can't really "have" a KinematicBody2D on an scene. That scene has to "be" a KinematicBody2D. If we attach a common script to the KinematicBody2D, one that might even facilitate composition (what I am currently working on), we lose proper access to the KinematicBody2D. The attached script completely masks the functionality of the root node.

I have seen a few other related issues and I am not in favor of allowing multiple scripts on the same node. I love the simplicity of Godot's node system and don't want to change it in such a big way.

However, I don't think attaching a script to a node should cause drastic net loss of functionality. We should keep the "script first" ideal of nodes, but we should not hide the fact that underneath that script, there is a real, in-engine node, in all its glory, that we can use.

For every node, two instances could be possible. One for the script if it exists, and one that represents the bare node type. No more. This still preserves encapsulation as outside nodes do not immediately know the actual node type, but the script itself can ensure that its actual node is of the correct type. The script can then downcast the actual node to the type it needs to be and access it through a property. This allows a common script class to "compose" itself of its actual node.

In this way, I don't believe we would be breaking OOP, at least not too badly!

@Duehok
Copy link

Duehok commented Oct 24, 2018

There is a feature proposal for C# 8.0 (release date soonish) which allows default implementation in interfaces.

https://www.infoq.com/articles/default-interface-methods-cs8
dotnet/csharplang#52

So in the future, IF and only IF this feature makes it into C#8 (I have not found any confirmation that it will be included), using an inteface with default method instead of an abstract class might help, with no change to Godot.

@clayjohn
Copy link
Member

Feature and improvement proposals for the Godot Engine are now being discussed and reviewed in a dedicated Godot Improvement Proposals (GIP) (godotengine/godot-proposals) issue tracker. The GIP tracker has a detailed issue template designed so that proposals include all the relevant information to start a productive discussion and help the community assess the validity of the proposal for the engine.

The main (godotengine/godot) tracker is now solely dedicated to bug reports and Pull Requests, enabling contributors to have a better focus on bug fixing work. Therefore, we are now closing all older feature proposals on the main issue tracker.

If you are interested in this feature proposal, please open a new proposal on the GIP tracker following the given issue template (after checking that it doesn't exist already). Be sure to reference this closed issue if it includes any relevant discussion (which you are also encouraged to summarize in the new proposal). Thanks in advance!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants