Support OnEditor<T>/Required<T>for #[export] similar to OnReady<T>

[Yarwin]

Currently the pattern for structs with Gd references to other nodes looks like:

#[derive(GodotClass)]
#[class(init, base=Node)]
pub struct SomeNode {
    #[export]
    required_node_a: Option<Gd<Node>>,
    #[export]
     required_node_b: Option<Gd<Node>>,
    #[export]
    optional_node: Option<Gd<Node>>,
    ...
}

#[godot_api]
impl INode for SomeNode {
    fn ready(&mut self) {
        let Some(required_node_a) = self.required_node_a.as_ref()  else { panic!("required node must be set!")};
        let Some(required_node_b) = self.required_node_b.as_ref()  else { panic!("required node must be set!")};
        if let Some(optional) = self.optional_node.as_ref() {
                godot_print!("optional node has been set!");
       }
    }
}

Thus forcing user to abuse the .unwrap().

Similar case in gdscript can be represented as:

extends Node

@export var required_node_a: Node
@export var required_node_b: Node
@export var optional_node: Node

func _ready() -> void:
	print(required_node_a.name)  # errors if required_node_a is not present
	print(required_node_b.name)  # errors if required_node_b is not present
	if optional_node:
		print(optional_node.name)

It would be great to create abstraction similar to OnReady<Gd> for required exports. The inner workings would be similar to OnReady, while the exported property itself could be represented by simple Required<Gd<Node>>.

[Bromeon]

Agree with this; I noticed the need for something like that and even started experimenting. One thing where I wasn't sure, and it's not obvious from your code either:

Should the initialization happen lazily or during ready? Because the latter would just be a special-cased OnReady<T> which also allows Export, no? Whereas the former could panic on Deref/DerefMut.

[Yarwin]

Personally I would prefer having it in ready – not supplying the required property is a logic error and user should be notified such. In broader context – I'm unsure if it wouldn't cause issues with various editor tools and plugins 🤷.

[Bromeon]

Is it guaranteed that #[export] fields are always initialized before ready()?
Also thinking of hot-reloading scenarios etc... 🤔

[Yarwin]

I did some digging and it seems that lazy Deref/DerefMut might be better option 🤔. I can move on to implementation if nobody has any objections

[Bromeon]

So the only reason for OnEditor to exist would be to avoid the explicit .as_ref().unwrap()?

I'm not saying that's not worth it -- I think these common cases should be as ergonomic as possible -- but I'd like to be clear about the goals 🙂 also then we can find the best possible name.

Furthermore, would this only exist for Gd<T> or also fields of other types?

[Yarwin]

So the only reason for OnEditor to exist would be to avoid the explicit .as_ref().unwrap()?

Yep. Moreover it would be handy if it could be used for Resources and Refcounted/Objects (almost half of my potential use cases are related to this).

Furthermore, would this only exist for Gd or also fields of other types?

For any T:ffi that implements GodotNullableFfi, 1:1 to option.

I have second thoughts though; it doesn't need to be a part of the core to work (can be external package instead), doesn't point to godot's abstraction directly (read - hard to discover) and implementing such wrapper for Option is trivial (shouldn't take more than 5 to 10 minutes? I pasted my wrapper below anyway).

here is my current wrapper, definitively naive (literally an Option<T> that deferences to T) albeit works.

pub struct Required<T> {
    inner: Option<T>
}

impl<T> Default for Required<T> {
    fn default() -> Self {
        Required { inner: None }
    }
}

impl<T> std::ops::Deref for Required<T>
{
    type Target = T;
    fn deref(&self) -> &Self::Target {
        match &self.inner {
            None => panic!(),
            Some(v) => v
        }
    }
}

impl<T> std::ops::DerefMut for Required<T>
{
    fn deref_mut(&mut self) -> &mut Self::Target {
        match &mut self.inner {
            None => panic!(),
            Some(v) => v
        }
    }
}


impl<T: GodotConvert> GodotConvert for Required<T>
    where
        Option<T::Via>: GodotType,
{
    type Via = Option<T::Via>;
}

impl<T: ToGodot> ToGodot for Required<T>
    where
        Option<T::Via>: GodotType,
{
    fn to_godot(&self) -> Self::Via {
        self.inner.to_godot()
    }

    fn into_godot(self) -> Self::Via {
        self.inner.into_godot()
    }
    fn to_variant(&self) -> Variant {
        self.inner.to_variant()
    }

}

