diff --git a/godot-core/src/builtin/meta/signature.rs b/godot-core/src/builtin/meta/signature.rs index 8ba1b5092..415f8f1d4 100644 --- a/godot-core/src/builtin/meta/signature.rs +++ b/godot-core/src/builtin/meta/signature.rs @@ -162,8 +162,8 @@ macro_rules! impl_signature_for_tuple { unsafe { <$R as sys::GodotFuncMarshal>::try_write_sys(&ret_val, ret) } .unwrap_or_else(|e| return_error::<$R>(method_name, &e)); - // FIXME should be inc_ref instead of forget - std::mem::forget(ret_val); + // FIXME is inc_ref needed here? + // std::mem::forget(ret_val); } } }; diff --git a/godot-core/src/macros.rs b/godot-core/src/macros.rs index d277cd10a..4d8cdd55f 100644 --- a/godot-core/src/macros.rs +++ b/godot-core/src/macros.rs @@ -413,8 +413,8 @@ macro_rules! gdext_ptrcall { )*); <$($RetTy)+ as sys::GodotFfi>::write_sys(&ret_val, $ret); - // FIXME should be inc_ref instead of forget - #[allow(clippy::forget_copy)] - std::mem::forget(ret_val); + // FIXME is inc_ref needed here? + // #[allow(clippy::forget_copy)] + // std::mem::forget(ret_val); }; } diff --git a/godot-core/src/obj/base.rs b/godot-core/src/obj/base.rs index 698b0cce1..4844d4c25 100644 --- a/godot-core/src/obj/base.rs +++ b/godot-core/src/obj/base.rs @@ -39,7 +39,8 @@ impl Base { pub(crate) unsafe fn from_sys(base_ptr: sys::GDExtensionObjectPtr) -> Self { assert!(!base_ptr.is_null(), "instance base is null pointer"); - let obj = Gd::from_obj_sys(base_ptr); + // Initialize only as weak pointer (don't increment reference count) + let obj = Gd::from_obj_sys_weak(base_ptr); // This obj does not contribute to the strong count, otherwise we create a reference cycle: // 1. RefCounted (dropped in GDScript) diff --git a/godot-core/src/obj/gd.rs b/godot-core/src/obj/gd.rs index b9f472711..700396cf0 100644 --- a/godot-core/src/obj/gd.rs +++ b/godot-core/src/obj/gd.rs @@ -101,13 +101,10 @@ where T::Mem::maybe_init_ref(&result); result*/ - let result = unsafe { + unsafe { let object_ptr = callbacks::create::(ptr::null_mut()); Gd::from_obj_sys(object_ptr) - }; - - T::Mem::maybe_init_ref(&result); - result + } } // FIXME use ```no_run instead of ```ignore, as soon as unit test #[cfg] mess is cleaned up @@ -138,10 +135,7 @@ where F: FnOnce(crate::obj::Base) -> T, { let object_ptr = callbacks::create_custom(init); - let result = unsafe { Gd::from_obj_sys(object_ptr) }; - - T::Mem::maybe_init_ref(&result); - result + unsafe { Gd::from_obj_sys(object_ptr) } } /// Hands out a guard for a shared borrow, through which the user instance can be read. @@ -210,7 +204,7 @@ impl Gd { None } else { // SAFETY: assumes that the returned GDExtensionObjectPtr is convertible to Object* (i.e. C++ upcast doesn't modify the pointer) - let untyped = unsafe { Gd::::from_obj_sys(ptr).ready() }; + let untyped = unsafe { Gd::::from_obj_sys(ptr) }; untyped.owned_cast::().ok() } } @@ -277,14 +271,6 @@ impl Gd { } } - /// Needed to initialize ref count -- must be explicitly invoked. - /// - /// Could be made part of FFI methods, but there are some edge cases where this is not intended. - pub(crate) fn ready(self) -> Self { - T::Mem::maybe_inc_ref(&self); - self - } - /// **Upcast:** convert into a smart pointer to a base class. Always succeeds. /// /// Moves out of this value. If you want to create _another_ smart pointer instance, @@ -366,7 +352,8 @@ impl Gd { let class_tag = interface_fn!(classdb_get_class_tag)(class_name.string_sys()); let cast_object_ptr = interface_fn!(object_cast_to)(self.obj_sys(), class_tag); - sys::ptr_then(cast_object_ptr, |ptr| Gd::from_obj_sys(ptr)) + // 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)) } pub(crate) fn as_ref_counted(&self, apply: impl Fn(&mut engine::RefCounted) -> R) -> R { @@ -401,11 +388,30 @@ impl Gd { ffi_methods! { type sys::GDExtensionObjectPtr = Opaque; - fn from_obj_sys = from_sys; - fn from_obj_sys_init = from_sys_init; + fn from_obj_sys_weak = from_sys; fn obj_sys = sys; fn write_obj_sys = write_sys; } + + /// Initializes this `Gd` from the object pointer as a **strong ref**, meaning + /// it initializes/increments the reference counter and keeps the object alive. + /// + /// This is the default for most initializations from FFI. In cases where reference counter + /// should explicitly **not** be updated, [`Self::from_obj_sys_weak`] is available. + #[doc(hidden)] + pub unsafe fn from_obj_sys(ptr: sys::GDExtensionObjectPtr) -> Self { + // Initialize reference counter, if needed + Self::from_obj_sys_weak(ptr).with_inc_refcount() + } + + /// Returns `self` but with initialized ref-count. + fn with_inc_refcount(self) -> Self { + // Note: use init_ref and not inc_ref, since this might be the first reference increment. + // Godot expects RefCounted::init_ref to be called instead of RefCounted::reference in that case. + // init_ref also doesn't hurt (except 1 possibly unnecessary check). + T::Mem::maybe_init_ref(&self); + self + } } /// _The methods in this impl block are only available for objects `T` that are manually managed, @@ -501,7 +507,6 @@ impl Gd { /// # Safety /// `init_fn` must be a function that correctly handles a _type pointer_ pointing to an _object pointer_. #[doc(hidden)] - // TODO unsafe on init_fn instead of this fn? pub unsafe fn from_sys_init_opt(init_fn: impl FnOnce(sys::GDExtensionTypePtr)) -> Option { // Note: see _call_native_mb_ret_obj() in godot-cpp, which does things quite different (e.g. querying the instance binding). @@ -569,7 +574,7 @@ impl Drop for Gd { impl Share for Gd { fn share(&self) -> Self { out!("Gd::share"); - Self::from_opaque(self.opaque).ready() + Self::from_opaque(self.opaque).with_inc_refcount() } } @@ -579,19 +584,23 @@ impl Share for Gd { impl FromVariant for Gd { fn try_from_variant(variant: &Variant) -> Result { let result = unsafe { - let result = Self::from_sys_init(|self_ptr| { + Self::from_sys_init(|self_ptr| { let converter = sys::builtin_fn!(object_from_variant); converter(self_ptr, variant.var_sys()); - }); - result.ready() + }) }; - Ok(result) + // The conversion method `variant_to_object` does NOT increment the reference-count of the object; we need to do that manually. + // (This behaves differently in the opposite direction `object_to_variant`.) + Ok(result.with_inc_refcount()) } } impl ToVariant for Gd { fn to_variant(&self) -> Variant { + // The conversion method `object_to_variant` DOES increment the reference-count of the object; so nothing to do here. + // (This behaves differently in the opposite direction `variant_to_object`.) + unsafe { Variant::from_var_sys_init(|variant_ptr| { let converter = sys::builtin_fn!(object_to_variant); diff --git a/itest/rust/src/object_test.rs b/itest/rust/src/object_test.rs index 148926fff..6dadd0c9e 100644 --- a/itest/rust/src/object_test.rs +++ b/itest/rust/src/object_test.rs @@ -36,6 +36,8 @@ pub fn run() -> bool { ok &= object_from_instance_id_unrelated_type(); ok &= object_user_convert_variant(); ok &= object_engine_convert_variant(); + ok &= object_user_convert_variant_refcount(); + ok &= object_engine_convert_variant_refcount(); ok &= object_engine_up_deref(); ok &= object_engine_up_deref_mut(); ok &= object_engine_upcast(); @@ -225,6 +227,44 @@ fn object_engine_convert_variant() { obj.free(); } +#[itest] +fn object_user_convert_variant_refcount() { + let obj: Gd = Gd::new(ObjPayload { value: -22222 }); + let obj = obj.upcast::(); + check_convert_variant_refcount(obj) +} + +#[itest] +fn object_engine_convert_variant_refcount() { + let obj = RefCounted::new(); + check_convert_variant_refcount(obj); +} + +/// Converts between Object <-> Variant and verifies the reference counter at each stage. +fn check_convert_variant_refcount(obj: Gd) { + // Freshly created -> refcount 1 + assert_eq!(obj.get_reference_count(), 1); + + { + // Variant created from object -> increment + let variant = obj.to_variant(); + assert_eq!(obj.get_reference_count(), 2); + + { + // Yet another object created *from* variant -> increment + let another_object = variant.to::>(); + assert_eq!(obj.get_reference_count(), 3); + assert_eq!(another_object.get_reference_count(), 3); + } + + // `another_object` destroyed -> decrement + assert_eq!(obj.get_reference_count(), 2); + } + + // `variant` destroyed -> decrement + assert_eq!(obj.get_reference_count(), 1); +} + #[itest] fn object_engine_up_deref() { let node3d: Gd = Node3D::new_alloc(); diff --git a/itest/rust/src/variant_test.rs b/itest/rust/src/variant_test.rs index d996fc68c..8d4cbafb3 100644 --- a/itest/rust/src/variant_test.rs +++ b/itest/rust/src/variant_test.rs @@ -287,6 +287,7 @@ fn variant_type_correct() { VariantType::Dictionary ); } + // ---------------------------------------------------------------------------------------------------------------------------------------------- fn roundtrip(value: T)