Skip to content

C# cleaner signal emission (v2) - #70424

Closed
OlliO6 wants to merge 1 commit into
godotengine:masterfrom
OlliO6:C#--Cleaner-Signal-Emission-(v2)
Closed

C# cleaner signal emission (v2)#70424
OlliO6 wants to merge 1 commit into
godotengine:masterfrom
OlliO6:C#--Cleaner-Signal-Emission-(v2)

Conversation

@OlliO6

@OlliO6 OlliO6 commented Dec 21, 2022

Copy link
Copy Markdown

Provides a type safe way to emit signals in c#.

Previous:

EmitSignal(SignalName.SomethingHappened, arg1, arg2, arg3);
// This does not have syntax highlighting and compile error when signature is not met.

Now:

GetEmitter<SignalEmitter.SomethingHappened>().Emit(arg1, arg2, arg3);

I also implemented that enums can be used.

Previous PR: C#: Cleaner signal emission
Can't be archieved seamlessly (allocations or pullotion)

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

  • Pollutes classes
  • Shorter code
  • Guidelines

@OlliO6
OlliO6 requested a review from a team as a code owner December 21, 2022 22:43
@YuriSizov YuriSizov added this to the 4.x milestone Dec 22, 2022
@dannflor

Copy link
Copy Markdown

I'm a huge fan of this. I like how the type is leveraged for better compile-time checking and I especially need the enums as args. Since this is going to break the signal emission API, I say the sooner we merge the better 👍

@kleonc

kleonc commented Dec 23, 2022

Copy link
Copy Markdown
Member

cc @raulsntos @neikeq

@raulsntos

Copy link
Copy Markdown
Member

This PR does not break compatibility so there's no rush to merge it.

I'd love to have a way to emit signals using a typed method but it's not clear which is the best approach. I personally favor my own implementation (#68233) over this PR.

My opinion on this PR:

  • I don't like how much more verbose this is compared to my version.
  • I prefer to follow the .NET event conventions more closely.
  • Accessing the Bounds property uses a callvirt, not sure how much of an impact this has on performance.
  • The new() constraint uses Activator.CreateInstance() which uses reflection, we may be able to avoid this in .NET 7 with static abstract members.

@OlliO6

OlliO6 commented Dec 23, 2022

Copy link
Copy Markdown
Author

I kinda agree to the verbosity of this.
Would it be better to use default instead of new?

@raulsntos

Copy link
Copy Markdown
Member

Would it be better to use default instead of new?

I think so, I'd also recommend using the struct constraint since we know all the TSignalEmitter types will always be structs and that will avoid the nullability warning.

@aaronfranke

Copy link
Copy Markdown
Member

The alternative implementation from @raulsntos was merged in PR #68233, so I'm closing this PR as superseded. Thank you for working on this!

@aaronfranke aaronfranke removed this from the 4.x milestone Nov 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants