The Wayback Machine - https://web.archive.org/web/20221211074754/https://github.com/godotengine/godot/pull/68747
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

GDScript: Unify StringName and String #68747

Merged

Conversation

rune-scape
Copy link
Contributor

@rune-scape rune-scape commented Nov 16, 2022

this change aims to fix #64171 by letting String and StringName be used nearly interchangeably in GDScript

registers all string methods to StringName in variant_call.cpp
-- So String methods can be used on StringNames ('&"stringname".has("str")' and vice versa)

fills in the gaps between String and StringName in variant_op.cpp
-- So '"str" in &"stringname"' works and among others

Array will silently coerce Strings and StringNames into each other when typed in any method that takes a variant input
-- So Array acts like they are almost the same type, but silently converts if inserted or anything into a typed Array
-- fixes #63965, all 4 tests return true

other functions in untyped Array will also compare Strings and StringNames
-- fixes #68918

Dictionary now compares String and StringName keys
-- which fixes #62957

lastly, the analyzer will do the same
-- So it doesn't tell you you can't assign Array[String] to Array[StringName]
and the compiler will make an exception in match statements for String and StringName, allowing comparisons
-- which fixes #60145

also added a check for StringName in a couple places (regex.cpp and scene_rpc_interface.cpp)

Notes and things to discuss:

  • Array ==, <, >, etc. still do not compare Strings and StringNames as equal, not sure if this is desired?
  • Dictionary ==, <, >, still DO compare String and StringName keys as equal, but only because StringNames are converted to Strings on insertion, does that make it different from Array?
  • Strings and StringNames still cannot be compared using <, >, etc. in GDScript, should this be allowed?
  • Variant could benefit from an is_string() method..
@MewPurPur
Copy link
Contributor

MewPurPur commented Nov 18, 2022

I've found no functionality regression.

As one would expect, has() and in are slower (in the ballpark of 2x as slow), insofar as arrays are concerned (and not only Array[StringName] or Array[String].) To get these results, I compared the Linux Mono editor artifact with beta 5, running lots of loops on GDScript and timing it with the Time singleton.

@rune-scape rune-scape force-pushed the rune-stringname-unification branch 2 times, most recently from a5b8389 to e7c97d6 Compare Nov 19, 2022
@rune-scape rune-scape marked this pull request as ready for review Nov 19, 2022
@rune-scape rune-scape requested review from a team as code owners Nov 19, 2022
@rune-scape rune-scape force-pushed the rune-stringname-unification branch from e7c97d6 to adf976f Compare Nov 19, 2022
@rune-scape
Copy link
Contributor Author

rune-scape commented Nov 19, 2022

@MewPurPur that performance diff shouldn't be the case, although i did push a new version since you tested
i think maybe the artifacts aren't production builds?
i got similar performance running this test on a production build vs master:
test_unify.zip

@rune-scape
Copy link
Contributor Author

rune-scape commented Nov 20, 2022

In order for the last check to pass, godotengine/godot-cpp#930 needs to be merged to solve ambiguous overload errors

@rune-scape rune-scape marked this pull request as draft Nov 21, 2022
@MewPurPur
Copy link
Contributor

MewPurPur commented Nov 21, 2022

Fait critique, the benchmarks need something to compare to.

@rune-scape rune-scape force-pushed the rune-stringname-unification branch 3 times, most recently from 4994dcd to 0c4b053 Compare Nov 23, 2022
@rune-scape rune-scape marked this pull request as ready for review Nov 23, 2022
@rune-scape rune-scape requested a review from a team as a code owner Nov 23, 2022
doc/classes/StringName.xml Outdated Show resolved Hide resolved
@rune-scape rune-scape force-pushed the rune-stringname-unification branch 2 times, most recently from edf69c7 to c2980b9 Compare Nov 26, 2022
@Begah
Copy link
Contributor

Begah commented Nov 27, 2022

I tested this pr and it seems to 'fix' the match statement, in gd3 this worked

match node.get_name():
    "test1":
         pass
     _:
         pass

And this PR makes this code work again ( Need to do StringName("test1") for the match to work, or String(node.get_name()) without this PR)

core/variant/dictionary.cpp Outdated Show resolved Hide resolved
core/variant/variant_op.cpp Outdated Show resolved Hide resolved
@rune-scape rune-scape force-pushed the rune-stringname-unification branch from c2980b9 to b54ffc2 Compare Nov 29, 2022
reduz
reduz approved these changes Dec 9, 2022
@KoBeWi
Copy link
Member

KoBeWi commented Dec 9, 2022

I didn't test, but looking at the code it also fixes #68834

@akien-mga akien-mga merged commit 907298d into godotengine:master Dec 9, 2022
13 checks passed
@akien-mga
Copy link
Member

akien-mga commented Dec 9, 2022

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment