Abstract `#[signal]` in `#[godot_dyn]` traits

[pablopenna]

Edit bromeon: rust code tags + rustfmt

I want to implement a system to choose a skill from a pool.

My current approach was defining a trait called and then implement it in different ways (e.g. one implementation for the AI and another one for the player - linked to UI)

pub(crate) trait SkillChooser {
    fn choose(
        &mut self,
        skill_pool: &Gd<SkillContainerModule>,
        target: &Gd<BattleEntity>,
    ) -> DynGd<Node, dyn SkillImplementation>;
}

The issue is that to link it to the UI, I would need to make this async. I can emit a signal once a skill is chosen. Something like:

pub(crate) trait SkillChooser {
    fn choose(&mut self, skill_pool: &Gd<SkillContainerModule>, target: &Gd<BattleEntity>);

   #[signal]    
   fn skill_chosen(skill: DynGd<Node, dyn SkillImplementation>);
}

However, I cannot declare a signal inside a trait (because I cannot annotate it with #[godot_api]).

I don't know if it is feasible or not to have this feature but I wanted to raise this just in case it is a valid request.

Thank you

[Bromeon]

Related to #1231, for #[func].

Not sure if this is something we want to implement, as traits are Rust-side abstractions, and integrating them fully with Godot will be challenging (even more so when Godot provides its own traits in the future).

Would this #[signal] mean that every type implementing the trait needs to provide the same signal with the same signature, i.e. it's a "virtual signal"?

Why does it need to be a signal, can you not just have a method skill_chosen that is implemented in the concrete impls? You can always dispatch to a signal there if needed.

[pablopenna]

Edit bromeon: rust tags

The main point for having the signal in the trait is that the consumers of SkillChooser only import the trait definition. If I define the signal in each implementation, it would no be available from the trait, which is what the consumers are getting.

e.g.

#[derive(GodotClass)]
#[class(base=Node2D)]
pub(crate) struct BattleEntity {
    ...
    #[export]
    skill_chooser: OnEditor<DynGd<Node, dyn SkillChooser>>,
    ...
}

...

#[func(gd_self)]
pub(crate) fn decide_skill(mut this: Gd<Self>) {
    let skills_container = this.bind().get_skills();
    let target = this.bind().get_target().unwrap();
    let skill_chooser_node = this.bind().get_skill_chooser().unwrap();
        
    let mut skill_chooser: DynGd<Node, dyn SkillChooser> = skill_chooser_node.to_variant().to();
    
    // At this point I want to link the chosen signal from skill_chooser that should be emitted by the choose() method.
    // Something like
    // skill_chooser.signals().skill_chosen().builder().flags(ConnectFlags::ONE_SHOT).connect(self, move |skill| Self::cast_skill(this, skill))

    skill_chooser.dyn_bind_mut().choose(&skills_container, &target);
 }

#[func(gd_self)]
pub(crate) fn on_skill_chosen(mut this: Gd<Self>, skill: DynGd<Node, dyn Skill>) {
     skill.dyn_bind_mut().cast(this.clone(), this.bind().target.clone().unwrap());
     ... // other stuff like ending the turn
}

The line skill_chooser.signals().skill_chosen()... is not possible because the signal is not declared in the trait.

As far as I understand, I could define a signal in the implementations but then BattleEntity (the struct above that imports the SkillChooser node) would need to specify the type of the specific implementation, breaking the flexibility that I am looking for in this case as I want to use different implementations .

Thank you for your time

[pablopenna]

Why does it need to be a signal, can you not just have a method skill_chosen that is implemented in the concrete impls? You can always dispatch to a signal there if needed.

I think the above comment should address your question. Short answer is no because the consumers only import the trait and they wouldn't be able to get the signal definition.

However, I have just realized that maybe I could make the abstract method from the trait return a signal. Then the consumers attach a callback to that signal.

Would this #[signal] mean that every type implementing the trait needs to provide the same signal with the same signature, i.e. it's a "virtual signal"?

Yes, that is what I thought

[Yarwin]

However, I have just realized that maybe I could make the abstract method from the trait return a signal. Then the consumers attach a callback to that signal.

This would be suggested approach; TypedSignals have proper method for that: https://godot-rust.github.io/docs/gdext/master/godot/register/struct.TypedSignal.html#method.to_untyped. I think we won't implement signals() on DynTraitObjects anytime soon (I offer full guidance and whatnot if anybody would like to dig into this topic though).

Although there are two obstacles I see:

1. #[godot_api(secondary)] don't allow to implement signals (and by extension, don't work with macros), requiring to write boilerplate (which is ultra painful for refactors and whatnot). It is not that bad, since it is covered by untyped signals API: https://godot-rust.github.io/docs/gdext/master/godot/classes/struct.Object.html#method.add_user_signal; https://docs.godotengine.org/en/stable/classes/class_object.html#class-object-method-add-user-signal, which isn't very ergonomic but gets a job done without much issues (and can be abstracted away by some utility function and whatnot – fn attach_trait_signal(to: Gd<...>)).
2. Connecting to signal can be troublesome too.

For example, this cannot work:

#[godot_api]
impl TraitNode {
    #[signal]
    fn trait_signal(val: i64, this: Gd<MyNode>);

}

// Generic value – `impl FnMut...` has unknown size on compilation time.
trait SomeTrait {
    fn connect_to_signal(&mut self, func: impl FnMut(i64, Gd<Node>) + 'static);
}

and instead one must declare

trait SomeTrait {
    fn connect_to_signal(&mut self, func: Callable);
}

and use it with something along the lines of

    #[func]
    fn incoming(&self, mut other: DynGd<Node, dyn SomeTrait>) {
        other.dyn_bind_mut().connect_to_signal(
            Callable::from_fn("my signal connection", |args| {
                let val: i64 = args.iter().next().unwrap().to();
                let this: Gd<Node> = args.iter().next().unwrap().to();
                godot_print!("My val: {val} and that {this}");
            })
        );
    }

which definitively could be improved.

[Bromeon]

Please use ```rs tags for syntax highlighting, see my edits 🙂

The main point for having the signal in the trait is that the consumers of SkillChooser only import the trait definition. If I define the signal in each implementation, it would no be available from the trait, which is what the consumers are getting.

The thing is, there is no such thing as a "signal" in Rust. Traits only work with associated types, consts and functions -- here it would be functions.

I assume you'd want this:

trait MyTrait {
    #[signal]
    fn my_signal(i: i32);
}

// impl block already declares a signal
#[godot_api]
impl MyClass {
    #[signal]
    fn my_signal(i: i32);
}

#[godot_dyn]
impl MyTrait for MyClass {
    // what here?
}

We currently only allow signals() in primary impl blocks (inherent, not trait impl). For example, the struct returned by self.signals() needs to contain the signal, and for that, all signals are currently defined in one place. We could possibly add another impl block for that struct, defining my_signal() outside 🤔

But #[signal] would need to transform the function to something like

// fn my_signal(i: i32)
fn __signal_my_signal(&mut self) -> TypedSignal<'_, Self, (i32,)>;

fn signals(&self) -> MyTraitSignalCollection;

Not saying it's technically impossible, but having done the whole signals API for months, this might be another huge endeavor that is rather low on my priority list 🙂 That said, if you'd like to tackle this, we can support you on the way, but it likely won't happen by itself.

[Bromeon]

I think the above comment should address your question. Short answer is no because the consumers only import the trait and they wouldn't be able to get the signal definition.

I think there's a misunderstanding, no one needs the signal definition to simply emit something.

trait SkillChooser {
    // Regular function
    fn choose_skill(skill: DynGd<Node, dyn SkillImplementation>);
}

impl SkillChooser for MyClass {
    // Regular function override
    fn choose_skill(skill: DynGd<Node, dyn SkillImplementation>) {
        // Here, do the actual emit.
        self.signals().skill_chosen().emit(&skill);
    }
}

Yes, it's a bit of repetition but it can be macro'ed away.

The other benefit is that you likely don't want to expose both connection and emission in the trait, but just one of them -- and this API can be slimmer.

[pablopenna]

Sorry @Bromeon , I think I am not explaining myself properly.

As per your last comment, I understand that a struct implementing the trait can emit a signal without it being in the trait.

My issue is with the consumers of the structs that implement said trait.

In my example above, the BattleEntity node has a reference to a Rust node that implements the SkillChooser trait (DynGd<Node, dyn SkillChooser>). BattleEntity does not now which implementation of the trait is. I can have several implementations like HighestDamageSkillChooser or PlayerSkillChooser.

The idea is that BattleEntity calls the choose_skill method from the Rust Node that implements SkillChooser and then waits for the skill_chosen signal to be emitted by that same Rust Node.

A key point that I think I have missed to mention is that the skill_chosen signal can be emitted asynchronously. What I mean with this is that in some cases it cannot be determined during the choose_skill method call. In the case of the player, the implementation has to wait for an interaction of the player with the UI where they are picking the skill they want to use.

Because the reference BattleEntity has of the Rust node is of type DynGd<Node, dyn SkillChooser> (not Gd<HighestDamageSkillChooser> nor Gd<PlayerSkillChooser>), I cannot get the skill_chosen signal reference from it and thus cannot connect it.

#[derive(GodotClass)]
#[class(base=Node2D)]
pub(crate) struct BattleEntity {
    ...
    #[export]
    skill_chooser: OnEditor<DynGd<Node, dyn SkillChooser>>,
    ...
}

pub(crate) fn decide_skill(mut this: Gd<Self>) {
    ...
    //Cannot do the line below
    skill_chooser.signals().skill_chosen().connect(self, move |skill| Self::cast_skill(this, skill));
    
    skill_chooser.dyn_bind_mut().choose_skill(&skills_container, &target);
 }

[Bromeon]

The idea is that BattleEntity calls the choose_skill method from the Rust Node that implements SkillChooser and then waits for the skill_chosen signal to be emitted by that same Rust Node.
[...]
Because the reference BattleEntity has of the Rust node is of type DynGd<Node, dyn SkillChooser> (not Gd<HighestDamageSkillChooser> nor Gd<PlayerSkillChooser>), I cannot get the skill_chosen signal reference from it and thus cannot connect it.

I might be missing something, but in this case, can you not add a method to the trait that returns the signal, or even a future using TypedSignal::to_future()?

There are many things one can do with a signal (connect, emit, await, introspect, ...) and returning a future would be a more specialized API that cannot be used to do anything else with the signal.

[pablopenna]

Thank you for the suggestion. Apologies for not realizing that. I will try that.