Issues exporting resources from Rust custom types

[HenryWConklin]

TL;DR: #[export] Gd<Foo> with non-null default causes bugs in the Godot editor, the fix using PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT is using the wrong type to initialize the field.

Using a non-null resource reference causes a bug with defaults in the Godot editor

#[derive(GodotClass)]
#[class(init, base=Node)]
struct ExampleNode {
    #[init(default=(Curve2D::new()))]
    #[export]
    foo: Gd<Curve2D>
}

#[godot_api]
impl NodeVirtual for ExampleNode {}

#[godot_api]
impl ExampleNode {}

Gives this warning sporadically while an instance of the custom class is in the tree:

core/object/class_db.cpp:1642 - Instantiated Curve2D used as default value for ExampleNode's "foo" property.

Which points to this issue in Godot where this caused issues with using the 'reset' button on internal types in the editor: godotengine/godot#36372

The same bug from that issue happens with the example custom type above.

Warning message from: https://github.com/godotengine/godot/blob/bfd78bb917887cfc1fd842ba23570394cad8bedb/core/object/class_db.cpp#L1723

Leads to PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT implemented here: https://github.com/godotengine/godot/blob/bfd78bb917887cfc1fd842ba23570394cad8bedb/editor/editor_data.cpp#L588

Core issue is that Godot seems to use the value of a newly instantiated object's field as the default value for that field, meaning Object fields that default to an instance will have that specific instance used as a default for resetting the field. So resetting the field may reset to a dirty object and have strange behavior. This is sort of similar to the footgun in Python with default parameters (e.g. def foo(x=[]): uses the same list for every call that uses the default value for the parameter).

For an extreme opinion: non-nullable exported Gd<Foo> fields seem like a bug and should be a compile-time warning/error. The specific combo seems to be all of:

• Type that maps to an Object*, e.g. Gd<Foo> or Option<Gd<Foo>>
• Exported
• With PROPERTY_USAGE_EDITOR on the usage flags (one of the defaults)
• A non-null default value, so Gd::new_default() or anything other than None on an Option<Gd<Foo>>. (Specifically, I think Godot needs to get a null whenever it makes a new instance to check for the default values so a custom getter that always returns non-null can trigger this too)

Preferred way to default to something other than null should be:

#[export]
#[var(usage_flags=[PROPERTY_USAGE_DEFAULT, PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT])]
foo: Option<Gd<ExampleResource>>

The field can still be set to null, but will at least start non-null when created in the editor.

PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT is broken for custom Rust types

Enabling PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT leads to weird Rust panics when creating custom nodes with custom types in the editor:

#[derive(GodotClass)]
#[class(init, base=Resource)]
struct ExampleResource {
    #[export]
    field: i32 
}

#[godot_api]
impl ResourceVirtual for ExampleResource {}

#[godot_api]
impl ExampleResource {}

#[derive(GodotClass)]
#[class(init, base=Node)]
struct ExampleNode {
    #[export]
    #[var(usage_flags=[PROPERTY_USAGE_DEFAULT, PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT])]
    foo: Option<Gd<ExampleResource>>
}

#[godot_api]
impl NodeVirtual for ExampleNode {}

#[godot_api]
impl ExampleNode {}

Gives this error:

$HOME/.cargo/git/checkouts/gdext-76630c89719e160c/88a7934/godot-core/src/lib.rs:154 - Rust function panicked in file $HOME/.cargo/git/checkouts/gdext-76630c89719e160c/88a7934/godot-core/src/builtin/meta/signature.rs at line 443. Context: set_foo
$HOME/cargo/git/checkouts/gdext-76630c89719e160c/88a7934/godot-core/src/lib.rs:102 - Panic msg:  set_foo: parameter [0] has type core::option::Option<godot_core::obj::gd::Gd<example::ExampleResource>>, which is unable to store argument Variant(ty=Object, val=<ExampleNode#1859704123003>)

Note that it's trying to set the ExampleResource field to an ExampleNode. Looking at where the PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT logic is done, it uses the class_name field of a PropertyInfo struct.
Looking at the constructor for that it uses the hint_string for resource types on one constructor, but there's a separate one for GdExtension below which does not and just copies the class_name over: https://github.com/godotengine/godot/blob/bfd78bb917887cfc1fd842ba23570394cad8bedb/core/object/object.h#L174

Looking at the Rust gdext macro implementation, the hint_string is set to the name of the resource class:

gdext/godot-core/src/obj/gd.rs

Lines 763 to 767 in 88a7934

	// Godot does this by default too; the hint is needed when the class is a resource/node,
	// but doesn't seem to make a difference otherwise.
	let hint_string = T::class_name().to_godot_string();
	ExportInfo { hint, hint_string }

But I think that class_name field is set here:

gdext/godot-macros/src/class/data_models/property.rs

Line 182 in 88a7934

class_name: #class_name_obj,

Which comes from the class name of the object that contains the property not the type of the property itself. I believe that should be set to the class name of the property's type instead, and doing that should fix the behavior with PROPERTY_USAGE_EDITOR_INSTANTIATE_OBJECT and make it possible for Option<Gd<Foo>> fields to 'default' to something other than None.

[HenryWConklin]

Happy to take a stab at the second issue. I think the first one is more up to opinion so probably needs more discussion.

[lilizoey]

The second issue is probably just a mistake someone made yeah.

For the first one we should probably try to mirror whatever GDScript does if GDScript's defaults here aren't too weird.

[HenryWConklin]

Similar issue in #283 wrt nulls in Godot causing problems with Gd<Foo> exports

[HenryWConklin]

Did some experiments with GDScript and it looks like it handles non-null default values just fine. If you put an initializer on a field it gets set in some step before _init even runs. If you set another value in the editor, it shows up in init with the default set in GDScript, then gets set afterward.

extends Node
class_name ObjNullDefault

@export
var no_default: Resource : 
	set(value):
		print("set, old value, no_default: ", no_default)
		print("set, new value, no_default: ", value)
		no_default = value
	get:
		print("get, no_default: ", no_default)
		return no_default

@export
var with_default: Resource = Resource.new() :
	set(value):
		print("set, old value, with_default: ", with_default)
		print("set, new value, with_default: ", value)
		with_default = value
	get:
		print("get, with_default: ", with_default)
		return with_default

func _init():
	print("_init")
	print("_init, no_default: ", no_default)
	print("_init, with_default: ", with_default)

Leaving the field alone in the editor and running:

_init
get, no_default: <null>
_init, no_default: <null>
get, with_default: <Resource#-9223372012997573485>
_init, with_default: <Resource#-9223372012997573485>

Setting a value in the editor and running;

_init
get, no_default: <null>
_init, no_default: <null>
get, with_default: <Resource#-9223372012980796269>
_init, with_default: <Resource#-9223372012980796269>
get, with_default: <Resource#-9223372012980796269>
set, old value, with_default: <Resource#-9223372012980796269>
set, new value, with_default: <Image#-9223372013081459562>

Digging through the Godot code I came across property_can_revert/property_get_revert which look like they get checked first before going to the "make an instance and cache whatever is in the field" strategy. Here's what looks like the main place that gets determined by the editor:

https://github.com/godotengine/godot/blob/6916349697a4339216469e9bf5899b983d78db07/editor/editor_inspector.cpp#L455-L464

Part of the GDExtensionBindings that defines them:

https://github.com/godotengine/godot/blame/6916349697a4339216469e9bf5899b983d78db07/core/extension/gdextension_interface.h#L303-L304

So with those implemented it might be fine to have a non-null default, though there's a lot of places in the Godot codebase that use the cached instance default so it's probably a "try it and see" situation plus maybe an upstream patch. Also not clear how you would generate that implementation without requiring the generated init.

[lokimckay]

Additionally, trying to use hint = RESOURCE_TYPE and hint_string = "Texture2D" to limit the selectable resources in the editor does not work (all resource types are shown)

I'm not sure if this is a separate issue or simply a result of the ones mentioned in this thread - does anyone know?

[Bromeon]

Is this problem still present on latest master?
If yes, does anyone feel like giving it a try? 🙂