Godot: Generic Typed GetNode C#

[hasahmed]

This is really a feature request and I think it may have been sort of touched on here #16309.
Anyway GetNode("path/to/mynode") will return a Node instance and will likely need a cast (i.e. GetNode("path/to/mynode") as Sprite2D), and often you will want to modify a property of the node so you will need parentheses (i.e.(GetNode("path/to/mynode") as Sprite2D).Position = foo();) I was thinking that this could be made a little more pretty by passing GetNode the type i.e. GetNode<Sprite>("path/to/mynode"); and then the need for casting (and by extension parentheses) could be avoided.

[mysticfall]

I agree that we need such a feature and it's also a safer one to have before attempting more complex approaches, like the one described in #16309.

However, it's my understanding that the current system auto generates the C# binding source from an XML metadata. So, I believe we'd need to find a way to customize the process(#15548) before we could deal with such a problem.

In the meanwhile, we can use C#'s extension method to mitigate the problem as I've done for my project:

• https://github.com/mysticfall/AlleyCat/blob/master/Program/AlleyCat/Common/NodeExtensions.cs

[akien-mga]

CC @neikeq

[paulloz]

This is massively asked for on Discord (maybe on other communication platforms too, I don't know).
Even if this is not a blocking issue, the current usage of .GetNode(NodePath) feels weird and really is not right. Needing to cast the function return is basicaly transforming what would be compilation errors to unexpected runtime errors.
Correcting this (as soon as possible) would be a good first step toward having more C# users, leading to more tests.

Not the best solution but: adding a small NodeExtension class to the glue would do the trick, even temporarly while a better option is chosen (i.e. making some classes generated from ClassDB partial so they can be extended).

[NathanWarden]

I like this idea. It still, in spirit, matches the GDScript API. It would be super easy to implement as well since it can essentially just be:

C# public T GetNode<T>(NodePath path) { return GetNode(path) as T; }

Although I haven't double checked, I'm pretty sure it's safe even if the original GetNode returns null due to no node existing at the path given.

[paulloz]

Well, you have to where T : Node or at least where T : class or it won't compile, but yes.
It is safe even if GetNode returns null (null as XXX is null).
I'm using the branch I linked above as my custom build for some time now without any issue to report.

[neikeq]

Should this generic method use a cast or as?
I think it should be the former, since I cannot see this being used without assuming the type (unless followed by ?).
Whatever we choose, we can also provide an alternative (GetNodeOrNull or GetNodeOrThrow).

[paulloz]

I personally tend to prefer as but I guess both have their advantages and inconveniences (and the most important is that a working solution is found 😄).
+1 for the alternative method GetNodeOrNull / GetNodeOrThrow.

[mysticfall]

I definitely agree that it's better to provide two different versions for a mandatory and optional dependency respectively.

As to the type, I'd use cast since I think it's better to design API unambiguously, so instead of returning null for both cases when it fails to find the requested node and when the node is of a wrong type, I prefer showing which is the case explicitly by the contract.

Of course, we can use as and add additional check for type safety but it'd be redundant since explicit cast would throw an appropriate exception in that case any way.

Probably the only real reason why we'd rather use as could be from a performance perspective, assuming it's better not to use exceptions anywhere. But I don't think such type of optimization is necessary, especially considering it's an error condition and people can always use something like GetNode<Node>(path) as ExpectedType and do a manual check if they are using it in a normal execution path and performance is of utmost importance.

[NathanWarden]

I'd just keep it simple and use as. Having two methods makes it more convoluted where only a knowledged programmer would know the difference between the two. But, if that same programmer writes their code properly, then this shouldn't constantly be called anyway since the programmer should just cache it once in the Ready function, so there shouldn't be any noticeable performance hit in a great majority of cases.

If that same well versed programmer has a specific use case where they're calling something like this every frame for some strange reason they can always make their own extension method that throws an exception.

So, I really don't see any reason why we should have two generic methods that do the same thing, but slightly differently. Just implementing the safer of the two seams more reasonable.

[neikeq]

Probably the only real reason why we'd rather use as could be from a performance perspective, assuming it's better not to use exceptions anywhere.

Raising the exception is more costly. However, if we go with a cast, it's assumed you know the object can be cast, therefore as would be slower because of the safety check.
I don't think we should worry too much about this though. I think the performance penalty is quite small, and GetNode is already quite slow so you want to call it only once (on _Ready for example) and cache the result.

and people can always use something like GetNode<Node>(path) as ExpectedType

You mean GetNode(path) as ExpectedType :stuck_out_tongue:

[mysticfall]

@NathanWarden The reason why we'd may want two different methods is that it's more than likely the API would be used to resolve required dependencies in such places like _Ready().

In that case, if we don't check for the exception path, it'd be passed to the user's responsibility to validate the result. However, if they have to check for a null value or type safety every time they invoke GetNode, I feel like it's defeating the purpose we consider providing such a shortcut for.

And obviously, we can't simply give them a version with such validation checks, since it's still possible they might want to use the API to resolve optional dependencies.

Personally, I don't see much problem why we shouldn't provide two different versions of such methods, especially when we consider that C#'s core API often provides many different overrides for the same functionality, and also the fact that it's quite common to see similar APIs tends to provide such alternatives like GetOrElse, or GetOrDefault to make it easier to write concise code and to use it inside a functional composition (i.e. Rx).

@neikeq Yeah you're right, I'm still clunky with C#, especially when I'm not using an IDE 😅

[mysticfall]

And as to the bound check, I'd suggest just to use where T : class rather than T : Node since it might be needed to use the API to resolve interfaces.

[neikeq]

I'd just keep it simple and use as. Having two methods makes it more convoluted where only a knowledged programmer would know the difference between the two.

The reasons I prefer a classic cast is that most of the times you use GetNode -and specially for this use case- you know the type beforehand.

I think the name of these methods is clear enough, and they would also have a description, so it won't be confusing.

If that same well versed programmer has a specific use case where they're calling something like this every frame for some strange reason they can always make their own extension method that throws an exception.

So, I really don't see any reason why we should have two generic methods that do the same thing, but slightly differently. Just implementing the safer of the two seams more reasonable.

This is a very common use case, so I think it's better to provide it ourselves rather than make the users define their own extension methods.

[NathanWarden]

@mysticfall

if we don't check for the exception path, it'd be passed to the user's responsibility to validate the result

The problem with this conclusion is that if you want safety in your code you have to do error checking regardless of which one you use. If it throws an exception you need to use a try/catch or if it passes null you'll need an if statement. The experienced C# developer will know what to do with as or a straight cast. The new C# programmer won't know what to do in either case and will probably ask someone more experienced, in which case the as version is a much more common (and much faster) method of error checking.

The reason why we'd may want two different methods is that it's more than likely the API would be used to resolve required dependencies in such places like _Ready()

This really only validates my point, there is very little performance implication in going with the slightly slower as if almost all uses of this function are in the Ready function, which is most likely the case. The only case that it will likely be used in something like the process function it will likely be doing a null check against whether it has a spawned node or not, in which case you'd still want to use the as version since a try/catch would be a pretty big performance hit if you have to do it every frame.

IE:

``` C#
public override void _Process(delta)
{
if (nodeThatGetsSpawned == null)
{
nodeThatGetsSpawned = GetNode(path); // as version
}
}

vs.

``` C#
public override void _Process(delta)
{
    if (nodeThatGetsSpawned == null)
     {
          try
          {
               nodeThatGetsSpawned = GetNode<NodeType>(path); // try/catch version
          }
          catch {}
     }
}

I still really don't see any significant reason why we need two different methods as it convolutes the API with very little grounds to do so with.

[NathanWarden]

@neikeq

This is a very common use case, so I think it's better to provide it ourselves rather than make the users define their own extension methods.

As in my post just above, I realized using a straight cast version every frame would actually be pretty big performance hit since you'd be forced to use a try/catch statement, in which case you'd still want to use the as version.

[paulloz]

@mysticfall GetNode will always return a class inheriting Node though, or at least Godot.Object.

[NathanWarden]

@paulloz I agree. Even if it didn't have to return a Node, using class would cause the API to do something other than what it explicitly tells you it does, which violates the rules of good API design.

[mysticfall]

@NathanWarden

If it throws an exception you need to use a try/catch or if it passes null you'll need an if statement.
The fact that a certain method throws an exception doesn't mean the caller must catch it with try-catch.

Normally it's better to let it propagate further up the hierarchy and be handled by some global exception handling logic, like logging to a file or showing an exception dialog, for example.

The reason why it is better to throw a specific exception instead of returning null for both the normal and exception paths is that it prevents such cases where it'd trigger an unexpected behavior further down the code as a side effect, like causing NullReferenceException in a different line, thus making it more difficult to track down the actual cause.

This really only validates my point, there is very little performance implication in going with the slightly slower as if almost all uses of this function are in the Ready function, which is most likely the case.

Sorry if I wasn't very clear with my previous comment, but performance wasn't my primary concern and I just mentioned it as a hypothetical argument that might be put _against_ my preferred approach.

@paulloz

GetNode will always return a class inheriting Node though.

It's true, but still there can be such cases where we'd want to reference a node by its interface, rather than by its concrete type.

Suppose we have a class which reads user settings, but in different ways according to the configuration. We can implement this by dynamically adding a child node which implement IConfigurable interface to the class. And while it's possible we can make an abstract class like ConfigurableNode : Node, there's no reason to make it mandatory.

Also, it's often considered better to program against interfaces rather than actual types, and I don't think it's better to exclude such cases where there are other classes that also represent something configurable, but not actually a node.

[paulloz]

@mysticfall I'd advise to use the non generic GetNode in this case and let the user cast the returned value as they want. So we can have a strong compilation error when GetNode<T> is used with a wrong type in the general use case.

And @mysticfall @neikeq: I think you convinced me that a () cast would be better than using as here.

[mysticfall]

@paulloz It sounds reasonable to me. I don't find it to be as importance an issue as whether or not we should provide different methods to resolve nodes anyway, so I wouldn't object if that's what the other people consider to be a better option.

[NathanWarden]

@mysticfall

The reason why it is better to throw a specific exception instead of returning null for both the normal and exception paths is that it prevent such cases where it'd trigger an unexpected behavior further down the code, like causing NullReferenceException in a different line, thus making it more difficult to track down the actual cause.

But, needing to know whether the exception was thrown because the node doesn't exist at all or if it's just the wrong type will be a very rare case. In most cases the person making the game will know the reason, and if they don't it's not hard to debug.

It still seems to me that needing a straight cast is mostly hypothetical and might want to be used in a few rare cases, in which case why not just use (MyType)GetNode(path) as it has far less typing than GetNodeOrThrow<MyType>(path)

Also, using as for GetNode<T>(path) where it just returns null is consistent with GetNode(path) where it returns null.

It's true, but still there can be such cases where we'd want to reference a node by its interface, rather than by its concrete type.

But, if you need this then you can just assign it to the non concrete type. IE. Object myObj = GetNode<Node>(path);

[mysticfall]

@NathanWarden

But, needing to know whether the exception was thrown because the node doesn't exist at all or if it's just the wrong type will be a very rare case. In most cases the person making the game will know the reason, and if they don't it's not hard to debug.

But what about the possibility that it could trigger a side effect? It's the same reason why we use assertions or argument validations to make the code fail immediately when a requirement is not met, rather than let it flow and cause unexpected secondary problems further down the road.

And even such a case where the developer needs to distinguish between a missing node and a node with a wrong type, it's still a valid use case while having multiple overrides or different versions of the same functionality is a commonly used practice.

So, unless we have any practical reason to keep the number of methods as few as possible, I don't see any reason why we should ignore a valid use case or best practice to save a few lines of a class that users don't need to edit.

[NathanWarden]

But, the only side effect would be that the node was null, in which case you'd get an exception thrown anyway when you try to reference it.

[mysticfall]

@NathanWarden It can be a few lines down, or a few method calls apart. And it's not even always the case that it causes NullReferenceException on the same reference, since the exception can be thrown by some other variable that fails to initialize because of the missing node, and it can also trigger a different branch without throwing an exception.

[NathanWarden]

If you're trying to reference the type is will always be a NullReferenceException

There's no way it's because it failed to get a value unless you did something like this:

``` C#
MyType node = GetNode();
int valueToGet = 0;

if (node != null)
{
valueToGet = node.GetValue();
}

DoSomethingWithValue(valueToGet);
```

You have to explicitly ignore your own error checking code for your scenario.

[mysticfall]

@NathanWarden I don't understand why you'd want to force users to do if-checks every time, if the reason why you're opposing the idea of adding additional methods to the API is the verbosity.

In my opinion, it's much better to save the users' work instead of that of the developers if we have a choice.

And if you can't think of any case where returning null would cause confusion, please consider the following scenario:

```c#
var node = GetNode(path);

// potentially more codes here.

// 'otherValue' also can be null.
if (node.GetValue() == "A" || otherValue.GetValue() == "B") {
//...
}

Or this :
```c#
MyType node = null;

if (someCondition) {
    node = GetNode<MyType>(path);
}

var value = node?.GetValue();

If value is null, how do you know if it was caused by a missing node rather than by someCondition being false?

There are numerous other cases that might obfuscate the real cause of the problem with returning null instead of throwing an exception.

And one of the reasons why we have exceptions in the first place is because it is generally accepted that it's better to explicitly notify the caller something is wrong than letting programmers use arbitrary values like -1 or null for error conditions and rely on manual checks.

[neikeq]

It still seems to me that needing a straight cast is mostly hypothetical and might want to be used in a few rare cases, in which case why not just use (MyType)GetNode(path) as it has far less typing that GetNodeWithException<MyType>(path)

Because we need a readable and convenient to write syntax for chaining a call to the returned node. e.g.:

• ((MyType)GetNode(path)).DoSomething()
• GetNode<MyType>(path).DoSomething()

The former is less readable because the amount of parenthesis, and less convenient to write because you would usually write GetNode( first (or (MyType)GetNode() and then go back to surround the cast result in order to be able to call the method. It's easier to refactor code with the latter for the same reason.

Regarding the InvalidCastException vs NullReferenceException, @mysticfall explains it well. The correct way is to throw the exception right where the problem happened.

We should also use the appropriate exception for the error. A NullReferenceException doesn't say much about the cause being an invalid cast, in fact it could make the user believe it is caused by something else. This applies even more for GetNode, which returns null when no node is found.

C# public override void _Process(delta) { if (nodeThatGetsSpawned == null) { try { nodeThatGetsSpawned = GetNode<NodeType>(path); // try/catch version } catch {} } }

The problem here is you are using the wrong tool for what you want to do. You can write the above code like:

``` C#
nodeThatGetsSpawned = GetNode(path) as NodeType;
if (nodeThatGetsSpawned != null) // ...

``` C#
GetNodeOrNull<NodeType>(path)?.DoSomething();

[NathanWarden]

I will give it over to throwing an exception team :)

I'd still prefer to have a less convoluted API though. If at all possible, can we please just keep it as GetNode<Type>(path) where it will throw an exception instead of having multiple methods like GetNodeOrThrow and GetNodeOrNull. There are other C# standard ways to handle this with as and null checking if this is what we want to do.

IE:

C# if (GetNode(path) is MyType myType) { myType.DoMyTypeStuff(); }

I just think adding multiple methods to the API when it could be done manually without much issue is bloating the API and making it inconsistent with the core API revealed via GDScript. If I'm not mistaken this is one of the Godot PR rules also that if it can be done via scripting to not add it.

[paulloz]

Side note: is this the full list of methods needing the same treatment?

• GetNode(NodePath)
• GetChild(int)
• GetOwner()
• GetParent()

[mysticfall]

@NathanWarden I'd like to point out how C#'s core API is handling such an issue, like for example, IEnumerable interface as shown below:

• https://msdn.microsoft.com/en-us/library/9eekhta0(v=vs.110).aspx#Extension%20Methods

You can see how it provides many useful extensions, including several overrides of the same method, or similar versions that do it in a slightly different ways, like Last and LastOrDefault (something very close to what we are intending to do).

Maybe we can consider keeping the Node API as concise as possible and provides many utilities as extension method, as it seems to be the standard approach in C#'s core API.

But I don't think we shouldn't include anything that if users cannot do it themselves. If it were the hard and fast rule, we probably need to remove about 2/3 of methods exposed by such classes like Vector3 or Transform.

And even though I agree that it's better to keep the codebase less convoluted, if it comes to a question whether we should add a few methods to the core API or we should make every users of the platform to spray plenty of null checks everywhere in their individual projects, I don't see any reason why we should regard the latter to be a more desirable situation, especially considering the former is pretty much a common practice that's used by the core API of C# itself.

[NathanWarden]

You can see how it provides many useful extensions

That's fine, I'm not against using extension methods at all if they're useful :)

But I don't think we shouldn't include anything that if users cannot do it themselves.

I agree, if it helps produce a project faster I'm all for it. I just don't think it makes sense to provide two additional generic versions of methods that do essentially the same thing when a person can easily add the other one (probably as an extension method) if they really want it.

So, I propose we just use the GetNodeOrThrow version, but just call it GetNode<T>(path) and anyone else can implement the GetNodeOrNull on their own.

[PJB3005]

Instead of a GetNodeOrNull, another option might be a bool TryGetNode<T>(string path, out T node), which follows the classic Try* pattern of using an out parameter and returning it if was written to successfully. You can still use it for assignment if you don't care about the possibility of null, but of course you can't do a chain like GetNode().Thing().

Also, a fundamental issue with extension methods is that they don't work if you're using implicitly this.:

public class Foo : Node
{
    public override void _Ready()
    {
        // Compiler error due GetNode not being generic, it's not finding the extension.
        GD.Print(GetNode<Sprite>("Sprite2D"));
    }
}

public static class Exts
{
    public static T GetNode<T>(this Node parent, string key) where T : Node
    {
        return (T)parent.GetNode(key);
    }
}

whereas this.GetNode<Sprite>(...) does work.

[paulloz]

Also, a fundamental issue with extension methods is that they don't work if you're using implicitly this.

That's one reason why I was saying it could be a temporary solution.

[maikramer]

I dont like the idea of using any kind of strings because if i change the name of the node in tree or use a refactor, i have a exception.
This is what i use in my game, then if anybody likes the idea, can be used.
Of course this is slow but is called only one time in _Ready()

public static T FindClass<T>(this Node node) where T : Node
        {
            var children = node.GetChildren();
            foreach (Node t in children)
            {
                if (t is T child)
                    return child;
                if (t.GetChildCount() > 0)
                {
                    var ret = t.FindClass<T>();
                    if (ret != null)
                        return ret;
                }
            }

            return null;
        }

Then i find what i want like this

//Extreme example (very slow)
var gameManager = GetTree().Root.FindClass<GameManager>();

//Normal example
var playerController = player.FindClass<Controller>();

[paulloz]

I think it's not a good habit to iterate over every node to find what you're looking for.
In addition, there's currently a correlation between the node name in the tree and the class name in the script. If you changed one you'll need to change the other so... Using the node name as a string or the type is pretty much the same thing.

[maikramer]

@paulloz Yes I understand, even if you are not forced to, is a good practice in Godot right? I could use maybe. After your commit.

node.GetNode<Car>(nameof(Car));

Or do you suggest something else?

[mysticfall]

@maikramer This is a version I use :
c# [NotNull] public static T GetChild<T>([NotNull] this Node node) where T : class { switch (node.GetChildren().FirstOrDefault(n => n is T)) { case T result: return result; default: throw new InvalidOperationException( $"Unable to find node of type '{typeof(T)}' in '{node.Name}'."); } }
However, I think it's something we may consider providing in addition to the API we've discussed above rather than replacing it. Often, we might have several children with the same type and other times it could be undesirable to iterate over many nodes, as @paulloz already mentioned.

[reduz]

I personally think throwing exceptions for this is likely not a good idea, given the Godot C++ API won't throw exceptions anywhere else. It will be completely unexpected and unintuitive.

I root for GetNode and GetNodeOrNull but that's it.

[paulloz]

I root for GetNode and GetNodeOrNull but that's it.

@reduz I don't see the difference between those two functions if none is throwing an exception 🤔

[reduz]

@reduz I don't see the difference between those two functions if none is throwing an exception

Because Godot is designed to not throw exceptions, instead it logs the errors and tries to recover as best as possible.

This is by design to ensure your game does not crash for a stupid mistake and remains stable once published, even if common errors happen.

In this case, GetNode will log the error and GetNodeOrNull will not.

[mysticfall]

@reduz I can understand how other languages handle it differently, but it is a quite expected and intuitive behavior in the context of C# API though, as it mimics the way IEnumerable.First / FirstOrDefault works exactly, which is one of the often used core APIs by C# developers.

And I believe it'd be better to provide an API that is idiomatic to each specific languages rather than a rough translation of Godot's C++ API. Aside from exceptions, GDScript doesn't have a concept of an interface too but I don't think we shouldn't use interfaces in our C# API simply because they are not used by some other bindings.

I think there could be more chance for a typical developer who wants to develop a Godot game using its C# API to be already familiar with C#'s conventions and its core API, than to be so with Godot's C++ API.

[paulloz]

I understand, however (correct me if I'm wrong) there is only a get_node function in GDScript that'll return a null instance and always log an error if the requested Node does not exist.

[reduz]

@paulloz yeah, i always thought about doing a get_node_or_null in GDScript too, so you avoid the has_node()

@mysticfall As I mentioned before, this is done by design, and if you get used to it you will find it to be a priceless feature, it's unrelated to idiomatic things because C++ also has exceptions and we don't use them.

And even in the case you still wanted exceptions, you have to understand that Godot is not a C# engine, it only provides C# bindings. If you want it to have idiomatic consistence with C# and all functions that can fail return exceptions, it would be needed to be done manually for every function in the C# bindings, at a huge human and performance cost. It's just not worth it either way.

[neikeq]

I actually like @maikramer's FindClass<T>() method without the mask parameter (it should include the parameters bool recursive = true, bool owned = true though). It's too slow though, because you are iterating the tree in managed code just to check if the node has a script of type T, and that will require marshaling, which creates a managed instance for each node iteration, even when these nodes don't have a C# script. I would implement it in native code and add it to the NodeExtensions.

In addition, there's currently a correlation between the node name in the tree and the class name in the script. If you changed one you'll need to change the other so...

No, the correlation is between the file name and class name, e.g.: Controller.cs and Controller.

[paulloz]

@neikeq Obviously, my bad I got confused 😰.

[mysticfall]

@reduz Personally, I do not agree that not throwing exceptions is a better way in every cases (i.e. how can it be possible to recover when the game can't find GameManager because of a typo?).

But if someone believes it to be so, what prevents him or her from using GetNodeOrDefault instead of GetNode everywhere? If it's a recommended practice in Godot, then we may put it in the documentation to advise users to prefer GetNodeOrDefault, or we may even flip the behavior so GetNode wouldn't throw an exception but something like GetNodeOrFail would.

And I don't see much need to provide two different versions if all the differences they have is whether to log the error. In that case, we would be giving a false impression to C# developers that GetNode would throw an exception by providing GetNodeOrDefault because the pair of such names will remind them of core C# APIs which behave that way.

So, if @reduz is so against the idea of throwing an exception anywhere in C# API, I'd suggest to drop the whole idea of providing two different versions of GetNode and let people who think differently to provide something like GetNodeOrFail for their own projects. I think not following a common C# convention is one thing but misleading users is a whole different matter.

[reduz]

@mysticfall well, get_node_or_null makes sense in the context of efficiency mostly, not the error or exceptions one to me. if (has_node): get_node() is less efficient than get_node_or_null(). This is not a specific C# related problem from my side, so I think different issues are being mixed up here.

[neikeq]

The problem with the exceptions in #17720 (except the InvalidCastException which is fine) is that they work differently than the non-generic methods. GetNode<T> throws NodeNotFoundException and GetChild<T> throws IndexOutOfRangeException, but GetNode and GetChild don't throw anything. GetParent<T> and GetOwner<T> shoudn't even throw a NodeNotFoundException IMO. It would be different if it would be explicit in the name that an exception can be thrown, like GetNodeOrThrow<T>.
The generated methods won't be made to throw exceptions. That would require hard-coding each of these methods in the bindings generator. It's also confusing that just that small subset of the generated API can throw exceptions.

I'm fine with having the non-generated API throw exceptions as long as it's documented and it doesn't cause confusion like the previous examples.

[mysticfall]

@reduz Yeah, I assumed to be so, and I agree that it'd be a good thing to provide the most efficient way to do things, although I somehow thought turning off the debug messages in a release build could be a common practice.

I think now I'm convinced by @neikeq's arguments that it's better to ensure generic and non-generic versions of the same method to behave in the same way.

So, what about flipping the behavior so as to make GetNode<T> to return null like its non-generic counterpart, while providing an alternate version as GetNodeOrFail or GetNodeOrThrow which throws an exception?

Personally, I believe it's good to provide a version with a fail fast behavior in the case of a missing required dependency, and also I agree with @neikeq that it's ok to use exceptions at least in the C# specific APIs.

But if we decide we shouldn't do that, I think we better avoid reusing the same naming convention as DoSomething / DoSomethingOrDefault in a different context than it is commonly used in the core C# API to prevent potential confusion.

[PJB3005]

@reduz

And even in the case you still wanted exceptions, you have to understand that Godot is not a C# engine, it only provides C# bindings. If you want it to have idiomatic consistence with C# and all functions that can fail return exceptions, it would be needed to be done manually for every function in the C# bindings, at a huge human and performance cost. It's just not worth it either way.

I think this is a pretty weak argument. If we're gonna have C# support in Godot, we should put the effort in to make it good. Unless of course the plan is to follow @mysticfall's idea and let a third party provide a proper API for Godot via C#.

Unrelated:

Also, I do agree that the generic method should be purely the non-generic method + cast. If we make GetNode<T> do a nullcheck, GetNode should too.

[reduz]

@PJB3005 It's not a weak argument if you understand the actual cost of doing this. Godot does all the checks within it's own functions. If there is an error, it will be logged and the function recovered.

From the C# side there is no way to know if something failed in the function you called so this can be converted to an exception, and the C++ side will not be modified to adapt to this convention, because it's simply undesired (in other words, current design works fine, and is tested and proven). Your only hope is doing function validation BEFORE the code enters C++, but that is not only a lot of manual work, but very inefficient (or impossible) in most functions.

Your only hope of achieving an exceptions based API in C# is pretty much rewriting all Godot C++ side to go along with that. This is not going to happen (because it would be pretty much just making Godot worse without any reason).

In other words, what I mean is that even if someone wanted to provide a "proper exceptions based API for Godot via C#", it's not even technically possible. What you have right now in this regard is take it or leave it.

[paulloz]

@neikeq, well the first version of this PR was not using any exceptions, it was only casting to the requested type. This was changed according to the general direction the discussion above (and in the PR) was going.
That can be changed back to the original PR if needed (or be done in another one, be either me or anyone) in no time ☺.

The important thing is that the cast needed with the current GetNode makes it a pain to write C# code and prevents users to try out the mono builds because it feels a bit hacky.

[PJB3005]

Well yeah, for a lot of cases I do understand that adding exception support is infeasible and painful, but stuff like GetNode is easy and common to do this for.

[reduz]

@PJB3005 then the problem switches to, is it worth doing it in some places rather than others and risk being inconsistent, or should the API be consistent? IMO it should be consistent, but i have no idea what others may think about it

[mysticfall]

So, what is a concensus on this matter now? I think it might be proper to list possible options we have so far:

1. Only implement GetNode<T> which returns null when failing.
2. Along with 1, also provide GetNodeOrFail<T> (or GetNodeOrThrow<T>) which throws an exception.
3. Implement GetNode<T> and GetNodeOrDefault<T> (or GetNodeOrNull<T>) both of which don't throw an exception, but make the former logging an error when fails.

If I have missed anything, please tell me so then I'll update this comment. (I didn't mention the FindClass that @maikramer suggested, as I believe it to be something might be done along with either one of the above options.)

Personally, I support 2 while opposing 3 for the above stated reasons. If there's a way to implement 3 without confusing users, I don't have a problem with it but even in that case, I'd like to have a fast failing version too.

On the other hand, I can live with 1 as I can provide my own version of GetNodeOrFail in that case.

So, what do you think to be the approach we should take?

[reduz]

@mysticfall I am probably going to implement GetNodeOrNull on the C++ side anyway, which is needed anyway if you want to throw exceptions (you need to check a null return value without error log. to make this... adding it in C++ also will make it available on C# automatically), so the actual question is much simpler:

1. GetNode<> throws exception if it fails, hence making it inconsistent with the rest of the Godot C# API.
2. GetNode<> does not throw exception, so you can use GetNodeOrNull<> if you want to consider null.

My point of view on this is that 2. makes a LOT more sense because of the following reasons:

• In most cases, you expect the node to return something, or you want to check if a node exists. Real usage cases of GetNodeOrNull are slim to none in a game anyway.
• As you will get a null reference, you will both get an error in the log (showing the node path you tried to access and failed) and your application will crash if you try to use it anyway, so an exception is pretty redundant.
• As mentioned before, in 99.9% of the cases, you will call GetNode expecting the node will be there, so handling an exception is literally pointless... will you treally put try/catch blocks everywhere when you expect this to almost always work? It's a total hassle. Just let it fail if you made a mistake, same effect.

Which, again, is why i vote for 2. It's more aligned to real life use cases and keeps api usage consistency.

[mysticfall]

@reduz As I said above, now I'm convinced by your and @neikeq's arguments that it'd be better to keep GetNode<T> consistent with its non-generic counterpart, so I suspect there's no one who supports 1 (in your comment, that is) at this point any more.

However, I think you have ommitted an option which I still support which is providing GetNodeOrFail<T> along with GetNode<T> as mentioned above. I don't believe anyone has produced any major argument against it yet, or we reached a concensus not to pursue that route.

As to a version which returns null without logging an error, I can see the usefulness of the method so I don't have anything against it in principle, as I already said above.

But I oppose to name it something like GetNodeOrDefault or GetNodeOrNull because it has a chance to mislead C# users, as the naming convention in that context suggests the other version would throw an exception which is not the case in Godot.

So, I'd like to have such a method to choose a different name, at least in C# API to avoid confusion, and regardless of it, I'd like to have a GetNodeOrFail which shows a fast failing behavior too.

As to the usefullness of such a method, I can enumerate real life cases of it which are abundant in the core C# API, and also can argue from the software engineering point of view, but I'm not sure if you have an opinion against this idea as well.

[reduz]

@mysticfall Think of it this way. For 99.9% of cases, you will expect to get the node. If you don't it's a failure and error needs to be logged (godot logs errors with a backtrace, btw). This means, this is the function that will be used in all games, tutorials, etc.

How does it make the most sense to name this function, given it will be used everywhere and in far most cases:

1. GetNode
2. GetNodeOrFail

I think It's clearly 1. It would be strange if Godot most commonly used function to get a node would be GetNodeOrFail. It definitely needs to be just GetNode, and it needs to work as you expect it to work in most cases.

[reduz]

And then again, you will not expect an exception to be thrown if Godot does not use them, and the method does not have throw when you do complete, so I think it's a non issue.

[mysticfall]

@reduz I think you have partially misunderstood my intention. It wasn't my position that we should replace GetNode<> with GetNodeOrFail<> but to provide the latter along side with the former which will behave as you proposed.

If you have any objection to adding 2 along with 1, then I'll try to explain my reasonings behind it.

[reduz]

@mysticdall what I understood you meant is:

• GetNode: gets node, does not throw error, just returns null if node not found
• GetNodeOrFail logs error if not found

Am I right?

[mysticfall]

No, I'm with @neikeq in that I believe it'd be ok to use exceptions in non-generated, or C# specific APIs, so it'd be:

• GetNode<> : Works as you suggested, and in accordance with the behavior of the non-generic version.
• GetNodeOrFail<>: Throws an exception when it can't find the node.

[reduz]

@mysticfall Ah, in that case I am fine. I think it's not really useful myself as it will likely not be used very often, but that's up to you guys.

[mysticfall]

@reduz I'm glad we reached an agreement. From my observations, it seems that different languages treat the subject in a quite different manner, so I understand people would have different opinions depending on their background or preferences. Probably it'd be another reason to offer such an alternative to satisfy everyone.

I'd like to see how other people think about the issue as well, and maybe we should also talk about if we need to include other useful methods like FindChild as suggested above.

[M4gma]

I think it's more useful if we keep the usual GetNode() function so we have more versatility when dealing with nodes and add just one GetNode<>, GetNodeOrFail because it's too strange to not know the type of the node you're requesting

[Fido789]

I am pretty new to Godot, but from my (C# developer) point of view we need four methods:

public static T GetChild<T>(this Node parent) where T : Node

public static T GetDescendant<T>(this Node parent) where T : Node

public static IEnumerable<T> GetChildren<T>(this Node parent) where T : Node

public static IEnumerable<T> GetDescendants<T>(this Node parent) where T : Node

Or the second two could probably return IList<T> or T[], but you get the point.

Of course, these must be backed by some kind of cache, so they would be blazing fast.

Also naming is much more intuitive than GetNode(). Btw, allowing node.GetNode() to get nodes anywhere from the scene is pretty weird. It should be able to access node's descendants only.

Singular methods will return null if no node is found, plural methods will return empty collections. There is no need for GetSomethingOrFail, this will only pollute API and if anyone needs it, it is easy to implement.

I guarantee you, that any sane C# developer will never ever use anything else (like magic strings) to get descendant nodes.

[mysticfall]

I am pretty new to Godot... I guarantee you, that any sane C# developer will never ever use anything else (like magic strings) to get descendant nodes.

I can guarantee you that you'll be referencing nodes by their paths from time to time (without losing your sanity), if you learn more about making non-trivial games with Godot.

And welcome to the community! :)

[Fido789]

And welcome to the community! :)

Thank you @mysticfall

I can guarantee you that you'll be referencing nodes by their paths from time to time (without losing your sanity), if you learn more about making non-trivial games with Godot.

Well that is possible, but from my experience I would expect it to be the other way around :)

[PJB3005]

While I absolutely do agree an overload of GetNode<T>() => GetNode<T>(T.GetType().Name) is a good idea, there are definitely still gonna be cases where you want both. What if you have two audio players on a node and you want to select each?

[Fido789]

@PJB3005

While I absolutely do agree an overload of GetNode() => GetNode(T.GetType().Name) is a good idea, there are definitely still gonna be cases where you want both. What if you have two audio players on a node and you want to select each?

Doesn't

public static IEnumerable<T> GetDescendants<T>(this Node parent) where T : Node

solve this issue?

[paulloz]

Well, I don't really see the point of this generic:

public static IEnumerable<T> GetDescendants<T>(this Node parent) where T : Node

Isn't it implying every child node is of type T? That's probably not the general use case. And if not, it's basically GetChildren() because you'll end up using it with Node as T :thinking:

(Also, this issue is about a specific C# generic version of GetNode. I don't think it's the right place to discuss what seems to be large changes to the Node API.)

[Fido789]

Isn't it implying every child node is of type T? That's probably not the general use case. And if not, it's basically GetChildren() because you'll end up using it with Node as T 🤔

No, it isn't. The method will return all nodes that are descendants of parent node AND are compatible with T.

[paulloz]

Oh yeah I see. I guess it could be useful in some cases.

Still don't get the whole dropping of NodePath thing tho.

I guarantee you, that any sane C# developer will never ever use anything else (like magic strings) to get descendant nodes.

I guess I'm not sane anymore :sweat_smile:, and a lot of Godot users aren't either :grin:

[mysticfall]

@Fido789 I'm afraid it might not be a proper place to discuss general game development issues, but if you are curious, think of a character node which contains mesh instances for its head, body, arms, legs, and more.

Get*<T> makes sense when what you want to fetch can be differentiated by a specific type, but not for homogenous items. What would you do if all those meshes are the same MeshInstance type?

Of course, you can create HeadMesh, BodyMesh, and so on to distinguish them. But besides being tedious, it also goes against the general principles of OOP, since the paradigm recommends subclassing a parent type to extend its funtionalities, not to differentiate among many instances of the exact same concept, namely MeshInstance in this case.

Like I said above, if you become more familiar with writing games, you'd probably see many such cases where you need to find certain nodes among others of the same type in a deeply nested hierarchy. Using a node path to efficiently fetch a specific node in such occasions is quite an essential part of the core API.

[paulloz]

@mysticfall @Fido789 Especially in Godot where you use nodes for everything.
It really is usual to have a lot of Timer nodes as children or to even just use a huge lot of Node/Node2D (the first example coming into my mind is that many people are doing FSM where every state and every transition is a Node2D node).

[Fido789]

@paulloz

I guess I'm not sane anymore 😅, and a lot of Godot developers aren't either 😁

Well, I didn't mean to offend anyone, don't take me too serious :)

@paulloz @mysticfall From your posts I see there is a lot of misunderstanding between us that it would be really out of scope of this thread to clarify them all. Of course there is probably need for keeping node paths, but where possible, I will use the four methods above. I just value readability and maintainability of code a lot.

Anyways, it is really cool to see that Godot has a living community and even on saturday there are people ready to discuss issues :thumbsup:

[maikramer]

@Fido789 i agree with everything you say, use magic strings is really a bad pratice, my team is working in a procedural game, and the map is divided in chunks, and of course them names are also generated, when i want to get all the chunks, i use my version of GetDescendent

My other team mates are artists, then i dont care if they rename everything in editor, or even move Map, make it child of another thing, etc.

I see a lot of people here thinking everybody knows what you do in code, and that others wont break anything just because they renamed map to NewEden in editor. Or move Map as a child of another node.

Another thing, @mysticfall has a great framework in his github, has lots of adaptations to c#, including a GetChild like extension method, that would be difficult to push to godot.

[Fido789]

@maikramer Exactly. Well, for sure, that these methods will be slower than directly accessing node by its path, but I will gladly trade the speed for the maintainability in most of the cases. The key is to use it carefully, for instance in _Ready method and not every frame, like you mentioned.

Let level designers use grenades, gas cans, or anything else, let them name these explosives however they wish, I will get them all

var explosives = scene.GetDescendants<IExplosive>();

and they all will be part of my

if (isGameOver)  explosives.ForEach(Explode);

spectacular explosion :)

[Fido789]

Btw, here is my quick, dirty, untested, slow, GC firing and probably buggy solution

public static class NodeExtensions
    {
        public static T GetDescendant<T>(this Node node)
            where T : Node
        {
            return (T)node.GetChildren().Cast<Node>().Traverse().FirstOrDefault(n => n is T);
        }

        public static IList<T> GetDescendants<T>(this Node node)
            where T : Node
        {
            return node.GetChildren().Cast<Node>().Traverse().OfType<T>().ToList();
        }

        public static T GetChild<T>(this Node node)
            where T : Node
        {
            return (T)node.GetChildren().FirstOrDefault(n => n is T);
        }

        public static IList<T> GetChildren<T>(this Node node)
            where T : Node
        {
            return node.GetChildren().OfType<T>().ToArray();
        }

        private static IEnumerable<Node> Traverse(this IEnumerable<Node> nodes)
        {
            var stack = new Stack<Node>();

            foreach (var node in nodes)
            {
                stack.Push(node);
            }

            while (stack.Count > 0)
            {
                var current = stack.Pop();

                yield return current;

                foreach (Node child in current.GetChildren())
                {
                    stack.Push(child);
                }
            }
        }
    }

But I will refactor it later and honestly, I would do anything to avoid magic strings.

[mrcdk]

Or... you could use groups http://docs.godotengine.org/en/3.0/getting_started/step_by_step/scripting_continued.html#groups

```c#
if (isGameOver) {
GetTree().CallGroup("explosive", "Explode");
}

Or signals http://docs.godotengine.org/en/3.0/getting_started/scripting/gdscript/gdscript_basics.html#signals and connect whatever need to know that's game over

```c#
if (isGameOver) {
    EmitSignal("game_over");
}

[Fido789]

@mrcdk Sure, I could, there's more than one way to skin a cat, but it will hardly be as easy and bulletproof as my two line solution.

[mysticfall]

First off, even if removing GetNode(path) was somehow justified I don't think it's a proper place to discuss it, because it'd be a whole different issue.

And if we can't remove GetNode(path) from the core API, we better provide a type safe, generic variant of it, which is the whole point of this particular issue.

And as to @Fido789's suggestion, I agree that GetDescendants would be nice to have even though I'd change its implementation to a bit more cleaner and efficient one.

But in general, it's considered a bad practice to rely on such an inefficient way to get references to dependencies in a game, so even if it could be useful for some corner cases it's not really a viable alternative to GetNode(path).

@Fido789 I appreciate sharing your thoughts and code snippets for this issue, but I'm afraid you have some misunderstanding of the "magic string is bad" principle, and general use cases of the node API in Godot.

I'd be happy to continue discussing on either points with you but it'd probably derail this thread even further which isn't my intention. So, I'd encourage you to join our Discord server and visit #general or #csharp channel and continue the discussion there.

[neikeq]

Closing as this was implemented by #17720 and #20298