Fix CallableCustom comparison function handling

[kisg]

In the GDExtension API the CallableCustom interface defines the hash_func, equal_func, and less_than_func methods as optional.

However, in godot-cpp a custom handler was always provided to Godot. This custom provider then failed if both Callables had specified nullptr as their equal or less_than_equal comparison function.

This commit fixes this, and only sets the custom handler, when a custom handler is actually specified by the developer.

Also, if the hash() function returns 0, then no hash function is provided to Godot, so it falls back to its default hash function.

This issue was originally discovered in Godot 4.3, so it might be a good idea to cherry-pick it to that branch.

Developed by Migeran

[dsnopek]

Thanks! However, I don't personally like this change.

In Godot itself, child classes of CallableCustom are required to implement hash(), get_compare_equal_func() and get_compare_less_func() and returning 0 or nullptr is not an option. So, if we merged this PR, it would mean that extending CallableCustom in godot-cpp wouldn't be compatible with extending CallableCustom in a Godot module, which goes against one of the core design goals of godot-cpp.

I understand that the GDExtension interface allows those callbacks to be optional, however, that doesn't mean that godot-cpp needs to allow that. godot-cpp attempts to adapt the GDExtension interface in a way that is in accordance with its design goals - other GDExtension bindings may approach this differently.

[kisg]

My main concern is that this makes Godot harder to use: different languages may have different implementation requirements for custom Callables.

Also, I don't understand why it should be required for engine users to implement their own comparison and hash operations, when there is already a suitable default implementation inside the engine.

What is even worse, you can specify nullptr for both the equal and less operators in custom Callables implemented in godot-cpp, and things will work, as long as you don't try to connect 2 of such callables to the same signal. (This is how we encountered this problem in some special case during beta testing - it went completely unnoticed during development.)

In my opinion, there are multiple options to have a consistent API both in-engine and out:

• Change the in-engine implementation of Callable to allow using the same default implementation for these methods that are already implemented for the GDExtension API and merge this PR for godot-cpp. I prefer this solution, because implementing proper hashing and comparison functions may be non-trivial, especially for novice developers.

• Make these functions mandatory also in the GDExtension API, and remove the default implementations.

• Change both the in-engine and the godot-cpp API to make it clear that the specification of these functions is mandatory (e.g. the methods shall return function references instead of raw pointers). If this is not possible, introduce checks that will fail immediately, if a CallableCustom implementation returns null for these functions.

What do you think?

[dsnopek]

• Change the in-engine implementation of Callable to allow using the same default implementation for these methods that are already implemented for the GDExtension API and merge this PR for godot-cpp.

Changing the API of CustomCallable within Godot first and then changing godot-cpp to match is an option!

Please feel free to make a Godot issue or PR to start a discussion with the core team.

• Make these functions mandatory also in the GDExtension API, and remove the default implementations.
• Change both the in-engine and the godot-cpp API to make it clear that the specification of these functions is mandatory (e.g. the methods shall return function references instead of raw pointers).

However, I don't personally see any reason to change the GDExtension interface.

I think having those callbacks optional within the GDExtension interface was the right decision, because it potentially makes it a little easier to create new GDExtension bindings. The GDExtension interface != godot-cpp, and it's only godot-cpp that has the goal of providing the same API as Godot modules (with the same limitations), so I don't think we should limit the GDExtension interface to only the things that godot-cpp wants to support. This isn't the only thing we've added to the GDExtension interface to make life easier for other bindings that godot-cpp chooses not to use.