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

Signals now accept parameters #279

Merged
merged 2 commits into from
Jun 6, 2023
Merged

Conversation

LeaoLuciano
Copy link
Contributor

Project for testing:

  • dodge-the-creeps-signals.zip
  • Added a signal named test in Player scene that is emitted when player dies
  • This signal is connected in Main scene which calls test_function, printing player's death position

@Bromeon
Copy link
Member

Bromeon commented May 18, 2023

Thanks! 🙂

Can you show some usage examples here (so people don't have to download the archive)?

@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels May 18, 2023
@LeaoLuciano
Copy link
Contributor Author

LeaoLuciano commented May 18, 2023

player.rs:

    #[signal]
    fn test(direction: Vector2);

    fn on_player_body_entered(&mut self, _body: Gd<PhysicsBody2D>) {
        ...
        let pos = self.get_position();
        self.base.emit_signal("test".into(), &[pos.to_variant()]);
    }

main_scene.rs:

    #[func]
    pub fn new_game(&mut self) {
        ...
        let mut player = self.base.get_node_as::<player::Player>("Player");
        player.bind_mut().connect(
            "test".into(),
            Callable::from_object_method(self.base.get_node_as::<Self>("."), "test_function"),
            0,
        );
    }

    #[func]
    fn test_function(&self, direction: Vector2) {
        println!("Player died at: {}", direction);
    }
Gravacao.de.tela.de.2023-05-18.08-31-44.webm

@LeaoLuciano
Copy link
Contributor Author

With 2 parameters:
player.rs:

    #[signal]
    fn test(direction: Vector2, distance: f32);


    #[func]
    fn on_player_body_entered(&mut self, _body: Gd<PhysicsBody2D>) {
        ...
        self.base.emit_signal("test".into(), &[pos.to_variant(), pos.length().to_variant()]);
    }

main_scene.rs:

    #[func]
    fn test_function(&self, direction: Vector2, distance: f32) {
        println!("Player died at: {} (distance {} from (0, 0))", direction, distance);
    }
Gravacao.de.tela.de.2023-05-18.08-39-44.webm

@Bromeon
Copy link
Member

Bromeon commented May 24, 2023

bors try

@GodotRust
Copy link

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

bors bot added a commit that referenced this pull request May 24, 2023
@bors
Copy link
Contributor

bors bot commented May 24, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@LeaoLuciano LeaoLuciano force-pushed the signal_parameters branch 3 times, most recently from ddcdc6a to b1817c2 Compare June 2, 2023 17:39
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 for the update! Added smaller comments. Since there is now quite a bit of signal functionality, you can also consider adding a new signal_test.rs file for these tests (don't forget license header). Or leave it as-is, if you prefer 🙂

Could you integrate your changes into the 2nd commit?

Comment on lines 920 to 923
emitter
.share()
.upcast::<Object>()
.emit_signal(signal_name.into(), arg);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
emitter
.share()
.upcast::<Object>()
.emit_signal(signal_name.into(), arg);
emitter.base.emit_signal(signal_name.into(), arg);

It's in the same file and for a test, so encapsulation is not the highest priority.

Comment on lines 887 to 889
self.used[2].set(true);
assert_eq!(self.base.share(), arg1);
assert_eq!(SIGNAL_ARG_STRING, arg2.to_string());
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to check the preconditions first and separate them with an empty line from the logic.
Same above.

Suggested change
self.used[2].set(true);
assert_eq!(self.base.share(), arg1);
assert_eq!(SIGNAL_ARG_STRING, arg2.to_string());
assert_eq!(self.base.share(), arg1);
assert_eq!(SIGNAL_ARG_STRING, arg2.to_string());
self.used[2].set(true);

Comment on lines 898 to 899
let emitter: Gd<SignalEmitter> = Gd::new_default();
let receiver: Gd<SignalReceiver> = Gd::new_default();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let emitter: Gd<SignalEmitter> = Gd::new_default();
let receiver: Gd<SignalReceiver> = Gd::new_default();
let emitter = Gd::<SignalEmitter>::new_default();
let receiver = Gd::<SignalReceiver>::new_default();

Turbofish! 🐟💨

Comment on lines 911 to 912
let signal_name = format!("signal_{}_arg", i);
let receiver_name = format!("receive_{}_arg", i);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let signal_name = format!("signal_{}_arg", i);
let receiver_name = format!("receive_{}_arg", i);
let signal_name = format!("signal_{i}_arg");
let receiver_name = format!("receive_{i}_arg");

let signal_name = format!("signal_{}_arg", i);
let receiver_name = format!("receive_{}_arg", i);

emitter.share().upcast::<Object>().connect(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
emitter.share().upcast::<Object>().connect(
emitter.base.connect(

@Bromeon
Copy link
Member

Bromeon commented Jun 6, 2023

bors try

bors bot added a commit that referenced this pull request Jun 6, 2023
@bors
Copy link
Contributor

bors bot commented Jun 6, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@LeaoLuciano LeaoLuciano marked this pull request as draft June 6, 2023 10:38
@LeaoLuciano LeaoLuciano marked this pull request as ready for review June 6, 2023 13:50
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!

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 6, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 7573721 into godot-rust:master Jun 6, 2023
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 this pull request may close these issues.

3 participants