-
-
Notifications
You must be signed in to change notification settings - Fork 211
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 and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create a new consolidated API to pass types across the ffi-boundary #421
Conversation
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-421 |
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<Gd> 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. |
That's already done at the moment, see e.g. |
ca9dea5
to
7f47fcc
Compare
There was a problem hiding this comment.
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.
godot-core/src/obj/raw.rs
Outdated
/// # Safety | ||
/// Does not transfer ownership and is thus unsafe. Also operates on shared ref. Either the parameter or | ||
/// the return value *must* be forgotten (since reference counts are not updated). | ||
pub(super) unsafe fn ffi_cast<U>(&self) -> Option<RawGd<U>> | ||
where | ||
U: GodotClass, | ||
{ | ||
if self.is_null() { | ||
// Null can be cast to anything. | ||
// Forgetting a null doesn't do anything, since dropping a null also does nothing. | ||
return Some(RawGd::new_null()); | ||
} | ||
|
||
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| RawGd::from_obj_sys_weak(ptr)) | ||
} |
There was a problem hiding this comment.
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<U>(&self) -> Option<Gd<U>>
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))
}
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
godot-core/src/obj/raw.rs
Outdated
"called free() on Gd<Object> which points to a RefCounted dynamic type; free() only supported for manually managed types." | ||
); | ||
|
||
assert!(!self.is_null(), "called free() on null object"); |
There was a problem hiding this comment.
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
7f47fcc
to
c8666af
Compare
There was a problem hiding this comment.
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 🙂
impl<T: GodotClass> Sealed for RawGd<T> {} | ||
impl<T: GodotClass> Sealed for Gd<T> {} |
There was a problem hiding this comment.
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 GodotType
s here?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 👍
godot-ffi/src/godot_ffi.rs
Outdated
/// Used to blanket implement various conversions over `Option<T>`. | ||
pub trait GodotNullableFfi: Sized + GodotFfi { | ||
fn flatten_option(opt: Option<Self>) -> Self; | ||
fn is_null(&self) -> bool; | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
// TODO: better error | ||
/// Performs the conversion. | ||
fn try_from_godot(via: Self::Via) -> Option<Self>; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense yeah.
There was a problem hiding this comment.
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 Result
s 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.
There was a problem hiding this comment.
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 Result
s? 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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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! 😊
godot-core/src/builtin/meta/mod.rs
Outdated
/// This trait cannot be implemented for custom user types, for that you should see [`GodotCompatible`] | ||
/// instead. |
There was a problem hiding this comment.
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.
godot-core/src/builtin/meta/mod.rs
Outdated
@@ -7,27 +7,120 @@ | |||
pub mod registration; | |||
|
|||
mod class_name; | |||
mod godot_compat; |
There was a problem hiding this comment.
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.
402569d
to
b7b2183
Compare
There was a problem hiding this comment.
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.
godot-core/src/obj/raw.rs
Outdated
/// # Safety | ||
/// Does not transfer ownership and is thus unsafe. Also operates on shared ref. Either the parameter or | ||
/// the return value *must* be forgotten (since reference counts are not updated). | ||
pub(super) unsafe fn ffi_cast<U>(&self) -> Option<RawGd<U>> | ||
where | ||
U: GodotClass, | ||
{ | ||
if self.is_null() { | ||
// Null can be cast to anything. | ||
// Forgetting a null doesn't do anything, since dropping a null also does nothing. | ||
return Some(RawGd::new_null()); | ||
} | ||
|
||
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| RawGd::from_obj_sys_weak(ptr)) | ||
} |
There was a problem hiding this comment.
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
.
godot-core/src/obj/raw.rs
Outdated
/// # Safety | ||
/// Does not transfer ownership and is thus unsafe. Also operates on shared ref. Either the parameter or | ||
/// the return value *must* be forgotten (since reference counts are not updated). | ||
pub(super) unsafe fn ffi_cast<U>(&self) -> Option<RawGd<U>> | ||
where | ||
U: GodotClass, | ||
{ | ||
if self.is_null() { | ||
// Null can be cast to anything. | ||
// Forgetting a null doesn't do anything, since dropping a null also does nothing. | ||
return Some(RawGd::new_null()); | ||
} | ||
|
||
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| RawGd::from_obj_sys_weak(ptr)) | ||
} |
There was a problem hiding this comment.
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.
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
Either Edit:
|
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 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 (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 |
That's not how it's intended to be used. If you want to accept more than one type you should use
As far as i know there is no way to make rust generate the proper calls to |
There was a problem hiding this comment.
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 turnVariant
intoRawGd
.ffi_cast
is needed foras_refcounted
andas_object
as_refcounted
is needed to manage the refcountas_object
is needed for printing (which is strictly speaking not needed inRawGd
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 intoGd
bind_mut
is used inscoped_mut
which is used inas_refcounted
andas_object
bind
could be moved intoGd
but it feels weird to only movebind
and notbind_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
.
godot-core/src/obj/raw.rs
Outdated
/// # Safety | ||
/// Does not transfer ownership and is thus unsafe. Also operates on shared ref. Either the parameter or | ||
/// the return value *must* be forgotten (since reference counts are not updated). | ||
pub(super) unsafe fn ffi_cast<U>(&self) -> Option<RawGd<U>> | ||
where | ||
U: GodotClass, | ||
{ | ||
if self.is_null() { | ||
// Null can be cast to anything. | ||
// Forgetting a null doesn't do anything, since dropping a null also does nothing. | ||
return Some(RawGd::new_null()); | ||
} | ||
|
||
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| RawGd::from_obj_sys_weak(ptr)) | ||
} |
There was a problem hiding this comment.
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
.
Okay, that's fair. Thanks for clearing this up!
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! |
2a103a0
to
2dc78d2
Compare
2dc78d2
to
3470977
Compare
- 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
3470977
to
ffe241a
Compare
We can now use
GodotCompatible
,ToGodot
, andFromGodot
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:Will keep on draft until i've cleaned up the
RawGd
andGd
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/fromVariant
.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 intoGodotFfi
.GodotNullablePtr
->GodotNullableFfi
, now has methods used to implement conversions forOption<T>
, could also be implemented forVariant
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 fromGd
and i will clean it up before making ready to merge.Limitations
Currently
ToGodot
conversions are assumed infallible, andFromGodot
only returns anOption
. In the future, we should make these both returnResult
s 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 theTo/FromGodot
andProperty
derive macros.The
To/FromGodot
macros are just a hacky rewrite of the originalTo/FromVariant
derive macros, and as such converts everything toVariant
instead of more appropriate types.closes #414