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 November 19, 2022 02:04
@rune-scape rune-scape marked this pull request as ready for review November 19, 2022 02:06
@rune-scape rune-scape requested review from a team as code owners November 19, 2022 02:06
@rune-scape
Copy link
Contributor Author

@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

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 November 21, 2022 08:08
@MewPurPur
Copy link
Contributor

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 November 23, 2022 02:32
@rune-scape rune-scape marked this pull request as ready for review November 23, 2022 17:46
@rune-scape rune-scape requested a review from a team as a code owner November 23, 2022 17:46
@rune-scape rune-scape force-pushed the rune-stringname-unification branch 2 times, most recently from edf69c7 to c2980b9 Compare November 26, 2022 09:58
@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
@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
@akien-mga
Copy link
Member

Thanks!

@Gastronok
Copy link
Contributor

I'm not sure if it belongs here, in a separate bug report, or in a feature request, but int lacks a StringName constructor and will give "Invalid type in 'int' constructor. Cannot convert argument 1 from StringName to String." if you try to do say int(some_node.name).

To me that feels logically related to the unification of StringName and String.

@donn-xx
Copy link

donn-xx commented May 19, 2023

Also not sure if this belongs here, but: funcs that take String like load() need something like this:

var a = ProjectSettings.get_global_class_list()
var find = a.filter(func(d): return &"ResourcePrinterPlants" in d["class"])
var c = load(str(find[0].path)).new()

It took me ages to think of using str()

@dalexeev
Copy link
Member

dalexeev commented May 19, 2023

Also not sure if this belongs here, but: funcs that take String like load() need something like this:

#77164 fixes this.

@donn-xx
Copy link

donn-xx commented May 19, 2023

Also not sure if this belongs here, but: funcs that take String like load() need something like this:

#77164 fixes this.

Had a look, but don't quickly see how.

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