Fix `DynGd<T,D>` export, implement proper export for `Array<DynGd<T,D>>` #1056

Yarwin · 2025-02-22T16:15:13Z

Implement element_type_string for DynGd<T, D>.
Extract object_export_element_type_string from Gd<T>'s ArrayElement` to share implementation between both.
Include parent_class in DynTrait registration info
#[export] for DynGd<T, D> works as it would be Gd<T> for Node-based classes – Godot doesn't allow to export multiple node types via editor.
#[export] for DynGd<T, D> works the same for resource-based classes.
Don't include non-resource based classes while creating hint_string for resources in debug mode.

Manual testing:

Given test library:

Test library

struct MyExtension;

#[gdextension]
unsafe impl ExtensionLibrary for MyExtension {}

trait ExampleTrait {
    fn dumb_method(&self) {
        godot_print!("Hello, I'm implementor of Example Trait");
    }
}

#[derive(GodotClass)]
#[class(init, base = Node)]
pub struct Collector {
    #[export]
    node: Option<DynGd<Node2D, dyn ExampleTrait>>,
    #[export]
    nodes: Array<DynGd<Node, dyn ExampleTrait>>,
    #[export]
    resources: Array<DynGd<Resource, dyn ExampleTrait>>,
}

#[godot_api]
impl INode for Collector {
    fn ready(&mut self) {
        godot_print!("{}", self.nodes);
        godot_print!("{}", self.resources);

        for n in self.nodes.iter_shared() {
            n.dyn_bind().dumb_method();
        }
        for r in self.resources.iter_shared() {
            r.dyn_bind().dumb_method();
        }
    }
}

#[derive(GodotClass)]
#[class(init, base = Node)]
pub struct A {}

#[godot_dyn]
impl ExampleTrait for A {}

#[derive(GodotClass)]
#[class(init, base = Resource)]
pub struct AR {}

#[godot_dyn]
impl ExampleTrait for AR {}

#[derive(GodotClass)]
#[class(init, base = Node)]
pub struct B {}

#[godot_dyn]
impl ExampleTrait for B {}

#[derive(GodotClass)]
#[class(init, base = Resource)]
pub struct BR {}

#[godot_dyn]
impl ExampleTrait for BR {}

#[derive(GodotClass)]
#[class(init, base = Node)]
pub struct C {}

#[godot_dyn]
impl ExampleTrait for C {}

#[derive(GodotClass)]
#[class(init, base = Resource)]
pub struct CR {}

#[godot_dyn]
impl ExampleTrait for CR {}


#[derive(GodotClass)]
#[class(init, base = Node2D)]
pub struct A2D {}

#[godot_dyn]
impl ExampleTrait for A2D {}

One is able to get desired results:

manual testing

Exporting non-dynGd node yields proper error on runtime:

E 0:00:00:0344   godot_core::private::report_call_error: godot-rust function call failed: Collector::ready()
    Reason: [panic]  FromGodot::from_variant() failed -- none of the classes derived from `Node` have been linked to trait `dyn ExampleTrait` with #[godot_dyn]: Gd { id: 24998053138, class: Node }
  at /home/irwin/apps/godot/opensource-contr/missing_docs/gdext/godot-core/src/meta/godot_convert/mod.rs:106
  <C++ Source>   /home/irwin/apps/godot/opensource-contr/missing_docs/gdext/godot-core/src/private.rs:336 @ godot_core::private::report_call_error()

[A:<A#24947721483>, B:<B#24964498700>, C:<C#24981275921>, A2D:<A2D#25014830355>]
[<AR#-9223372012007717619>, <AR#-9223372012058049266>]
Hello, I'm implementor of Example Trait
Hello, I'm implementor of Example Trait
Hello, I'm implementor of Example Trait
Hello, I'm implementor of Example Trait
Hello, I'm implementor of Example Trait
Hello, I'm implementor of Example Trait

~~For some reason~~ ClassDb might have not all the GDExtension classes loaded while generating hint_string for our exports – thus the fallback to registering parent_class while registering the class itself (built-in classes will always be accessible via ClassDb):

ol'reliable godot_print! debugging

EDIT: It seems that given PropertyHintInfo is being generated before all the GDExtension classes are properly registered in Godot, I'm not sure if there is any sane way – outside of applied workaround – to deal with this issue 🤷

GodotRust · 2025-02-22T16:25:47Z

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

Yarwin · 2025-02-22T17:01:29Z

Draft - allegedly there are some problems with exporting Nodes in Array (it allows to export only the very first one) which I have to investigate further.

Yarwin · 2025-02-23T12:17:02Z

One open issue – should we support something among the lines of

    #[export]
    nodes_concrete: Array<DynGd<SomeUserDeclaredGDExtensionNode, dyn ExampleTrait>>,
    #[export]
    resources_concrete: Array<DynGd<SomeUserDeclaredGDExtensionResource, dyn ExampleTrait>>,

?

I don't see reason to do so, since… we can just export Array<Gd<SomeUserDeclaredGDExtensionNode>> or Array<Gd<SomeUserDeclaredGDExtensionResource>> 🤔. For now I disallowed using concrete GDExtension user-defined classes as a base for DynGd's arrays and #[export].

Might be revisited after inheritance will be added to GDExtension.

Yarwin · 2025-02-24T09:02:49Z

godot-core/src/registry/class.rs

    D: ?Sized + 'static,
 {
+    // Exporting multiple node types is not supported.
+    if T::inherits::<classes::Node>() {


I'm not a fan of these multiple T::inherits:: checks when we are exporting Array<DynGd<...>> but it is rarely called (mostly in the editor), so we should be fine?

I can write independent implementation for all the cases (exporting DynGd, exporting Array<DynGd>, and finally Array<Gd>)

To understand, what is your concern here? That T::inherits::<classes::Node>() is a runtime check as opposed to trait static dispatch?

Because I would expect the compiler to be reasonably good at optimizing this, as all is known at compile time 🤔

hmm, that's true – it shouldn't be performance bottleneck in any capacity 🤔

Yeah, it's just much nicer to write procedural code than factor this out into multiple traits. The latter is often necessary because of bounds or because code wouldn't compile otherwise; not so here.

Same reason why C++ added if constexpr (aka "static if").

(Maybe there's an easy way to "cache" node/resource deriving, but I'd keep this separate from this PR)

Bromeon · 2025-02-24T09:43:09Z

godot-core/src/registry/class.rs

    D: ?Sized + 'static,
 {
+    // Exporting multiple node types is not supported.
+    if T::inherits::<classes::Node>() {


To understand, what is your concern here? That T::inherits::<classes::Node>() is a runtime check as opposed to trait static dispatch?

Because I would expect the compiler to be reasonably good at optimizing this, as all is known at compile time 🤔

Bromeon · 2025-02-24T09:44:08Z

godot-core/src/registry/class.rs

+    #[cfg(debug_assertions)]
+    let relations_iter = relations_iter.filter_map(|implementor| {
+        if classes::is_derived_base_cached(implementor.parent_class_name?, T::class_name()) {
+            Some(implementor)
+        } else {
+            None
+        }
+    });


Could you add a comment what this does?

Bromeon · 2025-02-24T09:45:22Z

godot-core/src/registry/class.rs

@@ -244,7 +243,8 @@ fn register_classes_and_dyn_traits(
        let metadata = ClassMetadata {};

        // Transpose Class->Trait relations to Trait->Class relations.
-        for (trait_type_id, dyn_trait_impl) in info.dynify_fns_by_trait.drain() {
+        for (trait_type_id, mut dyn_trait_impl) in info.dynify_fns_by_trait.drain() {
+            dyn_trait_impl.parent_class_name = info.parent_class_name;


Was this part of the bug, or why wasn't parent_class_name set already correctly in info?

We don't have access to parent_class_name while registering given DynTraitImpl (see: godot-macros/src/class/godot_dyn.rs) thus it must be copied over.

But why wasn't this present before this PR? Bug? 🙂

And could you add a short comment above that line, writing that it's only available now and not when registering DynTraitImpl?

But why wasn't this present before this PR

To make it clear – it is name of the base inherited by given rust class not the name of said class; It haven't been included earlier, because there was no need to do so 🤷.

I decided to extended the docs anyway:

docstring for parent_class_name tells us why do we need Base ClassName at all

gdext/godot-core/src/registry/plugin.rs

Lines 514 to 522 in 58aca38

/// Base inherited class required for `DynGd<T, D>` exports (i.e. one specified in `#[class(base = ...)]`).

///

/// Godot doesn't guarantee availability of all the GDExtension classes through the ClassDb while generating `PropertyHintInfo` for our exports.

/// Therefore, we rely on the built-in inherited base class in such cases.

/// Only [`class_name`][DynTraitImpl::class_name] is available at the time of adding given `DynTraitImpl` to plugin registry with `#[godot_dyn]`;

/// It is important to fill this information before registration.

///

/// See also [`get_dyn_property_hint_string`][crate::registry::class::get_dyn_property_hint_string].

pub(crate) parent_class_name: Option<ClassName>,

I also included the info why we are filling this info so late (mildly important, since #[export]s are impossible to test and filling it with rest of the class info works more often than not 😄)

gdext/godot-core/src/registry/class.rs

Line 247 in c526f69

// Note: Must be done after filling out the class info since plugins are being iterated in unspecified order.

Bromeon · 2025-02-24T09:45:50Z

godot-core/src/obj/dyn_gd.rs

+    fn element_type_string() -> String {
+        object_export_element_type_string::<T, String>(get_dyn_property_hint_string::<T, D>())
+    }


Maybe declare intermediate variable for the inner expr

Bromeon · 2025-02-24T15:33:59Z

godot-core/src/registry/property.rs

+/// Creates `hint_string` to be used for given `GodotClass` when used as an `ArrayElement`.
+pub(crate) fn object_export_element_type_string<T, S>(class_hint: S) -> String
+where
+    T: GodotClass,
+    S: Display,
+{


If you use impl Display instead of a named generic parameter S, the call site won't have to specify the type of the 2nd generic argument.

Bromeon · 2025-02-24T15:35:58Z

itest/rust/src/object_tests/dyn_gd_test.rs

@@ -515,7 +515,7 @@ struct RefcDynGdExporter {
    #[var]
    first: Option<DynGd<Object, dyn Health>>,
    #[export]
-    second: Option<DynGd<foreign::NodeHealth, dyn InstanceIdProvider<Id = InstanceId>>>,
+    second: Option<DynGd<Node, dyn InstanceIdProvider<Id = InstanceId>>>,


With concrete types being allowed again, this could again be foreign::NodeHealth.

Maybe add a comment before #[export]:

// Using DynGd with concrete type foreign::NodeHealth doesn't give benefits over Gd, but is allowed.

- Add base inherited class to DynTraitImpl - it is necessary due to lack of guaranteed order of registering classes (meaning that we are unable to check if given GDExtension class inherits the other – since it might not be loaded yet) . - Implement `element_type_string` for `DynGd<T, D>` ArrayElement. - Generate proper hint string while exporting Resource-based `DynGd<T, D>`. Don't include classes that don't inherit the base class T (for example Objects/Nodes for `DynGd<Resource, D>`). - Use base class while exporting Node-based `DynGd<T, D>`. In other words – `#[export] DynGd<T, D>` and `#[export] Gd<T>` works identically editor-wise. - 2.0 compatibility – allow to use `DynGd<MyRustClass, D>` and `Array<DynGd<MyRustClass, D>>` even if that doesn't make sense semantically (just use Gd<MyRustClass> instead). - Extract `object_export_element_type_string` from `Gd<T>`'s ArrayElement` to share implementation between both.

Bromeon

Thanks a lot!

Yarwin added the bug label Feb 22, 2025

Yarwin changed the title ~~Fix: Allow to export Array<DynGd<T,D>>~~ Fix Array<DynGd<T,D>> export Feb 22, 2025

Yarwin marked this pull request as draft February 22, 2025 16:52

Bromeon added the c: register Register classes, functions and other symbols to GDScript label Feb 22, 2025

Yarwin mentioned this pull request Feb 23, 2025

Implement the #[export] for Array<DynGd<D, T>> #1057

Closed

Yarwin linked an issue Feb 23, 2025 that may be closed by this pull request

Implement the #[export] for Array<DynGd<D, T>> #1057

Closed

Yarwin force-pushed the bugfix-fix-array-export-for-dyn-gd branch from 16149b7 to 2388568 Compare February 23, 2025 11:55

Yarwin marked this pull request as ready for review February 23, 2025 12:06

Yarwin marked this pull request as draft February 23, 2025 12:14

Yarwin marked this pull request as ready for review February 23, 2025 12:17

Yarwin changed the title ~~Fix Array<DynGd<T,D>> export~~ Fix DynGd<T,D> export, implement proper export for Array<DynGd<T,D>> Feb 23, 2025

Yarwin commented Feb 24, 2025

View reviewed changes

Bromeon reviewed Feb 24, 2025

View reviewed changes

Yarwin force-pushed the bugfix-fix-array-export-for-dyn-gd branch 4 times, most recently from ab0a097 to e67d86f Compare February 24, 2025 17:22

Yarwin force-pushed the bugfix-fix-array-export-for-dyn-gd branch from e67d86f to dcbaca9 Compare February 24, 2025 17:23

Bromeon approved these changes Feb 24, 2025

View reviewed changes

Bromeon added this pull request to the merge queue Feb 24, 2025

Bromeon added this to the 0.2.x milestone Feb 24, 2025

Merged via the queue into godot-rust:master with commit 5b09d29 Feb 24, 2025
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `DynGd<T,D>` export, implement proper export for `Array<DynGd<T,D>>` #1056

Fix `DynGd<T,D>` export, implement proper export for `Array<DynGd<T,D>>` #1056

Yarwin commented Feb 22, 2025 •

edited

Loading

GodotRust commented Feb 22, 2025

Yarwin commented Feb 22, 2025

Yarwin commented Feb 23, 2025 •

edited

Loading

Yarwin Feb 24, 2025 •

edited

Loading

Bromeon Feb 24, 2025

Yarwin Feb 24, 2025

Bromeon Feb 24, 2025 •

edited

Loading

Bromeon Feb 24, 2025

Bromeon Feb 24, 2025

Bromeon Feb 24, 2025

Yarwin Feb 24, 2025

Bromeon Feb 24, 2025

Yarwin Feb 24, 2025 •

edited

Loading

Bromeon Feb 24, 2025

Bromeon Feb 24, 2025

Bromeon Feb 24, 2025

Bromeon left a comment

	/// Base inherited class required for `DynGd<T, D>` exports (i.e. one specified in `#[class(base = ...)]`).
	///
	/// Godot doesn't guarantee availability of all the GDExtension classes through the ClassDb while generating `PropertyHintInfo` for our exports.
	/// Therefore, we rely on the built-in inherited base class in such cases.
	/// Only [`class_name`][DynTraitImpl::class_name] is available at the time of adding given `DynTraitImpl` to plugin registry with `#[godot_dyn]`;
	/// It is important to fill this information before registration.
	///
	/// See also [`get_dyn_property_hint_string`][crate::registry::class::get_dyn_property_hint_string].
	pub(crate) parent_class_name: Option<ClassName>,

Fix DynGd<T,D> export, implement proper export for Array<DynGd<T,D>> #1056

Fix DynGd<T,D> export, implement proper export for Array<DynGd<T,D>> #1056

Conversation

Yarwin commented Feb 22, 2025 • edited Loading

GodotRust commented Feb 22, 2025

Yarwin commented Feb 22, 2025

Yarwin commented Feb 23, 2025 • edited Loading

Yarwin Feb 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bromeon Feb 24, 2025 • edited Loading

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

Yarwin Feb 24, 2025 • edited Loading

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

Bromeon left a comment

Choose a reason for hiding this comment

Fix `DynGd<T,D>` export, implement proper export for `Array<DynGd<T,D>>` #1056

Fix `DynGd<T,D>` export, implement proper export for `Array<DynGd<T,D>>` #1056

Yarwin commented Feb 22, 2025 •

edited

Loading

Yarwin commented Feb 23, 2025 •

edited

Loading

Yarwin Feb 24, 2025 •

edited

Loading

Bromeon Feb 24, 2025 •

edited

Loading

Yarwin Feb 24, 2025 •

edited

Loading