Create a new consolidated API to pass types across the ffi-boundary by lilizoey · Pull Request #421 · godot-rust/gdext (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation60 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

We can now use GodotCompatible, ToGodot, and FromGodot to pass all values across the ffi-boundary. So for instance we can now pass custom user-defined types across the ffi-boundary like this:

struct MyType { a: i64, b: f64, }

impl GodotCompatible for MyType { type Via = Dictionary; }

impl ToGodot for MyType { fn to_godot(&self) -> Dictionary { dict! { "a": self.a, "b": self.b, } } }

impl FromGodot for MyType { fn try_from_godot(d: Dictionary) -> Option { let a = d.get("a")?.try_to::().ok()?; let b = d.get("b")?.try_to::().ok()?; Ok(Self { a, b }) } }

Will keep on draft until i've cleaned up the RawGd and Gd code.

I've tried to avoid changing filenames as much as possible to avoid confusing git, some is unavoidable though. And we probably should rename some files before merging.

Added Traits

• GodotCompatible
• ToGodot
• FromGodot
• GodotFfiVariant, Defines the primitive conversions of godot types into/from Variant.

Removed Traits

• FromVariant
• ToVariant
• GodotFuncMarshal
  All of these have now been folded into From/ToGodot + GodotCompatible.

Renamed/Replaced traits

• VariantMetadata -> GodotType, variant_type has been moved into GodotFfi.
• GodotNullablePtr -> GodotNullableFfi, now has methods used to implement conversions for Option<T>, could also be implemented for Variant if we want.

RawGd

We need a primitive type to represent nullable objects, so i added RawGd to represent that. This is currently pretty messily implemented as i just copy pasted over a bunch of code from Gd and i will clean it up before making ready to merge.

Limitations

Currently ToGodot conversions are assumed infallible, and FromGodot only returns an Option. In the future, we should make these both return Results to improve error messages from gdext. This PR also likely degrades error messages in some places because of this.

Property is currently independent of this new API, this should be changed but will likely require a unification and rewrite of the To/FromGodot and Property derive macros.

The To/FromGodot macros are just a hacky rewrite of the original To/FromVariant derive macros, and as such converts everything to Variant instead of more appropriate types.

closes #414

