Add compatibility methods for RenderingDevice BarrierMask

[pkdawson]

PR #79911 changed the values of BarrierMask, which breaks GDExtension compatibility for any method with a default parameter value of BARRIER_MASK_ALL_BARRIERS. I added compatibility methods for all of those.

I named everything with _79911, let me know if I should change that to something else.

[clayjohn]

We discussed #79911 quite heavily at the time and we all agreed that it was worth making the change without compatibility binds as the state of the BarrierMask is such that no one could be using it effectively from GDExtension yet anyway.

Is there are new reason to add the compatibility functions that we wouldn't have considered before? For example, are you aware of a project that needs these changes?

[pkdawson]

I have a GDExtension which calls draw_list_end (with the default barrier). I'd like to be able to compile it for 4.1 and have forward compatibility with 4.2, but without a compatibility method it breaks.

I suspect literally nobody is using custom barriers, but I did the whole conversion thing for the sake of completeness.

[BastiaanOlij]

I guess this is one way forward with it but it does feel like a lot of work with very little payoff. If you're using the default (ALL BARRIERS), is there something stopping you from just sending 0x7FFF (so the new value), this will still work with the old code as its a mask and the extra set bits will just be ignored.

[pkdawson]

The binding just breaks completely with godot-cpp. The method hash is different because the DEFVAL changed, I think.

ERROR: Method 'RenderingDevice.draw_list_end' has changed and no compatibility fallback has been provided. Please open an issue.
   at: gdextension_classdb_get_method_bind (core/extension/gdextension_interface.cpp:1147)
ERROR: Method bind was not found. Likely the engine method changed to an incompatible version.
   at: godot::RenderingDevice::draw_list_end (C:\git\imgui-godot\gdext\build\_deps\godot-cpp-build\gen\src\classes\rendering_device.cpp:690)

[BastiaanOlij]

The binding just breaks completely with godot-cpp. The method hash is different because the DEFVAL changed, I think.

AHH off course, I forgot about that :(

[pkdawson]

I noticed that some of these compatibility methods aren't actually required, which lead me to find a serious bug: MethodBind::get_hash() does not correctly hash default arguments unless all the arguments have defaults.

See the annotations at the bottom here, which only lists three methods:
https://github.com/godotengine/godot/pull/81356/files

I'm gonna file a separate issue for that. It's definitely a bug, and fixing it would break compatibility for a large fraction of the GDExtension interface. If we want to fix it and accept a compatibility break in 4.2, this PR would be unnecessary.

[pkdawson]

See #81386

[pkdawson]

Ready for review now that #81521 is merged. This PR is still required to maintain GDExtension compatibility.

I've trimmed it down to only the three methods where the hash actually changed.

[dsnopek]

Hm. It's still getting the "hash changed" error. I wonder if it's because adding the compatibility methods uses the new hashing algorithm, rather than the old broken one that was used when the methods were originally registered? If that's the case, you may need to add some more entries to gdextension_compat_hashes.cpp from PR #81521. I wonder if there's a better way to handle this so we don't end up having to keep addressing it?

[pkdawson]

@dsnopek The workflow diagnostic says "The following validation errors no longer occur", so I read that as a good thing :)

[dsnopek]

Hm, maybe I'm misreading it, but I was looking here:

https://github.com/godotengine/godot/actions/runs/6278800101/job/17053293875?pr=81356#step:17:28

The tests are passing, which I guess is a good thing. :-) But I'm not sure why I still see that in the console output of the build?

[dsnopek]

I don't know why this doesn't cause CI to fail, but I'm still seeing this in the logs for the "Linux / Editor w/ Mono" job:

Am I misunderstanding something? Or, is this being printed in error? Or, is there an issue with the CI?

[pkdawson]

If I run validate_extension_api.sh locally (with GITHUB_OUTPUT=1), I get output like this:

ERROR: New API lacks base array: enums
   at: compare_dict_array (core/extension/extension_api_dump.cpp:1135)
::warning file=misc/extension_api_validation//4.1-stable.expected,title=The following validation errors no longer occur (compared to 4.1-stable): ::Validate extension JSON: Error: Hash changed for 'classes/RenderingDevice/methods/barrier', from 0FE50041 to DD9E8DAB. This means that the function has changed and no compatibility function was provided.%0AValidate extension JSON: Error: Hash changed for 'classes/RenderingDevice/methods/compute_list_end', from 19365687 to E9B4FA8E. This means that the function has changed and no compatibility function was provided.%0AValidate extension JSON: Error: Hash changed for 'classes/RenderingDevice/methods/draw_list_end', from 19365687 to E9B4FA8E. This means that the function has changed and no compatibility function was provided.%0A

I think the only issue is that GitHub is not displaying the title of the annotation in the workflow log, so it's confusing.

So those printed errors are really just the diff compared to this file:

https://github.com/godotengine/godot/blob/master/misc/extension_api_validation/4.1-stable.expected

[dsnopek]

Ah, ok, I think I'm finally understanding what that message is! It's saying we've got exceptions in 4.1-stable.expected that are no longer needed because those errors aren't being printed. My brain just couldn't help seeing those lines as the errors themselves. :-)

In that case, I think this means we should remove those lines from 4.1-stable.expected since we now have compatibility methods fixing those errors.

[dsnopek]

Thanks, this looks good to me!

[YuriSizov]

Thanks!