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

feat: connect state signals to C# events using Callable.From #126

Closed

Conversation

Prakkkmak
Copy link

This merge request introduces the ability to connect Godot state signals to C# events using Callable.From, providing a more idiomatic way to handle signals in C#.

Changes:

  • Updated the StateChartState constructor to connect signals to event handlers using Callable.From.
  • Added methods to handle signals with the correct argument types:
    • OnStateEntered
    • OnStateExited
    • OnEventReceived
    • OnStateProcessing
    • OnStatePhysicsProcessing
    • OnStateStepped
    • OnStateInput
    • OnStateUnhandledInput
    • OnTransitionPending
  • Ensured each method raises the corresponding C# event with the correct parameters.

Motivation:
Connecting signals using Callable.From aligns with the recommended practices for handling signals in Godot using C#. This change improves type safety and readability of the code.

~ Co Created With Chat GPT

- Updated StateChartState constructor to connect signals using Callable.From.
- Added appropriate methods to handle signals with correct argument types.
- Ensured compatibility with Godot signal types and C# event handlers.
@derkork
Copy link
Owner

derkork commented Jun 23, 2024

Thank you very much for submitting this PR! While I agree on the intention of this, I think this approach has a few drawbacks.

  • The state charts library checks whether or not some signals are connected to improve performance. E.g. if you don't connect something to state_processing or state_physics_processing then the library will automatically disable the node's processing and thus avoid incurring runtime cost for something that isn't used. With the implementation given here, this optimization would no longer work.
  • C# events do not auto-disconnect themselves (except for events marked with [Signal] and even there it doesn't seem to work reliably). I find this to be a major hindrance in game development where actors can appear and disappear at any time. On the other hand, any connection made with Godot's Connect will be automatically removed if either the source or the destination are freed. This means when people use the new events introduced by this change, they will have to manually disconnect their receivers when they are freed. This is an extra required step and will likely raise quite a bit of support tickets here.

I think a better approach would be to provide a type-safe Connect variant. I have created a branch lining out the idea (check main...csharp-type-safe-signals). I think the external API is quite nice, you can do a:

my_state.StateEntered.Connect(OnStateEntered);

// ... 

void OnStateEntered() {
}

though internally it's a lot to write in the wrapper classes. Then again, it's a one and done.

@Prakkkmak
Copy link
Author

I followed the guideline of += and -= of the documentation to keep consistency:

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

I would use your approach eventually in future updates.

@derkork
Copy link
Owner

derkork commented Aug 16, 2024

I had a few chats with some people who know a lot more about the state of C# in Godot and me and they told me that my implementation approach didn't help with the signal disconnect issue at all. Since this is the case we can actually use events, so at least the usage mode is consistent with what the rest of Godot does. So i changed the implementation to something along these lines:

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

This way we get the += / -= syntax people are used to but still only subscribe to signals when actually needed.

derkork added a commit that referenced this pull request Aug 16, 2024
derkork added a commit that referenced this pull request Aug 16, 2024
* feat: type safe signals for C#

* docs: add description for new default values feature

* feat: type safe signals for C#

* feat: provide type safe signal wrappers for C#

fixes #126, #130
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants