C#: Support signals inherited from interfaces in classes

[Thomas-X]

Extends the ScriptSignalsGenerator to search for signals in the interface(s) that the class implements. Resolves #80033

Example usage

public IHealth
{
    [Signal]
    public delegate void OnHealthChangedEventHandler(int health);
}

public class Warrior : Node, IHealth
{
   public override void _Ready()
   {
       OnHealthChanged += () => { ... }; // available and re-usable through the IHealth interface
   }
}

Supports the following edge cases

Using the existing Diagnostics logic that was in place

SignalNameIsDuplicateAcrossImplementedInterfaces

Imagine something like:

public interface ISpell
{
    [Signal]
    public delegate void OnSpellCastEventHandler(int spellId);
}
public interface ISpell2
{
    [Signal]
    public delegate void OnSpellCastEventHandler(int spellId);
}

public class Warrior : Node, ISpell, ISpell2 // GD0204: Signal defined in OnSpellCastEventHandler already exists in other interfaces that are implemented in Warrior class.

SignalNameAlreadyExistsInClass

Imagine something like:

public interface ISpell
{
    [Signal]
    public delegate void OnSpellCastEventHandler(int spellId);
}

public class Warrior : Node, ISpell // GD0205: Member OnSpellCast in Warrior conflicts with signal OnSpellCastEventHandler from the interface(s) the class Warrior implements.
{
    public int OnSpellCast = 1;
}

Compatibility

I think this shouldn't break compatibility because having Signals in interfaces was not a feature before, so these errors should only occur if an user is implementing the features this PR adds.

Relevant doc PR: godotengine/godot-docs/pull/8318

[DavidGeg]

Can this please get reviewed? It is an important fix for C# developers who wish to rely upon the contract that an interface provides especially for signals as they are core to Godot.

[avnotaklu]

Is there any update on this issue?

[paulloz]

Is there any update on this issue?

This is on my (never-ending) list of reviews to do.

Of course, like any other feature, if other users treat this branch and comment with the problems (or lack thereof) they encounter, it helps.

[tehKaiN]

Works stable on my end after rebasing on 4.2 branch, but I have some problems with it:

1. signal can't emit interface type in its argument, like following code. Might be related to Add support for exporting C# interface types as Nodes or Resources godot-proposals#8722 so perhaps out of scope for this PR. No biggie for me.

public interface IDamageable
{
    [Signal]
    delegate void DestroyedEventHandler(IDamageable damageable); // Requires using `Node` or something else instead

    void Damage(float damageAmount);
}

2. Doesn't work with inherited interfaces. If it's a limitation and not something that could be easily fixed, it lacks proper error for it. No biggie for me too.

public interface ITeamMember
{
    [Signal]
    delegate void TeamChangedEventHandler(Node teamMember); // TODO: ITeamMember

    Team Team { get; set; }
}

public interface ITargetableTeamMember : ITeamMember, IMapObject { }

public partial class Turret : CharacterBody3D, ITargetableTeamMember
{
// ...
            EmitSignal(SignalName.TeamChanged, [this]); // this will emit CS1061 unless ITeamMember is explicitly in class interface list
// ...
}

3. Can't connect to signal via interface ref - now that's a biggie.

Without looking at sources - it kinda looks like signal isn't present at the interface itself, and they are only created in classes implementing it. Might be tricky to fix, huh?

if (node is ITeamMember member)
{
    member.TeamChanged += OnTeamChanged;
}

I'm a stakeholder for this PR, so I'll try to tackle it, starting with 2 since that should be the easiest one, and then 3, but I'm new to this whole Godot thingy, so that could take some time for me. It'd be greatly appreciated if someone else (PR author?) fixed it for me sooner. ;)

[tehKaiN]

Pardon for two comments one beneath the other, but I'm doing it for better visibility and perhaps getting some hints from whoever received notification and glanced at my original message.

I fooled around a bit around godot codebase and managed to fix 2), unless my approach has some side effects which I don't see yet. The fix is now in pending PR on @Thomas-X's fork.

I think that I know how to tackle 3). Assuming that csharp source generator emits something like following in codegen part of a user-defined partial class:

    private global::Godot.SourceGenerators.Sample.IHealth.OnHealthChangedEventHandler backing_OnHealthChanged;
    /// <inheritdoc cref="global::Godot.SourceGenerators.Sample.IHealth.OnHealthChangedEventHandler"/>
    public event global::Godot.SourceGenerators.Sample.IHealth.OnHealthChangedEventHandler OnHealthChanged {
        add => backing_OnHealthChanged += value;
        remove => backing_OnHealthChanged -= value;

and remaining functions EmitSignal/HasSignal/etc. for handling signals are overrides in a given class, only event global::Godot.SourceGenerators.Sample.IHealth.OnHealthChangedEventHandler OnHealthChanged needs to be visible on the inerface side. I did a quick hack and modified the interface:

    public interface IHealth
    {
        [Signal]
        public delegate void OnHealthChangedEventHandler(int health);

        event IHealth.OnHealthChangedEventHandler OnHealthChanged; // nasty stuff added manually
    }

and it looks like the signal can be properly registered on the interface level, as += and -= and backing field are properly handled by the implementing class.

So, one could either add that extra line which doesn't really make sense unless one knows how codegen works, or there could be a strong requirement to make interface partial (to similarly godot classes) and generate that part separately. I'm opting for latter but I don't really know how to tackle this properly.

From the quick inspection, it looks like the codegen for classes uses SelectGodotScriptClasses() LINQ expression to filter out which classes should receive extra treatment, and that's easily doable because they all inherit from Node or other godot type. For interfaces there is no codegen currently, and I don't know how to filter through relevant ones since there's no common ancestor, so either there needs to be one (INode or something, could be empty for now) or they could be scanned if they have godotic stuff inside them (like [Signal]s etc.). I think the common ancestor is a cleaner approach, but that's quite a big change and I'd like to get some opinions on this.

[Thomas-X]

Nice work @tehKaiN, merged your PR. Let's hope this PR gets merged soon 🤞

[raulsntos]

Thanks for contributing to the .NET module. Unfortunately, after careful consideration we won't be merging this PR. We apologize for the inconvenience.

The team discussed and decided that adding signals to interfaces is not in the best interest of the bigger picture, since the concept of interfaces doesn't exist in Godot. Signals are always registered in classes/scripts, so we feel like adding signals to interfaces is misleading.

We originally thought generating the signal's event in the class that implements the interface was an amazing idea. But, as pointed out by recent comments, this means you can't subscribe to the event when using the interface (you'd have to cast to the concrete type first). This limitation we failed to anticipate, made us re-evaluate this PR through a wider lens.

It was suggested to generate the event in the interface itself, but we don't want to generate code in types not exposed to Godot (as is the case with interfaces). In addition, adding signals to interfaces would invite users to question why interfaces can have signals but can't be used as signal parameters, or as exported properties. We believe this puts us in a weird spot that makes it look like interfaces are somewhat supported, but not everywhere, and that can be confusing.

In most cases, you can fulfill the same use case with normal C# events instead of Godot signals.

Again, we apologize for the inconvenience and taking so long to review this. Thanks for the contribution nonetheless.

[Sythelux]

I'd suggest a warning similar to the "class must be partial" one saying "interfaces can't have delegates with [Signal]" or something at least. so people don't run into a trap, because then currently there is nothing keeping you from trying to add that annotation and then wondering, why it isn't working.

[tehKaiN]

Excuse me, but I don't really get your point.

About not being able to use interface type as event parameter - builtin events usually don't expose emitter as signal parameter, so the official signaling convention skips this problem, and I'm cool with making captures or object-specific event handlers. When I really need the "this" parameter I can use type-safe "as" casting, so it's all cool for me.

About interfaces not being part of engine - ain't type traits/interfaces on the gdscript roadmap? Won't that be added to C# side as interfaces? That means it will eventually be solved, and from user's perspective I don't really mind a partial feature which allows more of its nuances with next engine releases - warning about unsupported things with custom analyzers is the way.

Signals are superior to bare C# events because they have extra layer of safety (well not completely because of #70414 but that can be worked around and I hope it'll be resolved soon instead of marked as wontfix) and using signals solely is better in the codebase because it makes it uniform.

I'd fight a bit more for implementing signals/interfaces properly because it's crucial part of modern programming techniques. If it's not ready for merging in yet, let's construct a high-level roadmap for making it happen - I'm a stakeholder for this feature, am here to stay for quite some time and willing to tackle anything that's in a way of adding it.

Or am I really stuck with custom godot builds with cherrypicking those changes on top of each release?

[raulsntos]

About interfaces not being part of engine - ain't type traits/interfaces on the gdscript roadmap? Won't that be added to C# side as interfaces?

We can't assume how such a potential future feature would be designed. And we therefore can't assume how we would handle interfaces at that time. We suggest following these related proposals:

• Allow explicitly defining interfaces in GDScript godot-proposals#4872
• Add a Trait system for GDScript godot-proposals#6416

I'd fight a bit more for implementing signals/interfaces properly because it's crucial part of modern programming techniques. If it's not ready for merging in yet, let's construct a high-level roadmap for making it happen

This specific PR being closed isn't supposed to end all discussions about interfaces and Godot. We indeed suggest discussing it on a higher-level, e.g.:

• Allow using interface as exported property type godot-proposals#307
• Add support for exporting C# interface types as Nodes or Resources godot-proposals#8722