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

Make C# signal connections straightforward like GDScript 2.0 is #5835

Closed
nonunknown opened this issue Nov 23, 2022 · 7 comments
Closed

Make C# signal connections straightforward like GDScript 2.0 is #5835

nonunknown opened this issue Nov 23, 2022 · 7 comments

Comments

@nonunknown
Copy link

Describe the project you are working on

Kart Racing Game

Describe the problem or limitation you are having in your project

Currently in order to connect to a signal you must do:

        kart.Connect(nameof(SignalName),new Callable(this, nameof(functionName)));

which is cumbersome

Describe the feature / enhancement and how it helps to overcome the problem or limitation

The solution is make it looks like gdscript2.0 which is quite simple:

signal_name.connect(function_name)
or
signal_name.connect( func(): do_stuff_here.... )

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

the solution is:

 SignalName.Connect(FunctionName);

and lambda support

SignalName.Connect(  ()=>{GD.Print("This is awesome")} );

If this enhancement will not be used often, can it be worked around with a few lines of script?

no

Is there a reason why this should be core and not an add-on in the asset library?

this requires changes on core

@nonunknown nonunknown changed the title Make C# signal connections straightforward with GDScript Make C# signal connections straightforward like GDScript 2.0 is Nov 24, 2022
@KoBeWi
Copy link
Member

KoBeWi commented Nov 24, 2022

Didn't C# support events? Something like signal += method.

@nonunknown
Copy link
Author

nonunknown commented Nov 24, 2022

LOL when creating this proposal I really forgot about this one! should I remove this proposal, or it makes sense to make connect that way, or at least then the COnnect function should be removed from C# then! because if its goin to remain then it should be improved!

Also keep in mind that using event requires to manually disconnect it, or bugs can happen:

public override void _EnterTree()
{
 signal += MethodName;
}

public override void _ExitTree()
{
 signal -= MethodName;
}

@KoBeWi
Copy link
Member

KoBeWi commented Nov 24, 2022

I don't think it can be removed easily. The API is auto-generated AFAIK.

@raulsntos
Copy link
Member

I don't think it's a good idea to remove Connect because some users may still find it useful as a last resort when they are working with dynamic untyped code (same use case as Get, Set, ...). Also, internally we use it to connect the C# events, see the generated event for Node's ready:

public event Action Ready
{
    add => Connect(SignalName.Ready, Callable.From(value));
    remove => Disconnect(SignalName.Ready, Callable.From(value));
}

Also keep in mind that using event requires to manually disconnect it, or bugs can happen

Not if they are Godot signals, the events generated by our source generators are just wrappers around the Connect and Disconnect methods and we automatically disconnect signals when the object is disposed (see https://github.com/godotengine/godot/blob/f6f8a48459f9bbe97ee76a7186b9ae37e71e724b/modules/mono/csharp_script.cpp#L1743-L1745).

@nonunknown
Copy link
Author

oh awesome, didnt know that! also I would like to know from you @raulsntos if this proposal makes sense or not!

@raulsntos
Copy link
Member

We can't add methods to C# events so we can't add the Signal.Connect syntax. I think C# developers should already be familiar with C# events and I believe it's as convenient as the GDScript signal.connect syntax, so in C# using events will be the preferred way to connect to signals moving forward.

Also, C# events provide type safety since it enforces the callback method to have the same signature as the signal, this is what users that choose a strongly typed language would want so I believe they'll be happy with the new event signals.

Users that are not very familiar with C# would also benefit from learning to use C# events since it's a concept they can then use in other non-Godot C# projects.

Documentation is still in progress (see godotengine/godot#64930) but basically, you can connect to a signal in various ways:

Connect(SignalName.MySignal, new Callable(this, nameof(MyCallback));
// Creating a Callable from a method group
Connect(SignalName.MySignal, Callable.From(MyCallback));
// Creating a Callable from a lambda
Connect(SignalName.MySignal, Callable.From(() => MyCallback());

// Using C# events with a method group
MySignal += MyCallback;
// Using C# events with a lambda
MySignal += () => MyCallback();

It's preferable to use the exposed signal names in the SignalName class because it's a cached StringName and that way you avoid allocating a new StringName, if you use a string literal (e.g.: "MySignal" or nameof(MySignal)) the string will be implicitly converted to StringName which has a cost. Also, using the SignalName class makes it easier to deal with the snake_case built-in signals.

There is an existing issue opened about Callable.Bind missing in C# (see godotengine/godot#66106). If you use the event syntax you can use lambdas and closures:

int a = 42;
MySignal += () => MyCallback(a);

The variable a is captured by the closure, it would be the equivalent of this GDScript:

var a = 42
my_signal.connect(my_callback.bind(a))

@nonunknown
Copy link
Author

ohh awesome, so my proposal became useless at all, because didnt know about Callable.From() which already make syntax sugary xD, thanks for the info!

@Calinou Calinou closed this as not planned Won't fix, can't repro, duplicate, stale Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants