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

[C#] Allow using the Signal attribute on events #94440

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

interacsion
Copy link

Motivation

The current method to declare a signal in a C# class requires declaring an inner delegate type. This prevents polymorphism using interfaces since unrelated classes cannot declare a signal of the same type.
Closes #80033, closes godotengine/godot-proposals#9373

Usage

This PR enables using the Signal attribute on events of existing delegate types (such as Action):

[Signal]
public event Action MySignal;

Emitting the signal is still done using EmitSignal(SignalName.MySignal), but now the event can be declared in an interface.

Potential Drawbacks

  • This exposes the Invoke method on signals declared using an event, which shouldn't be used.

@interacsion interacsion requested a review from a team as a code owner July 16, 2024 16:05
@AThousandShips AThousandShips added this to the 4.x milestone Jul 16, 2024
@raulsntos
Copy link
Member

This exposes the Invoke method on signals declared using an event, which shouldn't be used.

That's exactly the reason why we don't allow using the [Signal] attribute on C# events. We considered doing this during the 4.0 development, and reached the conclusion that it was not worth the trouble.

Calling Invoke on the C# event will only raise the C# event and not the Godot signal. C# users familiar with events will likely try to use Invoke without realizing this, and we don't want to make it easy for users to shoot themselves in the foot.

If there was a way to override the Invoke method to ensure it calls EmitSignal behind the scenes, we would most certainly have used event instead of delegate to declare signals. Unfortunately C# doesn't allow this, and it's unlikely to ever happen.

@interacsion
Copy link
Author

I understand. However, as I see it Interfaces are one of the main reasons to use C# over GDScript, and restricting Signals to Classes is definitely counter-productive.

But in theory, can't we add EmitSignal as a handler to events and let C# handlers to be invoked directly?

@paulloz
Copy link
Member

paulloz commented Oct 13, 2024

If not exactly related, I feel this touches the same discussion points as #83889.

@interacsion
Copy link
Author

#83889 was trying to solve the same problem, but inheritance from interfaces doesn't make a lot of sense; My solution enables declaring signals in interfaces as properties while still requiring the field to be part of the class, even if it's generated by [Signal]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move C# Signal declaration on event instead of delegate Cannot inherit signals from interfaces
4 participants