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

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

Open
ultrasuperpingu opened this issue Mar 24, 2024 · 1 comment · May be fixed by godotengine/godot#94440
Open

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

ultrasuperpingu opened this issue Mar 24, 2024 · 1 comment · May be fixed by godotengine/godot#94440

Comments

@ultrasuperpingu
Copy link

ultrasuperpingu commented Mar 24, 2024

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.

@ultrasuperpingu ultrasuperpingu changed the title Move C# Signal declaration on event instead of the delegate one Move C# Signal declaration on event instead of delegate Mar 24, 2024
@poohcom1
Copy link

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 godotengine/godot#68233 or godotengine/godot#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.

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

Successfully merging a pull request may close this issue.

3 participants