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

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

Merged
merged 2 commits into from
Sep 27, 2024

Conversation

raulsntos
Copy link
Member

@raulsntos raulsntos commented Nov 3, 2022

  • Generate signal event with the same accessibility as the delegate
    • Before this, if a user class declares a signal delegate with less accessibility than public it resulted in a compiler error because the delegate had less accessibility than the generated event which was always generated as public, with this PR the event is generated with the same accessibility that the delegate was declared with.
    • I can't find an issue about this but I'm pretty sure I saw someone affected by this, maybe it was outside of GitHub.
  • Generate On{EventName} method to raise signal events

Sample:

// User-defined signal
[Signal]
delegate void MySignalEventHandler(int a, float b, Node c, MyEnum d);

// Method generated by the source generator
protected void OnMySignal(int a, float b, Node c, MyEnum d)
{
    EmitSignal(SignalName.MySignal, a, b, c, (int)d);
}

@raulsntos raulsntos added this to the 4.x milestone Nov 3, 2022
@raulsntos raulsntos requested a review from a team as a code owner November 3, 2022 20:14
@kleonc
Copy link
Member

kleonc commented Nov 3, 2022

Generate On{EventName} method to raise signal events

  • Follows .NET naming conventions, (...)

In Godot there's kinda the opposite convention: names of the methods subscribed (connected) to the event (signal) by default start with the _on_ prefix (e.g. _on_Button_pressed). So On{EventName} might be confusing.

(...) but I can change it to Emit{EventName} if that's preferred.

Makes more sense to me.


I'd guess you've already considered many different options but do you think it would be possible to avoid incorporating {EventName} into the name of the method rasing the event? I mean e.g. having something like a EmitSignal<EventName> generic instead of Emit{EventName}.

@raulsntos
Copy link
Member Author

In Godot there's kinda the opposite convention: names of the methods subscribed (connected) to the event (signal) by default start with the _on_ prefix (e.g. _on_Button_pressed). So On{EventName} might be confusing.

That's true but we already prioritize .NET conventions over Godot conventions in other areas (e.g.: collections). You are probably right though that most users would be more familiar with Godot conventions in this case but I would love to gather more opinions on this before making a decision, that said I don't feel strongly either way about this.

I'd guess you've already considered many different options but do you think it would be possible to avoid incorporating {EventName} into the name of the method rasing the event? I mean e.g. having something like a EmitSignal<EventName> generic instead of Emit{EventName}.

In C# generic arguments are types and the name of the event is a string so this is not possible. Defining a method for each event follows the .NET guidelines, and since the methods are virtual it also allows you to customize how the event is raised in case the derived class needs it.

Also, keep in mind the EmitSignal method still exists if users prefer to use the more dynamic version like you can use Get and Set instead of the properties. This is necessary because otherwise C# scripts wouldn't be able to interop with more dynamic languages such as GDScript and internally the strongly-typed methods are just a wrapper over the EmitSignal method.

@neikeq
Copy link
Contributor

neikeq commented Nov 25, 2022

There's a conflict between the official .NET convention and the way Godot names callbacks (including the C# docs). I agree it's better to follow the .NET convention for the naming, but we need to think what to do about callbacks.

NOTE: This PR doesn't include the generated native types, which would benefit from this as well.

@OlliO6
Copy link

OlliO6 commented Nov 25, 2022

I first thought that you could override methods like for example OnBodyEntered so you would not need to connect it to yourself anymore, but since the bindings_generator isn't changed, i assume that's not the case. Personally i do agree to @kleonc that renaming On to Emit would be more understandable, because i think On sounds like it is connected to the signal/event but it's not, I mean when you emit the signal via EmitSignal it won't be called.

@raulsntos
Copy link
Member Author

but we need to think what to do about callbacks.

Isn't that up to the user to choose? Since they are the ones implementing those methods they are free to call them however they prefer. I guess my preference would be to name those methods after what they do and not after the signal that invokes them.

For example, let's say you have a signal named PlayerDied and a callback method that shows the Game Over screen. Instead of calling this method OnPlayerDied or PlayerDiedCallback I would call it ShowGameOverScreen:

