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

Add OnReady::node() + #[init(node = "...")] attribute #807

Merged
merged 1 commit into from
Jul 27, 2024
Merged

Add OnReady::node() + #[init(node = "...")] attribute #807

merged 1 commit into from
Jul 27, 2024

Conversation

Houtamelo
Copy link
Contributor

This feature aims to mimic GDScript's @onready annotation in a more ergonomic way.

It still uses the type OnReady<T>, but the initialization closure now accepts an argument of type Gd<Node>, which makes it easier to express the "classic" GDScript pattern of fetching a node reference before _ready (var node = $NodePath).

This is achieved by adding the #[onready] attribute to the pool of possible ways the user can customize their Rust "class"'s fields.
The attribute supports the following arguments, only one must be provided at a time:

  • node = "NodePath"
  • fn = |base| -> T { .. }

Example

#[derive(GodotClass)]
#[class(init, base = Node)]
struct OnReadyAttribute {
    base: Base<Node>,
    #[onready(node = "child")] // Runs `self.auto_node = self.base().get_node_as("child")`
    auto_node: OnReady<Gd<Node>>,
    #[onready(fn = |b| b.get_name())] // Runs `self.auto_primitive = self.base().get_name()`
    auto_primitive: OnReady<GString>,
}

Restrictions

  • The class must have an explicit base field.
  • The attribute is mutually exclusive with #[init(default)]
  • The initialization closure does not support typed Base<T>, it's always Gd<Node>

Caveats

Since the attribute essentially de-sugars into #[init(default)], it only works with the macro-generated init, the #[onready] attribute does nothing if the user manually creates the class (by overriding INode::init, or calling Gd::with_init_fn, etc).

@GodotRust
Copy link

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

@Bromeon
Copy link
Member

Bromeon commented Jul 22, 2024

Thanks a lot! 👍

Is there a way to use this without proc macros, e.g. OnReady::node("path")?

Also, do we really need the proc macros? I'd like to keep the proc-macro DSL limited, because there's a "magic budget" we shouldn't use carelessly. Especially the onready(fn = ...) syntax seems like it's much better dealt with in the constructor, also because it can capture context there. I'm not categorically against #[onready(node = "child")], but I'm not sure if we should add it right from the start, and with this syntax. There's also #[init(default = ...)] which plays a similar role.

Does having an OnReady<T> now require a Base<T> field? This is currently not the case, and may be an additional limitation for variables that don't access the own node.

Could you keep this separate from the changes in #795? Otherwise we need to review that code twice...

@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Jul 22, 2024
@Bromeon Bromeon added this to the 0.2 milestone Jul 22, 2024
@Bromeon Bromeon added the breaking-change Requires SemVer bump label Jul 22, 2024
@Houtamelo
Copy link
Contributor Author

Is there a way to use this without proc macros, e.g. OnReady::node("path")?

The only way I can think of is by using a pattern you already shared previously:

pub fn onready_node<O, T>(
    this: &Base<O>,
    path: impl Into<NodePath> + 'static,
) -> OnReady<Gd<T>>
where
    T: Inherits<Node>,
    O: Inherits<Node>,
{
    let self_obj = this.to_gd();
    OnReady::new(move || self_obj.upcast().get_node_as(path))
}

Which would have to be invoked in a manual init, defeating the purpose of the feature.

Does having an OnReady now require a Base field? This is currently not the case, and may be an additional limitation for variables that don't access the own node

Yes, otherwise we can't get a reference to the class's node. This could be worked around by putting this functionality in a different type (e.g: OnReadyNode)

Could you keep this separate from the changes in #795? Otherwise we need to review that code twice..

Yeah, I'll make sure the next commit is rebased in main.


My main argument in favor of this feature is that fetching nodes this way is much more ergonomic than using #[export] and having every field be Option<Gd>. The $NodePath syntax is very popular in GdScript, having some way of mimicking it would increase the ergonomics of the library by a substantial margin.

@Bromeon
Copy link
Member

Bromeon commented Jul 22, 2024

Which would have to be invoked in a manual init, defeating the purpose of the feature.

Why does it defeat the purpose? init is the constructor, which is the idiomatic way to initialize the object. The point of OnReady is that one cannot forget initialization during ready() and can avoid explicit unwrapping, not that one doesn't have to write a constructor 🙂

