Merge pull request #754 from godot-rust/bugfix/native-structure-objects

Fix native structures exposing `*mut Gd<Object>`, provide accessor

Author: Bromeon

godot-codegen/src/conv/type_conversions.rs

@@ -84,13 +84,24 @@ fn to_hardcoded_rust_enum(ty: &str) -> Option<&str> {
 /// Maps an input type to a Godot type with the same C representation. This is subtly different than [`to_rust_type`],
 /// which maps to an appropriate corresponding Rust type. This function should be used in situations where the C ABI for
 /// a type must match the Godot equivalent exactly, such as when dealing with pointers.
-pub(crate) fn to_rust_type_abi(ty: &str, ctx: &mut Context) -> RustTy {
-    match ty {
+pub(crate) fn to_rust_type_abi(ty: &str, ctx: &mut Context) -> (RustTy, bool) {
+    let mut is_obj = false;
+    let ty = match ty {
+        // In native structures, object pointers are mapped to opaque entities. Instead, an accessor function is provided.
+        "Object*" => {
+            is_obj = true;
+            RustTy::RawPointer {
+                inner: Box::new(RustTy::BuiltinIdent(ident("c_void"))),
+                is_const: false,
+            }
+        }
         "int" => RustTy::BuiltinIdent(ident("i32")),
         "float" => RustTy::BuiltinIdent(ident("f32")),
         "double" => RustTy::BuiltinIdent(ident("f64")),
         _ => to_rust_type(ty, None, ctx),
-    }
+    };
+
+    (ty, is_obj)
 }
 
 /// Maps an _input_ type from the Godot JSON to the corresponding Rust type (wrapping some sort of a token stream).

godot-codegen/src/generator/native_structures.rs

@@ -11,7 +11,7 @@ use crate::models::domain::{ExtensionApi, ModName, NativeStructure, TyName};
 use crate::util::ident;
 use crate::{conv, util, SubmitFn};
 use proc_macro2::TokenStream;
-use quote::quote;
+use quote::{format_ident, quote};
 use std::path::Path;
 
 pub fn generate_native_structures_files(
@@ -52,8 +52,15 @@ pub fn generate_native_structures_files(
 
 #[derive(Clone, Eq, PartialEq, Debug)]
 pub(crate) struct NativeStructuresField {
+    /// Identifier for the field name, e.g. `collider`.
     pub field_name: String,
+
+    /// Type which has a raw format that is latter mapped to `RustTy`.
+    ///
+    /// Corresponds to other Godot type names, e.g. `Object*` or `enum::TextServer.Direction`.
     pub field_type: String,
+
+    /// If the field is an array, this contains the number of elements.
     pub array_size: Option<usize>,
 }
 
@@ -64,13 +71,20 @@ fn make_native_structure(
 ) -> builtins::GeneratedBuiltin {
     let class_name = &class_name.rust_ty;
 
+    // TODO for native structures holding object pointers, we should encapsulate the raw object, as follows:
+    // - Make raw object pointer field private (and maybe also ID field, to keep in sync).
+    // - Add constructor that takes Gd<Object> instead of ID/raw-ptr fields.
+    // - Add getter and setter methods with Gd<Object> (already present).
+    // - Add Drop impl, which decrements refcount if the constructor was used, and does nothing if FromGodot pointer conversion was used.
+
     let imports = util::make_imports();
-    let fields = make_native_structure_fields(&structure.format, ctx);
+    let (fields, methods) = make_native_structure_fields_and_methods(structure, ctx);
     let doc = format!("[`ToGodot`] and [`FromGodot`] are implemented for `*mut {class_name}` and `*const {class_name}`.");
 
     // mod re_export needed, because class should not appear inside the file module, and we can't re-export private struct as pub
     let tokens = quote! {
         #imports
+        use std::ffi::c_void; // for opaque object pointer fields
         use crate::meta::{GodotConvert, FromGodot, ToGodot};
 
         /// Native structure; can be passed via pointer in APIs that are not exposed to GDScript.
@@ -82,6 +96,10 @@ fn make_native_structure(
             #fields
         }
 
+        impl #class_name {
+            #methods
+        }
+
         impl GodotConvert for *mut #class_name {
             type Via = i64;
         }
@@ -119,25 +137,36 @@ fn make_native_structure(
     builtins::GeneratedBuiltin { code: tokens }
 }
 
-fn make_native_structure_fields(format_str: &str, ctx: &mut Context) -> TokenStream {
-    let fields = parse_native_structures_format(format_str)
+fn make_native_structure_fields_and_methods(
+    structure: &NativeStructure,
+    ctx: &mut Context,
+) -> (TokenStream, TokenStream) {
+    let fields = parse_native_structures_format(&structure.format)
         .expect("Could not parse native_structures format field");
 
-    let field_definitions = fields
-        .into_iter()
-        .map(|field| make_native_structure_field_definition(field, ctx));
+    let mut field_definitions = vec![];
+    let mut accessors = vec![];
 
-    quote! {
-        #( #field_definitions )*
+    for field in fields {
+        let (field_def, accessor) = make_native_structure_field_and_accessor(field, ctx);
+        field_definitions.push(field_def);
+        if let Some(accessor) = accessor {
+            accessors.push(accessor);
+        }
     }
+
+    let fields = quote! { #( #field_definitions )* };
+    let methods = quote! { #( #accessors )* };
+
+    (fields, methods)
 }
 
-fn make_native_structure_field_definition(
+fn make_native_structure_field_and_accessor(
     field: NativeStructuresField,
     ctx: &mut Context,
-) -> TokenStream {
+) -> (TokenStream, Option<TokenStream>) {
     let field_type = normalize_native_structure_field_type(&field.field_type);
-    let field_type = conv::to_rust_type_abi(&field_type, ctx);
+    let (field_type, is_object_ptr) = conv::to_rust_type_abi(&field_type, ctx);
 
     // Make array if needed.
     let field_type = if let Some(size) = field.array_size {
@@ -146,11 +175,57 @@ fn make_native_structure_field_definition(
         quote! { #field_type }
     };
 
-    let field_name = ident(&conv::to_snake_case(&field.field_name));
+    let snake_field_name = ident(&conv::to_snake_case(&field.field_name));
+
+    let (field_name, accessor);
+    if is_object_ptr {
+        // Highlight that the pointer field is internal/opaque.
+        field_name = format_ident!("raw_{}_ptr", snake_field_name);
+
+        // Generate method that converts from instance ID.
+        let getter_name = format_ident!("get_{}", snake_field_name);
+        let setter_name = format_ident!("set_{}", snake_field_name);
+        let id_field_name = format_ident!("{}_id", snake_field_name);
+
+        // Current native structures treat all object pointers as Object (even if concrete ones like `collider` might be Node).
+        // Having node also avoids the lifetime issue with reference-counted objects (unclear if gdext should increment refcount or not).
+        // If other fields use different classes in the future, we'll need to update this.
+        accessor = Some(quote! {
+            /// Returns the object as a `Gd<Node>`, or `None` if it no longer exists.
+            pub fn #getter_name(&self) -> Option<Gd<Object>> {
+                crate::obj::InstanceId::try_from_u64(self.#id_field_name.id)
+                    .and_then(|id| Gd::try_from_instance_id(id).ok())
+
+                // Sanity check for consistency (if Some(...)):
+                // let ptr = self.#field_name as sys::GDExtensionObjectPtr;
+                // unsafe { Gd::from_obj_sys(ptr) }
+            }
+
+            /// Sets the object from a `Gd` pointer holding `Node` or a derived class.
+            pub fn #setter_name<T>(&mut self, #snake_field_name: Gd<T>)
+            where T: crate::obj::Inherits<Object> {
+                use crate::meta::GodotType as _;
 
-    quote! {
+                let obj = #snake_field_name.upcast();
+
+                assert!(obj.is_instance_valid(), "provided node is dead");
+
+                let id = obj.instance_id().to_u64();
+
+                self.#id_field_name = ObjectId { id };
+                self.#field_name = obj.obj_sys() as *mut std::ffi::c_void;
+            }
+        });
+    } else {
+        field_name = snake_field_name;
+        accessor = None;
+    };
+
+    let field_def = quote! {
         pub #field_name: #field_type,
-    }
+    };
+
+    (field_def, accessor)
 }
 
 fn normalize_native_structure_field_type(field_type: &str) -> String {

godot-core/src/obj/gd.rs

@@ -46,7 +46,7 @@ use crate::{classes, out};
 /// - **Manual**<br>
 ///   Objects inheriting from [`Object`] which are not `RefCounted` (or inherited) are **manually-managed**.
 ///   Their destructor is not automatically called (unless they are part of the scene tree). Creating a `Gd<T>` means that
-///   you are responsible of explicitly deallocating such objects using [`free()`][Self::free].<br><br>
+///   you are responsible for explicitly deallocating such objects using [`free()`][Self::free].<br><br>
 ///
 /// - **Dynamic**<br>
 ///   For `T=Object`, the memory strategy is determined **dynamically**. Due to polymorphism, a `Gd<Object>` can point to either
@@ -77,7 +77,7 @@ use crate::{classes, out};
 /// These provide interior mutability similar to [`RefCell`][std::cell::RefCell], with the addition that `Gd` simultaneously handles reference
 /// counting (for some types `T`).
 ///
-/// When you declare a `#[func]` method on your own class and it accepts `&self` or `&mut self`, an implicit `bind()` or `bind_mut()` call
+/// When you declare a `#[func]` method on your own class, and it accepts `&self` or `&mut self`, an implicit `bind()` or `bind_mut()` call
 /// on the owning `Gd<T>` is performed. This is important to keep in mind, as you can get into situations that violate dynamic borrow rules; for
 /// example if you are inside a `&mut self` method, make a call to GDScript and indirectly call another method on the same object (re-entrancy).
 ///
@@ -414,6 +414,13 @@ impl<T: GodotClass> Gd<T> {
         }
     }
 
+    /// Returns a callable referencing a method from this object named `method_name`.
+    ///
+    /// This is shorter syntax for [`Callable::from_object_method(self, method_name)`][Callable::from_object_method].
+    pub fn callable<S: Into<StringName>>(&self, method_name: S) -> Callable {
+        Callable::from_object_method(self, method_name)
+    }
+
     pub(crate) unsafe fn from_obj_sys_or_none(
         ptr: sys::GDExtensionObjectPtr,
     ) -> Result<Self, ConvertError> {
@@ -451,13 +458,6 @@ impl<T: GodotClass> Gd<T> {
     {
         self.raw.script_sys()
     }
-
-    /// Returns a callable referencing a method from this object named `method_name`.
-    ///
-    /// This is shorter syntax for [`Callable::from_object_method(self, method_name)`][Callable::from_object_method].
-    pub fn callable<S: Into<StringName>>(&self, method_name: S) -> Callable {
-        Callable::from_object_method(self, method_name)
-    }
 }
 
 impl<T: GodotClass> Deref for Gd<T> {

itest/rust/src/engine_tests/native_structures_test.rs

@@ -8,10 +8,12 @@
 use crate::framework::itest;
 
 use godot::builtin::{Rect2, Rid, Variant};
-use godot::classes::native::{AudioFrame, CaretInfo, Glyph, ObjectId};
+use godot::classes::native::{
+    AudioFrame, CaretInfo, Glyph, ObjectId, PhysicsServer2DExtensionShapeResult,
+};
 use godot::classes::text_server::Direction;
-use godot::classes::{ITextServerExtension, TextServer, TextServerExtension};
-use godot::obj::{Base, NewGd};
+use godot::classes::{ITextServerExtension, Node3D, RefCounted, TextServer, TextServerExtension};
+use godot::obj::{Base, NewAlloc, NewGd};
 use godot::register::{godot_api, GodotClass};
 
 use std::cell::Cell;
@@ -71,7 +73,7 @@ impl ITextServerExtension for TestTextServer {
 }
 
 #[itest]
-fn test_native_structures_codegen() {
+fn native_structure_codegen() {
     // Test construction of a few simple types.
     let _ = AudioFrame {
         left: 0.0,
@@ -93,7 +95,7 @@ fn test_native_structures_codegen() {
 }
 
 #[itest]
-fn test_native_structure_out_parameter() {
+fn native_structure_out_parameter() {
     // Instantiate a TextServerExtension and then have Godot call a
     // function which uses an 'out' pointer parameter.
     let mut ext = TestTextServer::new_gd();
@@ -127,7 +129,7 @@ fn test_native_structure_out_parameter() {
 }
 
 #[itest]
-fn test_native_structure_pointer_to_array_parameter() {
+fn native_structure_pointer_to_array_parameter() {
     // Instantiate a TextServerExtension.
     let ext = TestTextServer::new_gd();
     let result = ext
@@ -142,7 +144,7 @@ fn test_native_structure_pointer_to_array_parameter() {
 }
 
 #[itest]
-fn test_native_structure_clone() {
+fn native_structure_clone() {
     // Instantiate CaretInfo directly.
     let caret1 = CaretInfo {
         leading_caret: Rect2::from_components(0.0, 0.0, 0.0, 0.0),
@@ -169,15 +171,15 @@ fn test_native_structure_clone() {
 }
 
 #[itest]
-fn test_native_structure_partialeq() {
+fn native_structure_partialeq() {
     // Test basic equality between two identically-constructed
     // (but distinct) native structures.
     assert_eq!(sample_glyph(5), sample_glyph(5));
     assert_ne!(sample_glyph(1), sample_glyph(2));
 }
 
 #[itest]
-fn test_native_structure_debug() {
+fn native_structure_debug() {
     // Test debug output, both pretty-printed and not.
     let object_id = ObjectId { id: 256 };
     assert_eq!(
@@ -189,3 +191,66 @@ fn test_native_structure_debug() {
         String::from("ObjectId {\n    id: 256,\n}")
     );
 }
+
+#[itest]
+fn native_structure_object_pointers() {
+    let mut result = PhysicsServer2DExtensionShapeResult {
+        rid: Rid::new(12),
+        collider_id: ObjectId { id: 0 },
+        raw_collider_ptr: std::ptr::null_mut(),
+        shape: 0,
+    };
+
+    let retrieved = result.get_collider();
+    assert_eq!(retrieved, None);
+
+    let object = Node3D::new_alloc();
+    result.set_collider(object.clone());
+    assert_eq!(result.collider_id.id, object.instance_id().to_i64() as u64);
+
+    let retrieved = result.get_collider();
+    assert_eq!(retrieved, Some(object.clone().upcast()));
+
+    object.free();
+    assert_eq!(result.get_collider(), None);
+}
+
+#[itest(skip)] // Not yet implemented.
+fn native_structure_refcounted_pointers() {
+    let mut result = PhysicsServer2DExtensionShapeResult {
+        rid: Rid::new(12),
+        collider_id: ObjectId { id: 0 },
+        raw_collider_ptr: std::ptr::null_mut(),
+        shape: 0,
+    };
+
+    // RefCounted, increment ref-count.
+    let object = RefCounted::new_gd();
+    let id = object.instance_id();
+    result.set_collider(object.clone());
+    assert_eq!(result.collider_id.id, id.to_i64() as u64);
+
+    drop(object); // Test if Godot keeps ref-count.
+
+    let retrieved = result
+        .get_collider()
+        .expect("Ref-counted objects don't drop if ref-count is incremented");
+    assert_eq!(retrieved.instance_id(), id);
+
+    // Manually decrement refcount (method unexposed).
+    //Gd::<RefCounted>::from_instance_id(id).call("unreference".into(), &[]);
+
+    // RefCounted, do NOT increment ref-count.
+    let object = RefCounted::new_gd();
+    let id = object.instance_id();
+    result.set_collider(object.clone());
+    assert_eq!(result.collider_id.id, id.to_i64() as u64);
+
+    drop(object); // Test if Godot keeps ref-count.
+
+    let retrieved = result.get_collider();
+    assert!(
+        retrieved.is_none(),
+        "Ref-counted objects drop if ref-count is not incremented"
+    );
+}