Merge #78

78: Make any Object Variants with a nullptr inside instead be of type Nil. r=Bromeon a=mhoff12358

Partially resolves #77
This addresses the root issue, but requires all property setters with Object subclass arguments to use Variant rather than Gd<T>.

Co-authored-by: mhoff <[email protected]>
Co-authored-by: Jan Haller <[email protected]>

Author: 3 people

godot-core/src/builtin/node_path.rs

@@ -62,6 +62,7 @@ impl Display for NodePath {
 
 impl_builtin_traits! {
     for NodePath {
+        Default => node_path_construct_default;
         Clone => node_path_construct_copy;
         Drop => node_path_destroy;
     }

godot-core/src/builtin/variant/impls.rs

@@ -133,7 +133,6 @@ mod impls {
     use super::*;
 
     impl_variant_traits!(bool, bool_to_variant, bool_from_variant, Bool);
-    impl_variant_traits!(Dictionary, dictionary_to_variant, dictionary_from_variant, Dictionary);
     impl_variant_traits!(Vector2, vector2_to_variant, vector2_from_variant, Vector2);
     impl_variant_traits!(Vector3, vector3_to_variant, vector3_from_variant, Vector3);
     impl_variant_traits!(Vector4, vector4_to_variant, vector4_from_variant, Vector4);
@@ -142,6 +141,21 @@ mod impls {
     impl_variant_traits!(Color, color_to_variant, color_from_variant, Color);
     impl_variant_traits!(GodotString, string_to_variant, string_from_variant, String);
     impl_variant_traits!(StringName, string_name_to_variant, string_name_from_variant, StringName);
+    impl_variant_traits!(NodePath, node_path_to_variant, node_path_from_variant, NodePath);
+    /* TODO provide those, as soon as `Default` is available. Also consider auto-generating.
+    impl_variant_traits!(Rect2, rect2_to_variant, rect2_from_variant, Rect2);
+    impl_variant_traits!(Rect2i, rect2i_to_variant, rect2i_from_variant, Rect2i);
+    impl_variant_traits!(Plane, plane_to_variant, plane_from_variant, Plane);
+    impl_variant_traits!(Quaternion, quaternion_to_variant, quaternion_from_variant, Quaternion);
+    impl_variant_traits!(Aabb, aabb_to_variant, aabb_from_variant, AABB);
+    impl_variant_traits!(Basis, basis_to_variant, basis_from_variant, Basis);
+    impl_variant_traits!(Transform2D, transform_2d_to_variant, transform_2d_from_variant, Transform2D);
+    impl_variant_traits!(Transform3D, transform_3d_to_variant, transform_3d_from_variant, Transform3D);
+    impl_variant_traits!(Projection, projection_to_variant, projection_from_variant, Projection);
+    impl_variant_traits!(Rid, rid_to_variant, rid_from_variant, RID);
+    impl_variant_traits!(Callable, callable_to_variant, callable_from_variant, Callable);
+    impl_variant_traits!(Signal, signal_to_variant, signal_from_variant, Signal);
+    */
     impl_variant_traits!(Array, array_to_variant, array_from_variant, Array);
     impl_variant_traits!(PackedByteArray, packed_byte_array_to_variant, packed_byte_array_from_variant, PackedByteArray);
     impl_variant_traits!(PackedInt32Array, packed_int32_array_to_variant, packed_int32_array_from_variant, PackedInt32Array);
@@ -152,6 +166,7 @@ mod impls {
     impl_variant_traits!(PackedVector2Array, packed_vector2_array_to_variant, packed_vector2_array_from_variant, PackedVector2Array);
     impl_variant_traits!(PackedVector3Array, packed_vector3_array_to_variant, packed_vector3_array_from_variant, PackedVector3Array);
     impl_variant_traits!(PackedColorArray, packed_color_array_to_variant, packed_color_array_from_variant, PackedColorArray);
+    impl_variant_traits!(Dictionary, dictionary_to_variant, dictionary_from_variant, Dictionary);
 
     impl_variant_traits!(i64, int_to_variant, int_from_variant, Int, GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT64);
     impl_variant_traits_int!(i8, GDEXTENSION_METHOD_ARGUMENT_METADATA_INT_IS_INT8);

godot-core/src/builtin/variant/mod.rs

@@ -59,14 +59,41 @@ impl Variant {
     }
 
     /// Checks whether the variant is empty (`null` value in GDScript).
+    ///
+    /// See also [`Self::get_type`].
     pub fn is_nil(&self) -> bool {
-        self.sys_type() == sys::GDEXTENSION_VARIANT_TYPE_NIL
+        // Use get_type() rather than sys_type(), to also cover nullptr OBJECT as NIL
+        self.get_type() == VariantType::Nil
     }
 
-    // TODO test
+    /// Returns the type that is currently held by this variant.
+    ///
+    /// If this variant holds a type `Object` but no instance (represented as a null object pointer), then `Nil` will be returned for
+    /// consistency. This may deviate from Godot behavior -- for example, calling `Node::get_node_or_null()` with an invalid
+    /// path returns a variant that has type `Object` but acts like `Nil` for all practical purposes.
     pub fn get_type(&self) -> VariantType {
-        let ty_sys = unsafe { interface_fn!(variant_get_type)(self.var_sys()) };
-        VariantType::from_sys(ty_sys)
+        let sys_type = self.sys_type();
+
+        // There is a special case when the Variant has type OBJECT, but the Object* is null.
+        let is_null_object = if sys_type == sys::GDEXTENSION_VARIANT_TYPE_OBJECT {
+            // SAFETY: we checked that the raw type is OBJECT, so we can interpret the type-ptr as address of an object-ptr.
+            let object_ptr = unsafe {
+                crate::obj::raw_object_init(|type_ptr| {
+                    let converter = sys::builtin_fn!(object_from_variant);
+                    converter(type_ptr, self.var_sys());
+                })
+            };
+
+            object_ptr.is_null()
+        } else {
+            false
+        };
+
+        if is_null_object {
+            VariantType::Nil
+        } else {
+            VariantType::from_sys(sys_type)
+        }
     }
 
     // TODO test

godot-core/src/obj/gd.rs

@@ -366,11 +366,7 @@ impl<T: GodotClass> Gd<T> {
         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);
 
-        if cast_object_ptr.is_null() {
-            None
-        } else {
-            Some(Gd::from_obj_sys(cast_object_ptr))
-        }
+        sys::ptr_then(cast_object_ptr, |ptr| Gd::from_obj_sys(ptr))
     }
 
     pub(crate) fn as_ref_counted<R>(&self, apply: impl Fn(&mut engine::RefCounted) -> R) -> R {
@@ -500,32 +496,40 @@ impl<T: GodotClass> GodotFfi for Gd<T> {
 
 impl<T: GodotClass> Gd<T> {
     /// Runs `init_fn` on the address of a pointer (initialized to null). If that pointer is still null after the `init_fn` call,
-    /// then `None` will be returned; otherwise `from_obj_sys(ptr)`.
+    /// then `None` will be returned; otherwise `Gd::from_obj_sys(ptr)`.
     ///
     /// # Safety
-    /// `init_fn` must be a function that correctly handles an "type pointer" pointing to an "object pointer"
+    /// `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<Self> {
         // Note: see _call_native_mb_ret_obj() in godot-cpp, which does things quite different (e.g. querying the instance binding).
 
-        // return_ptr has type GDExtensionTypePtr = GDExtensionObjectPtr* = OpaqueObject* = Object**
-        // (in other words, the type-ptr contains the _address_ of an object-ptr).
-        let mut object_ptr: sys::GDExtensionObjectPtr = ptr::null_mut();
-        let return_ptr: *mut sys::GDExtensionObjectPtr = ptr::addr_of_mut!(object_ptr);
-
-        init_fn(return_ptr as sys::GDExtensionTypePtr);
-
-        // We don't need to know if Object** is null, but if Object* is null; return_ptr has the address of a local (never null).
-        if object_ptr.is_null() {
-            None
-        } else {
-            let obj = Gd::from_obj_sys(object_ptr); // equivalent to Gd::from_sys(return_ptr)
-            Some(obj)
-        }
+        // Initialize pointer with given function, return Some(ptr) on success and None otherwise
+        let object_ptr = raw_object_init(init_fn);
+        sys::ptr_then(object_ptr, |ptr| Gd::from_obj_sys(ptr))
     }
 }
 
+/// Runs `init_fn` on the address of a pointer (initialized to null), then returns that pointer, possibly still null.
+///
+/// # Safety
+/// `init_fn` must be a function that correctly handles a _type pointer_ pointing to an _object pointer_.
+#[doc(hidden)]
+pub unsafe fn raw_object_init(
+    init_fn: impl FnOnce(sys::GDExtensionTypePtr),
+) -> sys::GDExtensionObjectPtr {
+    // return_ptr has type GDExtensionTypePtr = GDExtensionObjectPtr* = OpaqueObject* = Object**
+    // (in other words, the type-ptr contains the _address_ of an object-ptr).
+    let mut object_ptr: sys::GDExtensionObjectPtr = ptr::null_mut();
+    let return_ptr: *mut sys::GDExtensionObjectPtr = ptr::addr_of_mut!(object_ptr);
+
+    init_fn(return_ptr as sys::GDExtensionTypePtr);
+
+    // We don't need to know if Object** is null, but if Object* is null; return_ptr has the address of a local (never null).
+    object_ptr
+}
+
 /// Destructor with semantics depending on memory strategy.
 ///
 /// * If this `Gd` smart pointer holds a reference-counted type, this will decrement the reference counter.

godot-ffi/src/lib.rs

@@ -203,3 +203,16 @@ pub fn force_mut_ptr<T>(ptr: *const T) -> *mut T {
 pub fn to_const_ptr<T>(ptr: *mut T) -> *const T {
     ptr as *const T
 }
+
+/// If `ptr` is not null, returns `Some(mapper(ptr))`; otherwise `None`.
+pub fn ptr_then<T, R, F>(ptr: *mut T, mapper: F) -> Option<R>
+where
+    F: FnOnce(*mut T) -> R,
+{
+    // Could also use NonNull in signature, but for this project we always deal with FFI raw pointers
+    if ptr.is_null() {
+        None
+    } else {
+        Some(mapper(ptr))
+    }
+}

itest/rust/src/variant_test.rs

@@ -5,7 +5,10 @@
  */
 
 use crate::itest;
-use godot::builtin::{FromVariant, GodotString, StringName, ToVariant, Variant, Vector2, Vector3};
+use godot::builtin::{
+    FromVariant, GodotString, NodePath, StringName, ToVariant, Variant, Vector2, Vector3,
+};
+use godot::engine::Node3D;
 use godot::obj::InstanceId;
 use godot::sys::{GodotFfi, VariantOperator, VariantType};
 use std::cmp::Ordering;
@@ -23,6 +26,7 @@ pub fn run() -> bool {
     ok &= variant_evaluate_total_order();
     ok &= variant_sys_conversion();
     ok &= variant_sys_conversion2();
+    ok &= variant_null_object_is_nil();
     ok
 }
 
@@ -197,6 +201,26 @@ fn variant_sys_conversion() {
     assert_eq!(v2, v);
 }
 
+#[itest]
+fn variant_null_object_is_nil() {
+    use godot::sys;
+
+    let mut node = Node3D::new_alloc();
+    let node_path = NodePath::from("res://NonExisting.tscn");
+
+    // Simulates an object that is returned but null
+    // Use reflection to get a variant as return type
+    let variant = node.call("get_node_or_null".into(), &[node_path.to_variant()]);
+    let raw_type: sys::GDExtensionVariantType =
+        unsafe { sys::interface_fn!(variant_get_type)(variant.var_sys()) };
+
+    // Verify that this appears as NIL to the user, even though it's internally OBJECT with a null object pointer
+    assert_eq!(raw_type, sys::GDEXTENSION_VARIANT_TYPE_OBJECT);
+    assert_eq!(variant.get_type(), VariantType::Nil);
+
+    node.free();
+}
+
 #[itest]
 fn variant_sys_conversion2() {
     /*