We can consider a proc-macro API, but it should happen on top of a regular OnReady API, not via magic only. There are many cases where people declare init() manually, we cannot just leave them out of this feature. Having a design that works with regular Rust types/methods also separates the business logic from macro parsing, and allows to delegate more to the Rust type system, which often helps maintenance and debuggability. Plus, it's friendlier to a potential future builder API.


Regarding my suggestion on Discord, what's the problem with it? It could be an associated function (constructor) of OnReady. Something like:

impl<T> OnReady<Gd<T>>
where T: Inherits<Node>
{
    pub fn node(path: impl Into<NodePath> + 'static) -> OnReady<Gd<T>> {
        let path = path.into();
        OnReady::new(move |node| node.get_node_as(path))
    }
}

That would also mean that the proc-macro no longer needs to have hardcoded magic. Instead, we could extend the #[init] attribute, which assigns initial values to each field in init():

#[derive(GodotClass)]
#[class(init, base = Node)]
struct MyClass {
    base: Base<Node>,

    #[init(node = "child")]
    auto_node: OnReady<Gd<Node>>,
}

Which would be equivalent to a user-defined init():

#[godot_api]
impl INode for MyClass {
    fn init(base: Base<Node>) -> Self {
        Self {
            base,
            auto_node: OnReady::node("child"),
        }
    }
}

The reason why I prefer #[init] is that the attribute specifies what to provide in the constructor (init), and does not change any code in ready(). The OnReady is already wired up to be initialized during ready().

It's also easier to understand for the user what the attribute does behind the scenes, and the above example might even be part of the documentation. What do you think?

I would wait with the fn attribute; closures in the struct declaration are a bit weird and there's not such a clear benefit like for node. We can always add something like this later.

@Houtamelo
Copy link
Contributor Author

Houtamelo commented Jul 23, 2024

We can consider a proc-macro API, but it should happen on top of a regular OnReady API, not via magic only. There are many cases where people declare init() manually, we cannot just leave them out of this feature. Having a design that works with regular Rust types/methods also separates the business logic from macro parsing, and allows to delegate more to the Rust type system, which often helps maintenance and debuggability. Plus, it's friendlier to a potential future builder API.

Regarding my suggestion on Discord, what's the problem with it? It could be an associated function (constructor) of OnReady. Something like:

impl<T> OnReady<Gd<T>>
where T: Inherits<Node>
{
    pub fn node(path: impl Into<NodePath> + 'static) -> OnReady<Gd<T>> {
        let path = path.into();
        OnReady::new(move |node| node.get_node_as(path))
    }
}

I totally misunderstood what you meant, in fact, this commit already has the method you mentioned above, it also has an additional one with_base, that isn't constrained to nodes:

impl<T: GodotClass + Inherits<Node>> OnReady<Gd<T>> {
    pub fn node(path: impl Into<NodePath>) -> Self {
        let path = path.into();

        Self {
            state: InitState::AutoPrepared {
                initializer: Box::new(move |base| base.get_node_as(path)),
            },
        }
    }
}

impl<T> OnReady<T> {
    pub fn with_base<F>(init_fn: F) -> Self
    where
        F: FnOnce(&Gd<Node>) -> T + 'static,
    {
        Self {
            state: InitState::AutoPrepared {
                initializer: Box::new(init_fn),
            },
        }
    }
}

That would also mean that the proc-macro no longer needs to have hardcoded magic. Instead, we could extend the #[init] attribute, which assigns initial values to each field in init():

#[derive(GodotClass)]
#[class(init, base = Node)]
struct MyClass {
    base: Base<Node>,

    #[init(node = "child")]
    auto_node: OnReady<Gd<Node>>,
}

Which would be equivalent to a user-defined init():

#[godot_api]
impl INode for MyClass {
    fn init(base: Base<Node>) -> Self {
        Self {
            base,
            auto_node: OnReady::node("child"),
        }
    }
}

The reason why I prefer #[init] is that the attribute specifies what to provide in the constructor (init), and does not change any code in ready(). The OnReady is already wired up to be initialized during ready().

It's also easier to understand for the user what the attribute does behind the scenes, and the above example might even be part of the documentation. What do you think?

I agree, #[init(node = "NodePath")] seems like the right way to express this, while still keeping the intended ergonomics.

I would wait with the fn attribute; closures in the struct declaration are a bit weird and there's not such a clear benefit like for node. We can always add something like this later.

I don't think it's necessary, I added it because it was simple enough. Personally I've never used @onready for anything but node-fetching.

Also, if the user wants to access base in #[init], they can instead:

#[init( default = OnReady::with_base(|b| b.get_name()) )]
node_name: OnReady<GString>,

It's slight more verbose, but reasonable.

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! Then we could converge on a #[init(node = "path")] syntax that would internally call OnReady::node("path") in the init constructor.

Regarding with_base, maybe rename it to from_base_fn? Then it's more consistent with Callable::from_fn or Gd::from_init_fn.

godot-core/src/obj/onready.rs Outdated Show resolved Hide resolved
@Houtamelo Houtamelo marked this pull request as ready for review July 23, 2024 20:33
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! Here are the synced docs of your PR 🙂
https://godot-rust.github.io/docs/gdext/pr-807/godot/obj/struct.OnReady.html

Could you reduce the number of commits as per guidelines?

There are also some CI jobs currently failing. You could use check.sh which helps with local development, see book.

godot-core/src/obj/onready.rs Outdated Show resolved Hide resolved
godot-core/src/obj/onready.rs Outdated Show resolved Hide resolved
godot-core/src/obj/onready.rs Outdated Show resolved Hide resolved
godot-core/src/obj/onready.rs Outdated Show resolved Hide resolved
Comment on lines 133 to 147
impl<T: GodotClass + Inherits<Node>> OnReady<Option<Gd<T>>> {
/// Variant of [OnReady::new], except this is hard-coded to fetch the node at `path` before ready.
///
/// This is the functional equivalent of the GdScript pattern: `@onready var node = $NodePath`
///
/// # Does not panic
/// If path does not point to a valid node, this will simply leave the field as `None`.
///
/// If you're certain `path` is valid, and you're willing to panic if it isn't,
/// consider changing the field type to `OnReady<Gd<T>>`, then use [OnReady<Gd<T>>::node] instead.
pub fn try_node(path: impl Into<NodePath>) -> Self {
let path = path.into();
Self::from_base_fn(|base| base.try_get_node_as(path))
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, did you encounter a use cases where this was helpful?

Wondering because OnReady is generally used to have ergonomic access to late-initialized fields, and Option requires again explicit .unwrap(). In GDScript, @onready var node = $NodePath followed by null checks is not a typical pattern, or is it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Houtamelo what are your thoughts on this? 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When writing that function, I had thought about a few use cases but after your question I re-considered it and realized the use cases were all redundant.

godot-core/src/obj/onready.rs Outdated Show resolved Hide resolved
godot-macros/src/class/derive_godot_class.rs Outdated Show resolved Hide resolved
godot-macros/src/class/derive_godot_class.rs Outdated Show resolved Hide resolved
itest/rust/src/object_tests/onready_test.rs Outdated Show resolved Hide resolved
itest/rust/src/object_tests/onready_test.rs Show resolved Hide resolved
@Bromeon Bromeon changed the title [Prototype][Feature] OnReady attribute (#[onready] field: OnReady<T>) Add OnReady::node() + #[init(node = "...")] attribute Jul 24, 2024
- Meant for fields of type: OnReady<Gd<T>> where T: Inherits<Node>
- This key accepts a single argument with the syntax: "NodePath"
- This key is mutually exclusive with the key `default`
- This key desugars into `default = OnReady::node("NodePath")`

  This feature required adding a `Gd<Node>` argument to the closure that initializes OnReady, which means that classes that use it must have an explicit `Base` field. (i.e. `base: Base<Node>`)
@Houtamelo
Copy link
Contributor Author

@Bromeon Should be all good now

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! Just the question about OnReady<Option<...>> is still there, but I can already merge the current version anyway 🙂

@Bromeon
Copy link
Member

Bromeon commented Jul 26, 2024

Merging currently blocked by upstream regression godotengine/godot#94755, PR is already on the way...

@Bromeon Bromeon added this pull request to the merge queue Jul 27, 2024
Merged via the queue into godot-rust:master with commit 4a4a8ce Jul 27, 2024
15 checks passed
@Houtamelo Houtamelo deleted the onready_attribute branch July 27, 2024 21:22
@Bromeon Bromeon mentioned this pull request Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Requires SemVer bump c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants