Fix some DEFVALs to use the right type

[raulsntos]

Change some default parameter values to use the right type, this affects how they are exposed in extension_api.json as well as the generated C# bindings.

• For StringNames, DEFVAL(StringName()) will be written as &"" as opposed to "" (which is used for String).
• For Variant, DEFVAL(Variant()) will be written as null. DEFVAL(Variant::NIL) is incorrect since that's an enum and will result in 0 as the written value. This change probably breaks behavior compatibility so I'm adding the label.

[akien-mga]

For the remaining two extension validation errors, you need to add compatibility methods so that the old hashes are preserved:

Validate extension JSON: Error: Hash changed for 'classes/AnimationPlayer/methods/play', from 846788DD to DC6A3489. This means that the function has changed and no compatibility function was provided.
Validate extension JSON: Error: Hash changed for 'classes/AnimationPlayer/methods/play_backwards', from A6228DE1 to E7E6D578. This means that the function has changed and no compatibility function was provided.

[raulsntos]

I find it odd that it's complaining about a hash change only in those methods though, I only changed the default value of a StringName parameter like in all the other methods.

[AThousandShips]

Could be because all other methods have a register hash in core/extension/gdextension_compat_hashes.cpp unsure but it's a pattern

[akien-mga]

I find it odd that it's complaining about a hash change only in those methods though, I only changed the default value of a StringName parameter like in all the other methods.

Yeah that's weird. Maybe the check aborts too early with the error and doesn't list the other missing hashes. If so fixing those two might unlock the next batch of errors? CC @RedworkDE

Edit: Or what AThousandShips wrote.

[akien-mga]

Could be because all other methods have a register hash in core/extension/gdextension_compat_hashes.cpp unsure but it's a pattern

I think this may actually be a significant issue with the compat hashes approach, as we may not do proper validation that the new hashes do match the methods we currently provide, and thus we might be missing errors.

The hashes for all methods that changed default argument values here are probably wrong in that file and would need to be fixed, or those compat hashes would be pointing at non-existing hashes.

And thus all methods modified here possibly need compat methods registered anyway, even if the checks are not complaining.

CC @dsnopek

[dsnopek]

I just posted PR https://github.com/godotengine/godot/pull/84973 which aims to expose issues where the mapped hash from core/extension/gdextension_compat_hashes.cpp is no longer valid. I didn't test it with the changes from this PR (I only did an artificial test where I added an invalid hash on purpose) - it'd be great to see what it turns up here.

[akien-mga]

Would be good to rebase this PR on #84973 so we can confirm whether that's the problem and if it will highlight it properly.

[akien-mga]

Nothing urgent, but would be good to revisit this sooner than later for the 4.3 cycle.

[AThousandShips]

@dsnopek the compat hashes are all out of date, how to update them/add them?

[akien-mga]

@dsnopek the compat hashes are all out of date, how to update them/add them?

I don't know if David had a handy script to do this, but I suppose the manual way would be to compile this PR, generate the extension API, and copy paste the new hashes from that JSON in place of the ones in gdextension_compat_hashes.cpp (provided they are indeed still the same method / compatible).

[dsnopek]

While I used a script to generate them originally, I don't have a script to update them.

How many hashes in gdextension_compat_hashes.cpp does this affect?

The mapping in gdextension_compat_hashes.cpp was added for an exceptional situation (we fixed a bug in the way the hashes are calculated) and so they really shouldn't need to be updated except in exceptional circumstances - normally, we can just add compatibility methods. This PR could be an exceptional circumstance that qualifies.

Although, looking back at this now: why can't we just add normal compatibility methods for this?

In the cases where the only thing that changes is the DEFVAL() in the ClassDB::bind_method() (as opposed to an actual signature change to the method), we probably don't even need to make a new method, we might be able to just call ClassDB::bind_compatibility_method() using the same function pointer but with the old DEFVAL()'s.

[akien-mga]

Thanks!