event Action PlayerDied;

PlayerDied += ShowGameOverScreen;

That way the code reads: When the player dies, show the Game Over screen.

@raulsntos raulsntos force-pushed the dotnet/raise-events branch 2 times, most recently from 194a094 to c36e7dc Compare November 26, 2022 00:41
@neikeq
Copy link
Contributor

neikeq commented Nov 27, 2022

I meant we should think what to do about callbacks in the Godot documentation. Many examples use the OnFoo naming convention. For example, OnTimerTimeout in this example: https://docs.godotengine.org/en/latest/getting_started/step_by_step/signals.html#connecting-a-signal-via-code. That one in particular won't cause issues as it connects to a signal from another class (plus in such cases, the convention seems to be to include the class name). But for connections to self, that would be this.Timeout += OnTimeout (conflict between OnTimeout callback and OnTimeout signal emitter).

We can't apply this change before first addressing that.

@raulsntos
Copy link
Member Author

raulsntos commented Nov 27, 2022

In that particular example we could call the method ToggleVisibility, the name of each callback would have to be decided on a case-by-case basis. Also, if we are not willing to change GDScript's convention, are we OK with following a different convention for naming callbacks in C#?

@neikeq
Copy link
Contributor

neikeq commented Nov 27, 2022

In that particular example we could call the method ToggleVisibility, the name of each callback would have to be decided on a case-by-case basis.

That won't work for the connections' dialog, which generates a signal name automatically. Granted, this dialog doesn't generate pascal case names for C# yet, but ideally that should change. Additionally, I don't think it's a good idea to ask doc writers to think about such names. That may discourage those who translate these examples to C#.

We need to come with a generic convention based only on the signal name (and class name if sender != receiver), even if we then recommend using a different convention.

Also, if we are not willing to change GDScript's convention, are we OK with following a different convention for naming callbacks in C#?

I don't think we have much influence as to change such GDScript conventions. That being said, the signal naming convention in the docs is inconsistent. There are already other examples that use a naming convention like the one you suggest. The reason for needing a generic convention is still there, if only because it's needed for the connections' dialog.

We are perfectly fine with following a different naming convention in C#, that's no problem.

Ideas for a generic callback naming convention: _SignalName, SignalNameCallback, SignalNameEmitted (prepend ClassName if sender != receiver).

@raulsntos
Copy link
Member Author

To be honest I totally forgot about that dialog but yeah now that you mention it I can see the need for it.

Ideas for a generic callback naming convention: _SignalName, SignalNameCallback, SignalNameEmitted (prepend ClassName if sender != receiver).

I like SignalNameCallback.

@pavlexander
Copy link

Interested to hear updates on this discussion. I can see that the @godotengine/dotnet was assigned as a reviewer - does that mean that the feature is waiting for it's final approval before the release? Or is there more work planned?

@Reintjuu
Copy link

@raulsntos, is there a way for me to help to get this merged? Do you know what needs to be done?

@raulsntos
Copy link
Member Author

This PR needs to be reviewed by the @godotengine/dotnet team. Users can review it too and it's appreciated.

There were previous concerns mentioned about the name of the emit method that conflicts with the name usually given by Godot to signal callbacks (#68233 (comment)). I suggest to change the callbacks names1 to something like {SignalName}Callback to avoid the conflict.

The source generator changes can be tested without merging this PR, just copy the source generator code. It won't generate emit methods for native Godot types (e.g.: Node) since that needs the changes to bindings_generator but it will generate them for user-defined types. Users can also build this PR locally and test with a local NuGet repository to provide feedback.

Footnotes

  1. Just to clarify, I mean changing the naming convention for the signal callback name in C#, GDScript would be unaffected.

@Reintjuu
Copy link

There were previous concerns mentioned about the name of the emit method that conflicts with the name usually given by Godot to signal callbacks (#68233 (comment)). I suggest to change the callbacks names1 to something like {SignalName}Callback to avoid the conflict.

It's about invocation, right? Registering callbacks is up to the user, just like you said in the discussion above, where methods possibly prefixed with On are for.

Wouldn't it be better to just name it Invoke{SignalName} or even an overload-like naming convention such as Emit{SignalName}? Or is that what the naming conflict is about?

@raulsntos
Copy link
Member Author

I think there might've been a misunderstanding, with callback I'm referring to the methods that are subscribed and called when the event is raised. I'm suggesting to rename the callback name, not the name of the method that emits the signal.

public class EventEmitter
{
	public event Action? MyEvent;

	// This is the method that raises the event.
	protected virtual void OnMyEvent()
	{
		MyEvent?.Invoke();
	}
}

public class EventReceiver
{
	// This is the method that will be invoked when the event is raised.
	// Godot currently uses the naming convention `On{SignalName}`
	// which conflicts with the name of the method that raises the event.
	public void MyEventCallback() { }
}

var emitter = new EventEmitter();
var receiver = new EventReceiver();
emitter.MyEvent += receiver.MyEventCallback;

The name of the method that raises the event must be On{EventName} to follow the .NET guidelines (emphasis mine):

✔️ DO use a protected virtual method to raise each event. This is only applicable to nonstatic events on unsealed classes, not to structs, sealed classes, or static events.

The purpose of the method is to provide a way for a derived class to handle the event using an override. Overriding is a more flexible, faster, and more natural way to handle base class events in derived classes. By convention, the name of the method should start with "On" and be followed with the name of the event.

The derived class can choose not to call the base implementation of the method in its override. Be prepared for this by not including any processing in the method that is required for the base class to work correctly.

@akien-mga
Copy link
Member

Does this address godotengine/godot-proposals#7891 fully? If so it could be added to the OP.

@raulsntos
Copy link
Member Author

Does this address godotengine/godot-proposals#7891 fully? If so it could be added to the OP.

That proposal is about adding an analyzer for the EmitSignal method which would show a warning when using the method with an incorrect number of parameters or parameters of the wrong type.

I think if we merge this PR we probably wouldn't need the analyzer and just tell users to use the generated methods instead. So in that sense this PR would address the proposal, but there's no reason why we can't have both.

The analyzer can be implemented externally though, so even if we don't implement it, users can create their own library and share it in NuGet.org.

@abelbascu
Copy link

but we need to think what to do about callbacks.

Isn't that up to the user to choose? Since they are the ones implementing those methods they are free to call them however they prefer. I guess my preference would be to name those methods after what they do and not after the signal that invokes them.

For example, let's say you have a signal named PlayerDied and a callback method that shows the Game Over screen. Instead of calling this method OnPlayerDied or PlayerDiedCallback I would call it ShowGameOverScreen:

event Action PlayerDied;

PlayerDied += ShowGameOverScreen;

That way the code reads: When the player dies, show the Game Over screen.

Fully agree with this. As non-expert game dev, there are quite a few use cases where one may want to be more explicit about what the subscribed methods are instead of enforcing to type a unique generic 'SignalNameCallback'.

@raulsntos raulsntos force-pushed the dotnet/raise-events branch 2 times, most recently from 235e4a4 to e00df4e Compare September 23, 2024 11:39
@akien-mga akien-mga modified the milestones: 4.x, 4.4 Sep 26, 2024
@akien-mga akien-mga merged commit 543fa16 into godotengine:master Sep 27, 2024
19 checks passed
@akien-mga
Copy link
Member

Thanks!

@raulsntos raulsntos deleted the dotnet/raise-events branch September 27, 2024 15:20
@Delsin-Yu
Copy link
Contributor

Delsin-Yu commented Sep 28, 2024

From the guidelines you mentioned, shouldn't the generated method be named Raise{SignalName} instead of On{SignalName}?
The On{SignalName} should be referring to Callee (the method that fires by the signal) instead of Caller (the method that raises the signal).

...The purpose of the method is to provide a way for a derived class to handle the event using an override...

That section is referring to the naming scheme for the Callee, not the Caller.

Nevermind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet