`TypedSignal`: document different ways of connecting signals #1443

lmsonic · 2025-12-13T21:27:20Z

Hello, I created the doctests I was talking about in the discord for the connect functions in TypedSignal :) I also added the availability warning for the signals api that is in the book but not in the API afaik? if im wrong, i'll erase it

GodotRust · 2025-12-13T22:36:10Z

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

Bromeon

Thank you for your contribution!

Regarding the example snippets: you have 75% of the code hidden with #, while only 25% is visible.

The question is, is it clear enough for a reader without the hidden code? The fact that a variable is OnEditor for example is absolutely not obvious. If not in code, there should be a comment.
But at the same time we want to keep things concise -- it's hard to strike a good balance here.
Can maybe the overall design be simplified? Definitely omit any useless code like comments in hidden parts...

Bromeon · 2025-12-13T22:44:22Z

godot-core/src/registry/signal/typed_signal.rs

+/// # Availability
+/// For typed signals to be available, you need:
+/// - A `#[godot_api] impl MyClass {}` block.
+///     - This must be an inherent impl, the `I*`` trait `impl`` won't be enough.


Double backtick doesn't render properly, see https://godot-rust.github.io/docs/gdext/pr-1443/godot/register/struct.TypedSignal.html

(You can test this by running cargo doc locally).

godot-core/src/registry/signal/typed_signal.rs

Bromeon · 2025-12-13T22:55:00Z

godot-core/src/registry/signal/typed_signal.rs

+    /// Example usage:
+    /// ```
+    /// # use godot::prelude::*;
+    /// # #[derive(GodotClass)]
+    /// # #[class(base=Node,init)]
+    /// # pub struct Player {
+    /// #     health_ui: OnEditor<Gd<HealthUI>>,
+    /// #     base: Base<Node>,
+    /// # }
+    /// # #[derive(GodotClass)]
+    /// # #[class(base=Node2D,init)]
+    /// # pub struct HealthUI {
+    /// #     base: Base<Node2D>,
+    /// # }
+    /// # #[godot_api]
+    /// # impl Player {
+    /// #     #[signal]
+    /// #     fn health_changed(health: i32);
+    /// # }
+    /// # impl Player {
+    /// #     fn change_health_anim(&mut self, health: i32) {
+    /// #         // if health > 0 cure anim, if health < 0 hit anim
+    /// #     }
+    /// # }
+    /// # impl HealthUI {
+    /// #     fn on_health_changed(&mut self, health: i32) {
+    /// #         // Update healthbar
+    /// #     }
+    /// # }
+    /// #[godot_api]
+    /// impl INode for Player {
+    ///     fn ready(&mut self) {
+    ///         self.signals() // Connect to self
+    ///             .health_changed()
+    ///             .connect_self(Self::change_health_anim);
+    ///         self.signals() // Connect to other object
+    ///             .health_changed()
+    ///             .connect_self(|s, amount| s.health_ui.bind_mut().on_health_changed(amount));
+    ///     }
+    /// }
+    /// ```


Some general remarks, before I do line-by-line bikeshedding:

Examples should come at the end, after other important information (here 2 bullet points).

They should start with an # Example or # Examples h1 section.

Comments in code must all start with uppercase (unless referring to a lowercase symbol) and end in period.

Use no_run in the opening code blocks, because we can't run any engine code during doctests. Here it still works because no code is actually run, but it's still better to skip that entirely.

Please follow formatting of existing code, e.g. #[class(init, base=Control)] with space instead of #[class(base=Control,init)].

Before doing those changes, we should however agree on an approach on structuring examples, see main thread discussion 🙂

Does the no-run check atleast check that it compiles?

Yes, no_run checks if it compiles: https://doc.rust-lang.org/rustdoc/write-documentation/documentation-tests.html#attributes

Bromeon · 2025-12-13T22:59:12Z

godot-core/src/registry/signal/typed_signal.rs

+    ///         self.button // Connect from other to self
+    ///             .signals()
+    ///             .pressed()
+    ///             .connect_other(&self.to_gd(), Self::execute_tool);


Suggested change

/// .connect_other(&self.to_gd(), Self::execute_tool);

/// .connect_other(self, Self::execute_tool);

I believe this works, or &*self otherwise 🤔

See also API docs.

I didn't know about this! This is why these docs are useful! :3 I had so many runtime borrow problems because I was passing self.to_gd()!

lmsonic · 2025-12-14T00:32:15Z

Thank you for your contribution!

Regarding the example snippets: you have 75% of the code hidden with #, while only 25% is visible.

* The question is, is it clear enough for a reader without the hidden code? The fact that a variable is `OnEditor` for example is absolutely not obvious. If not in code, there should be a comment.

* But at the same time we want to keep things concise -- it's hard to strike a good balance here.

* Can maybe the overall design be simplified? Definitely omit any useless code like comments in hidden parts...

I think rendering the whole code takes too much space, so maybe, the approach of warning that the variables are OnReady is the best one? While also showing the signatures of the functions?

lmsonic · 2025-12-14T00:45:00Z

Another thing, do you think the examples are okay? I tried to keep them relevant to gamedev, but not so complicated that they detract from the explanation of the function. I tried to see how I used the signals API myself, for inspiration

lmsonic · 2025-12-14T09:19:58Z

I'm not able to import classes in the doctest with use godot::classes::{Button, Control, IControl};. I tried to fix the ci using the ./check.sh script, but i ' m not able to I don't know why, I tried to search other examples in the codebase, they use godot::classes , or use godot_core instead of godot, but in this case it doesn't work..

…ilability paragraph from book

Bromeon

Some of my feedback wasn't addressed yet, I added a commit since it's faster than back-and-forth 🙂
For the future, please run rustfmt on nightly before pushing.

But there's still the basic question what you want to show. Now you call the same API multiple times, which is counter to your original suggestion to showcase different ways of connecting signals.

Bromeon · 2025-12-14T10:34:43Z

godot-core/src/registry/signal/typed_signal.rs

+    ///     fn ready(&mut self) {
+    ///         self.signals() // Connect to self
+    ///             .health_changed()
+    ///             .connect_self(Self::change_health_anim);
+    ///         self.signals() // Connect to other object (health_ui is OnEditor<Gd<HealthUI>>)
+    ///             .health_changed()
+    ///             .connect_self(|s, amount| s.health_ui.bind_mut().on_health_changed(amount));
+    ///     }


This is confusing:

Connect to other object

That's not true, you still connect to this object. You just manually delegate in to the other UI. Which raises more questions than it answers:

Why not connect a signal directly to the UI?

If not, why not address this in the same handler as change_health_anim?

How are these two different from a connect perspective?

I think this example is more complex than it needs to be. If the point is to demonstrate that one can use both function references and closures, then I'd use the same connection, with two different syntaxes. That's much clearer and immediately demonstrates the equivalence.

Okay, this makes sense, you would you connect_other in this case, mhh
EDIT: no okay, using connect_other gives to double borrow errors

Yeah, you can't use connect_other on self, that's why connect_self exists.

What I meant is: show the exact same connect_self call twice:

with a function reference

with a closure

This makes clear that it's possible to use different syntaxes.

Bromeon · 2025-12-14T10:36:00Z

godot-core/src/registry/signal/typed_signal.rs

+    ///     fn ready(&mut self) {
+    ///         self.button // Connect from other to self
+    ///             .signals()
+    ///             .pressed()
+    ///             .connect_other(self, Self::execute_tool);
+    ///         self.button // Connect from other to other (other_tool is OnEditor<Gd<OtherTool>>)
+    ///             .signals()
+    ///             .pressed()
+    ///             .connect_other(&*self.other_tool, |tool| tool.set_tool_enabled(false));
+    ///     }


Also here, it's twice the same way of connecting signals.

I thought the idea was to showcase different APIs? Since we're having the problem that examples are getting too long, we definitely don't have to show the same usage more than once.

The idea is that we are connecting first from the button to Self, and the second example is from the button to OtherTool, which is just another object. I can reduce it to just a single example if you think the distinction doesn't matter.

Ok fair, in this case it can make sense to showcase both 👍

lmsonic · 2025-12-14T11:06:09Z

As I said on the discord, I wanted to show how to use connect_self to connect to both self and another object, and to use connect_other to connect to self and to another object. And I showed different ways of using the API (Self::method, OtherObject::method, closure) while doing so so that they are also shown to be possible. If it is too confusing mixing the two concepts I understand
(For the rustfmt, i don't know why, but it wouldn't pass the ./check.sh if i put the imports as you put them, which is as rustfmt put them as well for me, so i thought it would error out the ci)

Bromeon · 2025-12-14T11:39:02Z

(For the rustfmt, i don't know why, but it wouldn't pass the ./check.sh if i put the imports as you put them, which is as rustfmt put them as well for me, so i thought it would error out the ci)

