Add a method to construct a NodePath from a StringName #72702

aaronfranke · 2023-02-04T03:28:58Z

This allows constructing a NodePath directly from a StringName, which skips the String parsing logic, if the given StringName does not contain a slash or colon and can be used as-is.

I considered a constructor, but this would break all uses in the C++ code that construct NodePath from char * literals, and we need to depend on two other constructors anyway, so this uses a static method instead.

Zireael07 · 2023-04-01T10:05:50Z

I just ran into a situation where code that worked fine in 3.x doesn't due to this. Can this get merged into one of the patch releases?

dalexeev · 2023-06-22T16:39:21Z

I considered a constructor, but this would break all uses in the C++ code that construct NodePath from char * literals, and we need to depend on two other constructors anyway, so this uses a static method instead.

Perhaps we just need to add the StringName(NodePath) and NodePath(StringName) constructors for GDScript only?

godot/core/variant/variant.cpp

Lines 558 to 563 in e74bf83

    
           case STRING: { 
        
           	static const Type valid[] = { 
        
           		NODE_PATH, 
        
           		STRING_NAME, 
        
           		NIL 
        
           	};

godot/core/variant/variant.cpp

Lines 714 to 718 in e74bf83

    
           case STRING_NAME: { 
        
           	static const Type valid[] = { 
        
           		STRING, 
        
           		NIL 
        
           	};

godot/core/variant/variant.cpp

Lines 722 to 726 in e74bf83

    
           case NODE_PATH: { 
        
           	static const Type valid[] = { 
        
           		STRING, 
        
           		NIL 
        
           	};

aaronfranke · 2023-06-22T20:15:25Z

@dalexeev Good idea, pushed a change to add that, but we should also keep the method exposed for explicitness.

dalexeev · 2023-06-23T09:19:59Z

See also:

godot/core/variant/variant_construct.cpp

Lines 79 to 82 in fd31dc7

    
           add_constructor<VariantConstructNoArgs<String>>(sarray()); 
        
           add_constructor<VariantConstructor<String, String>>(sarray("from")); 
        
           add_constructor<VariantConstructor<String, StringName>>(sarray("from")); 
        
           add_constructor<VariantConstructor<String, NodePath>>(sarray("from"));

godot/core/variant/variant_construct.cpp

Lines 176 to 182 in fd31dc7

    
           add_constructor<VariantConstructNoArgs<StringName>>(sarray()); 
        
           add_constructor<VariantConstructor<StringName, StringName>>(sarray("from")); 
        
           add_constructor<VariantConstructor<StringName, String>>(sarray("from")); 
        
           add_constructor<VariantConstructNoArgs<NodePath>>(sarray()); 
        
           add_constructor<VariantConstructor<NodePath, NodePath>>(sarray("from")); 
        
           add_constructor<VariantConstructor<NodePath, String>>(sarray("from"));

Perhaps it makes sense to add constructors only in GDScript, even if we can't add them in C++?

raulsntos

This looks good to me.

From the perspective of the C# bindings, I wonder if it'd be better to provide a static method or constructor that takes the StringName and just creates the NodePath assuming it contains no slashes or colons.

This would be an "unsafe" way to create a NodePath from StringName but I wouldn't expose this in the C# bindings. It would be used internally to create the NodePath when we can guarantee that the StringName doesn't contain slashes or colons. This means the logic to check this would be done in C# directly which would allow us to use IndexOfAny which is vectorized.

However, using IndexOfAny would require us to convert the StringName to a C# string first and that means creating a NodePath would take 2 interop calls instead of 1. I haven't benchmarked any of this so I don't know what would be better. It's possible that vectorizing the check for slashes and colons wouldn't bring much improvement over using this PR's static method, and I guess it's also possible to vectorize the C++ code in the future.

So, having said all that, I think this PR is still an improvement over the current way to create NodePaths from StringNames. I wonder if other language bindings authors are also interested in this API and would use it to either expose a way to create NodePaths from StringNames or, if it already exists, to improve their current implementation.

cc @Bromeon

doc/classes/NodePath.xml

AThousandShips · 2024-01-31T13:34:11Z

core/string/node_path.cpp

Makes me think it'd be interesting to see if we should add a findmc to complement findmk, and perhaps a contains_any or similar, there's many places that look for "is any of these present" and it'd be a good feature, just to think about I might look at an implementation later

Wouldn't that require constructing a Vector<char32_t>, so less efficient than as it is now?

But in general, that seems like a good idea.

For two arguments I'd tend to agree

We have Span now, so it could be Span<char32_t> based now :)

dalexeev · 2025-05-03T08:12:57Z

doc/classes/NodePath.xml

+		<method name="from_string_name" qualifiers="static">
+			<return type="NodePath" />
+			<param index="0" name="string_name" type="StringName" />
+			<description>
+				Constructs a NodePath from a [StringName]. This method will be faster than constructing from a String if the StringName does not contain any slashes or colons.
+			</description>
+		</method>


In my opinion, it makes no sense to expose identical constructor and static method. We can't make constructor in core for technical reasons, but in bindings we can choose what we will expose. However, I wonder if we will get the same problem with constructor in godot-cpp?

I think the static method is clearer, safer, and ensures we have a consistent API between C++ and GDScript. If both is not desired, then I will remove the constructor that you suggested (the second commit in this PR - glad I kept it separate 😛). EDIT: Removed, I have it saved locally if we need it back though.

Ivorforce

It sounds like you want to optimize the case where the NodePath argument contains no : or /. This sounds like a great idea, but I think it's better material for NodePath(const String &p_path). It would be nice for any String to NodePath conversion to benefit from this, not just StringName!

I'm also not sure if it's appropriate to add an implicit (strict) conversion from StringName to NodePath. Explicit (non-strict) conversions seem fine to me though, given that Node names may also be used as paths.

aaronfranke · 2025-06-16T09:17:22Z

@Ivorforce What specifically do you have in mind? What change to the NodePath(const String &p_path) constructor do you think we should make?

The NodePath(const String &p_path) constructor already loops through the input String to check for : and /. If there are none, then it will iterate again, copying a slice of the whole thing into the path. We could perhaps check if the path length is 1 and then just directly convert the String to a StringName and copy it in, but this check would have to account for other cases where there is 1 slice but we don't want the whole thing, such as ^"/test", and that check would be somewhat complicated, to the point that I'm not sure we should include that. Even if it makes some cases slightly faster, it would be another additional check that is slightly slower for paths that do contain a : or /, so I can't be confident it's a good trade-off. Or I guess we could have the initial loops for : and / keep track of whether any exist in a boolean, but that's also a minor cost to be paid. It would still require 2 out of the 3 existing loops to run, and converting to StringName.

The advantage of checking for this with StringName is that we expect users to only call this with inputs that do not contain : or /, meaning that for the common case, we only run 1 loop, and avoid StringName construction. Then we also have this safety check for the uncommon case, which will end up running 4 loops, and converting to StringName. The gains are greater with the StringName constructor compared to the String constructor. We could perhaps refactor the String constructor to have a single loop upfront that checks for : or /, but that would also have implications for the performance of the constructor where we expect those to occur commonly.

Ivorforce · 2025-06-16T09:45:06Z

No, you're right, it does make sense to have a StringName constructor considering that NodePath using StringName path elements. I think I made the comment assuming that String path elements were used.

Ivorforce

With my updated understanding this totally makes sense.
Just some comments that need to be addressed before we can move forwards.

core/string/node_path.cpp

Ivorforce · 2025-06-16T09:47:43Z

core/string/node_path.cpp

We have Span now, so it could be Span<char32_t> based now :)

Ivorforce · 2025-06-16T09:49:00Z

core/string/node_path.h

 	void simplify();
 	NodePath simplified() const;

+	static NodePath from_string_name(const StringName &p_string_name);


I do like factory methods more than constructors, but we currently don't do this in the codebase much.
Perhaps we should go over this idea quickly in a core meeting to run it through the rest of the core team.

We don't? I thought we do. The existing cases in the codebase (will be helpful information during the meeting):

ArgumentDoc::from_dict Basis::from_euler Basis::from_scale Color::from_hsv Color::from_ok_hsl Color::from_ok_hsv Color::from_rgba8 Color::from_rgbe9995 Color::from_string ConstantDoc::from_dict DocData::ClassDoc::from_dict EnumDoc::from_dict GLTFCamera::from_dictionary GLTFCamera::from_node GLTFLight::from_dictionary GLTFLight::from_node GLTFPhysicsBody::from_dictionary GLTFPhysicsBody::from_node GLTFPhysicsShape::from_dictionary GLTFPhysicsShape::from_node GLTFPhysicsShape::from_resource GLTFSkin::from_dictionary GodotPosition::from_lsp MethodDoc::from_dict MethodInfo::from_dict PropertyDoc::from_dict PropertyInfo::from_dict PropertyTweener::from_current Quaternion::from_euler RID::from_uint64 ScriptClassInfoUpdate::from_file_info ThemeItemDoc::from_dict TutorialDoc::from_dict Vector2::from_angle

Though, most of the GLTF ones came from me, so I'm biased. 😛

Ivorforce · 2025-06-16T09:49:45Z

core/variant/variant_call.cpp

 	bind_method(NodePath, slice, sarray("begin", "end"), varray(INT_MAX));
 	bind_method(NodePath, get_as_property_path, sarray(), varray());
 	bind_method(NodePath, is_empty, sarray(), varray());
+	bind_static_method(NodePath, from_string_name, sarray("string_name"), varray());


I'm not sure we need to expose this function. From GDScript anyway the performance benefits are probably not that important (unless you want to test this?).

The API available through GDScript is the same as what's available through GDExtension. Also, I don't see a reason to prevent GDScript from using this method.

It's often brought up that we want to keep the GDScript API surface small and simple.
GDExtension is a good point. Personally I'd love to be able to expose methods to just GDExtension (and not GDScript when it's not useful there), but this is currently impossible, I suppose.

aaronfranke added enhancement topic:core labels Feb 4, 2023

aaronfranke added this to the 4.x milestone Feb 4, 2023

aaronfranke requested review from a team as code owners February 4, 2023 03:28

aaronfranke force-pushed the nodepath-from-sname branch from 536d2b5 to 8f31602 Compare April 3, 2023 03:59

aaronfranke force-pushed the nodepath-from-sname branch from 8f31602 to 1c44cf2 Compare April 28, 2023 02:19

aaronfranke force-pushed the nodepath-from-sname branch from 1c44cf2 to 79f230d Compare June 11, 2023 07:28

AThousandShips mentioned this pull request Jun 18, 2023

get_node() does not accept StringName variables as a valid argument #71319

Open

aaronfranke force-pushed the nodepath-from-sname branch from 79f230d to 25c5898 Compare June 22, 2023 20:08

aaronfranke force-pushed the nodepath-from-sname branch 2 times, most recently from 269b481 to 3475d55 Compare June 24, 2023 20:37

aaronfranke force-pushed the nodepath-from-sname branch from 3475d55 to 21f28d8 Compare August 13, 2023 08:37

KoBeWi mentioned this pull request Sep 3, 2023

Add the ability to convert StringName to NodePath in C# godotengine/godot-proposals#7602

