Changing scope of generatated On{SignalName} C# functions.

[pmoosi]

I just started using the newly generated On{SignalName} functions introduced by #68233. They make emitting signals a lot nicer (type safety, enum types, null, ...).

These functions are protected virtual in accordance with the C# guidelines. However, this does not align with the scope of EmitSignal which is public. The latter makes sense in situations where signals are emitted outside of the signal defining class (e.g., when using some kind of EventBus).

I think it might be worth considering to make the generated function public virtual to be more in line with the current EmitSignal implementation.

[raulsntos]

We consider GodotObject methods like Get, Set, Call, and EmitSignal to be reflection-like methods so it makes sense those can access non-public members. However, we want the generated On{SignalName} methods to be idiomatic C# so we want those to be more strict regarding types and accessibility/visibility.

If you want to expose these methods you can always create a method to do so explicitly (you can also generate these methods with source generators if you want):

public void EmitMySignal()
{
    OnMySignal();
}

You can also use C# reflection to access non-public methods:

var obj = new MyClass();
var emitMySignal = typeof(MyClass).GetMethod("OnMySignal", BindingFlags.NonPublic | BindingFlags.Instance);
emitMySignal.Invoke(obj, null);

Or using [UnsafeAccessor]:

var obj = new MyClass();
EmitMySignal(obj);

[UnsafeAccessor(UnsafeAccessorKind.Method, Name = "OnMySignal")]
extern static void EmitMySignal(MyClass obj);

[pmoosi]

I'm not sure that the current approach is idiomatic C#. Signals are right now defined with the delegate keyword, so they should be invokable from outside. A better approach might be to allow Signals declaration with the event keyword (as in this pull request) and set the scope of the On... function depending on that.

I get the workaround, but the major disadvantage right now is that the On{SignalName} functions are not a drop-in replacement for EmitSignal in these cases.

[raulsntos]

The delegate is just the type of the event that is generated by Godot's source generators. We can't let the user declare the event directly because calling Invoke on it will not work as expected, it will only raise the C# event but not the Godot signal.

I get the workaround, but the major disadvantage right now is that the On{SignalName} functions are not a drop-in replacement for EmitSignal in these cases.

Yes, but we don't think these cases are the most common scenario. Signals are usually only emitted from inside the class that declares them, and in the rare case where an external class should be able to emit them it's common to expose a custom method that may do more than just emitting the signal.

[pmoosi]

Fair enough. I guess I will just keep using EmitSignal for these cases.