Make base_mut not clone Gd and work in init and predelete

Author: beicause

godot-core/src/classes/class_runtime.rs

@@ -216,30 +216,6 @@ pub(crate) fn ensure_object_inherits(derived: ClassId, base: ClassId, instance_i
     )
 }
 
-#[cfg(debug_assertions)]
-pub(crate) fn ensure_binding_not_null<T>(binding: sys::GDExtensionClassInstancePtr)
-where
-    T: GodotClass + Bounds<Declarer = bounds::DeclUser>,
-{
-    if !binding.is_null() {
-        return;
-    }
-
-    // Non-tool classes can't be instantiated in the editor.
-    if crate::classes::Engine::singleton().is_editor_hint() {
-        panic!(
-            "Class {} -- null instance; does the class have a Godot creator function? \
-            Ensure that the given class is a tool class with #[class(tool)], if it is being accessed in the editor.",
-            std::any::type_name::<T>()
-        )
-    } else {
-        panic!(
-            "Class {} -- null instance; does the class have a Godot creator function?",
-            std::any::type_name::<T>()
-        );
-    }
-}
-
 // ----------------------------------------------------------------------------------------------------------------------------------------------
 // Implementation of this file
 

godot-core/src/obj/base.rs

@@ -320,7 +320,7 @@ impl<T: GodotClass> Base<T> {
 
     /// Returns `true` if this `Base<T>` is currently in the initializing state.
     #[cfg(debug_assertions)]
-    fn is_initializing(&self) -> bool {
+    pub(crate) fn is_initializing(&self) -> bool {
         self.init_state.get() == InitState::ObjectConstructing
     }
 
@@ -344,12 +344,6 @@ impl<T: GodotClass> Base<T> {
     /// # Safety
     /// Caller must ensure that the underlying object remains valid for the entire lifetime of the returned `PassiveGd`.
     pub(crate) unsafe fn constructed_passive(&self) -> PassiveGd<T> {
-        #[cfg(debug_assertions)] // debug_assert! still checks existence of symbols.
-        assert!(
-            !self.is_initializing(),
-            "WithBaseField::base(), base_mut() can only be called on fully-constructed objects, after I*::init() or Gd::from_init_fn()"
-        );
-
         // SAFETY: object pointer is valid and remains valid as long as self is alive (per safety precondition of this fn).
         unsafe { PassiveGd::from_obj_sys(self.obj.obj_sys()) }
     }

godot-core/src/obj/guards.rs

@@ -232,13 +232,14 @@ macro_rules! make_base_mut {
         #[doc = concat!("See [`", stringify!($doc_type), "::base_mut()`](", stringify!($doc_path), "::base_mut()) for usage.\n")]
         pub struct $ident<'a, T: $bound> {
             passive_gd: PassiveGd<T::Base>,
-            _inaccessible_guard: InaccessibleGuard<'a, T>,
+       		// Option because we can't make a guard yet since the instance storage is null during initializing.
+    	    _inaccessible_guard: Option<InaccessibleGuard<'a, T>>,
         }
 
         impl<'a, T: $bound> $ident<'a, T> {
             pub(crate) fn new(
                 passive_gd: PassiveGd<T::Base>,
-                inaccessible_guard: InaccessibleGuard<'a, T>,
+                inaccessible_guard: Option<InaccessibleGuard<'a, T>>,
             ) -> Self {
                 Self {
                     passive_gd,

godot-core/src/obj/raw_gd.rs

@@ -440,7 +440,12 @@ where
     /// Storage object associated with the extension instance.
     ///
     /// Returns `None` if self is null.
+    ///
+    /// # Panics
+    /// In Debug mode, if binding is null.
     pub(crate) fn storage(&self) -> Option<&InstanceStorage<T>> {
+        #[cfg(debug_assertions)]
+        self.ensure_storage_not_null();
         // SAFETY:
         // - We have a `&self`, so the storage must already have been created.
         // - The storage cannot be destroyed while we have a `&self` reference, so it will not be
@@ -450,7 +455,7 @@ where
 
     /// Storage object associated with the extension instance.
     ///
-    /// Returns `None` if self is null.
+    /// Returns `None` if self is null, or if binding is null.
     ///
     /// # Safety
     ///
@@ -474,18 +479,36 @@ where
         }
     }
 
+    #[cfg(debug_assertions)]
+    pub(crate) fn ensure_storage_not_null(&self) {
+        let binding = self.resolve_instance_ptr();
+        if !binding.is_null() {
+            return;
+        }
+        // Non-tool classes can't be instantiated in the editor.
+        if crate::classes::Engine::singleton().is_editor_hint() {
+            panic!(
+                "Class {} -- null instance; does the class have a Godot creator function? \
+                Ensure that the given class is a tool class with #[class(tool)], if it is being accessed in the editor.",
+                std::any::type_name::<Self>()
+            )
+        } else {
+            panic!(
+                "Class {} -- null instance; does the class have a Godot creator function?",
+                std::any::type_name::<Self>()
+            );
+        }
+    }
+
     /// Retrieves and caches pointer to this class instance if `self.obj` is non-null.
-    /// Returns a null pointer otherwise.
+    /// Returns a null pointer otherwise, or if binding is null.
     ///
     /// Note: The returned pointer to the GDExtensionClass instance (even when `self.obj` is non-null)
     /// might still be null when:
     /// - The class isn't instantiable in the current context.
     /// - The instance is a placeholder (e.g., non-`tool` classes in the editor).
     ///
     /// However, null pointers might also occur in other, undocumented contexts.
-    ///
-    /// # Panics
-    /// In Debug mode, if binding is null.
     fn resolve_instance_ptr(&self) -> sys::GDExtensionClassInstancePtr {
         if self.is_null() {
             return ptr::null_mut();
@@ -509,8 +532,9 @@ where
 
         let ptr: sys::GDExtensionClassInstancePtr = binding.cast();
 
-        #[cfg(debug_assertions)]
-        crate::classes::ensure_binding_not_null::<T>(ptr);
+        if ptr.is_null() {
+            return ptr::null_mut();
+        }
 
         self.cached_storage_ptr.set(ptr);
         ptr

godot-core/src/obj/script.rs

@@ -503,7 +503,7 @@ impl<'a, T: ScriptInstance> SiMut<'a, T> {
         let guard = self.cell.make_inaccessible(self.mut_ref).unwrap();
         let passive_gd = self.base_ref.to_script_passive();
 
-        ScriptBaseMut::new(passive_gd, guard)
+        ScriptBaseMut::new(passive_gd, Some(guard))
     }
 }
 

godot-core/src/obj/traits.rs

@@ -12,7 +12,7 @@ use crate::builtin::GString;
 use crate::init::InitLevel;
 use crate::meta::inspect::EnumConstant;
 use crate::meta::ClassId;
-use crate::obj::{bounds, Base, BaseMut, BaseRef, Bounds, Gd};
+use crate::obj::{bounds, Base, BaseMut, BaseRef, Bounds, Gd, PassiveGd};
 use crate::registry::signal::SignalObject;
 use crate::storage::Storage;
 
@@ -516,26 +516,26 @@ pub trait WithBaseField: GodotClass + Bounds<Declarer = bounds::DeclUser> {
         // We need to construct this first, as the mut-borrow below will block all other access.
         // SAFETY: lifetime is re-established at the bottom BaseMut construction, since return type of this fn has lifetime bound to instance.
         let passive_gd = unsafe { self.base_field().constructed_passive() };
+        // SAFETY: The object ptr of `Gd<Self::Base>` is guaranteed to be valid `Gd<Self>`.
+        let gd = unsafe { PassiveGd::from_obj_sys(passive_gd.obj_sys()) };
 
-        let gd = self.to_gd();
+        // If the object is fully constructed, the associated instance storage must have been created.
+        #[cfg(debug_assertions)]
+        if !self.base_field().is_initializing() {
+            gd.raw.ensure_storage_not_null();
+        }
 
         // SAFETY:
-        // - We have a `Gd<Self>` so, provided that `storage_unbounded` succeeds, the associated instance
-        //   storage has been created.
+        // - `storage_unbounded` will succeed if the object is fully constructed, otherwise it returns None.
         //
         // - Since we can get a `&'a Base<Self::Base>` from `&'a self`, that must mean we have a Rust object
         //   somewhere that has this base object. The only way to have such a base object is by being the
         //   Rust object referenced by that base object. I.e. this storage's user-instance is that Rust
         //   object. That means this storage cannot be destroyed for the lifetime of that Rust object. And
         //   since we have a reference to the base object derived from that Rust object, then that Rust
         //   object must outlive `'a`. And so the storage cannot be destroyed during the lifetime `'a`.
-        let storage = unsafe {
-            gd.raw
-                .storage_unbounded()
-                .expect("we have Gd<Self>; its RawGd should not be null")
-        };
-
-        let guard = storage.get_inaccessible(self);
+        let storage = unsafe { gd.raw.storage_unbounded() };
+        let guard = storage.map(|s| s.get_inaccessible(self));
 
         // Narrows lifetime again from 'static to 'self.
         BaseMut::new(passive_gd, guard)

itest/rust/src/object_tests/base_init_test.rs

@@ -6,11 +6,11 @@
  */
 
 use godot::builtin::real_consts::FRAC_PI_3;
-use godot::builtin::Vector2;
+use godot::builtin::{Array, PackedArray, Variant, Vector2};
 use godot::classes::notify::ObjectNotification;
-use godot::classes::{ClassDb, IRefCounted, RefCounted};
+use godot::classes::{mesh, ArrayMesh, ClassDb, IArrayMesh, IRefCounted, RefCounted};
 use godot::meta::ToGodot;
-use godot::obj::{Base, Gd, InstanceId, NewAlloc, NewGd, WithBaseField};
+use godot::obj::{Base, Gd, IndexEnum, InstanceId, NewAlloc, NewGd, WithBaseField};
 use godot::register::{godot_api, GodotClass};
 use godot::task::TaskHandle;
 
@@ -197,3 +197,69 @@ fn base_postinit_refcounted() -> TaskHandle {
     assert_eq!(obj.get_reference_count(), 2);
     next_frame(move || assert_eq!(obj.get_reference_count(), 1, "eventual dec-ref happens"))
 }
+
+fn make_mesh_arrays() -> Array<Variant> {
+    let mut arrays = Array::new();
+    arrays.resize(mesh::ArrayType::ENUMERATOR_COUNT, &Variant::nil());
+    let indices = PackedArray::<i32>::from([0, 1, 2]);
+    let vertex = PackedArray::<Vector2>::from([
+        Vector2::new(0.0, 0.0),
+        Vector2::new(1.0, 0.0),
+        Vector2::new(0.0, 1.0),
+    ]);
+    arrays.set(mesh::ArrayType::INDEX.to_index(), &indices.to_variant());
+    arrays.set(mesh::ArrayType::VERTEX.to_index(), &vertex.to_variant());
+    arrays
+}
+
+#[derive(GodotClass)]
+#[class(base=ArrayMesh)]
+struct InitArrayMeshTest {
+    base: Base<ArrayMesh>,
+}
+
+#[rustfmt::skip]
+#[godot_api]
+impl IArrayMesh for InitArrayMeshTest {
+    fn init(base: Base<ArrayMesh>) -> Self {
+        let mut sf = Self { base };
+        sf.base_mut()
+            .add_surface_from_arrays(mesh::PrimitiveType::TRIANGLES, &make_mesh_arrays());
+        sf
+    }
+
+    fn on_notification(&mut self, what: ObjectNotification) {
+        if what == ObjectNotification::PREDELETE {
+            let arr = make_mesh_arrays();
+            self.base_mut().add_surface_from_arrays(mesh::PrimitiveType::TRIANGLES, &arr);
+
+            assert_eq!(self.base().get_surface_count(), 2);
+            assert_eq!(self.base().surface_get_arrays(0), arr);
+            assert_eq!(self.base().surface_get_arrays(1), arr);
+        }
+    }
+
+    fn get_surface_count(&self) -> i32 { unreachable!(); }
+    fn surface_get_array_len(&self, _index: i32) -> i32 { unreachable!(); }
+    fn surface_get_array_index_len(&self, _index: i32) -> i32 { unreachable!(); }
+    fn surface_get_arrays(&self, _index: i32) -> Array<Variant> { unreachable!(); }
+    fn surface_get_blend_shape_arrays(&self, _index: i32) -> Array<Array<Variant>> { unreachable!(); }
+    fn surface_get_lods(&self, _index: i32) -> godot::builtin::Dictionary { unreachable!(); }
+    fn surface_get_format(&self, _index: i32) -> u32 { unreachable!(); }
+    fn surface_get_primitive_type(&self, _index: i32) -> u32 { unreachable!(); }
+    #[cfg(feature = "codegen-full")]
+    fn surface_set_material(&mut self, _index: i32, _material: Option<Gd<godot::classes::Material>>) { unreachable!(); }
+    #[cfg(feature = "codegen-full")]
+    fn surface_get_material(&self, _index: i32) -> Option<Gd<godot::classes::Material>> { unreachable!(); }
+    fn get_blend_shape_count(&self) -> i32 { unreachable!(); }
+    fn get_blend_shape_name(&self, _index: i32) -> godot::builtin::StringName { unreachable!(); }
+    fn set_blend_shape_name(&mut self, _index: i32, _name: godot::builtin::StringName){ unreachable!(); }
+    fn get_aabb(&self) -> godot::builtin::Aabb { unreachable!(); }
+}
+
+#[itest]
+fn base_mut_init_array_mesh() {
+    let mesh = InitArrayMeshTest::new_gd();
+    assert_eq!(mesh.get_surface_count(), 1);
+    assert_eq!(mesh.surface_get_arrays(0), make_mesh_arrays());
+}