GDExtension: Use ObjectID when creating custom callable

[dsnopek]

I have very mixed feelings about this change, but I thought I'd propose it and get some feedback.

As described in this comment, we could make the godot-cpp API mimick the Godot API a little better (without an extra unnecessary hash map lookup) if GDExtensionCallableCustomInfo took an ObjectID rather than an Object *.

However, I do think the current API taking an Object * makes the most sense when just looking at the gdextension_interface.h API in isolation. In order to pass in an ObjectID we need to make an extra call into the engine, whereas we're about to call into the engine anyway to pass in the GDExtensionCallableCustomInfo, so why not let the engine lookup the ObjectID on its side?

So, I'm just really not sure. Please let me know what you think!

UPDATE: I think I've come around to support the change in this PR. First of all, I think the most common case is just to not have an object on a custom callable. But in the case where there is an object, the GDExtension will probably want to hold on to the ObjectID (rather than Object *) anyway, in order to check it for validity. So, since it will probably have that data already, it's really not any extra work to provide it when creating the custom callable. Also, it doesn't seem like anyone opposes the change, and in the comments there has been feedback from maintainers of Rust, Lua and Python bindings.

[Trey2k]

If overall it takes more work to use an ObjectID I say its fine to let the apis differ from each other. My comment was under the assumption that this was just type semantics. I don't think being 1:1 with the base engine API is worth worse performance.

[dsnopek]

I don't think being 1:1 with the base engine API is worth worse performance.

Well, it depends on what data you have already, and if it's the common case to have that data or not.

For example, if you have an Object *, getting the ObjectID would be an extra call into the engine. But what if you need the ObjectID for something else anyway? For example, if we mimick the default implementation of is_valid() (which we also discussed on the other issue) then we'd need the ObjectID anyway. And if that's the common case, then passing the ObjectID doesn't cost anything more.

It'd be great to have the perspectives of some other language bindings.

[Trey2k]

Well, it depends on what data you have already, and if it's the common case to have that data or not.

Atleast for my bindings, I do already have the object pointer its self. under module builds I call obj->get_instance_id(). Which like you mentioned is an extra call. However looking over it I could easily change this code so it only stored the ObjectID. All I use the object for myself is for the get_object method.

[Bromeon]

Maybe to bring another argument to the discussion: ObjectID can be checked for validity, pointers cannot.

So if I have a Callable in an extension that points to an object, and that object is destroyed, then the pointer will become dangling. Subsequent operations on the Callable would then trigger UB.

How does this work in GDScript by the way? Are callables checking their object's validity?

[maiself]

Made a comment in the other PR, godotengine/godot-cpp#1280 (comment)

Basically, I don't have strong feelings either way, but changing may be more in line with the rest of the API and it's general usage (tho even core does weird things here). I think if we go this way it would be best to do this now rather than later.

Also, I think there is a typedef for object ids that should be used rather than int.

[dsnopek]

Thanks for the feedback, Everyone!

Personally, I think I'm starting to lean towards the API change in this PR.

@Bromeon:

Maybe to bring another argument to the discussion: ObjectID can be checked for validity, pointers cannot.

Right, and internally Godot is storing an ObjectID - we're passing an Object * but it just immediately turns it into ObjectID. :-)

But if the binding is going to have its own object/struct representing a Callable, it may need to store some reference to the object, and due to the reason you give above, they'd probably actually store an ObjectID. This points more towards the idea that the binding may already have that data.

@maiself:

I think if we go this way it would be best to do this now rather than later.

I agree. This API is new, so if we can sneak a change in now before 4.2, that'd be great. But if we release with the current API, I don't know if it's worth all the backcompat trouble (ie making the duplicate struct and function) to change it in 4.3+.

Also, I think there is a typedef for object ids that should be used rather than int.

Ah, good call! I've update this PR to use GDObjectInstanceID. There's a couple other places in gdextension_interface.h that should probably be changed to use it as well.

