Consolidated API to pass custom types over FFI boundary

[bluenote10]

This issue is similar to #227 and perhaps also related to @lilizoey's comment on improving error messages. So I'm not quite sure if the necessary features are already all there, and I'm just failing to connect the dots.

I'm wondering how to support custom primitive data types in function signatures. For instance:

// Example 1

#[derive(Debug, Clone, Copy, PartialEq, FromVariant, ToVariant)]
enum EventKind {
    DrawStart,
    Drag,
    DragEnd,
}

#[godot_api]
impl CustomProgressBar {
    #[signal]
    fn progress_bar_change(progress: i64, event_kind: EventKind);
}

// Example 2

#[godot_api]
impl CustomItemList {
    #[signal]
    fn item_selected(index: Option<u32>);
}

In these examples the compiler complains:

• First example: that neither GodotFfi nor EngineEnum is implemented for EventKind.
• Second example: that neither EngineEnum nor GodotNullablePtr is implemented Option<u32> / u32.

My work-around is typically to move the encoding/decoding into the user code, for instance:

#[godot_api]
impl CustomProgressBar {
    #[signal]
    fn progress_bar_change(progress: i64, event_kind: Variant);
}

But now this signature is kind of vague, and every implementator of the signal must remember how to actually decode that variant.

In both cases it would be great if I could specify a kind of encoding/decoding such that e.g. the Option<u32> encodes the None as -1 etc.

I'd assume that the FromVariant / ToVariant traits solve this problem partially, but somehow even in the case where the custom type implements these traits, it is not possible to use it directly. The error message also mentions VariantMetadata is required to be implemented as well, but according to this comment, it is probably not a trait the user should implement manually?

[Bromeon]

This all boils down to provide an official, proper API to allow custom types to be passed over the FFI boundary.

GodotFfi is not it, in my opinion. This was designed for internal use from the very beginning, is extremely low-level and would just be a recipe to shoot yourself in the foot. For users, we should provide higher-level abstractions that don't need unsafe.

---

I think ToVariant/FromVariant is a good start, but those traits and their derives have to become more versatile, and the GDScript-side representations need to follow some rules.

I'd assume that the FromVariant / ToVariant traits solve this problem partially, but somehow even in the case where the custom type implements these traits, it is not possible to use it directly.

Can you elaborate?

[bluenote10]

Can you elaborate?

I'm not quite sure what derive(FromVariant, ToVariant) actually does under the hood e.g. for a plain enum. My assumption was that it somehow already implements "encode T as something that has GodotFfi" and "decode T from something that has GodotFfi" (plus the variant type tracking). So in the case of an enum, it gets internally encoded as an integer for instance.

If this functionality is there, I would have hoped that the encoding can be leveraged directly in the binding. I.e., being able to write:

#[derive(Debug, Clone, Copy, PartialEq, FromVariant, ToVariant)]
enum SomeEnum { Foo, Bar }

#[godot_api]
impl ExampleNode {
    #[func]
    fn some_func(&self, some_enum: SomeEnum) {
        // some_enum got decoded on the binding level already
    }
}

// and if called from Rust, the encoding also happens on binding level:
example_node.bind().some_func(SomeEnum::Foo);

instead of having to do the encoding/decoding manually in both places:

#[derive(Debug, Clone, Copy, PartialEq, FromVariant, ToVariant)]
enum SomeEnum { Foo, Bar }

#[godot_api]
impl ExampleNode {
    #[func]
    fn some_func(&self, some_enum_variant: Variant) {
        // manually decode from variant
        let some_enum = SomeEnum::from_variant(some_enum_variant);
    }
}

// and if called from Rust, also manually encode to variant.
example_node.bind().some_func(SomeEnum::Foo.to_variant());

The latter is obviously much more bug prone, because the interface is essentially just exposing an "any" type wrapper, instead of a proper type.

But the more I think about it, I'm actually not sure if the former is feasible though.

[lilizoey]

So there are two calling conventions in godot. ptrcall and varcall. ptrcall uses pointers to directly pass values of the appropriate type over, whereas varcalls converts everything into a variant and passes it over as an array of variants.

To/Fromvariant could be used to make arbitrary custom functions that do varcalls. But currently we require everything used in functions to support ptrcalls. ptrcalls can currently only be made for types that implement GodotFuncMarshal as well as to/fromvariant. which is mainly the types that directly map to some godot type (GodotFfi), and some other conveniences like i8.

It may be possible to allow people to make their own types ptrcall compatible, but the easiest solution may be to just support only varcalls for custom types. We do still need to statically dispatch somehow to figure out what calling convention to use however, im a bit unsure how to do that, we wouldn't want to make all user-defined functions use varcalls if we can avoid it.

The main problem with varcalls is that they are slower than ptrcalls. By how much is uncertain, it may be negligible.

Im experimenting a bit with restructuring the traits we use here to see if we can make user-defined ptrcall compatible types possible as well as safe to implement.

My current idea is to replace GodotFuncMarshal with three traits (names up for bikeshedding if this works out)

trait FfiCompatible {
  type Via: GodotFfi;
}

trait ToFfi: FfiCompatible {
  fn to_via(self) -> Self::Via;
}

trait FromFfi: FfiCompatible {
  fn try_from_via(via: Self::Via) -> Result<Self, Error>;
}

(more stuff is needed than just this but this is the basic idea)

And then make every function implementation rely entirely on those three traits instead. If this is safe then we could expose those traits to users.

I dont think we ever should expose GodotFfi to users, at least not safely (maybe compiling against custom modules could use it? even then it'd probably be better to fork gdext). and To/FromVariant dont do so, they're just convenient ways to convert to/from Variant.

[Bromeon]

I'm still not sure if we should provide two competing APIs (variants and FFI) for users. I consider ptrcall/varcall an implementation detail of GDExtension, which is not necessarily relevant for someone building a game.

In practice, this would mean:

• Users have to learn both ways. They need to know when to use which, and why we don't just have the "faster" one in the first place. The marshalling APIs are not as simple as they seem; for example, variant conversions can come with all sorts of errors that need proper meta-info attached to be debuggable. There's quite some know-how needed because GDScript cannot represent certain types, and so on.
• To/FromVariant is more versatile: it not only allows FFI passing but also storing arbitrary data in arrays/dictionaries, passing/receiving it from engine APIs, etc. It also has built-in reflection/introspection capabilities that "raw" types do not provide. Functionally, it is thus a superset of ptrcall passing.
• For flexibility, user types might then need integration into 2 separate marshalling systems (aka 4+ traits) just for Godot itself. Add to that any third-party interfaces like serde, rkyv, Protobuf, custom savegame formats, ...
• Complex data is anyway likely to be modeled as struct, which is always passed efficiently via Gd<T>.

The only reason for user-side ptrcall is performance, and "is probably faster" is not enough to justify extra complexity in my opinion. We haven't done any measurements in that regard, and even if we see something like 10x speedup, this would only affect games that move tons of data over FFI and for some reason insist on using inefficient types (not objects or packed arrays) -- which is rather a niche. Users tend to think they always need max performance, and as such might feel pressured to choose complexity even if it doesn't benefit them.

We also need to consider that gdnative moved in the exact opposite direction: by default, ptrcalls were globally disabled in godot-rust/gdnative#973 for engine-side method calls (so not the #[func] equivalent). This affects all types, even things like integers. People seem to be fine with it, or they might have turned on the Cargo feature. But my guess is most didn't even notice 😉

[Bromeon]

Maybe there is a way to consolidate the two systems, i.e. provide ToVariant/FromVariant for free when the other marshalling is provided. This might need a bit more thought and design exploration. But we should avoid getting into serde-like genericity...

If we go that route, I would still advocate all the "front-end" API to focus on the ptrcall marshalling API. ToVariant/FromVariant would then rather be a second-class citizen, and might not even need its own derive macros and other fancy things anymore.

---

Alternatively, if we stick to ToVariant/FromVariant, the #[func] API needs to become more ergonomic, so that the library can at least try to infer which system is used. At the moment, a lot of confusion stems from the two systems interfering.

[lilizoey]

Maybe there is a way to consolidate the two systems, i.e. provide ToVariant/FromVariant for free when the other marshalling is provided. This might need a bit more thought and design exploration. But we should avoid getting into serde-like genericity...

If we go that route, I would still advocate all the "front-end" API to focus on the ptrcall marshalling API. ToVariant/FromVariant would then rather be a second-class citizen, and might not even need its own derive macros and other fancy things anymore.

I'm actually exploring that atm, it's seeming possible but it's a very invasive change so it's hard to get everything working together again.

[lilizoey]

So i got this working on this branch, https://github.com/lilizoey/gdextension/tree/feature/godot-compat

There's still a lot of things to do, improve error reporting, update to master, fix the to/fromvariant derives (well change them to be to/fromgodot derives) and just general cleanup. but here's an example of how it works:

#[derive(Debug)]
struct SomeNewtype(i64);

impl GodotCompatible for SomeNewtype {
    type Via = i64;
}

impl ToGodot for SomeNewtype {
    fn to_godot(&self) -> Self::Via {
        self.0
    }
}

impl FromGodot for SomeNewtype {
    fn try_from_godot(via: Self::Via) -> Option<Self> {
        Some(Self(via))
    }
}

#[derive(GodotClass)]
#[class(init, base = Node)]
pub struct Foo {}

#[godot_api]
impl Foo {
    #[func]
    fn foo(&self, my_new_type: SomeNewtype) {
        godot_print!("i got {my_new_type:?}");
    }
}

Now GodotFfi is an entirely internal trait, only used for types that directly and fully are able to represent godot types. Meaning:

• i64
• f64
• bool
• all the builtins
• RawGd
• Variant

This did mean I had to add a nullable version of Gd called RawGd. (which currently is kinda messily put together atm as i just kinda copied code from Gd and added/removed some null checks as appropriate)

But then all ffi-passing is defined in terms of the three traits GodotCompatible, ToGodot and FromGodot. And if you're missing one, it'll appropriately report that it's missing, and not start complaining about missing EngineEnum or VariantMetadata and so on. This should also be an entirely safe api for doing this.

i only had three failures (well two proper failures and a segfault) when i finally was able to run itest, one was a missing null check in RawGd, one was an error message changing, and one was a kinda weird edge case where i cloned an array but the array didnt expect to be cloned, but that i had already accounted for though i just didn't use it properly.

Im planning on cleaning it up and fixing the above mentioned issues if this seems like a good solution to this issue.

A new thing this enables is that we can now store anything in arrays that implements GodotCompatible. We can easily change that with another trait though if we want. Because storing anything that requires complicated/expensive conversion logic in an array is likely gonna be an anti-pattern, like String.