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

Add the ability to convert StringName to NodePath in C# #7602

Open
AndreSacilotto opened this issue Sep 3, 2023 · 2 comments
Open

Add the ability to convert StringName to NodePath in C# #7602

AndreSacilotto opened this issue Sep 3, 2023 · 2 comments

Comments

@AndreSacilotto
Copy link

AndreSacilotto commented Sep 3, 2023

Describe the project you are working on

3D Battle

Describe the problem or limitation you are having in your project

Some NodePaths are simply the property name:

CreateTween().TweenProperty(Camera, "position", endPos, delay);

So I thought I could use PropertyName:

CreateTween().TweenProperty(Camera, Node3D.PropertyName.Position, endPos, delay);

I could cast it to string and then the implicitly conversion of string -> NodePath.

CreateTween().TweenProperty(Camera, Node3D.PropertyName.Position.ToString(), endPos, delay);

But I think you should be able to convert from StringName to NodePath without ever using a string:

CreateTween().TweenProperty(Camera, (NodePath)Node3D.PropertyName.Position, endPos, delay);

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

Looking at source code of StringName and NodePath I notice they are oddly equal.
I even ask myself if there any point in having both, other than just matching GDScript?

Can't StringName and NodePath have a common ancestor? I think that is possible since both are not auto-generated. And the only fields difference between then are godot_string_name and godot_node_path, but both are the exact same thing.

With that question aside I think there some solutions (In the order I like the most):

  1. Makes somehow possible to cast StringName to NodePath. (If they have a common ancestor this would be trivial)

  2. Make all functions that accept NodePath also accept StringName

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

CreateTween().TweenProperty(Camera, (NodePath)Node3D.PropertyName.Position, endPos, delay);
CreateTween().TweenProperty(Camera, Node3D.PropertyName.Position, endPos, delay);

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

It can be work around by simple doing a string cast, but that will call: https://github.com/godotengine/godot/blob/fa3428ff25bc577d2a3433090478a6d615567056/modules/mono/glue/GodotSharp/GodotSharp/Core/StringName.cs#L90C32-L90C40
and then:
https://github.com/godotengine/godot/blob/fa3428ff25bc577d2a3433090478a6d615567056/modules/mono/glue/GodotSharp/GodotSharp/Core/NodePath.cs#L119

Which is not ideal

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

StringName and NodePath are built-in classes

@KoBeWi
Copy link
Member

KoBeWi commented Sep 3, 2023

Related godotengine/godot#72702
It's implemented in core, so maybe it works with C# too.

@raulsntos
Copy link
Member

I even ask myself if there any point in having both, other than just matching GDScript?

There is a point, type safety. Some methods in Godot's API may take StringName and others may take NodePath, having separate types ensure you don't accidentally use the wrong one. There's a semantic difference between a StringName and a NodePath. Also, internally we need to know which type it is.

the only fields difference between then are godot_string_name and godot_node_path, but both are the exact same thing.

No, they are not the same thing. They are both pointers but in Core they are different types. If you were to use a pointer to a StringName in an API that expects NodePath it will fail, the data they point to has a different structure:

https://github.com/godotengine/godot/blob/75de1ca76871fdf7f5a9e081aa57ec0e33061107/core/string/node_path.h#L38-L47

https://github.com/godotengine/godot/blob/75de1ca76871fdf7f5a9e081aa57ec0e33061107/core/string/string_name.h#L54-L68

  1. Makes somehow possible to cast StringName to NodePath.

There's currently no way to go from StringName to NodePath in Core1, so the C# implementation would have to cast to string first. This implementation would be the same as what you are doing right now, but it will hide the fact that it's going through string which may give some users the impression that it's better, I'd rather not hide that.

But, if godotengine/godot#72702 gets merged, I think it'd be fine to provide an explicit conversion.

  1. Make all functions that accept NodePath also accept StringName

Core does not support method overloads. Maybe you are suggesting that the C# generator should generate the overloads, this would result in an explosion of overloads. For example imagine a method with 2 NodePath parameters:

void MyMethod(NodePath a, NodePath b);

Then, we would have to generate 3 overloads to consider all possible combinations:

void MyMethod(StringName a, StringName b);
void MyMethod(StringName a, NodePath b);
void MyMethod(NodePath a, StringName b);

Also, this would make the conversion between StringName and NodePath implicit, hiding that there is a cost in this conversion. It would be preferable for the user to make this conversion explicitly, to express intent.

Footnotes

  1. https://github.com/godotengine/godot/pull/72702 would add this functionality to Core.

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

4 participants