godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.14k stars 93 forks source link

Move C# Signal declaration on event instead of delegate #9373

Open ultrasuperpingu opened 6 months ago

ultrasuperpingu commented 6 months ago

Describe the project you are working on

I'm implementing a library defining different kinds of "Path3D" (BSpline, Hermite, etc) and I'd like to use the regular Path3D as an implementation for Bezier.

Describe the problem or limitation you are having in your project

I have a interface ICurve3D declaring all methods/properties/etc I need.

For my curves implementation, I made an abstract class implementing those (and declaring abstract methods/properties/etc for some of them). I declared a Signal on this class, so, it's ok for all my classes deriving from this.

But I also implemented a class derivated from Path3D (so it cannot derivate from my abstract class) and implemeted the ICurve3D interface. And for it, as I can't declare the signal from the interface, when I have an instance of the interface, I can't use the signal...

For now, I check if the interface is an instance of my abstract class or an instance of my class derivated from Path3D to register to the event but it's not really a good thing.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

For now, declaring a signal for a class is made by:

[Signal]
public delegate void MySignalEventHandler();

I tried to declare the signal in the interface and it compiles but it does not work (the event is not generated in class implementing the interface.

I suggest to modify with to:

public delegate void MySignalEventHandler();
[Signal]
public event MySignalEventHandler MySignal;

So, I could use it like that:

public interface IInterface
{
    public delegate void MySignalEventHandler();
    public event MySignalEventHandler MySignal;
}
public partial class Concrete : Node3D, IInterface
{
    [Signal]
    public event IInterface.MySignalEventHandler MySignal;
}

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Here, I'm not sure to understand how the code generator works. I saw it implements the event like this:

    private global::MyType.MySignalEventHandler backing_MySignal;
    /// <inheritdoc cref="global::MyType.MySignalEventHandler"/>
    public event global::MyType.MySignalEventHandler MySignal {
        add => backing_MySignal += value;
        remove => backing_MySignal -= value;
}

I guess the backing_MySignal is needed fo some reason but I also think (hope) it's possible to do that an other way...

If this enhancement will not be used often, can it be worked around with a few lines of script?

No, it needs to be core.

Is there a reason why this should be core and not an add-on in the asset library?

It's not possible in an add-on.

poohcom1 commented 4 months ago

I am highly in favor of this. Right now I have 2 major issues with the current signal implementation:

  1. Lack of type-safe arguments
  2. Bad IDE refactor and lookup support

Issue 1 would be resolved with current signal rework PRs like https://github.com/godotengine/godot/pull/68233 or https://github.com/godotengine/godot/pull/70424. However, this doesn't address the second issue of IDE support. Because the signal event is source generated, it doesn't respond to IDE refactor or lookup requests. Renaming the delegate won't rename the signals and you'll need to fix it manually. If we flip it around and make the event part of the main code and the rest source generated, there would be no issues with renaming the event.

However, I'm fairly certain there are major technical or performance reason that the C# contributors went for the source generated solution. Maybe someone more familiar with the C# signals could give more insights on this.