godotengine
Add a method to construct a NodePath from a StringName
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.
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?
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?
https://github.com/godotengine/godot/blob/e74bf831c2d3ece79b849405f03999281f807648/core/variant/variant.cpp#L558-L563
https://github.com/godotengine/godot/blob/e74bf831c2d3ece79b849405f03999281f807648/core/variant/variant.cpp#L714-L718
https://github.com/godotengine/godot/blob/e74bf831c2d3ece79b849405f03999281f807648/core/variant/variant.cpp#L722-L726
@dalexeev Good idea, pushed a change to add that, but we should also keep the method exposed for explicitness.
See also:
https://github.com/godotengine/godot/blob/fd31dc7f32855f23f0c38792fe42d501165f708c/core/variant/variant_construct.cpp#L79-L82
https://github.com/godotengine/godot/blob/fd31dc7f32855f23f0c38792fe42d501165f708c/core/variant/variant_construct.cpp#L176-L182
Perhaps it makes sense to add constructors only in GDScript, even if we can't add them in C++?
@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.
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.
