Callable not throwing any errors

[aekobear]

On gdext 0.2.4 this code properly throws errors:

  let instance: Gd<Node> = whatever;
  
  // error because function is not defined
  instance.call("_not_real", &[]); 
  // error because function def has a parameter
  instance.call("_param_required", &[]);

But when using the Callable, all failures are silent:

  let instance: Gd<Node> = whatever;

  // nothing happens
  instance.callable("_not_real").call(&[]);
  // nothing happens
  instance.callable("_param_required", &[]);

I would expect callable.call() here to throw the same errors that instance.call and gdscript's Callable().call() do. It would also be awesome to have a result on callable, though this deviates from gdscript's behavior:

let Some(callable) = instance.callable("_not_real") else {
  println!("method not found!");
  return;
};

[Bromeon]

Thanks for reporting! This has not been implemented yet:

gdext/itest/rust/src/builtin_tests/containers/callable_test.rs

Lines 207 to 209 in 0289637

	// TODO(bromeon): this causes a Rust panic, but since call() is routed to Godot, the panic is handled at the FFI boundary.
	// Can there be a way to notify the caller about failed calls like that?
	assert_eq!(callable.callv(&varray!["string"]), Variant::nil());

I agree that it would be nice to offer a try_call() method returning Result, and a call() one panicking, just like with Object. Maybe the same API can also be extended to Variant::call().

Would you be interested in looking into this?

[aekobear]

Thanks for reporting! This has not been implemented yet:
...
Would you be interested in looking into this?

I'm pretty tied up this month but I'll have more time to contribute in May and can scoop this up then if it's still open!

[Bromeon]

@aekobear still open, it might be a cool addition to the v0.3 cycle in case you're still interested! 🙂

[aekobear]

@aekobear still open, it might be a cool addition to the v0.3 cycle in case you're still interested! 🙂

Sure, I'll start digging into it today!

[aekobear]

@Bromeon I could use some help wrapping my head around the FFI bindings. What I don't understand is:

This is the binding we are calling out to:

// core/variant/variant_call.cpp
static void func_Callable_call(Variant *v, const Variant **p_args, int p_argcount, Variant &r_ret, Callable::CallError &r_error) {
  Callable *callable = VariantGetInternalPtr<Callable>::get_ptr(v);
  callable->callp(p_args, p_argcount, r_ret, r_error);
}

bind_custom(Callable, call, _VariantCall::func_Callable_call, true, Variant);

Its call signature matches various class functions where we do get an error pointer back, eg:

// godot-core/src/meta/signature.rs
class_fn(
  method_bind.0,
  object_ptr,
  variant_ptrs.as_ptr(),
  variant_ptrs.len() as i64,
  return_ptr,
  &raw mut err,
)

but Callables are using this raw_ptrcall instead which has a different signature and no err pointer:

// godot-core/src/meta/signature.rs
Self::raw_ptrcall(args, &call_ctx, |explicit_args, return_ptr| {
  builtin_fn(
    type_ptr,
    explicit_args.as_ptr(),
    return_ptr,
    explicit_args.len() as i32,
  );
})

So I guess my question is- where is this different signature coming from and why doesn't the error pointer for builtin functions make it across the FFI like it does for class functions? I tried searching gdextension_interface.cpp but I couldn't find this signature anywhere

[aekobear]

Also for what it's worth, we have some options:

1. GDScript always calls Callables via Variant.call. If we cast Callables to Variant first then call the call method with Variant.call, we get error handling through Variant. I don't like the added overhead here, but it matches GDScript at least.

2. We can manually catch invalid Callables via is_valid() and return our own error instead of propagating Godot's error. This is similar to how we handle EG PackedByteArray where we make the same checks that Godot would make. I don't like this route because we're duplicating logic which should be handled by Godot, and in this case we still need to call out to Godot for is_valid so we're not getting a speed boost for handling it natively

If for some reason we cannot propagate the error pointer from Godot, one of these two would have to be our strategy. But I really feel like it should be possible to get the error pointer so we can handle builtin methods and class methods the same way

[Bromeon]

Thanks a lot for looking into this, it's very appreciated! ❤️
Great observations as well.

Godot calling conventions

For some background, Godot has 2 calling conventions:

• ptrcall (pointer call): passes arguments as raw pointers, no error handling
• varcall, call (variant call): passes arguments marshalled as variants, with error handling

Some functions on Callable aren't manually defined but directly forwarded to the generated code. call is one of them:

gdext/godot-codegen/src/special_cases/special_cases.rs

Lines 440 to 446 in 5a4846e

	// Callable
	| ("Callable", "call")
	| ("Callable", "call_deferred")
	| ("Callable", "bind")
	| ("Callable", "get_bound_arguments")
	| ("Callable", "rpc")
	| ("Callable", "rpc_id")

Its generated code looks like this:

pub fn call(&self, varargs: &[Variant]) -> Variant {
    type CallRet = Variant;
    type CallParams = ();
    let args = ();
    unsafe {
        let method_bind = sys::builtin_method_table().fptr_by_index(607usize);
        Signature::<CallParams, CallRet>::out_builtin_ptrcall_varargs(
            method_bind,
            "Callable",
            "call",
            sys::SysPtr::force_mut(self.sys()),
            args,
            varargs,
        )
    }
}

Excursion: builtin varcalls

I was looking at ways to perform a varcall on builtins:

1. Perform a low-level variant call directly on the Callable, without conversion. This would save the variant marshalling, but would also be very involved:
   • There seems to be no such existing method on the trait, we only have Signature::out_class_varcall for classes (not builtins).
   • We would need to find a way to get a stable ID (the 607 above). There is some prior art for class method tables of some named methods.
2. We could use the raw C API. This is even more low-level, using GDExtensionPtrBuiltInMethod and then call that directly.

However, it seems that GDExtension doesn't offer a varcall API for builtins, only ptrcall -- so both 1) and 2) wouldn't work anyway 🤔 so I think there's no direct way to use error diagnostics on Callable.

Solution

As you suggested, we can convert the callable to a variant using self.to_variant() and then forward to Variant::call(). This should provide the error diagnostics and propagate panics to the caller.

Regarding performance, I can always add Callable::fast_call which doesn't do variant conversion and error checking, but in a later API iteration.

[Yarwin]

Can we ask upstream for such feature (varcall API for builtins or ptrcall that would allow us to pass/retrieve an error)? It seems like something that could/should be fixed/implemented on Godot's side.

[Bromeon]

Can we ask upstream for such feature (varcall API for builtins or ptrcall that would allow us to pass/retrieve an error)? It seems like something that could/should be fixed/implemented on Godot's side.

Is there real demand for it? I'd be hesitant to bloat GDExtension with features that barely add value over the status quo, as it increases API complexity and puts more strain on binding developers to support all possible APIs.

Already now, it's a colossal feat to support all sorts of function calls in and out of the engine: class methods (ptrcall, varcall), builtin methods, utility methods, fixed and varargs, virtual methods, script-virtual methods, builtin conversion constructors, custom callables, a set of hardcoded callbacks (to_string etc.), hardcoded builtin APIs (packed_*_array_operator_index), native structs, ... possibly in the future GDScript traits and structs...

[Yarwin]

Is there real demand for it?

I would argue that there is. To not look too far – Godot's PhysicsServer uses callables for Callbacks https://docs.godotengine.org/en/stable/classes/class_physicsserver2d.html– which might affect rapier; we also have several users who put heavy focus on using callables as go-to command objects (and to talk with gdscript and whatnot – for example recently we had some dude on discord asking about making a schema to support such task).

as it increases API complexity and puts more strain on binding developers to support all possible APIs.

That's totally valid concern – but can't we just replace GDExtensionPtrBuiltInMethod with something along the lines of GDExtensionPtrBuiltInMethod2 (https://github.com/godotengine/godot/blob/master/core/extension/gdextension_interface.h#L203)? I would argue that having different API for gdscript/inner side of engine (both Callable::callp and obj->callp have CallError &r_call_error argument: link) and GDExt already puts some strain and adds complexity 🤔

EDIT: Now I think it would probably need extra APIs for several vastly different builtin calls? Which is for sure harder to maintain 🤔 I would have to properly investigate it first

(I can try to implement it albeit I would need some guidance + gosh, that's really a helluva lot of stuff)
(EDIT2: it would cause breakage of astronomical proportions)

[aekobear]

I think it's worth asking for clarification / thoughts from the Godot team, at least. I admittedly have very little experience with C APIs, but the idea that GDExtension exposes functions where errors are swallowed seems like something that will inevitably need to be resolved one way or another.

I'd like to see if they've maybe already thought about this or have a long-term vision. It's possible they may just say that converting to variant is the way to go, given how GDScript works

[Bromeon]

OK, I was probably a bit imprecise before, but there seem to be several misconceptions with how function calls in GDExtension work. I'll try to elaborate.

1. Calling convention ptrcall works on "typed pointers". These are pointers to the actual #[repr(C)] types at hand: i64, f64, Object*, a struct with a defined layout (xyz f32), etc.

   • Ptrcalls are fast, since they operate directly on the objects.
   • Ptrcalls cannot propagate errors. They need to write some value into the returned object (of a concrete type), typically a default-constructed one.

2. Calling convention varcall ("call" in Godot) works on variants, hence the name. Arguments and return types are passed as variants, meaning there is marshalling: Rust type -> Variant -> C++ type, and the other way for return types or Godot-to-Rust calls.

   • Due to marshalling overhead, varcall is slower than ptrcall.
   • Varcalls can return NIL on failure, and additionally indicate the error via dedicated output parameter.

3. vararg means "variable number of arguments". This is not a calling convention.

   • Variable arguments are used in dynamic scenarios, when the signature isn't known at compile time. For example reflection-based calls, such as Object.call(). Custom callables also use this.
   • There can be both fixed and variable arguments in a function, e.g. Object.emit_signal(signal_name, ...).
   • Functions with varargs can be called with either ptrcall or varcall, however this only affects the subject (this pointer) and fixed parameters. Any variable arguments are always marshalled to variants, just like varcalls.
   • Encoding variable arguments as variants is necessary, as Godot cannot know individual parameter types otherwise. Variants are self-contained and encode the type as a tagged union (like a Rust enum).

4. Unless I'm overlooking something, we can easily support error reporting on builtin calls by using varcall. We need to convert the callable to a variant for this. If we worry about performance, we need to keep in mind that:

   • errors in Godot most often indicate wrong usage of an API, meaning errors that do occur are likely unexpected, i.e. they can be slow
   • we anyway pay the variant marshalling for all variable args, in both ptrcall and varcall
   • we can still write an implementation that reports errors in Debug and transparently switches to ptrcall in Release
   • if this is really still an issue, we can optimize further by creating CachedCallable which holds a warm variant ready to skip the marshalling step
   • if we still worry, let's start up a profiler

---

As far as I see it, our use case "allow error reporting" can be handled by converting to variants 🙂

Just to be clear, we can't change calling conventions in Godot without creating breaking changes of astronomic proportions; this would need a new major version if it can happen at all. So the best we can do is work with varcall and ptrcall the way they exist. Ptrcall is very fast, and dynamicness (number/types of arguments) has its price. Possibly some improvements in Godot are possible (like using structs for physics APIs rather than dictionaries), but they are specialized to certain APIs and not widely applicable.

There's also the chance that something was overlooked; for example I implemented godotengine/godot#80075 and thus avoided a ton of unnecessary copies, making ptrcalls in the engine faster across the board.