If you don't have nightly rust, check.sh will emit a warning about this.

lmsonic · 2025-12-14T11:51:47Z

I thought of a simplified version of the examples, in line with the other ones already in the API

use godot::{classes::Button, prelude::*};

#[derive(GodotClass)]
#[class(base=Node,init)]
pub struct ExampleClass {
    button: OnEditor<Gd<Button>>,
    other_button: OnEditor<Gd<Button>>,
    base: Base<Node>,
}

#[godot_api]
impl ExampleClass {
    #[signal]
    fn signal();

    fn method(&mut self) {}
    fn method_with_args(&mut self, value: bool) {}
}
impl ExampleClass {
    fn example(&mut self) {
    // This would be the only part shown in the API
        self.signals().signal().connect_self(Self::method);
        self.signals()
            .signal()
            .connect_self(|s| s.method_with_args(true));
        self.button
            .signals()
            .pressed()
            .connect_other(self, Self::method);
        self.button
            .signals()
            .toggled()
            .connect_other(&*self.other_button, |other_button, toggled| {
                other_button.set_pressed(!toggled)
            });
// To here
    }
}
``

Bromeon · 2025-12-14T12:05:52Z

Removing the context such as how #[signal] is declared is probably more confusing.

An idea to clear this up would be to not add an example on individual methods, but have a new section # Examples at the end of TypedSignal struct docs. Then, we can show the different ways to connect signals at once, and the reader gets an immediate overview.

Please also read my review feedback:

Comments in code must all start with uppercase (unless referring to a lowercase symbol) and end in period.

Please follow formatting of existing code, e.g. #[class(init, base=Control)] with space instead of #[class(base=Control,init)].

It has already taken me a lot of time to review this, and I even corrected one commit myself because these changes weren't applied. I'd appreciate if I didn't have to repeat myself. Thanks 🙂

lmsonic · 2025-12-14T12:14:03Z

It has already taken me a lot of time to review this, and I even corrected one commit myself because these changes weren't applied. I'd appreciate if I didn't have to repeat myself. Thanks 🙂

I'm sorry, i swear I had pushed two commits applying the changes and trying to fix ci. I don't see those edits in your formatting commit

Bromeon added documentation Improvements or additions to documentation c: core Core components labels Dec 13, 2025

Bromeon reviewed Dec 13, 2025

View reviewed changes

docs(typed_signal): doctests for connect_self and connect_other + ava…

546dbd6

…ilability paragraph from book

Bromeon force-pushed the typed-signal-doctest branch from 99a5e84 to 193e3f0 Compare December 14, 2025 10:37

Bromeon reviewed Dec 14, 2025

View reviewed changes

Formatting

cedb206

Bromeon force-pushed the typed-signal-doctest branch from 193e3f0 to cedb206 Compare December 14, 2025 10:44

feat: new docexample on top of the page, removing others

7944427

Bromeon changed the title ~~docs(typed_signal): doctests for connect_self and connect_other + availability paragraph from book~~ TypedSignal: document different ways of connecting signals Dec 17, 2025

	/// .connect_other(&self.to_gd(), Self::execute_tool);
	/// .connect_other(self, Self::execute_tool);

Uh oh!

TypedSignal: document different ways of connecting signals #1443

Are you sure you want to change the base?

TypedSignal: document different ways of connecting signals #1443

Uh oh!

Conversation

lmsonic commented Dec 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GodotRust commented Dec 13, 2025

Uh oh!

Bromeon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmsonic Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmsonic commented Dec 14, 2025

Uh oh!

lmsonic commented Dec 14, 2025

Uh oh!

lmsonic commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmsonic Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lmsonic commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bromeon commented Dec 14, 2025

Uh oh!

lmsonic commented Dec 14, 2025

Uh oh!

Bromeon commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lmsonic commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`TypedSignal`: document different ways of connecting signals #1443

`TypedSignal`: document different ways of connecting signals #1443

lmsonic commented Dec 13, 2025 •

edited

Loading

lmsonic Dec 14, 2025 •

edited

Loading

lmsonic commented Dec 14, 2025 •

edited

Loading

Bromeon left a comment •

edited

Loading

lmsonic Dec 14, 2025 •

edited

Loading

lmsonic commented Dec 14, 2025 •

edited

Loading

Bromeon commented Dec 14, 2025 •

edited

Loading

lmsonic commented Dec 14, 2025 •

edited

Loading