Use the class_name for the field when building PropertyInfo #441

HenryWConklin · 2023-10-05T22:24:58Z

Godot uses the class_name to construct an instance of the right type in a few places, in particular for the PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT flag.

Partial fix for #440

GodotRust · 2023-10-06T08:40:55Z

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

HenryWConklin · 2023-10-06T13:41:24Z

Getting some new errors in unchanged files with the update to rust 1.73.0. Added fixes in a new commit, let me know if you want me to squash them or move it to a separate PR.

Bromeon

Thanks! 🙂

The logic around the actual bugfix is solely in property.rs, if I see correctly?
Could you elaborate what precisely was broken and how it's fixed?

Clippy fixes are fine as-is in a separate commit 👍

Bromeon · 2023-10-06T18:16:31Z

godot-core/src/builtin/macros.rs

@@ -68,10 +68,10 @@ macro_rules! impl_builtin_traits_inner {
        impl Eq for $Type {}
    };

-    ( PartialOrd for $Type:ty => $gd_method:ident ) => {


Nice improvement!

Thanks! Clippy actually flagged the whole PartialOrd implementation as "incorrect" because there was an implementation for Ord which was interesting.

Bromeon · 2023-10-06T18:19:58Z

itest/rust/src/object_tests/property_test.rs

+#[godot_api]
+impl ResourceVirtual for CustomResource {
+    fn init(_base: godot::obj::Base<Self::Base>) -> Self {
+        Self {}
+    }
+}


Such blocks can be replaced with #[class(..., init)] (also below).

Bromeon · 2023-10-06T18:32:56Z

itest/rust/src/object_tests/property_test.rs

+    // `class_name` should be empty for non-Object variants.
+    assert_eq!(
+        property.get_or_nil("class_name"),
+        "CustomResource".to_variant()
+    );
+    assert_eq!(
+        property.get_or_nil("type"),
+        (VariantType::Object as i32).to_variant()
+    );
+    assert_eq!(
+        property.get_or_nil("hint"),
+        (PropertyHint::PROPERTY_HINT_RESOURCE_TYPE.ord()).to_variant()
+    );
+    assert_eq!(
+        property.get_or_nil("hint_string"),
+        "CustomResource".to_variant()
+    );
+    assert_eq!(
+        property.get_or_nil("usage"),
+        (PropertyUsageFlags::PROPERTY_USAGE_DEFAULT.ord()
+            | PropertyUsageFlags::PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT.ord())
+        .to_variant()
+    );
+
+    let property = class
+        .get_property_list()
+        .iter_shared()
+        .find(|c| c.get_or_nil("name") == "bar".to_variant())
+        .unwrap();
+    // `class_name` should be empty for non-Object variants.
+    assert_eq!(
+        property.get_or_nil("class_name"),
+        "NewNameCustomResource".to_variant()
+    );
+    assert_eq!(
+        property.get_or_nil("type"),
+        (VariantType::Object as i32).to_variant()
+    );
+    assert_eq!(
+        property.get_or_nil("hint"),
+        (PropertyHint::PROPERTY_HINT_RESOURCE_TYPE.ord()).to_variant()
+    );
+    assert_eq!(
+        property.get_or_nil("hint_string"),
+        "NewNameCustomResource".to_variant()
+    );
+    assert_eq!(
+        property.get_or_nil("usage"),
+        PropertyUsageFlags::PROPERTY_USAGE_DEFAULT
+            .ord()
+            .to_variant()
+    );


You have a lot of code following the same pattern, which can be extracted into a function.

fn check_property(property: &Dictionary, key: &str, expected: impl ToVariant) { assert_eq!(property.get_or_nil(key), expected.to_variant()); }

This would then simply the code to:

Suggested change

// `class_name` should be empty for non-Object variants.

assert_eq!(

property.get_or_nil("class_name"),

"CustomResource".to_variant()

);

assert_eq!(

property.get_or_nil("type"),

(VariantType::Object as i32).to_variant()

);

assert_eq!(

property.get_or_nil("hint"),

(PropertyHint::PROPERTY_HINT_RESOURCE_TYPE.ord()).to_variant()

);

assert_eq!(

property.get_or_nil("hint_string"),

"CustomResource".to_variant()

);

assert_eq!(

property.get_or_nil("usage"),

(PropertyUsageFlags::PROPERTY_USAGE_DEFAULT.ord()

| PropertyUsageFlags::PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT.ord())

.to_variant()

);

let property = class

.get_property_list()

.iter_shared()

.find(|c| c.get_or_nil("name") == "bar".to_variant())

.unwrap();

// `class_name` should be empty for non-Object variants.

assert_eq!(

property.get_or_nil("class_name"),

"NewNameCustomResource".to_variant()

);

assert_eq!(

property.get_or_nil("type"),

(VariantType::Object as i32).to_variant()

);

assert_eq!(

property.get_or_nil("hint"),

(PropertyHint::PROPERTY_HINT_RESOURCE_TYPE.ord()).to_variant()

);

assert_eq!(

property.get_or_nil("hint_string"),

"NewNameCustomResource".to_variant()

);

assert_eq!(

property.get_or_nil("usage"),

PropertyUsageFlags::PROPERTY_USAGE_DEFAULT

.ord()

.to_variant()

);

// `class_name` should be empty for non-Object variants.

check_property(&property, "class_name", "CustomResource");

check_property(&property, "type", VariantType::Object as i32);

check_property(&property, "hint", PropertyHint::PROPERTY_HINT_RESOURCE_TYPE.ord());

check_property(&property, "hint_string", "CustomResource");

check_property(&property, "usage", PropertyUsageFlags::PROPERTY_USAGE_DEFAULT.ord()

| PropertyUsageFlags::PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT.ord());

let property = class

.get_property_list()

.iter_shared()

.find(|c| c.get_or_nil("name") == "bar".to_variant())

.unwrap();

// `class_name` should be empty for non-Object variants.

check_property(&property, "class_name", "NewNameCustomResource");

check_property(&property, "type", VariantType::Object as i32);

check_property(&property, "hint", PropertyHint::PROPERTY_HINT_RESOURCE_TYPE.ord());

check_property(&property, "hint_string", "NewNameCustomResource");

check_property(&property, "usage", PropertyUsageFlags::PROPERTY_USAGE_DEFAULT.ord());

(Maybe check for mistakes)

Godot uses the class_name to construct an instance of the right type in a few places, in particular for the PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT flag.

HenryWConklin · 2023-10-06T19:26:54Z

Yeah, the fix is just the changes in property.rs. There are some more links to code and things in the issue, but the gist is Godot does a ClassDB::instantiate(property_info.class_name) to make a value to fill in a field when you use the PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT flag. Currently that class_name is set to the name of the containing class, but should be set to the name of the property's type. From the test I added, it used to be "ExportResource" for both fields, now it's "CustomResource"/"NewNameCustomResource".

There might be other places where this comes up but that's the case I was running into.

Bromeon

One small comment, but it's not important, we can clean this up once we fix the bug to remove empty impls.

Thanks a lot for this! You said it fixes #440 only partially, so I guess that issue needs to stay open?

Bromeon · 2023-10-06T19:25:42Z

itest/rust/src/object_tests/property_test.rs

+#[godot_api]
+impl ResourceVirtual for CustomResource {}


The whole impl is not needed anymore 😉

(the inherent impl above is still required due to a bug, though...)

Neat, when did that happen? Thought I was getting errors for not having that pretty recently

The inherent one is still needed, but not the trait one.

HenryWConklin · 2023-10-06T19:32:10Z

Yeah #440 should stay open. There's another issue with exported Gd<Resource> fields triggering a bug in the Godot editor doesn't have as obvious of a fix.

Bromeon mentioned this pull request Oct 6, 2023

Fix #[func]-like attrs being ignored with macros below them #442

Merged

Bromeon reviewed Oct 6, 2023

View reviewed changes

Bromeon added bug c: register Register classes, functions and other symbols to GDScript labels Oct 6, 2023

HenryWConklin added 2 commits October 6, 2023 15:19

Use the class_name for the field when building PropertyInfo

57ad44e

Godot uses the class_name to construct an instance of the right type in a few places, in particular for the PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT flag.

Clippy fixes for Rust v1.73.0

f7217c2

HenryWConklin force-pushed the master branch from b6bcada to f7217c2 Compare October 6, 2023 19:20

Bromeon approved these changes Oct 6, 2023

View reviewed changes

Bromeon added this pull request to the merge queue Oct 6, 2023

Merged via the queue into godot-rust:master with commit de73f39 Oct 6, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the class_name for the field when building PropertyInfo #441

Use the class_name for the field when building PropertyInfo #441

HenryWConklin commented Oct 5, 2023

GodotRust commented Oct 6, 2023

HenryWConklin commented Oct 6, 2023

Bromeon left a comment

Bromeon Oct 6, 2023

HenryWConklin Oct 6, 2023

Bromeon Oct 6, 2023

Bromeon Oct 6, 2023 •

edited

Loading

HenryWConklin commented Oct 6, 2023

Bromeon left a comment

Bromeon Oct 6, 2023

HenryWConklin Oct 6, 2023

Bromeon Oct 6, 2023

HenryWConklin commented Oct 6, 2023 •

edited

Loading

Use the class_name for the field when building PropertyInfo #441

Use the class_name for the field when building PropertyInfo #441

Conversation

HenryWConklin commented Oct 5, 2023

GodotRust commented Oct 6, 2023

HenryWConklin commented Oct 6, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Oct 6, 2023

Choose a reason for hiding this comment

HenryWConklin Oct 6, 2023

Choose a reason for hiding this comment

Bromeon Oct 6, 2023

Choose a reason for hiding this comment

Bromeon Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

HenryWConklin commented Oct 6, 2023

Bromeon left a comment

Choose a reason for hiding this comment

Bromeon Oct 6, 2023

Choose a reason for hiding this comment

HenryWConklin Oct 6, 2023

Choose a reason for hiding this comment

Bromeon Oct 6, 2023

Choose a reason for hiding this comment

HenryWConklin commented Oct 6, 2023 • edited Loading

Bromeon Oct 6, 2023 •

edited

Loading

HenryWConklin commented Oct 6, 2023 •

edited

Loading