godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.09k stars 21.18k forks source link

GDExtension: document when `CallableCustom` equal and less operators are invoked #81901

Open Bromeon opened 1 year ago

Bromeon commented 1 year ago

Godot version

4.2-dev (e0ee985f647273dbac6d05c46cefe3a69460fa55)

System information

Windows 10

Issue description

Continuation of a discussion from recently merged #79005. Not confirmed to be a bug, but it's unclear how the API is being used.

Custom callables in GDExtension allow several functions to be defined. I tested the following:


From the original discussion:

less_than_func is used primarily when callables are placed in a container such as an ordered map or set.

You mean in Godot's own C++ code?

Because that ordering relation isn't anyhow exposed to GDExtension, as Array is unordered and Dictionary is a hashmap...

I'm a bit surprised that equal_func isn't being called for you tho, I don't have an explanation.

Is it called for you? If yes, in which context?

I tried 3 different scenarios, none of which calls my custom equal_func:

  1. callable == callable
  2. Variant(callable) == Variant(callable)
  3. insert two same keys into a dictionary
    • hardcoded the hash_func to a constant, which is indeed invoked
    • so the hash map should resort to equality for bucket selection, but it doesn't use my function

Steps to reproduce

See above

Minimal reproduction project

N/A

maiself commented 1 year ago

less_than_func is used primarily when callables are placed in a container such as an ordered map or set.

You mean in Godot's own C++ code?

Yes

I'm a bit surprised that equal_func isn't being called for you tho, I don't have an explanation.

Is it called for you? If yes, in which context?

It's called for me during callable to signal connection / disconnection. I think equally via variant equally operator was also working, but it's been awhile and I can't actually check this very moment.

I don't know if this is relevant for your case, but the function pointers for these functions must match on both callables or they won't be called.

AThousandShips commented 1 year ago

The compare equal function is only invoked if it's not the same exact callable, if the pointers equal it returns true immediately (false for <), they must also have the exact same compare functions

Bromeon commented 1 year ago

The compare equal function is only invoked if it's not the same exact callable, if the pointers equal it returns true immediately (false for <), they must also have the exact same compare functions

This explains it! Even if not very intuitive, this makes sense. We definitely have to document this (I would keep this issue open to track that).


Some questions remain though, mainly about <. maiself mentioned:

less_than_func is used primarily when callables are placed in a container such as an ordered map or set. Yes [in Godot's own C++ code].

Also here, I'd like to clarify semantics and document them in the header:

  1. I made some observations from GDExtension. Are these all correct and intended?

    • Callable doesn't expose an operator < in the JSON.
    • Variant::evaluate() fails accordingly when comparing two callables.
    • Array::sort() with multiple callables does not invoke the user-defined operator <.
  2. Can we define precisely in which scenarios less_than_func is used? Godot internally has a get_compare_less_func() for various Callable types. Can someone with better insights explain the semantics?

  3. What are the implications if a GDExtension binding developer does not provide a less_than_func? Can anything break?

  4. Same as 3. but for equal_func?

dsnopek commented 1 year ago

3. What are the implications if a GDExtension binding developer does not provide a less_than_func? Can anything break? 4. Same as 3. but for equal_func?

The current doc comments in gdextension_interface.h already address that both less_than_func and equal_func are optional, and that if they aren't specified a default will be provided:

     * `hash_func`, `equal_func`, and `less_than_func` are optional. If not provided both `call_func` and
     * `callable_userdata` together are used as the identity of the callable for hashing and comparison purposes.

In godot-cpp, the default is fine, because the full identity of a callable is defined by the address of the call_func and callable_userdata. The binding author will need to evaluate if that's true in their case too, or if they need to dig deeper into the callable_userdata or something.

dsnopek commented 1 year ago

I did some more research on @Bromeon's other questions:

1. I made some observations from GDExtension. Are these all correct and intended?

  • Callable doesn't expose an operator < in the JSON.
  • Variant::evaluate() fails accordingly when comparing two callables.
  • Array::sort() with multiple callables does not invoke the user-defined operator <.

Yes, this appears to be correct, and it all comes back to the same thing. In variant_op.cpp, we never register an OperatorEvaluatorLess for Callable. I can only assume this is intentional - not too many variant types have registered an OperatorEvaluatorLess.

2. Can we define precisely in which scenarios less_than_func is used? Godot internally has a get_compare_less_func() for various Callable types. Can someone with better insights explain the semantics?

This one was the biggest mystery to me personally.

Digging around, I can see it used in Object::Connection::operator<():

bool Object::Connection::operator<(const Connection &p_conn) const {
    if (signal == p_conn.signal) {
        return callable < p_conn.callable;
    } else {
        return signal < p_conn.signal;
    }
}

It's also used in CustomCallableBind / CustomCallableUnbind:

  1. In CallableCustomBind::_less_func() where it does:
    if (a->callable < b->callable) {
        return true;
    } else if (b->callable < a->callable) {
        return false;
    }
  2. In CallableCustomUnbind::_less_func() where it does:
    if (a->callable < b->callable) {
        return true;
    } else if (b->callable < a->callable) {
        return false;
    }

However, those serve the same purpose as our less_than_func, so the mystery remains.

I tried commenting out both Callable::operator<() and Object::Connection::operator<() and compiling the engine to see if I'd get any errors. It turned up only one place where it's actually used:

So, I guess we have to have it because it's a required part of CallableCustom (ie CallableCustom::get_compare_less_func() is a pure virtual function), but it seems it's only currently actually used for ordering the connections when saving scenes. :-)

However, if at some point in the future any part of the engine put a Callable or Object::Connection in an ordered list, or in a normal list and sorted it, then this function would get called.

Bromeon commented 1 year ago

Wow, thanks a lot for your investigation 👍

In godot-cpp, the default is fine, because the full identity of a callable is defined by the address of the call_func and callable_userdata. The binding author will need to evaluate if that's true in their case too, or if they need to dig deeper into the callable_userdata or something.

That sounds like a good default. To clarify, this is godot-cpp and not Godot itself?

So, I guess we have to have it because it's a required part of CallableCustom (ie CallableCustom::get_compare_less_func() is a pure virtual function), but it seems it's only currently actually used for ordering the connections when saving scenes. :-)

However, if at some point in the future any part of the engine put a Callable or Object::Connection in an ordered list, or in a normal list and sorted it, then this function would get called.

Great summary! That is indeed a very niche use case at the moment. I'm probably not going to expose < to the Rust user, some arbitrary but deterministic ordering relation by the library should be enough.

For == and hash(), it seems there is more direct impact.

dsnopek commented 1 year ago

That sounds like a good default. To clarify, this is godot-cpp and not Godot itself?

Godot doesn't provide a default - CallableCustom::get_compare_equal_func() and CallableCustom::hash() are both pure virtual functions.

It's the code for GDExtension added by Mai's PR that provides the defaults:

But godot-cpp is able to use those defaults to implement callable_mp() and callable_mp_static(). I actually have plans to add support to godot-cpp for allowing developers to make their own custom callable types, and for that, it won't use the defaults.