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 @Signal Macro #626

Merged
merged 1 commit into from
Dec 22, 2024
Merged

Conversation

samdeane
Copy link
Contributor

@samdeane samdeane commented Dec 6, 2024

Implements #586.
See also previous discussion on #584.

TL;DR

This PR adds a new @Signal macro, giving a cleaner and more consistent way to add user-defined signals from Swift.

@Godot
class MyNode: Node {
  @Signal var mySignal: SimpleSignal
  @Signal var signalWithArgs: SignalWithArguments<Int, String>
}

These signals can be used in the same way as built-in signals:

node.mySignal.connect { /* do something */ }

node.signalWithArgs.connect { intArg, stringArg in
  // do something
}

node.mySignal.emit()
node.signalWithArgs.emit(1, "foo")

Motivation

The existing #signal macro defines the signal as a static property on the class that contains it.

This is different from the generated implementation of built-in signals, and means that we have to use a different syntax to work with them -- which feels... non-optimal.

Implementation

We add a new @Signal macro expansion which generates a computed property on the class, using GenericSignal.
This is the same approach that is used to implement the signals imported from the Godot API.

We also add an emit() method to GenericSignal, which is of course essential to support emitting signals defined in Swift.

We add SimpleSignal and SignalWithArguments as slightly clearer aliases for GenericSignal

This change should be 100% additive, and should not break the old #signal macro.

To Do

If this change is adopted, it's probably worth updating the documentation and deprecating the old macro.

@samdeane samdeane marked this pull request as ready for review December 6, 2024 16:36
@migueldeicaza
Copy link
Owner

This is lovely! Would you mind testing the Object.callv approach? If it works, that might be better to maintain in the long run.

@samdeane samdeane force-pushed the new-signal-macro branch 3 times, most recently from 5678cae to 2862306 Compare December 18, 2024 11:43
@samdeane
Copy link
Contributor Author

Using Object.callv seems to work well.
Have also squashed all the commits for a cleaner history.

@migueldeicaza
Copy link
Owner

Thank you!

Merging.

@migueldeicaza migueldeicaza merged commit 9c15f48 into migueldeicaza:main Dec 22, 2024
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants