StringName: Fix empty hash

[rune-scape]

fixes #96553
fixes #77894

fixes the underlying issue that an empty String hashes different than an empty StringName
and the default animation library is an empty string
so when the String key is converted to a StringName, it hashes differently and misses the entry in the hashmap

possibly solves other unknown bugs related to the new StringName Dictionaries

[akien-mga]

This seems to make the C# glue generation fail for some reason: https://github.com/godotengine/godot/actions/runs/10713374687/job/29705303641?pr=96586

CC @raulsntos

[AThousandShips]

I think it breaks the general method hashes for compatibility it's just that the C# check runs before the API check, it probably breaks the hash for methods with StringName() default arguments

[dsnopek]

I think it breaks the general method hashes for compatibility it's just that the C# check runs before the API check, it probably breaks the hash for methods with StringName() default arguments

That sounds likely.

It'd be great to see how many method hashes this changes. The last time we made a change to hashing, we ended up making gdextension_compat_hashes.cpp to avoid adding a ton of compatibility methods, so adding to that is an option, although, if the number is really small doing compatibility methods might be better since that's the normal way to do it (whereas the other was an exceptional case).

[raulsntos]

I think you are both correct. The C# bindings generator seems to be getting confused about the method hashes and generating invalid C# that doesn't compile. For example, for the Object::tr method, this is what's in the GDExtension API dump:

"name": "tr",
"is_const": true,
"is_vararg": false,
"is_static": false,
"is_virtual": false,
"hash": 1195764410,
"hash_compatibility": [
    1195764410,
    2475554935
],

As you can see, the current hash matches one of the compatibility hashes, which is why the C# bindings generator gets confused and tries to generate the method twice.

For your convenience, I'll upload the GDExtension API dump generated from before and after the commit in this PR:

• extension_api.old.json (from commit b6223c0)
• extension_api.new.json (from commit 94631ef)

Also, I wonder if the hash for an empty String should match the hash for an empty StringName since they are different types. I would not necessarily expect the hashes to match. And the linked issue seems to be closed already, so is this change still needed?

[rune-scape]

dict[StringName()] (and also dict.get(StringName()) before #70096 was merged) will use a StringName hash to lookup a String key and not find the equivalent value so this discrepancy affects (in a small way) at least Dictionary for all of 4.0 (and i seemed to have assumed it in e79be6c, oops). it at least seems intuitive that empty String and StringName hash the same given how similar the usage and APIs are and how every other hash is the same

it seems possible to automate detecting empty StringName default parameters and tweaking the compat hashes and such, but im not familiar with how the system works or what those things mean

66 hash changes

[dsnopek]

66 hash changes

That's probably enough that these should be added to gdextension_compatibility_hashes.cpp.

It's fairly simple to do, although quite tedious. Here's an example mapping for a class:

	mappings.insert("AESContext", {
		{ "start", 3167574919, 3122411423 },
	});

So, you give the name of the function, the old hash (before the hashing change) and the new hash.

The only tricky part is if the function is already listed there, you'll need to update the older mapping too. That's because the mappings don't "chain together", like, you can't map from an old hash to another old hash, because Godot won't recursively look them up, you always need to map all the way to a hash that really exists in ClassDB (although, that does include hashes for normal compatibility methods, which is why we don't need to constantly update this file).

[dsnopek]

@raulsntos:

Also, I wonder if the hash for an empty String should match the hash for an empty StringName since they are different types. I would not necessarily expect the hashes to match.

This is a good question!

However, I think in the case of default arguments, it's probably harmless for them to match. So long as the hashing of the argument types is distinct (which I think it still is), we should be good.

That said, I'm only thinking about GDExtension compatibility, I don't know if this could have an effect on other things in the engine where we'd want Strings and StringNames to hash differently?

As you can see, the current hash matches one of the compatibility hashes, which is why the C# bindings generator gets confused and tries to generate the method twice.

I think Object::tr() and Object::tr_n() is a special case. In PR #84906, you fixed these previously having a default of StringName() when the argument was really String and added compatibility methods. With the changes here, we should probably remove those compatibility methods (then in gdextension_compatibility_hashes.cpp add mappings that go in the other direction, if necessary - I'm not sure about that).

[rune-scape]

i had to add an unexpected extra hash for AnimationPlayer::play that wasn't there before, but other than that ive carefully added and swapped all the necessary hashes and it seems happy :)

and removed compatibility methods for methods previously having a default arg of String() when the param type was StringName

default parameters of Callable() were also affected btw (because of the hash of empty method)

[dsnopek]

From the perspective of GDExtension compatibility, this looks good to me! I didn't evaluate its potential impact on anything else.

[raulsntos]

I found no issues regarding C# compatibility either.

[akien-mga]

Thanks!