Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mimic onready vars #130

Closed
WinstonHartnett opened this issue Feb 20, 2023 · 3 comments · Fixed by #534
Closed

Mimic onready vars #130

WinstonHartnett opened this issue Feb 20, 2023 · 3 comments · Fixed by #534
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library

Comments

@WinstonHartnett
Copy link
Contributor

Currently the pattern for structs with Gd references (which have to be manually initialized in ready) looks like:

#[derive(GodotClass)]
#[class(base=Node)]
struct MyStruct {
    my_child: Option<Gd<Node>>
    #[base]
    base: Base<Node>
}

Then, map, unwrap, or user-defined accessors are needed to bypass the Option.

How about:

#[derive(GodotClass)]
#[class(base=Node)]
struct MyStruct {
    #[on_ready("MyChild")]
    my_child: OnReady<Gd<Node>>
    #[base]
    base: Base<Node>
}

where OnReady is a MaybeUninit<Gd<T>> with a Deref/DerefMut instance to avoid unwrapping.

It certainly clutters the struct definition a bit more, but you avoid:

  • Initializing an Option in ready (which can easily be forgotten)
  • Using map or accessors to get the obviously initialized field
  • Paying for the Option<Gd<T>>'s 16 bytes compared to Gd<T>
@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Feb 20, 2023
@Bromeon
Copy link
Member

Bromeon commented Feb 20, 2023

That's a nice idea, and it already came up in godot-rust/gdnative#758 (comment) 🙂

A few things to note:

  • Option<Gd<T>> could in theory be optimized by storing the object pointer as NonNull<...> instead of OpaqueObject, or another encoding pattern of which rustc recognizes the null state. Or we use a union and do it manually. This is very low priority though.
  • "Using map or accessors to get the obviously initialized field" -- this is not true. _ready is invoked when the node is added to the tree; initialization happens before. There are even two callbacks _init and _enter_tree that happen prior to _ready. Which implies that the container must be able to represent a null state -- preferably a safe one.
  • The OnReady<T> type needs to make sure it correctly handles reference-counting and destruction, especially in case it doesn't directly hold a Gd<T> type.

@chitoyuu
Copy link

chitoyuu commented Feb 21, 2023

An alternative syntax to consider is generalization into a trait (all identifiers subject to bikeshedding):

trait Fetch {
    fn fetch(root: Gd<Node>, path: &str) -> Result<Self, FetchError>;
}

impl<T: GodotClass + Inherits<Node>> Fetch for Gd<T> {
    // snip
}

#[derive(GodotClass)]
#[class(base=Node)]
struct MyStruct {
    #[on_ready(fetch("MyChild"))]
    my_child: Option<Gd<Node>>,
    #[base]
    base: Base<Node>
}

Which could later open up possibilities of using separate types for sub-trees, for code organization and/or reuse:

#[derive(Fetch)]
struct MyChildren {
    #[fetch("MyChild")]
    my_child: Gd<Node>,
    #[fetch("MyChild2")]
    my_child_2: Gd<Node>,
    #[fetch("MyChild3")]
    my_child_3: Gd<Node>,
}

#[derive(GodotClass)]
#[class(base=Node)]
struct MyStruct {
    #[on_ready(fetch("."))]
    my_child: Option<MyChildren>,
    #[base]
    base: Base<Node>
}

There is some existing discussion about this syntax here, that apart from the safety aspects should also apply to gdextension: godot-rust/gdnative#980

@Bromeon
Copy link
Member

Bromeon commented Oct 9, 2023

I have some experiments ongoing on branch late-init 🙂

It's indeed a bit tricky, my current approach is a specific OnReady<T> type, which is basically Option<T> but more ergonomic. I'm not yet sure if/how it will play together with the proc-macro API, but also I'm not too big of a fan of #[init(default)], so some things may change there...

Anyway, I'll open a PR once there's something ready (no pun intended).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants