C#: Cleaner signal emission

[OlliO6]

I'd thought that emitting signals from c# does have some problems and could be a lot better.
So i made them better :)

Currently when you want to emit a signal from c# it would look something like this:

// From the class itself
EmitSignal(SignalName.SomethingHappened, arg1, arg2);
// From an other object (you don't usually do that)
obj.EmitSignal(ObjNamespace.ObjType.SignalEmit.SomethingHappened, arg1, arg2);

I think that this looks a bit clunky, however with this PR a property named Emit will be generated it is of the newly generated type called SignalEmit.
The SignalEmit type works a bit like the SignalName type and it will contain functions to quickly emit all available signals.

// From the class itself
Emit.SomethingHappened(arg1, arg2);
// From an other object
obj.Emit.SomethingHappened(arg1, arg2);

This already looks better and is a lot more readable in my opinion.
But there is another big advantage, the parameters will match the types and the names that the signal EventHandler uses.
So when you for example add an argument or change the type of an argument from the signal EventHandler, you will instantly get error syntax highlighting and the compilation won't even accept signal emissions that would not work and break the game.

[raulsntos]

• Alternative: C#: Generate strongly-typed method to raise signal events and fix event accessibility #68233

[raulsntos]

Thanks for contributing!

I prefer #68233 because it allows you to override the emit methods and follows the event guidelines more closely but this is an interesting alternative.

[neikeq]

My problem with this one is the need for extra allocations, one for each derived type. I understand it's needed for inheritance in order for Emit to include the base signals, but the overhead is not worth it IMO.

I would have preferred this solution if it weren't for the allocations, as it does not pollute the members.

[neikeq]

Syntax-wise I think this can be achieved with structs + default interface implementations. That would avoid the allocations. Unfortunately, I think accessing the bound field from the default interface implementation would result in a virtual call.

EDIT: Never mind... There's a limitation with structs and default interface implementations. You need to box structs in order to access default interface implementations... I love C# 🥰

[OlliO6]

Thats like telepathy, i thought about using structs or records with default interface implementations to.
Can't we use inherited record types or would that also be bad because of inheritence stuff.
And what do you mean with boxing the struct?

Edit: Ok, i now know what boxing means 👍

[neikeq]

Can't we use inherited record types or would that also be bad because of inheritence stuff.

Internally, records are just classes, so they also allocate. record struct has the same limitation as struct in not supporting inheritance.

[OlliO6]

I think i found something that works without the bixing issue. It's a bit hacky though. https://twitter.com/FreyaHolmer/status/1414921814564876289?t=m4g3M0ZZOoWDs12mpz0oUw&s=19

[neikeq]

That's very good if it indeed works, and I wouldn't consider it hacky in my book. The downside is that extension classes can't be nested, so they pollute the namespace.

[OlliO6]

I would just have one partial static class containing all emit extensions.
And i can confirm it works.

[neikeq]

I would just have one partial static class containing all emit extensions. And i can confirm it works.

Indeed, although it would have to be in the global namespace. Sounds good to me. I wonder what @raulsntos think of this compared to his solution. I think adding each emit method to the class pollutes the members. Specially considering we want to add them to the native types as well. That's why I gravitate more towards this Emit.SignalName() solution over OnSignalName.

[raulsntos]

I'd prefer to follow the .NET guidelines, I think it would be more idiomatic and would feel more familiar to .NET developers.

Also, would this Emit property be public or protected? Because in my PR the OnSignalName methods are protected as per the guidelines since it's not usually a good idea to raise an event from the outside. The methods are also virtual which may be useful in inheritance scenarios but I don't have a strong use case for this right now.

I kind of agree with your concern about polluting the members, which is why I wish we could just do SignalName.Invoke. But to be honest it doesn't really bother me that much, I mean, how many signals can there be in a Node?

[neikeq]

... which is why I wish we could just do SignalName.Invoke. ...

I very much wish that was the case. I can't believe events have add and remove overloads, but not raise.

[OlliO6]

Also, would this Emit property be public or protected?

I think it should be public since EmitSignal already is public, but it could also be protected if that's preferred.

And my goal actually was to create something similar to SignalName.Invoke.

[raulsntos]

I think it should be public since EmitSignal already is public

I don't see EmitSignal and this new API as the same thing, I think of EmitSignal the same way I see Get and Set. Take a look at this example code:

[Export]
public int PublicNumber { get; set; }

[Export]
private int _privateNumber;

You'll be able to get and set both members with the Get and Set method because they are both exported, regardless of their accessibility, but member accessors in C# won't let you access the private one:

GD.Print(myObj.PublicNumber); // OK
GD.Print(myObj._privateNumber); // Compiler error

GD.Print(myObj.Get("PublicNumber")); // OK
GD.Print(myObj.Get("_privateNumber")); // OK

I think once you use Get, Set and the rest of the "dynamic" API you have explicitly opted-out of type safety, it's kind of like using reflection.

In the same way, I think if you use EmitSignal you are using the more "dynamic" API that allows you to access Godot's API directly but without the safety guarantees of a strongly typed API. It's a lower level API that I expect would only be used in advanced scenarios, whereas the new API would try to push users towards good practices.

If you need to emit a signal from the outside you can always expose a method in your class that emits the signal. This private-by-default approach requires you to explicitly expose the emitting methods for each signal that you want exposed so that there's intention behind it, rather than accidentally allowing outsiders from raising events which breaks OOP encapsulation.

And my goal actually was to create something similar to SignalName.Invoke.

Note that SignalName.Invoke doesn't allow you to raise events from outside the class that declares the event, in fact you are not even able to invoke an event from a derived class, which is why .NET guidelines recommend you to implement the OnSignalName methods as protected so they can be raised from derived classes.

[OlliO6]

Ok, the way you see EmitSignal makes sence to me, so i think making it protected could be a good call.
And I for some reason mixed up gdscript's SignalName.emit and c#'s EventName.Invoke.

[OlliO6]

There's a problem and i can't find a good solution.

For example Node2D has a VisibilityChanged signal and Node3D has one too.
That would be fine but unfortunately it's not possible to do something like this in the extensions class:

public static void VisibilityChanged<T>(this T from) where T : Node2D.ISignalEmit
{
    from.Bound.EmitSignal(Node3D.SignalName.VisibilityChanged);
}
// Makes compile error
public static void VisibilityChanged<T>(this T from) where T : Node3D.ISignalEmit
{
    from.Bound.EmitSignal(Node3D.SignalName.VisibilityChanged);
}

[raulsntos]

Generic constraints are not part of method signatures so they don't count for overload resolution.

Maybe you can create a different extension class for each Node type, but I think then the method invocation would be ambiguous and would force you to be explicit by specifying the extension class in the invocation.

[OlliO6]

Generic constraints are not part of method signatures so they don't count for overload resolution.

Maybe you can create a different extension class for each Node type, but I think then the method invocation would be ambiguous and would force you to be explicit by specifying the extension class in the invocation.

That would pollute the Godot namespace a lot since you can't have a extension class as a subclass.

[OlliO6]

I just thought about this again and I don't think that this whole Emit.SomethingHappened() idea can work in c# without bad allocations or bad pollution.

But suddenly this idea came to mind:

Signal.SomethingHappened.GetEmitter(this).Emit(arg);
// 'SignalName' I think should be renamed to just 'Signal' for this purpose because it will have more functionality than just retrieving the name.
// Or
Signal.SomethingHappened[this].Emit(arg);
// Or ('Signal' is 'SignalName' again)
GetSignalEmitter(SignalName.SomethingHappened).Emit(arg);

All of these solutions share one main behaviour, that is getting a struct and calling it's Emit method.
Personally, I like the first or third the most.

The struct could be somewhere like ClassName.SignalEmitter.SomethingHappened

Edit:

The last one would be like this:

GetEmitter<SignalEmmiter.SomethingHappened>().Emit(arg);