impl<T: FromGodot> FromGodot for Required<T>
    where
        Option<T::Via>: GodotType,
{
    fn try_from_godot(via: Self::Via) -> Result<Self, ConvertError> {
        match Option::<T>::try_from_godot(via) {
            Ok(val) => Ok( Required { inner: val }),
            Err(e) => Err(e)
        }
    }

    fn from_godot(via: Self::Via) -> Self {
        Required { inner: Option::<T>::from_godot(via) }
    }

    fn try_from_variant(variant: &Variant) -> Result<Self, ConvertError> {
        Ok(Required {inner: Option::<T>::try_from_variant(variant)?})
    }

    fn from_variant(variant: &Variant) -> Self {
        return Required {inner: Option::<T>::from_variant(variant)}
    }
}

impl<T> Var for Required<T>
    where
        T: Var + FromGodot,
        Option<T>: GodotConvert<Via = Option<T::Via>>,
        Required<T>: GodotConvert<Via = Option<T::Via>>
{
    fn get_property(&self) -> Self::Via {
        Option::<T>::get_property(&self.inner)
    }

    fn set_property(&mut self, value: Self::Via) {
        Option::<T>::set_property(&mut self.inner, value as Self::Via);
    }
}

impl<T> Export for Required<T>
    where
        T: Export + GodotType,
        T::Ffi: GodotNullableFfi,
        Option<T>: Var,
        Required<T>: Var
{
    fn default_export_info() -> PropertyHintInfo {
        Option::<T>::default_export_info()
    }
}

[bcolloran]

So the only reason for OnEditor to exist would be to avoid the explicit .as_ref().unwrap()?

I'm not saying that's not worth it -- I think these common cases should be as ergonomic as possible -- but I'd like to be clear about the goals 🙂 also then we can find the best possible name.

Just a quick note here: for me it's not about avoiding writing out .as_ref().unwrap() so much as correctly modeling the semantics of my nodes (which is the big reason I choose rust over GDscript! 🙂).

As @Yarwin mentions above, I have some cases where some an exported item is legitimately optional, and so is well modeled by:

    #[export]
    optional_node: Option<Gd<Node>>,

But I'm also making heavy use of a pattern where I'm using Resources to configure nodes, and the resource is required for the node to function correctly. I think an approved/recommended way of doing this would be nice, whether that means including a wrapper type, an export attribute, or even just mentioning Yarvin's recipe above in the "Recipes" section of the book.

Thanks Bromeon, loving gdext!

[Yarwin]

note: The declaration of mentioned recipe changed slightly, I'll update it later 😅

I have some cases where some an exported item is legitimately optional

More usecases can be found in: godotengine/godot-proposals#162

I think an approved/recommended way of doing this would be nice, whether that means including a wrapper type, and export attribute

@bcolloran the main issues are discoverability, consistency with Gdscript's user API and avoiding cognitive overload. I think I can made PR with said recipe for a book and then we'll see how wildly it's used?

[bcolloran]

Thanks @Yarwin!

[Yarwin]

bit late, but I did another round to check if this feature is worth adding to the core;

so far I was investigating if it can be supplemented with #[export] #[init(val = SomeDefault)] Gd<T>. I did few round of surface/user-side manual tests with following snippet:

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

#[derive(GodotClass)]
#[class(init, base=Node)]
struct ExampleExporter {
    #[export]
    #[init(val=MyCustomNode::new_alloc())]
    some_node_export: Gd<MyCustomNode>,
    base: Base<Node>
}

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

The main, glaring issue is insane amount of warnings –  core/object/class_db.cpp:2085 - Instantiated MyCustomNode used as default value for ExampleExporter's "some_node_export" property.. There is no way to disable it https://github.com/godotengine/godot/blob/master/core/object/class_db.cpp#L2088. In larger scenes the amount of emitted warnings can slow down the editor.

Changing the value emits yet another confusing error: Cannot get path of node as it is not in a scene tree..

I was unable to enforce null on given property – even if it is nil (set via scene tree, inspector or tool script) it is being changed to default value on runtime (which is fine!). Additionally trying to set null on ExampleExporter if it is marked as tool yields proper error:

```bash Cannot get path of node as it is not in a scene tree. core/string/node_path.cpp:263 - Condition "!p_np.is_absolute()" is true. Returning: NodePath() Node not found: "" (relative to "/root/@EditorNode@16886/@panel@13/@VBoxContainer@14/DockHSplitLeftL/DockHSplitLeftR/DockHSplitMain/@VBoxContainer@25/DockVSplitCenter/@VSplitContainer@52/@VBoxContainer@53/@PanelContainer@98/MainScreen/@CanvasItemEditor@9272/@VSplitContainer@9094/@HSplitContainer@9096/@HSplitContainer@9098/@control@9099/@SubViewportContainer@9100/@SubViewport@9101/Node/ExampleExporter2"). gdext/godot-core/src/private.rs:335 - godot-rust function call failed: ExampleExporter::set_some_node_export() Reason: parameter #0 (godot_core::obj::gd::Gd) conversion Source: `Gd` cannot be null: null ```

Every property with Gd<T> creates leaks if memory for this very type is manually managed (ie: if it is not RefCounted). Every property creates its own unique default instance, even if given property is set and they are not cleared on scene change (so every time one open a scene it creates n memory leaks, where n is amount of #[export] Gd<T> – I was unable to create more memory leaks).

The only way to avoid these problems is creating Gd::null or something like that, which is too horrifying to consider 😬

Theoretically it could be used for instantiating default, non-null RefCounted types. Following snippet:

#[derive(GodotClass)]
#[class(init, base=Resource)]
struct MyCustomResource {
    #[export]
    some_value: i64
}


#[derive(GodotClass)]
#[class(init, base=Node)]
struct ExampleExporter {
    #[export]
    #[init(val=MyCustomResource::new_gd())]
    some_resource_export: Gd<MyCustomResource>,
    base: Base<Node>
}

Works fine and memory leaks are not an issue anymore, but it still emits staggering amount of warnings 😒.

So, the questions are:
-> Should export for Gd<T> be kept? It seems hilariously broken and I can't really find any proper usecase (Instead export should be provided with associated types – like Option<Gd<T>> and whatnot)
-> Do we want to introduce Required\NonNull\OnEditor types?
-> if so, should they deref and panic on runtime, or should they work similar to OnReady (check before ready, limiting them to nodes)? (personally I'm in favor of the former, to support resources as well)

[bcolloran]

Thank you for looking into this more @Yarwin !

-> if so, should they deref and panic on runtime, or should they work similar to OnReady (check before ready, limiting them to nodes)? (personally I'm in favor of the former, to support resources as well)

For whatever it's worth:

• +1 on an abstraction that can be used for Resources. My main use case is configuring nodes with resources (sometimes nested resources).
• if the item is required, I think auto deref is appropriate
• I think a runtime panic feels like a good way to handle a Required\NonNull\OnEditor type that is not configured in the editor. I'm looking for some way to be explicit about what needs to be configured in the editor, and what can be replaced with a default value or fallback behavior if not configured, and for my needs an early panic pointing at the unconfigured item would be a helpful, because if the item is not configured correctly in the editor it would presumably result in some less specific panic later, or (perhaps worse) unexpected application behavior that I might not notice.

[Bromeon]

Should export for Gd<T> be kept? It seems hilariously broken and I can't really find any proper usecase (Instead export should be provided with associated types – like Option<Gd<T>> and whatnot)

It should be removed in v0.3, I agree. People mostly use it because Option<Gd<T>> is clunky to access with .as_mut().unwrap(), but if we find a better abstraction like OnEditor, that point might be moot.

---

Do we want to introduce Required`NonNull\OnEditor` types?

I think yes. There seems to be quite some demand for a non-optional, ergonomic, exportable object type.

The question is whether this has some use outside of objects 🤔

I think it might, although we would then go a bit into "custom territory" -- we could have a specific value for a type that is seen as "uninitialized" and has to be set by the user. Examples:

• i32: -1
• Vector2: (NaN, NaN)
• ...

Obviously we'd need to let the user customize this, but it should be easy with constructors:

#[init(val = OnEditor::uninit(0))]
hp: OnEditor<i32>,

// or even:

#[init(editor_uninit = 0)]
hp: OnEditor<i32>,

Just brainstorming...

---

if so, should they deref and panic on runtime, or should they work similar to OnReady (check before ready, limiting them to nodes)? (personally I'm in favor of the former, to support resources as well)

True, supporting Resource is a good point.

Also here, it might technically be possible to provide both via constructors OnEditor::lazy(...), OnEditor::on_ready(...), but we should first settle for one default and then only provide the other if absolutely needed.

Lazy init does have the specialty that if it's never accessed, a missing initialization won't be detected. That can be both a bug or a feature. It can also add some non-determinism depending on where it is initialized, but generally that should be OK and also performance-wise acceptable (most field values are reasonably lightweight).

So, should we start with lazy initialization? I.e. panic on Deref? (I don't care much about "not recommended")

---

Then there's also the name... I find Required a bit generic, and there's no relation to the editor. The type can technically be generalized, but are people really needing lazy init if not in the editor? There's also Rust's LazyLock etc for those cases...

What I like about OnEditor is that it has a nice symmetry to OnReady (being initialized in the editor rather than ready), and expresses its most common use. It's also specific enough to not collide, and to be recognizable in code, while still conveying the purpose.