Allow CallableCustom objects to be created from GDExtensions

[maiself]

Based off of @dsnopek's work in #78813. This exposes a few more methods from the CallableCustom interface.

I was originally going to just comment on the original patch, but ended up needing to actually test some ideas out. I don't know if there a better way of submitting a derived patch like this, please let me know if there is!

The changes here were needed for better support in converting Python callables:

hash_func and equal_func are needed as objects representing functions in Python can have the same value (compare equal) but not necessarily have the same identity (differing object pointers). Bound method instances are an example. Weak references to callables as well as weakref.WeakMethod are another case where hashing and comparing pointers is insufficient.

Without proper hashes and comparisons connecting and disconnecting Callables to Signals is unreliable.

to_string_func and the new default of "CallableCustom" greatly improve printing of values. Callables would print as just an empty string before.

callable_custom_get_userdata was added to allow getting the original object back out of a callable, useful for storing metadata or round trip extraction of callables.

Only minimal changes would be needed to godotengine/godot-cpp#1155 as the defaults should still suffice.

Thanks to @dsnopek for putting the original together! Please feel free to take over the code again.

[dsnopek]

Thanks!

In general, these changes makes sense to me. I just have a couple of questions on some minor points.

[dsnopek]

This is looking really good to me now!

@touilleMan @Bromeon Will this work for your bindings?

My first pass at implementing this (in a previous PR) was pretty C++-centric, so it'd be good to get some more perspectives on the API :-)

[dsnopek]

I think we may need to update the comparison functions in light of this recently merged PR #72346

[maiself]

I think we may need to update the comparison functions in light of this recently merged PR #72346

I'm not sure there's any relevance to our implementation of custom callables here.

Pointers we are comparing are in the very likely case pointing to static memory, and the performance issue mentioned looks to be an artifact of the container rather than the comparisons.

[dsnopek]

Ah, yes, you're right. I misinterpreted what that PR was doing. I thought it was switching away from comparing callables by their pointer addresses (which we're also doing in this PR) to comparing them by some value. But it's actually just comparing the addresses byte-by-byte, but now in reverse order. :-)

[dsnopek]

This PR needs a rebase due to conflicts with recent changes

[RadiantUwU]

Please 🙏 we need this ASAP in 4.2

[fuzzybinary]

This looks reasonable to me with a few minor nits.

A few questions about how this works in practice, though I feel like that's just me not having used Callables much yet. The Object that is passed in is never sent back to the Extension. Is this by design? Should it be sent as a parameter to call_func? Or should there be a callable_custom_get_object function?

Second, what should be passed into gdextension_callable_custom_create's r_callable parameter? I'm assuming it's a pointer to some amount of allocated memory (enough for a Callable)? Can we be clearer about how much memory that is?

Also, how do you delete the returned callable?

[fuzzybinary]

Does it makes sense to have this say <CallableCustomExtension> instead?

[maiself]

I'm not sure the distinction between CallableCustomExtension used for GDExtension and CallableCustom used by Godot makes much difference here, if more clarity is needed it would be better to provide a to_string_func with any needed details about the callable.

[fuzzybinary]

Nit: it's

[dsnopek]

@fuzzybinary:

Thanks for reviewing!

The Object that is passed in is never sent back to the Extension. Is this by design? Should it be sent as a parameter to call_func?

The object is optional and only used "informationally" by the engine. Basically, if the callable you're creating represents a method on an Object * recognized by Godot, then you should (but aren't required to) set object to that. This allows Callable.get_object() to return something nice to the developer.

If your GDExtension actually needs the Object * to make the callable work, then that should be stored as part of the callable_userdata, which is passed to call_func.

Or should there be a callable_custom_get_object function?

We don't need one, because you can call Callable.get_object() (even from within your GDExtension) and it'll return it :-)

Second, what should be passed into gdextension_callable_custom_create's r_callable parameter? I'm assuming it's a pointer to some amount of allocated memory (enough for a Callable)? Can we be clearer about how much memory that is?

Yes, it's a pointer to a buffer of 16 bytes, which is defined by extension_api.json (under "builtin_class_sizes" -> "sizes"). This isn't something defined in gdextension_interface.h, because some types have different sizes depending on the platform and/or build options used when compiling Godot.