Anyway, I think I'm going to takes this PR out of draft. But if anyone objects to this change, please let me know!

[raulsntos]

Maybe to bring another argument to the discussion: ObjectID can be checked for validity, pointers cannot.

Isn't ObjectID just an unsigned 64-bit integer? How do you check for validity? The is_valid method just checks that the integer is not 0, we can do the same with pointers but that doesn't mean they point to a valid instance.

But if the binding is going to have its own object/struct representing a Callable, it may need to store some reference to the object, and due to the reason you give above, they'd probably actually store an ObjectID. This points more towards the idea that the binding may already have that data.

In the current C# bindings (which don't use the GDExtension APIs) we store a reference to the Object (basically a Object *). We can retrieve the Object instance from the C# delegate when creating a Callable from a lambda, but retrieving the ObjectID would require an extra interop call which we'd prefer to avoid.

When the Callable is created from a C# delegate, we technically don't need the Object instance because we store the C# delegate and use it to invoke the Callable. So I was wondering, what is the Object used for in custom Callables? Would it matter what the Object is set to?

To expand on that. Is it safe for the Object to be null even if the method is not static? If the custom Callable implements its own call behavior then I can call a method on whatever Object I want, and if I can retrieve that information from userdata, does it matter what I set the Object to?

What I'm trying to get to is: if setting the Object is not necessary and this PR is merged, I think in C# we would never set the Object and avoid the extra interop call. But I'm not sure if there are side-effects to doing that.

Note that I haven't used the GDExtension APIs much yet. I'm commenting from the perspective of the current C# scripting implementation, and because asking these questions will help me when implementing the C# GDExtension bindings in the future.

[Bromeon]

Isn't ObjectID just an unsigned 64-bit integer? How do you check for validity? The is_valid method just checks that the integer is not 0, we can do the same with pointers but that doesn't mean they point to a valid instance.

With this function -- it returns null if the ID doesn't point to a valid object.
One of the greatest GDExtension functions that allows the Rust bindings to achieve a high level of safety 😊

godot/core/extension/gdextension_interface.h

Lines 2247 to 2257 in c21c270

	/**
	* @name object_get_instance_from_id
	* @since 4.1
	*
	* Gets an Object by its instance ID.
	*
	* @param p_instance_id The instance ID.
	*
	* @return A pointer to the Object.
	*/
	typedef GDExtensionObjectPtr (*GDExtensionInterfaceObjectGetInstanceFromId)(GDObjectInstanceID p_instance_id);

---

To expand on that. Is it safe for the Object to be null even if the method is not static? If the custom Callable implements its own call behavior then I can call a method on whatever Object I want, and if I can retrieve that information from userdata, does it matter what I set the Object to?

No, the object doesn't need to be set for custom callables. If it's set, it will be returned by Callable::get_object(). It probably has some other subtle implications inside Godot, of which I'm not aware.

I implemented custom callables without objects -- sometimes there is not even an object in the Godot sense available (e.g. for closures/lambda expressions).

[dsnopek]

To expand on that. Is it safe for the Object to be null even if the method is not static? If the custom Callable implements its own call behavior then I can call a method on whatever Object I want, and if I can retrieve that information from userdata, does it matter what I set the Object to?

No, the object doesn't need to be set for custom callables. If it's set, it will be returned by Callable::get_object(). It probably has some other subtle implications inside Godot, of which I'm not aware.

Yeah, in an ideal world, the object associated with the Callable is just informational - you can get it from Callable::get_object(), but it otherwise doesn't mean anything. In fact, for custom callables, I'd probably expect that not having any object at all is the more common case -- you could just use standard callables for methods on objects registered with Godot.

In practice, though, there's a number of places in Godot that are checking the object directly to determine if the callable is valid or not. We fixed a whole bunch of them in PR #82695 - we'll probably fix more during the Godot 4.3 development cycle.

But for the sake of discussion around this API, I think we should consider the object used for a custom callable to be optional and informational, because that is what we're aiming for it to be.

[akien-mga]

Thanks!