I realize null is a variant type in Godot, but in the interest of being more idiomatic on the Rust side, would it make sense to represent nullable types as just Option (or Option in the case where it's specialized to an object)? This would be similar to how Rust handles nullable pointers on the FFI side. There might be a way to take advantage of the null pointer optimization even.

but in the interest of being more idiomatic on the Rust side, would it make sense to represent nullable types as [...] Option

That's already done at the moment, see e.g. try_load().

lilizoey marked this pull request as ready for review

October 3, 2023 00:01

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments to RawGd where i remember there being significant changes from what was before. Since git doesn't quite get that a lot of it was copied over.

Comment on lines 167 to 176

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically unchanged except null-check

unsafe fn ffi_cast(&self) -> Option<Gd> where U: GodotClass, { let class_tag = interface_fn!(classdb_get_class_tag)(U::class_name().string_sys()); let cast_object_ptr = interface_fn!(object_cast_to)(self.obj_sys(), class_tag);

    // Create weak object, as ownership will be moved and reference-counter stays the same
    sys::ptr_then(cast_object_ptr, |ptr| Gd::from_obj_sys_weak(ptr))
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, when do we need to cast between null objects?

A Gd<T> can't hold nulls, and Option<Gd<T>> would be None.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar thoughts apply to other methods doing null checks. If we don't expect those, an assert!(!null) could potentially prevent logic errors.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use casting when we turn Variant into RawGd, and so if you turn a Variant into an Option<Gd<T>> then you'd first turn the Variant into a RawGd<Object>, then cast that into RawGd<T> then turn that into Option<Gd<T>>. So if the Variant is null we'd be casting a null object.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! Can you write down this conversion chain in a comment?
It's enough to do it here in ffi_cast.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this here, otherwise unchanged

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic work again, thanks so much for this huge but necessary refactoring!

I will likely review the PR over several sessions due to its sheer size, here is my first batch 🙂

Comment on lines +91 to +92

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do both need to be GodotTypes here?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might be able to only make Gd a GodotType actually ig. I'll try that

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it isn't really possible, RawGd must implement GodotType. And unless we want people to use RawGd if they ever want to implement GodotCompatible with objects as the backing type, we'd need Gd to implement GodotType. And while i dont think people would want to use objects as the backing type often (it's likely easier to just make a GodotClass type), i dont see a particular reason to completely prevent people from doing so, and i'd rather people not use RawGd directly since it's meant to be a low-level type.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, let's leave it then 👍

Comment on lines 139 to 146

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also mention for which (kinds of) types this trait is implemented.

A user probably won't need to face this directly?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah they shouldn't. We could consider implementing this for Variant, since they have a meaningful null-state. But for now it's only implemented for RawGd, since that's the least amount of changes from the current behavior.

Comment on lines 55 to 57

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if we don't have an error yet, should we not return Result<Self, ()> for now? That would just mean changing the Err type later, rather than changing from Option to Result.

It would also come with #[must_use] etc.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense yeah.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually thinking about it, it might be better to not do that. I can add a #[must_use], but turning this into a Result<Self, ()> means that i have to change every use-case to handling Results instead usually by ignoring the Err value. Which means that when we change this later we can't rely on the compiler to tell us where we're not properly handling the error value, since we're just ignoring them everywhere.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But do we have many places that ignore errors now but shouldn't ignore once they are Results? From a surface level, that seems wrong. On the other hand, I think the error type is much less important than the fact that there is an error. In gamedev practice, Err is mostly for debugging information; I don't think people frequently dispatched on the variant conversion errors in gdnative, for example.

But if you think there are too many changes or too high risk of missing result handling, we can also do it later (maybe make TODO more descriptive in that case).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this PR doesn't contain the change to using Result initially is that it requires us to change a lot of code to make sure the Err is propagated properly and all error info is preserved throughout call chains where desired.

So instead of doing that, this PR is currently just failing with generic error messages in those cases. If we then change to Result in preparation for the future change, then we'd either need to just ignore the Err and fail with generic error messages still, or we'd need to actually make sure Err values are propagated through everything as they should. But propagating Err as we should was what i was trying to avoid doing so that the PR wouldn't get too big in the first place. And just ignoring Err and failing with generic error messages isn't really much better than just using Option. And it also means that we're more likely to miss result handling when we do make the change in the future.

I was planning on working on the error handling after this PR btw so it should hopefully not be too long a delay either way.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Makes sense to split it then to a later change, thanks for elaboration!

Either way, it's not very urgent. Having this PR is a great base! 😊

Comment on lines 103 to 104

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly a short sentence how the two differ on a semantic level could help here. If it's difficult to explain without also mentioning 12 other things, maybe give an example?

Similar to the explanation of GodotType vs GodotFfi below.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would name it godot_compatible, otherwise it sounds like a GDExtension compat module.

Bromeon added feature

Adds functionality to the library

c: core

Core components

c: ffi

Low-level components and interaction with GDExtension API

labels

Oct 4, 2023

Member

Bromeon left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next round, mostly around RawGd<T> 😎

I see that a type that represents "objects" in the GDScript sense (i.e. nullable) has some benefits, but I'm still not sure if it makes sense to move all the high-level Gd functionality, such as cast/bind/instance_id/free etc. into RawGd.

Now it looks like we're actively losing type safety and have to do null checks, which are also not free. I'm generally not too concerned about these performance things, but still it's something we don't need with the current design. The changes here would also leave Gd as mostly a delegate/facade without much own business logic.

Where do you intend to use RawGd other than in Gd, that could also benefit from these high-level functions? Because I'm thinking maybe these should stay in Gd itself, and RawGd could focus on the FFI conversions and the nullability-related logic. It would split responsibilities and also reduce the magnitude and complexity of this changeset.

Comment on lines 167 to 176

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious, when do we need to cast between null objects?

A Gd<T> can't hold nulls, and Option<Gd<T>> would be None.

Comment on lines 167 to 176

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar thoughts apply to other methods doing null checks. If we don't expect those, an assert!(!null) could potentially prevent logic errors.

Next round, mostly around RawGd<T> 😎

I see that a type that represents "objects" in the GDScript sense (i.e. nullable) has some benefits, but I'm still not sure if it makes sense to move all the high-level Gd functionality, such as cast/bind/instance_id/free etc. into RawGd.

• owned_cast is needed to turn Variant into RawGd.
• ffi_cast is needed for as_refcounted and as_object
• as_refcounted is needed to manage the refcount
• as_object is needed for printing (which is strictly speaking not needed in RawGd but can help with error messages)
• instance_id_or_none is needed in a couple of different places, but maybe we can remove it with the new deref liveness checking? i haven't tried that properly.
• free could be moved into Gd
• bind_mut is used in scoped_mut which is used in as_refcounted and as_object
• bind could be moved into Gd but it feels weird to only move bind and not bind_mut

Now it looks like we're actively losing type safety and have to do null checks, which are also not free. I'm generally not too concerned about these performance things, but still it's something we don't need with the current design. The changes here would also leave Gd as mostly a delegate/facade without much own business logic.

I dont think we have to do a lot more null-checks than before. But we could get rid of the null-checks in some places by using Option<T> and unwrap_unchecked.

Where do you intend to use RawGd other than in Gd, that could also benefit from these high-level functions? Because I'm thinking maybe these should stay in Gd itself, and RawGd could focus on the FFI conversions and the nullability-related logic. It would split responsibilities and also reduce the magnitude and complexity of this changeset.

Either RawGd must manage the refcount, which is what requires at least some of the high-level stuff. Or we need to start adding drop-guards to methods that convert Gd into RawGd. I dont like the idea of adding drop-guards, it adds more complexity and types and such, especially since i think Gd is the only type that would need them.

Edit:

free could be moved into Gd without issue.
instance_id_or_none could also be moved, but i still needed a way to check if a RawGd is valid, for purposes of the Debug impl on RawGd.

Quick design Q: if one wanted to make it possible to cast more than one possible Godot type to your Rust type (e.g. allow both int and float to convert to your custom type), and say your Rust type's ToGodot always casts to a dictionary, they'd have to implement a casting from dictionary on FromGodot (since that's their Self::Via), but also override from_variant to accept other types than just dictionary, correct?

Just asking this because it might be interesting to ponder how flexible we want this API to be (preferably in some way to avoid Variant). I wonder if it'd be possible to replace Self::Via: GodotType by something like Self::Via: GodotCompatible, which could allow , for instance, using Rust enums to signify multiple possible types that can be received (and we'd just keep calling .to_godot() / .from_godot() until we reached a primitive type, which could use some separate method in GodotCompatible, such as is_primitive(), to signal we have reached the end of the chain). That could, however, be non-viable due to the inner workings of GDExtension and Godot FFI, which could just end up imposing one to use Variant in this situation instead, so this is mostly just a thought.

(Here I'm being kinda inspired from some previous experience as a contributor to the Typst project compiler, where casts of Rust types from and to Value - equivalent to Godot's Variant - mostly involve using match to accept different value types. E.g., it might make sense to accept both an int and a float to cast to a custom "real" type, and even be able to output a third type when casting to Value / Variant.)

Quick design Q: if one wanted to make it possible to cast more than one possible Godot type to your Rust type (e.g. allow both int and float to convert to your custom type), and say your Rust type's ToGodot always casts to a dictionary, they'd have to implement a casting from dictionary on FromGodot (since that's their Self::Via), but also override from_variant to accept other types than just dictionary, correct?

That's not how it's intended to be used. If you want to accept more than one type you should use Variant as the Via. Otherwise you could get confusing type errors where a call to a rust function from gdscript may succeed or fail depending on whether you statically type your variables or not.

Just asking this because it might be interesting to ponder how flexible we want this API to be (preferably in some way to avoid Variant). I wonder if it'd be possible to replace Self::Via: GodotType by something like Self::Via: GodotCompatible, which could allow , for instance, using Rust enums to signify multiple possible types that can be received (and we'd just keep calling .to_godot() / .from_godot() until we reached a primitive type, which could use some separate method in GodotCompatible, such as is_primitive(), to signal we have reached the end of the chain). That could, however, be non-viable due to the inner workings of GDExtension and Godot FFI, which could just end up imposing one to use Variant in this situation instead, so this is mostly just a thought.

(Here I'm being kinda inspired from some previous experience as a contributor to the Typst project compiler, where casts of Rust types from and to Value - equivalent to Godot's Variant - mostly involve using match to accept different value types. E.g., it might make sense to accept both an int and a float to cast to a custom "real" type, and even be able to output a third type when casting to Value / Variant.)

As far as i know there is no way to make rust generate the proper calls to to_godot() until we reach a GodotType (as this may be arbitrarily many calls, and may not even terminate possibly generating an infinitely long call graph). so we'd basically need to add a where Via: GodotType anywhere we'd use it to interface with godot anyway. which would make To/FromGodot basically just a poor version of Into/From.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

• owned_cast is needed to turn Variant into RawGd.
• ffi_cast is needed for as_refcounted and as_object
• as_refcounted is needed to manage the refcount
• as_object is needed for printing (which is strictly speaking not needed in RawGd but can help with error messages)
• instance_id_or_none is needed in a couple of different places, but maybe we can remove it with the new deref liveness checking? i haven't tried that properly.
• free could be moved into Gd
• bind_mut is used in scoped_mut which is used in as_refcounted and as_object
• bind could be moved into Gd but it feels weird to only move bind and not bind_mut

Thanks for this great list! It looks like as_refcounted blocks everything else from being moved one level up. We're somewhat facing the OOP problem of "everything in the base class", just with composition. Gd then doesn't have much purpose beyond fulfilling some traits and delegating functionality. But it's fine, better than exposing the raw one directly 🙂

Maybe you could move up free and if possible, display/debug? Although I don't know if the latter would then make error reporting harder. They should also stay together. Either way, this can also be changed again later, so it's not crucial.

Can you make RawGd #[doc(hidden)]? Users should not need to know about it.
Also rename GodotCompatible -> GodotConvert.

Comment on lines 167 to 176

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense! Can you write down this conversion chain in a comment?
It's enough to do it here in ffi_cast.

That's not how it's intended to be used. If you want to accept more than one type you should use Variant as the Via. Otherwise you could get confusing type errors where a call to a rust function from gdscript may succeed or fail depending on whether you statically type your variables or not.

Okay, that's fair. Thanks for clearing this up!

As far as i know there is no way to make rust generate the proper calls to to_godot() until we reach a GodotType (as this may be arbitrarily many calls, and may not even terminate possibly generating an infinitely long call graph). so we'd basically need to add a where Via: GodotType anywhere we'd use it to interface with godot anyway. which would make To/FromGodot basically just a poor version of Into/From.

It's possible, but I agree it'd probably be very hacky, so it's better to just not do this haha. Thanks for the answer!
I believe this design should work just fine then, with those considerations. 👍

• Rename VariantMetadata -> GodotMetadata
• Add GodotCompatible, ToGodot, FromGodot
• Add RawGd
• Change everything so that To/FromGodot is the canonical way of passing values over the ffi-boundary
• Change GodotMetadata to sealed GodotType
• Implement GodotType for various types
• Add GodotFfiVariant
• Add tests for creating newtype wrappers around godot types
• Fix to/from variant derives

This was referenced

Oct 14, 2023

Labels

c: core

Core components

c: ffi

Low-level components and interaction with GDExtension API

feature

Adds functionality to the library