-
-
Notifications
You must be signed in to change notification settings - Fork 218
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
Experimental refactoring of GodotFfi
#625
Conversation
7a4161c
to
c4c3181
Compare
@Bromeon i think some of these refactoring ideas are fairly useful. But i didn't like discuss these ideas or anything before trying them. So i'm just wondering if you think this seems useful enough to proceed with? any comments etc. I dont mind scrapping this or massively overhauling it. It was an experiment that i wasn't sure was gonna pan out, but it worked out so far so just wanna see what you think. |
API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-625 |
Thanks a lot for this! 💪
That sounds like a good change, if it helps simplify code.
But the current code seems to work without it, so why would we add a 3rd one?
I don't really like the proliferation of these low-level traits, it's already near-impossible for a new contributor to navigate along Can we not add this function without a trait? A lot of the FFI methods are generated through macros anyway, so there doesn't need to be compile-time polymorphism.
The existing functions are supposed to be read as "from sys-init" and "from sys-init, default" -- not "from sys + init", "from sys + init default" 🙂 but yes, maybe naming can be improved. I'm not sure about the Another option would be
This is a great change, long overdue. I wanted to look at this when I added
Again I'm kind of skeptical to move "conventions" (as in naming) to "type system constraints" (traits), especially if there's both pros/cons to the change. Having a field named And this change in particular adds a significant amount of boilerplate code for each type, which wasn't previously necessary: impl<T: GodotType> sys::HasOpaque for Array<T> {
type Opaque = sys::types::OpaqueArray;
unsafe fn borrow_opaque<'a>(ptr: *const Self::Opaque) -> &'a Self {
&*(ptr as *mut Self)
}
fn from_opaque(opaque: Self::Opaque) -> Self {
Self {
opaque,
_phantom: PhantomData,
}
}
fn opaque(&self) -> &Self::Opaque {
&self.opaque
}
fn opaque_mut(&mut self) -> &mut Self::Opaque {
&mut self.opaque
}
}
We had existing I also wonder if |
The main case i was thinking would be if we want to do something like:
Which may be useful in some cases, in #621 i had a case where i wanted that but im unsure if it's actually needed there. So it's probably fine to not include that here and add it in #621 if it really truly is needed, and even then maybe just adding special cased methods for it if it's only needed for one or two types.
That's fair, im not sure if it's something that is needed as a separate trair. Maybe it's possible that some trait bound can be added to the method to keep it in
Maybe by just renaming
huh, i haven't noticed that actually (the doc hidden limitation).
i really find the A minor inconsistency is also that maybe Unrelated but thinking about it,
The biggest issue is that some ffi functions that really should take const pointers, dont. like variant conversion functions. but other than that it was fairly pain free.
Yeah i think i mostly agree, it didn't turn out as useful as I'd hoped. Unless i can add a blanket impl for
Maybe we can have a trait pub trait SysPtr {
type MutPtr;
type ConstPtr;
type UninitPtr;
fn as_mut_ptr(self) -> Self::MutPtr;
fn as_const_ptr(self) -> Self::ConstPtr;
fn as_uninit_ptr(self) -> Self::UninitPtr;
} and just implement that for all the relevant pointer types? Then converting between them is fairly straightforward. though as designed this trait wouldn't guarantee that So maybe just have a literal combination of Maybe this trait should be |
So an interesting thing i now found, by turning all the godot-ffi types that use So instead of having three different cases The only negative impact is that we have some superfluous And now almost all the unsafe impl GodotFfi for StringName {
fn variant_type() -> sys::VariantType {
sys::VariantType::StringName
}
ffi_methods! { type sys::GDExtensionTypePtr; ..}
}
|
bb63357
to
537e382
Compare
The problem is that
Hm, interesting 🤔 I guess we stopped using it at some point and forgot to remove it. I thought at least some of the builtins needed it, but apparently not. |
Doing that would already make a lot of things more complicated though. And would basically require us to rewrite most things anyway. Like the reason We can also do similar to what we did for
I think it was only used for |
That code exists in a working state now, and while the macro impl is not the most beautiful, it's also not particularly complex. Plus, once But now that |
We can already do most of those things with single-field
I dont really think there's much that really could be simplified, we could change some minor syntactic stuff but most of it would still need to be there. |
I wonder why
Also, it looks like FFI declaration is slightly more complex in very few cases? E.g. for - ffi_methods! { type sys::GDExtensionTypePtr = *mut Self; .. }
+ ffi_methods! { type sys::GDExtensionTypePtr;
+ fn new_from_sys;
+ fn sys;
+ fn sys_mut;
+ fn new_with_uninit;
+ fn from_arg_ptr;
+ fn move_return_ptr;
+ }
+
+ unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self
+ where
+ Self: Sized,
+ {
+ let mut rid = Rid::Invalid;
+ init_fn(&mut rid);
+
+ rid
+ } |
Sure, i was referring to the opaque types, for these types we can already use the equivalent logic.
I think it was mostly to make sure these types had the same memory layout as the equivalent types in godot-cpp? i dont know if we really need that? but it's likely an expectation people would have.
Yes, We've had several issues before with |
Not sure -- I don't think people use godot-rust and godot-cpp simultaneously, and as long as we don't document such guarantees, it's not something "official". It's also quite involved to know the actual sizes (depending on float/double, 32/64 configuration), and should anyway only matter for extremely low-level things? That we provide them for vectors and the other builtins I mentioned makes sense. But apart from those, we should probably remove both |
Im changing it, but also apparently we are assuming that some of these types are transparent already by doing things like We're also very explicitly relying on it being possible to cast a sys-ptr to a |
Another issue: If we add extra state to refcounted types, it then becomes impossible to optimize accesses to those types by treating pointers to them as references thus avoiding refcounting. For instance: pub unsafe extern "C" fn get_virtual<T: cap::ImplementsGodotVirtual>(
_class_user_data: *mut std::ffi::c_void,
name: sys::GDExtensionConstStringNamePtr,
) -> sys::GDExtensionClassCallVirtual {
let borrowed_string = StringName::borrow_string_sys(name);
let method_name = borrowed_string.to_string();
T::__virtual_call(method_name.as_str())
} Here i avoid incrementing the stringname's refcount by merely borrowing it, by turning the pointer into a |
You have some good points!
That would be appreciated, thanks!
I think that's fine though --
True. If we do such a refactor, we'd have to remove Could you add a On another note, my |
0d10f87
to
c43eb78
Compare
Update `ffi_methods` to account for that Replace `AsUninit` with `SysPtr` Add `borrow_string_sys` to `StringName`
…hrough explicit methods that do what they want
remove reprs
c43eb78
to
4c58c1a
Compare
I started cleaning up a couple of things in |
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.
Thanks a lot!
godot-core/src/builtin/callable.rs
Outdated
ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; | ||
fn from_sys; | ||
fn new_from_sys; | ||
fn sys; | ||
fn from_sys_init; | ||
fn sys_mut; | ||
fn new_with_uninit; | ||
fn move_return_ptr; | ||
fn from_arg_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.
Maybe minor thing, but would it be possible to order those in a way so that constructors, getters and other functions are grouped respectively?
godot-core/src/builtin/callable.rs
Outdated
@@ -396,7 +387,7 @@ mod custom_callable { | |||
r_error: *mut sys::GDExtensionCallError, | |||
) { | |||
let arg_refs: &[&Variant] = | |||
Variant::unbounded_refs_from_sys(p_args, p_argument_count as usize); | |||
Variant::borrow_var_ref_slice(p_args, p_argument_count as usize); |
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.
borrow_ref_slice
should be enough, as it's no longer about the sys
pointers (where we typically use "var" to differentiate variant from type pointers).
type_: self.variant_type.sys(), | ||
name: self.property_name.string_sys(), | ||
class_name: self.class_name.string_sys(), | ||
name: sys::SysPtr::force_mut(self.property_name.string_sys()), | ||
class_name: sys::SysPtr::force_mut(self.class_name.string_sys()), | ||
hint: u32::try_from(self.hint.ord()).expect("hint.ord()"), | ||
hint_string: self.hint_string.string_sys(), | ||
hint_string: sys::SysPtr::force_mut(self.hint_string.string_sys()), | ||
usage: u32::try_from(self.usage.ord()).expect("usage.ord()"), |
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.
Would it make sense to add a string_sys_mut()
method?
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.
it already exists but it takes a &mut self
, but here we have a &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.
I see. Pity that these methods aren't const-correct in Godot.
Maybe if they add a v2 of GDExtensionPropertyInfo
, such things could be adjusted.
process_return_ptr(return_ptr) | ||
}); | ||
let ffi = <<T::Via as GodotType>::Ffi as sys::GodotFfi>::new_with_init(|return_val| { | ||
let return_ptr = sys::GodotFfi::sys_mut(return_val); |
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'd use the more common return_val.sys_mut()
for recognizability here, even if it means bringing GodotFfi
into scope.
return &[]; | ||
} | ||
|
||
// raw pointers and references have the same memory layout. |
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.
See above
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.
Just realized that these actually don't need this note in this or the next one, since we're not transmuting a pointer to a reference here. We're turning a pointer into a *const Variant
here but in the previous one we turn it into a *const &Variant
.
return &mut []; | ||
} | ||
|
||
// raw pointers and references have the same memory layout. |
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.
See above.
Btw, if this comment appears multiple times, we should maybe mention it once before all functions.
godot-ffi/src/godot_ffi.rs
Outdated
/// This will increment reference counts if the type is reference counted. If you need to avoid this then a `borrow_sys` method should | ||
/// usually be used. Which is a method that takes a sys-pointer and returns it as a `&Self` reference. This must be manually implemented for | ||
/// each relevant type as not all types can be borrowed like this. |
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.
/// This will increment reference counts if the type is reference counted. If you need to avoid this then a `borrow_sys` method should | |
/// usually be used. Which is a method that takes a sys-pointer and returns it as a `&Self` reference. This must be manually implemented for | |
/// each relevant type as not all types can be borrowed like this. | |
/// This will increment reference counts if the type is reference counted. If you need to avoid this, then a `borrow_sys` associated function should | |
/// usually be used. That function takes a sys-pointer and returns it as a `&Self` reference. This must be manually implemented for | |
/// each relevant type, as not all types can be borrowed like this. |
godot-ffi/src/godot_ffi.rs
Outdated
/// # Safety | ||
/// `init_fn` must be a function that correctly handles a (possibly-uninitialized) _type ptr_. | ||
unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self | ||
where | ||
Self: Sized, // + Default | ||
{ | ||
// SAFETY: this default implementation is potentially incorrect. | ||
// By implementing the GodotFfi trait, you acknowledge that these may need to be overridden. | ||
Self::from_sys_init(|ptr| init_fn(sys::AsUninit::force_init(ptr))) | ||
|
||
// TODO consider using this, if all the implementors support it | ||
// let mut result = Self::default(); | ||
// init_fn(result.sys_mut().as_uninit()); | ||
// result | ||
} | ||
/// While this does call `init_fn` with a `&mut Self` reference, that value may have a broken safety invariant. | ||
/// So `init_fn` must ensure that the reference passed to it is fully initialized when the function returns. | ||
#[doc(hidden)] | ||
unsafe fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> 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.
Is there an example where such an invariant is necessary?
So it's generally not possible to make this method safe?
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 believe RawGd
requires it, atm with how its safety invariant is structured. Since for RawGd
the rtti info and object pointer must remain unchanged after construction, but if you use this method you'd need to overwrite them with more appropriate values. i can check if it's possible to rework some things to make it safe though.
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.
Ah don't worry, if there's a concrete case that needs it, it's fine to keep unsafe
. Maybe add a brief comment, I was just not sure if this was leftover from refactoring.
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 this, does RawGd
even need it? like sure we set the object pointer and rtti info to be a null-object. But as long as we're careful to never let this constructed RawGd
leak to user-code before we change the object pointer/rtti info to the correct info, then it is impossible to tell that the RawGd
had this happen to it.
So i guess yeah it can be a safe function, it just means that user-code could only ever use it to construct a null-object. Which is probably fine? I mean it's #[doc(hidden)]
now anyway so it's private.
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.
Since it's #[doc(hidden)]
, we don't provide any guarantees towards the user. It's fine to assume users will never call it.
godot-ffi/src/toolbox.rs
Outdated
/// Verifies at compile time that two types `T` and `U` have the same size and alignment. | ||
#[macro_export] | ||
macro_rules! static_assert_eq_size_align { |
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.
With this, do we still need static_assert_eq_size
? Or would other checks also benefit from alignment comparison?
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 dont think we do, really what we want to check in all the cases we use static_assert_eq_size
is that the two types have compatible layouts. Which means they should all have the same size and alignment.
I'm not entirely sure what these CI errors are, they seem unrelated to my PR? |
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.
There was a problem with the nightly repo. I fixed some things, after rerun they passed.
Thanks a lot!
MSRV and other CI issues addressed in #651. |
This is not finalized because this is a refactoring that i just randomly tried today. Before i work more on it and finish it up (beyond making it compile if it doesn't compile on all targets yet) i'd want to see if this is something we want to actually do.
So basically, i had an issue while working on #621 where the names of
GodotFfi
methods (as well as the types they use) don't entirely reflect what the methods do or are intended to do (and some other minor correctness things). So a summary here:from_sys
This creates a new value of type
Self
from an ffi pointer, however this is documented to not increment any refcounts or anything. Which is weird. If it creates aSelf
it really should be a fully valid value of typeSelf
right?So instead i split this in two:
new_from_sys(ptr: ..) -> Self
, which does in fact create a new value from a sys pointer. Including incrementing refcounts and such.borrow_sys<'a>(ptr: ..) -> &'a Self
which creates a reference out of the pointer.Might consider adding a third one, which has the old behavior. I.e it constructs a
Self
without incrementing refcounts or anything, explicitly for the case where we want that behavior. It could be callednew_from_sys_weak
or something?Unfortunately,
borrow_sys
cannot be implemented forRawGd
, so i had to split it into a new traitGodotFfiBorrowable
. However it's still fairly useful in that we can now explicitly borrow a reference to types likeStringName
. It is also used to implementnew_from_sys
in some cases.This has cleaned up some ugly code, like we dont need to remember to
std::mem::forget(string_name.clone())
whenever we create one. And we can useStringName::borrow_sys(ptr)
instead ofManuallyDrop::new(StringName::from_sys(ptr))
where we used that pattern.from_sys_init
/from_sys_init_default
From the name, one would assume it takes a sys pointer and returns a
Self
. But they dont. They create a brand new value from an initialization function. So i renamed them tonew_with_uninit
andnew_with_init
.Now since
new_with_init
(from_sys_init_default
) is gonna pass along an initialized pointer, i changed it from taking a closure which takes a pointer, to taking a closureFnOnce(&mut Self)
. Which also makes this a safe function.new_with_init
doesn't have a default implementation anymore, instead theffi_methods
macro will try to implement it withDefault::default()
. Which will fail in some cases so needs to be manually implemented in those cases.Using
Const
pointersMany places just use the non-const version of pointers even if the const one is more accurate. i changed this around a bit so now:
sys()
returns a const pointersys_const()
sys_mut()
doesn't have a default impl (we cannot implement this correctly through callingsys()
)new_from_sys()
takes a const pointerThis does mean we need to convert const-pointers into their non-const form sometimes where we didnt before. But overall it actually worked fairly well in most places. And it does help with some type safety in various places, like making sure we use
sys_mut()
when it's actually possible to and more correct to do so.New traits
SysPtr
AsUninit
is replaced bySysPtr
which contains conversions for both uninit and const pointers.