I don't know that we need explain this in gdextension_interface.h just for this one instance: there's already a whole bunch of functions that take uninitialized Variants (as GDExtensionUninitializedVariantPtr r_dest) on the exact same principle. Although, perhaps we should document this as a whole in a dedicated PR? But I think it's out-of-scope here.

Also, how do you delete the returned callable?

The same as any Callable that you'd get from Godot, even one that was created in GDScript and passed to a method defined in GDExtension: by calling the function pointer for the destructor fetched by variant_get_ptr_destructor(GDEXTENSION_VARIANT_TYPE_CALLABLE).

[fuzzybinary]

Okay, @dsnopek I think that all makes sense.

I do think there might be a need to be clear about what that GDExtensionUninitializedTypePtr is. The only other places I really see it are in constructors where you're passing in the type. This one seems a little bit more confusing to me.

[dsnopek]

This feature is in high demand - it continues to come up on a regular basis. More testing and review would of course be great, but I've personally thought this was ready since mid-July :-)

[maiself]

The object is optional and only used "informationally" by the engine.

This is actually incorrect, tho it seems like it should be true. The discussion has reminded me about this bit of workaround code from my extension: https://github.com/maiself/godot-python-extension/blob/master/src/variant/callable.cpp#L12

Basically Godot has checks for object (I don't currently have a source location, likely in object.cpp) as a way to check if the callable is valid. This seems wrong, especially for custom callables, which may not actually have an associated object. In my extension I had to create an object that exists for no reason other than to pass those checks. I had meant to dig deeper into Godot's code and make a pull request to address this, but had forgotten until now. This seems a bit separate from this pull request, but I thought I'd mention it anyways.

[dsnopek]

@maiself:

Basically Godot has checks for object (I don't currently have a source location, likely in object.cpp) as a way to check if the callable is valid.

Hm. Looking at CallableCustomStaticMethodPointer (in callable_method_pointer.h), those don't have objects ever, and seem to work fine? That class overrides is_valid() to always return true, but we can do that with custom callables created by this PR as well (via the is_valid_func property).

[Bromeon]

Tested it with Rust and got a basic use case working! 🎉

Here is a simple test case demonstrating it. I didn't test every possible interaction, but calling and destruction seems to work. Any other potential issues would likely show up soon 😎

[maiself]

@dsnopek, I looked a bit thru the code, here are some examples where object-less callables are rejected:

• Object::emit_signalp
• Object::connect
• Object::_disconnect

I haven't looked to see if there are more cases (I merely searched object.cpp for get_object()). When I disable the workaround in my extension these functions do fail with errors:

ERROR: Cannot connect to 'test_signal' to callable '<function TestSprite._ready.<locals>.<lambda> at 0x7f61b18e58a0>': the callable object is null.
   at: connect (core/object/object.cpp:1303)

CallableCustomStaticMethodPointer is never connected to an object in Godot. There are a few cases of things like MessageQueue::get_singleton()->push_callable(callable_mp_static(...), ...); and callable_mp_static(...).call_deferred(); but not ->connect(..., callable_mp(...)); I don't see any real reason for the rejection of object-less callables.

[dsnopek]

Ah, ok, great finds! Would be good to fix in a follow up PR :-)

[maiself]

I've created a separate issue to keep track of it: #81887. Not sure when I can get a PR together, I'd want to make sure all cases are found and no new bugs introduced, so might take a little time to be thorough.

[Bromeon]

When is the less_than function used?

Operator < is not exposed for Callables in the extension_api.json, and Variant::evaluate with OPERATOR_LESS also fails according to some quick tests, even if I specify a less_than_func.

My custom equal_func function also doesn't seem to be invoked if I use a == b for 2 custom callables.

On the other hand, hash_func and to_string_func work.

[maiself]

less_than_func is used primarily when callables are placed in a container such as an ordered map or set. I don't even implement it in my extension as the default works just fine for Python. I'm a bit surprised that equal_func isn't being called for you tho, I don't have an explanation.

[Bromeon]

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

[akien-mga]

Thanks!

---

Welp it seems like I merged the PR mid conversation, I had it queued and forgot to check the latest status.

Let me know if the discussion calls for a follow-up issue or PR, or points at an actual breaking problem in this current implementation.

Edit: Reading through, I think this was all discussion of future changes needed and not blocking for this PR.

[Bromeon]

No worries, I think it's great to have an initial version merged, then I can also integrate my work so far into godot-rust's master 🙂

I continued in #81901.

[maiself]

Moved comment to #81901