Open

raulsntos mentioned this pull request Sep 24, 2023

C#: Tween.TweenProperty()'s NodePath parameter is not compatible with PropertyName #82219

Open

aaronfranke force-pushed the nodepath-from-sname branch from 21f28d8 to 0f23e61 Compare September 24, 2023 19:12

raulsntos reviewed Nov 1, 2023

View reviewed changes

aaronfranke force-pushed the nodepath-from-sname branch from 0f23e61 to 974c0ae Compare November 1, 2023 20:18

aaronfranke force-pushed the nodepath-from-sname branch from 974c0ae to a8fbe09 Compare December 17, 2023 19:37

aaronfranke mentioned this pull request Dec 17, 2023

Add a get_partial_path method to NodePath godotengine/godot-proposals#7148

Closed

aaronfranke force-pushed the nodepath-from-sname branch from a8fbe09 to 6690df6 Compare January 6, 2024 08:30

aaronfranke force-pushed the nodepath-from-sname branch from 6690df6 to e1c9aea Compare January 31, 2024 06:10

AThousandShips reviewed Jan 31, 2024

View reviewed changes

doc/classes/NodePath.xml Outdated Show resolved Hide resolved

AThousandShips reviewed Jan 31, 2024

View reviewed changes

aaronfranke force-pushed the nodepath-from-sname branch from e1c9aea to d2c2b7b Compare February 26, 2024 12:42

aaronfranke force-pushed the nodepath-from-sname branch from d2c2b7b to 55433db Compare March 13, 2024 19:53

aaronfranke force-pushed the nodepath-from-sname branch from 55433db to 1210349 Compare April 13, 2024 00:26

aaronfranke force-pushed the nodepath-from-sname branch from 1210349 to e317196 Compare May 24, 2024 10:23

aaronfranke force-pushed the nodepath-from-sname branch from e317196 to c82bfb7 Compare June 26, 2024 02:11

aaronfranke force-pushed the nodepath-from-sname branch from c82bfb7 to f4276f1 Compare August 4, 2024 05:22

aaronfranke force-pushed the nodepath-from-sname branch from f4276f1 to bf0112f Compare August 30, 2024 10:26

aaronfranke force-pushed the nodepath-from-sname branch from bf0112f to 648bbf7 Compare October 2, 2024 22:31

aaronfranke force-pushed the nodepath-from-sname branch from 648bbf7 to 56240cf Compare October 31, 2024 03:47

aaronfranke force-pushed the nodepath-from-sname branch from 56240cf to 87598fe Compare December 15, 2024 08:02

aaronfranke force-pushed the nodepath-from-sname branch from 87598fe to 5dd92f0 Compare January 15, 2025 23:30

aaronfranke force-pushed the nodepath-from-sname branch from 5dd92f0 to 47a26de Compare February 15, 2025 14:51

aaronfranke force-pushed the nodepath-from-sname branch from 47a26de to 9bb2d5e Compare March 9, 2025 10:07

aaronfranke force-pushed the nodepath-from-sname branch from 9bb2d5e to 44d703c Compare March 29, 2025 08:43

aaronfranke force-pushed the nodepath-from-sname branch from 44d703c to 5b4ecd7 Compare May 2, 2025 00:18

dalexeev reviewed May 3, 2025

View reviewed changes

Ivorforce reviewed May 12, 2025

View reviewed changes

Ivorforce reviewed Jun 16, 2025

View reviewed changes

aaronfranke force-pushed the nodepath-from-sname branch 2 times, most recently from 995f842 to 84aa752 Compare June 16, 2025 12:11

aaronfranke force-pushed the nodepath-from-sname branch from 84aa752 to 250419d Compare August 20, 2025 13:52

aaronfranke force-pushed the nodepath-from-sname branch from 250419d to 0d3b01c Compare September 24, 2025 21:07

Add a method to construct a NodePath from a StringName

b7d957a

aaronfranke force-pushed the nodepath-from-sname branch from 0d3b01c to b7d957a Compare October 21, 2025 16:58

Uh oh!

Add a method to construct a NodePath from a StringName #72702

Are you sure you want to change the base?

Add a method to construct a NodePath from a StringName #72702

Conversation

aaronfranke commented Feb 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zireael07 commented Apr 1, 2023

Uh oh!

dalexeev commented Jun 22, 2023

Uh oh!

aaronfranke commented Jun 22, 2023

Uh oh!

dalexeev commented Jun 23, 2023

Uh oh!

raulsntos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronfranke Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Ivorforce left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronfranke commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Ivorforce commented Jun 16, 2025

Uh oh!

Ivorforce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aaronfranke commented Feb 4, 2023 •

edited

Loading

aaronfranke Jun 16, 2025 •

edited

Loading

Ivorforce left a comment •

edited

Loading

aaronfranke commented Jun 16, 2025 •

edited

Loading