Skip to content

Inherited typed signals #1134

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

Merged
merged 11 commits into from
Apr 21, 2025
Merged

Inherited typed signals #1134

merged 11 commits into from
Apr 21, 2025

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Apr 21, 2025

Following previous work on typed signals in #1000 and #1111.
Closes #1112.

Features

It is now possible to use signals from the base class without the .clone().upcast().signals() workaround:

let mut node: Gd<MyClass> = ...;

node.signals()
    .renamed()
    .connect(|| println!("renamed"));

or from self:

self.signals()
    .renamed()
    .connect_self(Self::handler);

You can also trivially emit base signals:

self.signals().renamed().emit();

Alternatives and trade-offs

This took me forever since I kept running into a wall with inheritance and trait hell. I had many ideas, which I implemented to varying degrees. Writing them down in case the question comes up why the current approach was chosen and not X, or to better understand what exactly led to this design.

  1. Make TypedSignal completely "unlinked" without lifetime 'c, letting it only store a Gd<T>.

    • Works well for connecting (which just calls Godot APIs), however emit can easily cause double-borrow errors when invoked from self.
    • This means we have to keep &mut self borrowed (precisely: BaseMut guard), which then requires a lifetime.
  2. Back and forth between the "signal collection" (struct returned by signals()) being able to call one or multiple signals.

    • Allowing let s = obj.signals(); s.first_sig(); s.second_sig(); is possible, but requires 2nd lifetime TypedSignal<'a, 'c, ...>.
    • 2nd lifetime 'a renders temporary calls obj.signals.first_sig() impossible, which is a big downgrade.
  3. Signal collection methods taking self or &mut self:

    • Owned: seems logical if only 1 signal can be called anyway.
    • However, this doesn't work with Deref-enabled methods, so can't work for base classes.
    • So, I settled for &mut self with runtime checks to be invoked only once. Will be documented.
  4. Generic argument C (the static class on which signals() is invoked)

    • Previously, TypedSignal was just typed with the class that declared the signals (e.g. Node). Now C can be MyClass even if a Node signal is manipulated.
    • C is necessary for only one thing: connect_self() type safety
    • This requires passing C through all signal collections and all individual signal structs.
  5. Instead of fluent API, use something like Signals::connect(self, Self::renamed, Self::handler)

    • Flatter -- wouldn't need to duct C and 'c deeply through multiple layers.
    • Constants Self::renamed cannot be Deref-inherited... so we'd still need Signals::connect(self.signals().renamed(), Self::handler)
    • For emit, we need to pass in &mut self but can only do so once; Signals::emit(self.signals().damage_taken(), 123) would then still need to deep-borrow self through self.signals().
  6. A macro connect! { Node::renamed => Self::handler }

    • Sounds good for simple cases, but would need fancy DSL for more complex ones.
    • Can still be added later, probably good to have a typed API as foundation anyway.
  7. Explicitly selecting signals through self.signals_of::<Node>().renamed()

    • My 2nd favorite approach and the one I would have gone with, had the current one not worked out (I was considering giving up a few times 😬 ).
    • It makes people aware where signals are declared and doesn't mix the scopes.
    • On the other hand, GDScript throws all signals together with all other symbols (func, var, const, global API, class names, ...) and it's not really a point of complaint. Often people don't actually care (e.g. button handlers are in BaseButton, not Button). We at least have a dedicated signals() namespace, and usually the number of total signals in a class is manageable.
    • Something like might still be interesting for generic programming based on Inherits<Base> bound, similar to the upcast_ref pattern.
  8. Duplicate signals in each derived class

    • Alternative to Deref, but generates a ton of symbols in both Godot and user APIs.
    • However, would work with far less generic code and wouldn't be subject to the "&mut self despite once" issue.

The current approach adds quite a bit of internal complexity and extra generic code. Hopefully the monomorphization is bearable; if it becomes a problem, we might look for simpler alternatives, e.g. splitting connect/emit APIs or duplicating signals in each class rather than Deref.

Either way, the signals API is completely new, and this PR is just another addition in the chain of many before it. I'd like to settle for an initial version, but future API changes are still possible. We'll likely see more of the practical impact once more users start working with typed signals.

Bromeon added 11 commits April 21, 2025 09:23
Upcast: signal collection jumps to nearest superclass with own signals.
Import some symbols inside generated `signals` module.
Allows type-safe connect_self().
Addresses immediate issue where a `pub struct MyClass` has a private
#[signal] declaration. This caused "leaks private type" problems with
the associated type <MyClass as WithSignals>::SignalCollection, that
mentions a generated (private) type.

This approach can possibly be extended to the inverse case later, to
allow the inverse case: pub #[signal]s in private classes.
@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Apr 21, 2025
@GodotRust
Copy link

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

@Bromeon Bromeon added this pull request to the merge queue Apr 21, 2025
@Bromeon Bromeon added this to the 0.3 milestone Apr 21, 2025
Merged via the queue into master with commit c4b74c8 Apr 21, 2025
16 checks passed
@Bromeon Bromeon deleted the feature/signal-collection-inheritance branch April 21, 2025 13:36
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.

connect_self() is not available for engine classes
2 participants