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

String and StringName don't autoconvert and are incompatible #64171

Closed
TokisanGames opened this issue Aug 9, 2022 · 5 comments · Fixed by #68747
Closed

String and StringName don't autoconvert and are incompatible #64171

TokisanGames opened this issue Aug 9, 2022 · 5 comments · Fixed by #68747

Comments

@TokisanGames
Copy link
Contributor

TokisanGames commented Aug 9, 2022

Godot version

Godot 4 Alpha 13

System information

Windows 11/64, RTX 3070, Vulkan

Issue description

In GD3 we don't have to think hard about Strings. They were cheap and easy to use.

In GD4, String usage has been broken since StringNames are now used all over the engine and the two are not compatible.

  • They don't concatenate:
# This worked in Godot 3. It should work in GD4.
print("Node: " + node.name)
# Invalid operands "StringName" and "String" for "+" operator.

# This doesn't work:
print(&"Node: " + node.name)

# This works:
print("Node: " + str(node.name))
  • None of the String functions (eg ends_with, substr) work on StringNames because they don't auto convert and random engine functions that used to return String now return StringName.
node.name.ends_with(ending)
# Cannot find property "ends_with" on base "StringName".
# Function "ends_with()" not found in base StringName.

# This works:
str(node.name).ends_with(ending)


Because of these issues, much of our old code is broken. Moving forward, we have to care about the type we're getting back, and write in hacky explicit conversions in GDScript that may or may not work when it should be implicit and performed by C++.

Engine members and functions have no rhyme or reason as to what they return. AnimationPlayer.animation_get_next(), and find_animation() return StringName. But Skeleton3D.get_bone_name() returns String. Node has name:StringName, but also scene_file_path:String.

The StringName docs say, You will usually just pass a String to methods expecting a StringName and it will be automatically converted...". However there are a lot of instances where conversion doesn't happen.

It also says Comparing them is much faster than with regular Strings, because only the pointers are compared, not the whole strings. However String says, Strings are reference-counted and use a copy-on-write approach, so passing them around is cheap in resources. If they're already unique and cheap to pass around or compare, why are we going through all this trouble? Why not just drop StringName, revert back to GD3 behavior and enjoy our easier code and reference-counted, cheap Strings?

@Spartan322
Copy link
Contributor

Spartan322 commented Aug 9, 2022

I feel like doing something like print(&"Node: " + node.name) should probably work and give you a warning, as making a StringName only to produce a "add" to it would negate any advantage of using a StringName type explicitly.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 9, 2022

For this particular case you can just do print("Node: ", node.name) and it will work normally.

It's intended that StringNames are missing many String methods, because they are basically named pointers. Any string-based operation would require conversion to string, which is very slow.

That said, Node's name being changed to StringName seems to be causing lots of confusion and ideas for more StringName methods are floating around.

@TokisanGames
Copy link
Contributor Author

For this particular case you can just do print("Node: ", node.name) and it will work normally.

Ok thank you. But as is natural, I have hundreds of other scenarios where StringName members and return values are being used. I gave two.

It's intended that StringNames are missing many String methods, because they are basically named pointers. Any string-based operation would require conversion to string, which is very slow.

It's slower in GDScript. We now have to do explicit conversions through the language processor, when it should be implicit and done in C++. We cannot access pointers, so TBH they don't concern us as GDScript users.

What we as users experience is, in GD3: using Strings and String functions and operators works everywhere in the engine. Don't think about it. The String manual page said they were unique, and passed around by reference so were already super efficient to pass or compare.

Now for some unknown (and undesired by us) reason, half of all string functionality doesn't work because half of the engine String usage has been converted to another type. The manual page says there is an auto conversion, which doesn't work. Much of our string based code doesn't work. And there's no clear conversion path forward.

It's a guessing game as we work out hacky code to get around the limitations of the new system. It's not just Node.name. It's all over. Functions are randomly one type or the other. AnimationPlayer.animation_get_next(), and find_animation() return StringName. But Skeleton3D.get_bone_name() returns String. Node has name:StringName, but also scene_file_path:String. There's no rhyme or reason (from a users perspective). It wouldn't matter if it auto converted, but since it doesn't the incompatible types breaks our code.

That said, Node's name being changed to StringName seems to be causing lots of confusion and ideas for more StringName methods are floating around.

I think I will rename this issue to the larger scope I mentioned, as concatenation is just a symptom of the bigger problem. If Godot's string philosophy changed fine, but the new solution is poorly implemented so far, broken in some cases, and has not been communicated well so far or wrongly in some cases (manual page).

@TokisanGames TokisanGames changed the title String and StringName don't concatenate String and StringName don't autoconvert and are incompatible Aug 9, 2022
@Zireael07
Copy link
Contributor

@tinmanjuggernaut Seconding, I've had to convert lots of code and along with String(int) going mysteriously away, this required me to make a lot of changes to my code.

@MewPurPur
Copy link
Contributor

MewPurPur commented Nov 16, 2022

I am under the assumption that in general, we want users to not have to worry about StringNames and NodePaths. Instead, those should be something for experienced users to improve performance, and when less experienced users see them, they would just think "I can pass a string, it okay."

They don't compare in Arrays with in, has, or function return values

Nor are they supposed to, I'm pretty sure. You're searching if an array has something different. It doesn't work with int and float, even though those compare. This is actually super dirty as it doesn't throw an error.

There has been a long discussion on RocketChat over this, with all sorts of ideas thrown around, but so far I think the most actionable one is to only keep StringNames and NodePaths as accepted types in methods, since Strings can be autoconverted without trouble there. This would get performance benefits with no regressions.

When users get things like node.name or Node.get_groups(), however, they often don't want something immutable, so I don't think there should be any properties or any methods that return StringNames, NodePaths, or arrays of either. This is a performance regression by default, but if you're a poweruser, you can still do a StringName cast. The improvement in usability is worth it IMO.

This would repair compatibility with 3.x, but break compatibility with previous betas.

Edit: Perhaps scratch out NodePath. I don't know many places where they are used personally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants