PR #68233 Broken project compatibility

[scgm0]

Tested versions

v4.4.dev3.mono.official [f4af8201b]

System information

Godot v4.4.dev3.mono - Arch Linux #1 ZEN SMP PREEMPT_DYNAMIC Tue, 01 Oct 2024 14:40:29 +0000 on Wayland - Wayland display driver, Single-window, 1 monitor - Vulkan (Forward+) - dedicated NVIDIA GeForce GTX 1650 (nvidia; 560.35.03) - Intel(R) Core(TM) i5-9300H CPU @ 2.40GHz (8 threads)

Issue description

Before #68233, people used to use the On{SignalName} method as the callback method of Signal, because this is the naming convention in godot, and in gds it is on_signal_name. However, #68233 will generate the On{SignalName} method for custom signals for emit signals, which is different from the previous specification and may cause compatibility issues. For example, this c# addon: https://godotengine.org/asset-library/asset/2324

Steps to reproduce

as mentioned above

Minimal reproduction project (MRP)

N/A

[magian1127]

I have a project that extensively uses OnXX, and PR #68233 has broken the project. I need to make significant modifications to the signals in order to use 4.4.

[raulsntos]

Thanks for the report, we are aware of this issue and currently evaluating the situation. We'd love to get more data on how many projects are broken by https://github.com/godotengine/godot/pull/68233.

[TML233]

I solely use C# events for custom "signals", but I do use OnXxx for event & signal callback functions. If I convert those events to signals, many self-listening signal connections will break immediately.

[mpewsey]

Rather than generating a new emit method, which, depending on its name, could break projects, would it be possible to return a generated wrapper struct like the below example instead?

The example demonstrates what would roughly be generated for an OnHitPointsChanged signal. The wrapper is written with + and - operator overloads so that it can be used as a drop-in replacement for the existing generated event, but can include the desired additional emit functionality. It also opens things up so that additional functionality could potentially be added in the future without running into additional name conflict issues.

A minimal project with the example is here:

GodotEventWrapper-main.zip

using Godot;

[GlobalClass]
public partial class EventTest : Node
{
    // This is a delegate that would normally be wrapped with the Signal attribute.
    // [Signal]
    public delegate void OnHitPointsChangedEventHandler();

    // *** Generated Class Contents Begin Here ***
    private OnHitPointsChangedEventHandler backing_OnHitPointsChanged;

    public OnHitPointsChangedEventWrapper OnHitPointsChanged
    {
        get => new OnHitPointsChangedEventWrapper(this);
        set { } // This needs to be exposed so that we can use the += and -= operators, but it does nothing.
    }

    public readonly struct OnHitPointsChangedEventWrapper
    {
        /// The source type should match the generated class name.
        private EventTest Source { get; }

        public OnHitPointsChangedEventWrapper(EventTest source)
        {
            Source = source;
        }

        public static OnHitPointsChangedEventWrapper operator +(OnHitPointsChangedEventWrapper wrapper, OnHitPointsChangedEventHandler value)
        {
            if (IsInstanceValid(wrapper.Source))
                wrapper.Source.backing_OnHitPointsChanged += value;
            return wrapper;
        }

        public static OnHitPointsChangedEventWrapper operator -(OnHitPointsChangedEventWrapper wrapper, OnHitPointsChangedEventHandler value)
        {
            if (IsInstanceValid(wrapper.Source))
                wrapper.Source.backing_OnHitPointsChanged -= value;
            return wrapper;
        }

        /// The signature of this would changed based on signal arguments.
        public void Emit()
        {
            if (IsInstanceValid(Source))
                Source.backing_OnHitPointsChanged?.Invoke();
        }
    }

    // *** Example Usage Begins Here ***
    public override void _Ready()
    {
        base._Ready();
        OnHitPointsChanged += Action1;
        OnHitPointsChanged += Action2;
        OnHitPointsChanged.Emit();
        OnHitPointsChanged -= Action2;
        OnHitPointsChanged.Emit();
    }

    private void Action1()
    {
        GD.Print("Action 1 Fired.");
    }

    private void Action2()
    {
        GD.Print("Action 2 Fired.");
    }
}

[TheSecondReal0]

This method naming strategy also goes against some existing documentation, for example the C# signals article:

[Export]
public MyClass Target { get; set; }

public override void _EnterTree()
{
    Target.MySignal += OnMySignal;
}

public override void _ExitTree()
{
    Target.MySignal -= OnMySignal;
}

Link: https://docs.godotengine.org/en/4.3/tutorials/scripting/c_sharp/c_sharp_signals.html

Given that this naming scheme is provided in official documentation, it's likely that many projects are using it. Projects using this naming scheme would break anywhere objects connect to their own signals. There's also a chance of breaking projects where objects declare and connect to signals with the same names (like mine).

[scgm0]

Is there any plan to fix this issue?