Skip to content
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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Mar 29, 2024
Merged

Experimental refactoring of GodotFfi #625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bd76361
Rename `GodotFfi` methods
lilizoey Mar 9, 2024
112ccbb
Fix some api-version related issues
lilizoey Mar 9, 2024
bab1a29
Some cleanup
lilizoey Mar 9, 2024
3c4a892
More cleanup
lilizoey Mar 10, 2024
5bd0698
Remove the `ptr_from_sys` methods on `Variant` so people have to go t…
lilizoey Mar 10, 2024
c8b7ff0
Fix clippy stuff
lilizoey Mar 10, 2024
4c58c1a
Tweaks
lilizoey Mar 13, 2024
57cc60d
Final touchups.
lilizoey Mar 16, 2024
600d3e9
Fixes from review
lilizoey Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions godot-codegen/src/generator/builtins.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ fn make_builtin_class(class: &BuiltinClass, ctx: &mut Context) -> GeneratedBuilt
pub fn from_outer(outer: &#outer_class) -> Self {
Self {
_outer_lifetime: std::marker::PhantomData,
sys_ptr: outer.sys(),
sys_ptr: sys::SysPtr::force_mut(outer.sys()),
}
}
#special_constructors
Expand Down Expand Up @@ -154,7 +154,7 @@ fn make_special_builtin_methods(class_name: &TyName, _ctx: &Context) -> TokenStr
{
Self {
_outer_lifetime: std::marker::PhantomData,
sys_ptr: outer.sys(),
sys_ptr: sys::SysPtr::force_mut(outer.sys()),
}
}
}
Expand Down
99 changes: 39 additions & 60 deletions godot-core/src/builtin/array.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ use super::meta::{
// for `T: GodotType` because `drop()` requires `sys_mut()`, which is on the `GodotFfi`
// trait, whose `from_sys_init()` requires `Default`, which is only implemented for `T:
// GodotType`. Whew. This could be fixed by splitting up `GodotFfi` if desired.
#[repr(C)]
pub struct Array<T: GodotType> {
// Safety Invariant: The type of all values in `opaque` matches the type `T`.
opaque: sys::types::OpaqueArray,
Expand Down Expand Up @@ -225,7 +224,7 @@ impl<T: GodotType> Array<T> {
/// # Panics
///
/// If `index` is out of bounds.
fn ptr(&self, index: usize) -> *const Variant {
fn ptr(&self, index: usize) -> sys::GDExtensionConstVariantPtr {
let ptr = self.ptr_or_null(index);
assert!(
!ptr.is_null(),
Expand All @@ -236,24 +235,23 @@ impl<T: GodotType> Array<T> {
}

/// Returns a pointer to the element at the given index, or null if out of bounds.
fn ptr_or_null(&self, index: usize) -> *const Variant {
fn ptr_or_null(&self, index: usize) -> sys::GDExtensionConstVariantPtr {
// SAFETY: array_operator_index_const returns null for invalid indexes.
let variant_ptr = unsafe {
let index = to_i64(index);
interface_fn!(array_operator_index_const)(self.sys(), index)
};

// Signature is wrong in GDExtension, semantically this is a const ptr
let variant_ptr = sys::to_const_ptr(variant_ptr);
Variant::ptr_from_sys(variant_ptr)
sys::SysPtr::as_const(variant_ptr)
}

/// Returns a mutable pointer to the element at the given index.
///
/// # Panics
///
/// If `index` is out of bounds.
fn ptr_mut(&self, index: usize) -> *mut Variant {
fn ptr_mut(&mut self, index: usize) -> sys::GDExtensionVariantPtr {
let ptr = self.ptr_mut_or_null(index);
assert!(
!ptr.is_null(),
Expand All @@ -264,14 +262,12 @@ impl<T: GodotType> Array<T> {
}

/// Returns a pointer to the element at the given index, or null if out of bounds.
fn ptr_mut_or_null(&self, index: usize) -> *mut Variant {
fn ptr_mut_or_null(&mut self, index: usize) -> sys::GDExtensionVariantPtr {
// SAFETY: array_operator_index returns null for invalid indexes.
let variant_ptr = unsafe {
unsafe {
let index = to_i64(index);
interface_fn!(array_operator_index)(self.sys(), index)
};

Variant::ptr_from_sys_mut(variant_ptr)
interface_fn!(array_operator_index)(self.sys_mut(), index)
}
}

/// # Safety
Expand Down Expand Up @@ -456,7 +452,7 @@ impl<T: GodotType> Array<T> {
// SAFETY: The array is a newly created empty untyped array.
unsafe {
interface_fn!(array_set_typed)(
self.sys(),
self.sys_mut(),
type_info.variant_type.sys(),
type_info.class_name.string_sys(),
script.var_sys(),
Expand Down Expand Up @@ -490,8 +486,8 @@ impl<T: GodotType + FromGodot> Array<T> {
// Panics on out-of-bounds
let ptr = self.ptr(index);

// SAFETY: `ptr()` just verified that the index is not out of bounds.
let variant = unsafe { &*ptr };
// SAFETY: `ptr` is a live pointer to a variant since `ptr.is_null()` just verified that the index is not out of bounds.
let variant = unsafe { Variant::borrow_var_sys(ptr) };
T::from_variant(variant)
}

Expand All @@ -501,8 +497,8 @@ impl<T: GodotType + FromGodot> Array<T> {
if ptr.is_null() {
None
} else {
// SAFETY: `ptr.is_null()` just verified that the index is not out of bounds.
let variant = unsafe { &*ptr };
// SAFETY: `ptr` is a live pointer to a variant since `ptr.is_null()` just verified that the index is not out of bounds.
let variant = unsafe { Variant::borrow_var_sys(ptr) };
Some(T::from_variant(variant))
}
}
Expand Down Expand Up @@ -663,7 +659,7 @@ impl<T: GodotType + ToGodot> Array<T> {

// SAFETY: `ptr_mut` just checked that the index is not out of bounds.
unsafe {
*ptr_mut = value.to_variant();
value.to_variant().move_into_var_ptr(ptr_mut);
}
}

Expand Down Expand Up @@ -744,24 +740,7 @@ unsafe impl<T: GodotType> GodotFfi for Array<T> {
sys::VariantType::Array
}

ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
fn from_sys;
fn sys;
fn from_sys_init;
fn move_return_ptr;
}

unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
let mut result = Self::default();
init_fn(result.sys_mut());
result
}

unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
let array = Self::from_sys(ptr);
std::mem::forget(array.clone());
array
}
ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque; .. }
}

impl<T: GodotType> GodotConvert for Array<T> {
Expand Down Expand Up @@ -825,9 +804,9 @@ impl<T: GodotType> Clone for Array<T> {
fn clone(&self) -> Self {
// SAFETY: `self` is a valid array, since we have a reference that keeps it alive.
let array = unsafe {
Self::from_sys_init(|self_ptr| {
Self::new_with_uninit(|self_ptr| {
let ctor = ::godot_ffi::builtin_fn!(array_construct_copy);
let args = [self.sys_const()];
let args = [self.sys()];
ctor(self_ptr, args.as_ptr());
})
};
Expand Down Expand Up @@ -891,7 +870,7 @@ impl<T: GodotType> Default for Array<T> {
#[inline]
fn default() -> Self {
let mut array = unsafe {
Self::from_sys_init(|self_ptr| {
Self::new_with_uninit(|self_ptr| {
let ctor = sys::builtin_fn!(array_construct_default);
ctor(self_ptr, std::ptr::null_mut())
})
Expand Down Expand Up @@ -937,9 +916,9 @@ impl<T: GodotType> GodotType for Array<T> {
impl<T: GodotType> GodotFfiVariant for Array<T> {
fn ffi_to_variant(&self) -> Variant {
unsafe {
Variant::from_var_sys_init(|variant_ptr| {
Variant::new_with_var_uninit(|variant_ptr| {
let array_to_variant = sys::builtin_fn!(array_to_variant);
array_to_variant(variant_ptr, self.sys());
array_to_variant(variant_ptr, sys::SysPtr::force_mut(self.sys()));
})
}
}
Expand All @@ -954,9 +933,9 @@ impl<T: GodotType> GodotFfiVariant for Array<T> {
}

let array = unsafe {
sys::from_sys_init_or_init_default::<Self>(|self_ptr| {
sys::new_with_uninit_or_init::<Self>(|self_ptr| {
let array_from_variant = sys::builtin_fn!(array_from_variant);
array_from_variant(self_ptr, variant.var_sys());
array_from_variant(self_ptr, sys::SysPtr::force_mut(variant.var_sys()));
})
};

Expand All @@ -977,7 +956,7 @@ impl<T: GodotType + ToGodot, const N: usize> From<&[T; N]> for Array<T> {
/// Creates a `Array` from the given slice.
impl<T: GodotType + ToGodot> From<&[T]> for Array<T> {
fn from(slice: &[T]) -> Self {
let array = Self::new();
let mut array = Self::new();
let len = slice.len();
if len == 0 {
return array;
Expand All @@ -987,14 +966,14 @@ impl<T: GodotType + ToGodot> From<&[T]> for Array<T> {
// the nulls with values of type `T`.
unsafe { array.as_inner_mut() }.resize(to_i64(len));

let ptr = array.ptr_mut_or_null(0);
for (i, element) in slice.iter().enumerate() {
// SAFETY: The array contains exactly `len` elements, stored contiguously in memory.
// Also, the pointer is non-null, as we checked for emptiness above.
unsafe {
*ptr.offset(to_isize(i)) = element.to_variant();
}
// SAFETY: `array` has `len` elements since we just resized it, and they are all valid `Variant`s. Additionally, since
// the array was created in this function, and we do not access the array while this slice exists, the slice has unique
// access to the elements.
let elements = unsafe { Variant::borrow_slice_mut(array.ptr_mut(0), len) };
for (element, array_slot) in slice.iter().zip(elements.iter_mut()) {
*array_slot = element.to_variant();
}

array
}
}
Expand Down Expand Up @@ -1027,14 +1006,13 @@ impl<T: GodotType + FromGodot> From<&Array<T>> for Vec<T> {
fn from(array: &Array<T>) -> Vec<T> {
let len = array.len();
let mut vec = Vec::with_capacity(len);
let ptr = array.ptr(0);
for offset in 0..to_isize(len) {
// SAFETY: Arrays are stored contiguously in memory, so we can use pointer arithmetic
// instead of going through `array_operator_index_const` for every index.
let variant = unsafe { &*ptr.offset(offset) };
let element = T::from_variant(variant);
vec.push(element);
}

// SAFETY: Unless `experimental-threads` is enabled, then we cannot have concurrent access to this array.
// And since we dont concurrently access the array in this function, we can create a slice to its contents.
let elements = unsafe { Variant::borrow_slice(array.ptr(0), len) };

vec.extend(elements.iter().map(T::from_variant));

vec
}
}
Expand All @@ -1057,7 +1035,8 @@ impl<'a, T: GodotType + FromGodot> Iterator for Iter<'a, T> {
let element_ptr = self.array.ptr_or_null(idx);

// SAFETY: We just checked that the index is not out of bounds, so the pointer won't be null.
let variant = unsafe { &*element_ptr };
// We immediately convert this to the right element, so barring `experimental-threads` the pointer wont be invalidated in time.
let variant = unsafe { Variant::borrow_var_sys(element_ptr) };
let element = T::from_variant(variant);
Some(element)
} else {
Expand Down
39 changes: 14 additions & 25 deletions godot-core/src/builtin/callable.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ use sys::{ffi_methods, GodotFfi};
/// Currently it is impossible to use `bind` and `unbind` in GDExtension, see [godot-cpp#802].
///
/// [godot-cpp#802]: https://github.com/godotengine/godot-cpp/issues/802
#[repr(C, align(8))]
pub struct Callable {
opaque: sys::types::OpaqueCallable,
}
Expand All @@ -48,10 +47,10 @@ impl Callable {
// upcast not needed
let method = method_name.into();
unsafe {
sys::from_sys_init_or_init_default::<Self>(|self_ptr| {
sys::new_with_uninit_or_init::<Self>(|self_ptr| {
let ctor = sys::builtin_fn!(callable_from_object_method);
let raw = object.to_ffi();
let args = [raw.as_arg_ptr(), method.sys_const()];
let args = [raw.as_arg_ptr(), method.sys()];
ctor(self_ptr, args.as_ptr());
})
}
Expand Down Expand Up @@ -140,7 +139,7 @@ impl Callable {
fn from_custom_info(mut info: sys::GDExtensionCallableCustomInfo) -> Callable {
// SAFETY: callable_custom_create() is a valid way of creating callables.
unsafe {
Callable::from_sys_init(|type_ptr| {
Callable::new_with_uninit(|type_ptr| {
sys::interface_fn!(callable_custom_create)(type_ptr, ptr::addr_of_mut!(info))
})
}
Expand All @@ -151,7 +150,7 @@ impl Callable {
/// _Godot equivalent: `Callable()`_
pub fn invalid() -> Self {
unsafe {
Self::from_sys_init(|self_ptr| {
Self::new_with_uninit(|self_ptr| {
let ctor = sys::builtin_fn!(callable_construct_default);
ctor(self_ptr, ptr::null_mut())
})
Expand Down Expand Up @@ -270,10 +269,6 @@ impl Callable {
pub fn as_inner(&self) -> inner::InnerCallable {
inner::InnerCallable::from_outer(self)
}

fn inc_ref(&self) {
std::mem::forget(self.clone())
}
}

impl_builtin_traits! {
Expand All @@ -300,21 +295,17 @@ unsafe impl GodotFfi for Callable {
}

ffi_methods! { type sys::GDExtensionTypePtr = *mut Opaque;
fn from_sys;
fn new_from_sys;
fn new_with_uninit;
fn from_arg_ptr;
fn sys;
fn from_sys_init;
fn sys_mut;
fn move_return_ptr;
}

unsafe fn from_arg_ptr(ptr: sys::GDExtensionTypePtr, _call_type: sys::PtrcallType) -> Self {
let callable = Self::from_sys(ptr);
callable.inc_ref();
callable
}

unsafe fn from_sys_init_default(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Self {
fn new_with_init(init_fn: impl FnOnce(&mut Self)) -> Self {
let mut result = Self::invalid();
init_fn(result.sys_mut());
init_fn(&mut result);
result
}
}
Expand Down Expand Up @@ -395,8 +386,7 @@ mod custom_callable {
r_return: sys::GDExtensionVariantPtr,
r_error: *mut sys::GDExtensionCallError,
) {
let arg_refs: &[&Variant] =
Variant::unbounded_refs_from_sys(p_args, p_argument_count as usize);
let arg_refs: &[&Variant] = Variant::borrow_ref_slice(p_args, p_argument_count as usize);

let c: &mut C = CallableUserdata::inner_from_raw(callable_userdata);

Expand All @@ -413,8 +403,7 @@ mod custom_callable {
) where
F: FnMut(&[&Variant]) -> Result<Variant, ()>,
{
let arg_refs: &[&Variant] =
Variant::unbounded_refs_from_sys(p_args, p_argument_count as usize);
let arg_refs: &[&Variant] = Variant::borrow_ref_slice(p_args, p_argument_count as usize);

let w: &mut FnWrapper<F> = CallableUserdata::inner_from_raw(callable_userdata);

Expand Down Expand Up @@ -454,7 +443,7 @@ mod custom_callable {
let c: &T = CallableUserdata::inner_from_raw(callable_userdata);
let s = crate::builtin::GString::from(c.to_string());

s.move_string_ptr(r_out);
s.move_into_string_ptr(r_out);
*r_is_valid = true as sys::GDExtensionBool;
}

Expand All @@ -465,7 +454,7 @@ mod custom_callable {
) {
let w: &mut FnWrapper<F> = CallableUserdata::inner_from_raw(callable_userdata);

w.name.clone().move_string_ptr(r_out);
w.name.clone().move_into_string_ptr(r_out);
*r_is_valid = true as sys::GDExtensionBool;
}
}
Loading
Loading