Document and refactor `PluginItem` related stuff #1003

lilizoey · 2025-01-06T19:47:45Z

Split PluginItem into separate structs per variant
Make constructors + builders for the structs
Move more things out of proc-macros and into struct constructors/builders
Rework the safety invariants of godot_dyn
Document many of these apis better

Whether using builders in here is an improvement isn't 100% clear always. It could be used for the builder-api later, but it's also a bit low-level at the moment. And may be split at the wrong level of granularity. It does however reduce a lot of code in the proc-macros, which is an advantage.

The DynTraitImpl refactor i think works pretty well, a lot of the safety invariants have been moved into the type system, and we dont need to generate much unsafe code in the proc-macro anymore. I also removed DynToClassRelation and replaced it with DynTraitImpl to simplify some of the code.

We could maybe make a util-struct that generates code to use the builders, however currently it's only used in two places so im not sure it's much of an advantage yet. However in the future it may be useful if we're gonna be using the builder-api under the hood.

Git is very confused by the PluginItem refactor unfortunately. It should largely be the same as it was before but just split up, except for DynTraitImpl. However there was an upstream change to the plugin items while i was working on this which made a bit of a messy merge, but i think i resolved it properly.

GodotRust · 2025-01-06T20:01:25Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1003

Bromeon

Thanks a lot for the clean up 🙂

Where I'm not exactly sure from the changes, how would the process of e.g. adding a new property to Struct look now, vs. before? It seems there are now more places that one needs to keep track of and update, no?

Bromeon · 2025-01-06T19:55:40Z

godot-core/src/registry/callbacks.rs

+/// # Safety
+///
+/// `obj` must be castable to `T`.
+pub unsafe fn dynify_fn<T, D>(obj: Gd<Object>) -> ErasedDynGd


Might need #[deny(unsafe_op_in_unsafe_fn)].

Bromeon · 2025-01-06T19:57:51Z

godot-core/src/registry/class.rs

+/// This only looks for an `AsDyn<D>` implementation in the dynamic class, the conversion will fail if the dynamic class doesn't
+/// implement `AsDyn<D>` even if there exists some superclass that does implement `AsDyn<D>`. This restriction could in theory be


Maybe upgrade , -> ; and add another , 🙂

Suggested change

/// This only looks for an `AsDyn<D>` implementation in the dynamic class, the conversion will fail if the dynamic class doesn't

/// implement `AsDyn<D>` even if there exists some superclass that does implement `AsDyn<D>`. This restriction could in theory be

/// This only looks for an `AsDyn<D>` implementation in the dynamic class; the conversion will fail if the dynamic class doesn't

/// implement `AsDyn<D>`, even if there exists some superclass that does implement `AsDyn<D>`. This restriction could in theory be

Bromeon · 2025-01-06T20:02:39Z

godot-core/src/registry/plugin.rs

+    /// The type id of the trait object this was registered with.
+    pub fn dyn_trait_typeid(&self) -> &any::TypeId {
+        &self.dyn_trait_typeid
+    }


TypeId implements Copy. Although its docs don't specifically mention that it's "lightweight", we can probably assume that it's likely some sort of integer. Should we return by value, like we do with Vector4, Quaternion etc?

Bromeon · 2025-01-06T20:04:41Z

godot-core/src/registry/plugin.rs

+        // SAFETY: `DynTraitImpl::new` ensures that this function is safe to call when `object` is castable to `self.class_name`.
+        // Since the dynamic class of `object` is `self.class_name`, it must be castable to `self.class_name`.
+        let erased_dyn = unsafe { (self.erased_dynify_fn)(object.upcast_object()) };


Should upcast_object() be lifted out of the unsafe?

i think that makes sense, as is it may not be entirely clear that it's calling the erased_dynify_fn that is unsafe.

Bromeon · 2025-01-06T20:05:25Z

godot-core/src/registry/plugin.rs

+
+    /// Convert a [`Gd<T>`] to a [`DynGd<T, D>`] using `self`.
+    ///
+    /// This will fail if the dynamic class of `object` does not match the [`ClassName`] stored in `self`.


Suggested change

/// This will fail if the dynamic class of `object` does not match the [`ClassName`] stored in `self`.

/// This will fail with `Err(object)` if the dynamic class of `object` does not match the [`ClassName`] stored in `self`.

Bromeon · 2025-01-06T20:07:50Z

godot-macros/src/class/data_models/interface_trait_impl.rs

+    let item_constructor = quote! { {
+            let mut item = #prv::ITraitImpl::new::<#class_name>(#docs);
+            #(#modifiers)*
+            item
+        }
+    };


Suggested change

let item_constructor = quote! { {

let mut item = #prv::ITraitImpl::new::<#class_name>(#docs);

#(#modifiers)*

item

}

};

let item_constructor = quote! {

{

let mut item = #prv::ITraitImpl::new::<#class_name>(#docs);

#(#modifiers)*

item

}

};

I'm also not 100% sure if "modifiers" is such a good term here -- it's typically used to denote values (flags as in Ctrl/Shift/...).

Since the inserted code aren't modifiers themselves but rather modifications, maybe name it something like item_modifications or item_updates?

(This mostly applies to the 2nd variable declaration; the list of (cfg_attrs, ident) is probably ok with the existing name).

Bromeon · 2025-01-06T20:14:38Z

godot-macros/src/docs.rs

+            }
        })
    })()
-    .unwrap_or(quote! { docs: None })
+    .unwrap_or(TokenStream::new())


Damn, we still have this (|| { ... })() pattern 😁

I need to make a note to clean this up at some point...

this actually doesnt work properly, i didnt check with register-docs enabled at first so i missed that this was wrong

i replaced this occurence with a let-else while i was at it

Bromeon · 2025-01-06T20:20:43Z

godot-core/src/registry/plugin.rs

+    pub(crate) is_instantiable: bool,
+    #[cfg(all(since_api = "4.3", feature = "register-docs"))]
+    pub(crate) docs: Option<StructDocs>,
+}


Suggested change

pub(crate) is_instantiable: bool,

#[cfg(all(since_api = "4.3", feature = "register-docs"))]

pub(crate) docs: Option<StructDocs>,

}

pub(crate) is_instantiable: bool,

/// Documentation extracted from the struct's RustDoc.

#[cfg(all(since_api = "4.3", feature = "register-docs"))]

pub(crate) docs: Option<StructDocs>,

}

Bromeon · 2025-01-06T20:24:26Z

godot-core/src/registry/plugin.rs

@@ -5,38 +5,67 @@
 * file, You can obtain one at https://mozilla.org/MPL/2.0/.
 */

+use crate::classes::Object;


Maybe use the qualified version classes::Object in code, since classes is already imported below?

Bromeon · 2025-01-06T20:25:52Z

godot-core/src/registry/plugin.rs

+    pub fn with_generated<T: GodotClass + cap::GodotDefault>(mut self) -> Self {
+        set(&mut self.generated_create_fn, callbacks::create::<T>);
+        #[cfg(since_api = "4.2")]
+        {
+            set(&mut self.generated_recreate_fn, callbacks::recreate::<T>);
+        }
+        self
+    }


Does this need the { }, or could it just be

Suggested change

pub fn with_generated<T: GodotClass + cap::GodotDefault>(mut self) -> Self {

set(&mut self.generated_create_fn, callbacks::create::<T>);

#[cfg(since_api = "4.2")]

{

set(&mut self.generated_recreate_fn, callbacks::recreate::<T>);

}

self

}

pub fn with_generated<T: GodotClass + cap::GodotDefault>(mut self) -> Self {

set(&mut self.generated_create_fn, callbacks::create::<T>);

#[cfg(since_api = "4.2")]

set(&mut self.generated_recreate_fn, callbacks::recreate::<T>);

self

}

oh yeah i guess it does, i think it used to be an assignment, and it doesn't work for assignments.

lilizoey · 2025-01-06T20:49:09Z

Thanks a lot for the clean up 🙂

Where I'm not exactly sure from the changes, how would the process of e.g. adding a new property to Struct look now, vs. before? It seems there are now more places that one needs to keep track of and update, no?

So what you need to do to add a field to Struct (im using Struct to refer to either PluginItem::Struct before this change, or the Struct struct after) is:

Add field to Struct
Configure the ClassRegistrationInfo struct for whatever you're planning on doing (or something else if that's what you want to do), and make sure it is updated based on the new field.
In the GodotClass derive macro: detect when this new field should be added

Then the two diverge, for the old implementation:
4. Add a variable for the new field, and set it to None or Some depending step 3
5. Set the field like new_field: #new_field in the constructor

For the new implementation:
4. Add a builder-method for the new field. (Struct derives Default so it'll automatically be set in the constructor, you can override it if desired).
5. Do modifiers.push(quote! { new_field }) (possibly with turbofish) depending on step 3

How you choose to split these up into individual steps exactly is a matter of taste, but it's not a big difference to me at least.

Bromeon

Looks good to me!

Since you mentioned this might conflict with #998, could you coordinate the two?

To me it doesn't matter which one is merged first, but since this PR is quite foundational and has high conflict risk for any changes in the plugin registration, it would be good to merge it soon. If #998 takes more than a couple of days, I'd say we merge this first and address the conflicts in that other PR. Makes sense?

Bromeon · 2025-01-19T13:20:55Z

godot-core/src/registry/plugin.rs

-    // Wrapper needed because Debug can't be derived on function pointers with reference parameters, so this won't work:
-    // pub type ErasedRegisterFn = fn(&mut dyn std::any::Any);
+    // A wrapper is needed here because Debug can't be derived on function pointers with reference parameters, so this won't work:
+    // `pub type ErasedRegisterFn = fn(&mut dyn std::any::Any);``


Two backticks at the end.

Also, we don't handle backticking in // comments very consistently, and I don't think it should follow the same rules -- as these comments are never formatted, a backtick doesn't help much with readability (unless it's maybe inline text, where it's unclear which are code symbols, but even there that's often clear from the context).

For separate lines that show code, I wouldn't backtick them. If there's a bigger code block, having empty lines in between is more effective.

(Also below).

i like having them there but i also dont really mind either way, so i removed them

Make constructors + builders for the structs Move more things out of proc-macros and into struct constructors/builders Rework the safety invariants of `godot_dyn` Document lots of these apis better

Bromeon · 2025-01-21T18:36:29Z

Thank you!

lilizoey added quality-of-life No new functionality, but improves ergonomics/internals c: register Register classes, functions and other symbols to GDScript labels Jan 6, 2025

lilizoey force-pushed the refactor/plugin-item branch 2 times, most recently from d1f40a9 to bfe2b8d Compare January 6, 2025 19:57

Bromeon reviewed Jan 6, 2025

View reviewed changes

lilizoey force-pushed the refactor/plugin-item branch from bfe2b8d to 76ffad8 Compare January 6, 2025 20:31

lilizoey mentioned this pull request Jan 7, 2025

Implement Var and Export for DynGd<T, D> #998

Merged

lilizoey force-pushed the refactor/plugin-item branch from 76ffad8 to 030c7b0 Compare January 8, 2025 16:51

Bromeon approved these changes Jan 12, 2025

View reviewed changes

lilizoey force-pushed the refactor/plugin-item branch 2 times, most recently from 7dab4fd to 9f0f65b Compare January 18, 2025 18:29

Bromeon reviewed Jan 19, 2025

View reviewed changes

Split PluginItem into separate structs per variant

5cd8fa1

Make constructors + builders for the structs Move more things out of proc-macros and into struct constructors/builders Rework the safety invariants of `godot_dyn` Document lots of these apis better

lilizoey force-pushed the refactor/plugin-item branch from 9f0f65b to 5cd8fa1 Compare January 21, 2025 15:06

Bromeon approved these changes Jan 21, 2025

View reviewed changes

Bromeon added this pull request to the merge queue Jan 21, 2025

Merged via the queue into godot-rust:master with commit 97b66ec Jan 21, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Document and refactor `PluginItem` related stuff #1003

Document and refactor `PluginItem` related stuff #1003

lilizoey commented Jan 6, 2025

GodotRust commented Jan 6, 2025

Bromeon left a comment

Bromeon Jan 6, 2025

Bromeon Jan 6, 2025

Bromeon Jan 6, 2025

Bromeon Jan 6, 2025

lilizoey Jan 7, 2025

Bromeon Jan 6, 2025

Bromeon Jan 6, 2025

Bromeon Jan 6, 2025

Bromeon Jan 6, 2025

lilizoey Jan 6, 2025

lilizoey Jan 7, 2025

Bromeon Jan 6, 2025

Bromeon Jan 6, 2025

Bromeon Jan 6, 2025

lilizoey Jan 6, 2025

lilizoey commented Jan 6, 2025 •

edited

Loading

Bromeon left a comment

Bromeon Jan 19, 2025

lilizoey Jan 21, 2025

Bromeon commented Jan 21, 2025

		/// This only looks for an `AsDyn<D>` implementation in the dynamic class, the conversion will fail if the dynamic class doesn't
		/// implement `AsDyn<D>` even if there exists some superclass that does implement `AsDyn<D>`. This restriction could in theory be

	/// This will fail if the dynamic class of `object` does not match the [`ClassName`] stored in `self`.
	/// This will fail with `Err(object)` if the dynamic class of `object` does not match the [`ClassName`] stored in `self`.

Document and refactor PluginItem related stuff #1003

Document and refactor PluginItem related stuff #1003

Conversation

lilizoey commented Jan 6, 2025

GodotRust commented Jan 6, 2025

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilizoey commented Jan 6, 2025 • edited Loading

Bromeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon commented Jan 21, 2025

Document and refactor `PluginItem` related stuff #1003

Document and refactor `PluginItem` related stuff #1003

lilizoey commented Jan 6, 2025 •

edited

Loading