Merge pull request #1310 from godot-rust/perf/togodot-by-ref

Restore `ToGodot` pass-by-ref for objects

Author: Bromeon

godot-core/src/meta/args/as_arg.rs

@@ -9,8 +9,8 @@ use std::ffi::CStr;
 
 use crate::builtin::{GString, NodePath, StringName, Variant};
 use crate::meta::sealed::Sealed;
-use crate::meta::traits::GodotFfiVariant;
-use crate::meta::{CowArg, GodotType, ToGodot};
+use crate::meta::traits::{GodotFfiVariant, GodotNullableFfi};
+use crate::meta::{CowArg, GodotType, ObjectArg, ToGodot};
 use crate::obj::{bounds, Bounds, DynGd, Gd, GodotClass, Inherits};
 
 /// Implicit conversions for arguments passed to Godot APIs.
@@ -423,7 +423,9 @@ impl AsArg<NodePath> for &String {
 /// See [`ToGodot::Pass`].
 pub trait ArgPassing: Sealed {
     /// Return type: `T` or `&'r T`.
-    type Output<'r, T: 'r>;
+    type Output<'r, T: 'r>
+    where
+        Self: 'r;
 
     /// FFI argument type: `T::Ffi` or `T::ToFfi<'f>`.
     #[doc(hidden)]
@@ -515,7 +517,90 @@ impl ArgPassing for ByRef {
         T::Via: GodotType,
     {
         // Use by-ref conversion if possible, avoiding unnecessary clones when passing to FFI.
-        value.to_godot().to_ffi()
+        GodotType::to_ffi(value.to_godot())
+    }
+}
+
+/// Pass arguments to Godot by object pointer (for objects only).
+///
+/// Currently distinct from [`ByRef`] to not interfere with the blanket impl for `&T` for all `ByRef` types. Semantics are largely the same.
+///
+/// See [`ToGodot::Pass`].
+pub enum ByObject {}
+impl Sealed for ByObject {}
+impl ArgPassing for ByObject {
+    type Output<'r, T: 'r> = &'r T;
+
+    type FfiOutput<'f, T>
+        = ObjectArg
+    where
+        T: GodotType + 'f;
+
+    fn ref_to_owned_via<T>(value: &T) -> T::Via
+    where
+        T: ToGodot<Pass = Self>,
+        T::Via: Clone,
+    {
+        // For ByObject types, do like ByRef: clone the reference to get owned value.
+        value.to_godot().clone()
+    }
+
+    fn ref_to_ffi<T>(value: &T) -> ObjectArg
+    where
+        T: ToGodot<Pass = Self>,
+        T::Via: GodotType,
+    {
+        let obj_ref: &T::Via = value.to_godot(); // implements GodotType.
+        unsafe { obj_ref.as_object_arg() }
+    }
+}
+
+/// Pass optional arguments by returning `Option<&T::Via>`, allowing delegation to underlying type's strategy.
+///
+/// This enables `Option<T>` to benefit from the underlying type's efficient passing without cloning. [`ByRef`] doesn't support this because it
+/// would transform `Option<T>` to `&Option<T>`; however, we need `Option<&T>` instead.
+///
+/// See [`ToGodot::Pass`].
+pub enum ByOption<Via> {
+    // Uses `Via` generic type to work around the near-impossibility of Output<'r, T> pointing to a metafunction that transforms Option<T> to
+    // Option<&'r T>. Such a metafunction cannot be implemented via trait (overlapping impls cause coherence issues), and we would need to
+    // pollute also the other `By*` types by anything. Using a generic parameter on the trait rather than the associated type avoids that.
+    _Phantom(std::marker::PhantomData<Via>),
+}
+impl<Via> Sealed for ByOption<Via> {}
+impl<Via> ArgPassing for ByOption<Via>
+where
+    Via: GodotType,
+    for<'f> Via::ToFfi<'f>: GodotNullableFfi,
+{
+    type Output<'r, T: 'r>
+        = Option<&'r Via>
+    where
+        Self: 'r;
+
+    type FfiOutput<'f, T>
+        = <Via as GodotType>::ToFfi<'f>
+    where
+        T: GodotType + 'f;
+
+    // value:  &Option<U>
+    // return: T::Via = Option<U::Via>
+    fn ref_to_owned_via<T>(value: &T) -> T::Via
+    where
+        T: ToGodot<Pass = Self>,
+        T::Via: Clone,
+    {
+        value.to_godot_owned()
+    }
+
+    fn ref_to_ffi<T>(value: &T) -> Self::FfiOutput<'_, T::Via>
+    where
+        T: ToGodot<Pass = Self>,
+        T::Via: GodotType,
+    {
+        // Reuse pattern from impl GodotType for Option<T>:
+        // Convert Option<&Via> to Option<Via::ToFfi> and then flatten to Via::ToFfi with null handling.
+        GodotNullableFfi::flatten_option(value.to_godot().map(|via_ref| via_ref.to_ffi()))
     }
 }
 

godot-core/src/meta/args/mod.rs

@@ -13,7 +13,9 @@ mod ref_arg;
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Public APIs
 
-pub use as_arg::{owned_into_arg, ref_to_arg, ArgPassing, AsArg, ByRef, ByValue, ToArg};
+pub use as_arg::{
+    owned_into_arg, ref_to_arg, ArgPassing, AsArg, ByObject, ByOption, ByRef, ByValue, ToArg,
+};
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Internal APIs
 

godot-core/src/meta/args/object_arg.rs

@@ -9,7 +9,10 @@ use std::ptr;
 
 use godot_ffi::{ExtVariantType, GodotFfi, GodotNullableFfi, PtrcallType};
 
-use crate::obj::{Gd, GodotClass, Inherits, RawGd};
+use crate::builtin::Variant;
+use crate::meta::error::ConvertError;
+use crate::meta::traits::GodotFfiVariant;
+use crate::obj::{Gd, GodotClass};
 use crate::{obj, sys};
 
 /// View for object arguments passed to the Godot engine. Never owning; must be null or backed by `Gd<T>`.
@@ -34,29 +37,13 @@ impl ObjectArg {
         }
     }
 
-    /// Creates `ObjectArg` from a `RawGd`.
+    /// Creates `ObjectArg` from an `Option<Gd>`.
     ///
     /// # Safety
-    /// The referenced `RawGd` must remain valid for the lifetime of this `ObjectArg`.
-    pub unsafe fn from_raw_gd<T: GodotClass>(obj: &RawGd<T>) -> Self {
-        // Runtime check is necessary, to ensure that object is still alive and has correct runtime type.
-        if !obj.is_null() {
-            obj.check_rtti("from_raw_gd");
-        }
-
-        Self {
-            object_ptr: obj.obj_sys(),
-        }
-    }
-
-    /// Creates `ObjectArg` from `Option<&Gd<U>>`, handling upcast to target type `T`.
-    pub fn from_option_gd_ref<T, U>(opt: Option<&Gd<U>>) -> Self
-    where
-        T: GodotClass,
-        U: GodotClass + Inherits<T>,
-    {
-        match opt {
-            Some(gd) => unsafe { Self::from_gd(gd) },
+    /// The referenced `Gd`, if not `None`, must remain valid for the lifetime of this `ObjectArg`.
+    pub unsafe fn from_option_gd<T: GodotClass>(obj: Option<&Gd<T>>) -> Self {
+        match obj {
+            Some(gd) => Self::from_gd(gd),
             None => Self::null(),
         }
     }
@@ -72,11 +59,6 @@ impl ObjectArg {
     pub fn is_null(&self) -> bool {
         self.object_ptr.is_null()
     }
-
-    /// Returns the raw object pointer
-    pub fn raw_ptr(&self) -> sys::GDExtensionObjectPtr {
-        self.object_ptr
-    }
 }
 
 // #[derive(Clone)] doesn't seem to get bounds right.
@@ -130,6 +112,16 @@ unsafe impl GodotFfi for ObjectArg {
     }
 }
 
+impl GodotFfiVariant for ObjectArg {
+    fn ffi_to_variant(&self) -> Variant {
+        obj::object_ffi_to_variant(self)
+    }
+
+    fn ffi_from_variant(_variant: &Variant) -> Result<Self, ConvertError> {
+        unreachable!("ObjectArg should only be passed *to* Godot, not *from*.")
+    }
+}
+
 impl GodotNullableFfi for ObjectArg {
     fn null() -> Self {
         Self::null()

godot-core/src/meta/godot_convert/impls.rs

@@ -8,6 +8,7 @@
 use godot_ffi as sys;
 
 use crate::builtin::{Array, Variant};
+use crate::meta;
 use crate::meta::error::{ConvertError, ErrorKind, FromFfiError, FromVariantError};
 use crate::meta::{
     ArrayElement, ClassName, FromGodot, GodotConvert, GodotNullableFfi, GodotType,
@@ -83,41 +84,49 @@ where
     fn godot_type_name() -> String {
         T::godot_type_name()
     }
+
+    // Only relevant for object types T.
+    unsafe fn as_object_arg(&self) -> meta::ObjectArg {
+        match self {
+            Some(inner) => unsafe { inner.as_object_arg() },
+            None => meta::ObjectArg::null(),
+        }
+    }
 }
 
-impl<T: GodotConvert> GodotConvert for Option<T>
+impl<T> GodotConvert for Option<T>
 where
+    T: GodotConvert,
     Option<T::Via>: GodotType,
 {
     type Via = Option<T::Via>;
 }
 
-impl<T: ToGodot> ToGodot for Option<T>
+impl<T> ToGodot for Option<T>
 where
-    Option<T::Via>: GodotType,
+    // Currently limited to holding objects -> needed to establish to_godot() relation T::to_godot() = Option<&T::Via>.
+    T: ToGodot<Pass = meta::ByObject>,
+    // Extra Clone bound for to_godot_owned(); might be extracted in the future.
+    T::Via: Clone,
+    // T::Via must be a Godot nullable type (to support the None case).
     for<'f> T::Via: GodotType<
         // Associated types need to be nullable.
         Ffi: GodotNullableFfi,
         ToFfi<'f>: GodotNullableFfi,
     >,
-    T::Via: Clone,
+    // Previously used bound, not needed right now but don't remove: Option<T::Via>: GodotType,
 {
-    // Potential optimization: use underlying T::Pass instead of ByValue.
-    // Problem is that ByRef requires to_godot() -> &Self::Via, which is &Option<T::Via>. We would however need Option<&T::Via>.
-    // Might need a third ArgPassing impl, or different design.
-    type Pass = crate::meta::ByValue;
+    // Basically ByRef, but allows Option<T> -> Option<&T::Via> conversion.
+    type Pass = meta::ByOption<T::Via>;
 
-    fn to_godot(&self) -> Option<T::Via> {
-        self.as_ref().map(T::to_godot_owned)
+    fn to_godot(&self) -> Option<&T::Via> {
+        self.as_ref().map(T::to_godot)
     }
 
-    fn to_godot_owned(&self) -> Self::Via
+    fn to_godot_owned(&self) -> Option<T::Via>
     where
         Self::Via: Clone,
     {
-        // Default implementation calls underlying T::to_godot().clone(), which may be wrong.
-        // Some to_godot_owned() calls are specialized/overridden, we need to honor that.
-
         self.as_ref().map(T::to_godot_owned)
     }
 
@@ -248,7 +257,7 @@ macro_rules! impl_godot_scalar {
         }
 
         impl ToGodot for $T {
-            type Pass = crate::meta::ByValue;
+            type Pass = meta::ByValue;
 
             fn to_godot(&self) -> Self::Via {
                *self
@@ -264,10 +273,10 @@ macro_rules! impl_godot_scalar {
 }
 
 // `GodotType` for these three is implemented in `godot-core/src/builtin/variant/impls.rs`.
-crate::meta::impl_godot_as_self!(bool: ByValue);
-crate::meta::impl_godot_as_self!(i64: ByValue);
-crate::meta::impl_godot_as_self!(f64: ByValue);
-crate::meta::impl_godot_as_self!((): ByValue);
+meta::impl_godot_as_self!(bool: ByValue);
+meta::impl_godot_as_self!(i64: ByValue);
+meta::impl_godot_as_self!(f64: ByValue);
+meta::impl_godot_as_self!((): ByValue);
 
 // Also implements ArrayElement.
 impl_godot_scalar!(
@@ -334,7 +343,7 @@ impl GodotConvert for u64 {
 }
 
 impl ToGodot for u64 {
-    type Pass = crate::meta::ByValue;
+    type Pass = meta::ByValue;
 
     fn to_godot(&self) -> Self::Via {
         *self
@@ -374,7 +383,7 @@ impl<T: ArrayElement> GodotConvert for Vec<T> {
 }
 
 impl<T: ArrayElement> ToGodot for Vec<T> {
-    type Pass = crate::meta::ByValue;
+    type Pass = meta::ByValue;
 
     fn to_godot(&self) -> Self::Via {
         Array::from(self.as_slice())
@@ -392,7 +401,7 @@ impl<T: ArrayElement, const LEN: usize> GodotConvert for [T; LEN] {
 }
 
 impl<T: ArrayElement, const LEN: usize> ToGodot for [T; LEN] {
-    type Pass = crate::meta::ByValue;
+    type Pass = meta::ByValue;
 
     fn to_godot(&self) -> Self::Via {
         Array::from(self)
@@ -432,7 +441,7 @@ impl<T: ArrayElement> GodotConvert for &[T] {
 }
 
 impl<T: ArrayElement> ToGodot for &[T] {
-    type Pass = crate::meta::ByValue;
+    type Pass = meta::ByValue;
 
     fn to_godot(&self) -> Self::Via {
         Array::from(*self)
@@ -453,7 +462,7 @@ macro_rules! impl_pointer_convert {
         }
 
         impl ToGodot for $Ptr {
-            type Pass = crate::meta::ByValue;
+            type Pass = meta::ByValue;
 
             fn to_godot(&self) -> Self::Via {
                 *self as i64

godot-core/src/meta/traits.rs

@@ -140,6 +140,25 @@ pub trait GodotType: GodotConvert<Via = Self> + sealed::Sealed + Sized + 'static
     fn qualifies_as_special_none(_from_variant: &Variant) -> bool {
         false
     }
+
+    /// Convert to `ObjectArg` for efficient object argument passing.
+    ///
+    /// Implemented in `GodotType` because Rust has no specialization, and there's no good way to have trait bounds in `ByObject`, but not in
+    /// other arg-passing strategies `ByValue`/`ByRef`.
+    ///
+    /// # Panics
+    /// If `Self` is not an object type (`Gd<T>`, `Option<Gd<T>>`). Note that `DynGd<T>` isn't directly implemented here, but uses `Gd<T>`'s
+    /// impl on the FFI layer.
+    ///
+    /// # Safety
+    /// `self` must be kept alive while return value is in use. Might be addressed with lifetime in the future.
+    #[doc(hidden)]
+    unsafe fn as_object_arg(&self) -> crate::meta::ObjectArg {
+        panic!(
+            "as_object_arg() called for non-object type: {}",
+            std::any::type_name::<Self>()
+        )
+    }
 }
 
 // ----------------------------------------------------------------------------------------------------------------------------------------------

godot-core/src/obj/dyn_gd.rs

@@ -553,9 +553,10 @@ where
     T: GodotClass,
     D: ?Sized,
 {
+    // Delegate to Gd<T> passing strategy.
     type Pass = <Gd<T> as ToGodot>::Pass;
 
-    fn to_godot(&self) -> Self::Via {
+    fn to_godot(&self) -> &Self::Via {
         self.obj.to_godot()
     }