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

Add C# signal automatic disconnection info #8815

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

31
Copy link
Contributor

@31 31 commented Jan 23, 2024

After #8812, I looked a bit further at this Delegate.Target quirk that I remember noticing a while ago but hadn't looked into. This is just based on experimentation rather than tracking it down in Godot/.NET source, so I'm not 100% sure my reasoning is correct.

I don't know if all this specific stuff is already tracked by godotengine/godot#70414 / godotengine/godot#73730, or if there's more to pull out into a separate functional issue. This has been at the back of my mind for a while since it comes up in the C# Discord chat fairly often, so I figured now that I've looked into it a bit more, I'd just write it as a doc either way. 😅

@skyace65 skyace65 added enhancement topic:dotnet area:manual Issues and PRs related to the Manual/Tutorials section of the documentation labels Jan 23, 2024
@paulloz
Copy link
Member

paulloz commented Jan 27, 2024

This seems fine to me. The ordering within the page is just a bit odd now:

  • Signals as C# events
  • Disconnecting automatically when the receiver is freed
  • Custom signals as C# events
  • Signal emission
  • Bound values
  • Signal creation at runtime
  • Using Connect and Disconnect

Maybe the new section should instead be moved at the very end, or before the last one?

@31 31 force-pushed the dev/31/signal-connect branch from 3a0c812 to 14d63c8 Compare January 27, 2024 22:31
@31
Copy link
Contributor Author

31 commented Jan 27, 2024

Good point, I didn't really consider the order all that carefully. Adding it at the bottom makes sense to me.

That makes me think about how I expect people to actually get here... I think calling it out in the summary with the type of exception someone is probably seeing if they've hit this quirk is a good way to go.

Pushed the changes.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this has made me realize how complicated C# signals are at the moment. I feel like users shouldn't need to know all this just to use signals in C#, but since that's the current state of things it's better to document it than to leave users confused.

tutorials/scripting/c_sharp/c_sharp_signals.rst Outdated Show resolved Hide resolved
tutorials/scripting/c_sharp/c_sharp_signals.rst Outdated Show resolved Hide resolved
@31 31 force-pushed the dev/31/signal-connect branch from 14d63c8 to 08aa63c Compare January 28, 2024 04:51
@31
Copy link
Contributor Author

31 commented Jan 28, 2024

Yeah, initially I thought using .Connect was solving everything and the situation was ok, but this is quite unfortunate.

If we end up stuck here, I think it might be simpler if the docs included manual disconnection everywhere for C#, and this page mentioned that automatic disconnection happens in a few specific cases. That's a pretty big change though, and if godotengine/godot#73730 ends up making things simpler (or ends up with us manually tying references to our delegates to the lifetime of a Godot object, or... something), not sure it's worth too much churn simplifying where we're at now.

Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me as it is now. We can update the docs again when/if godotengine/godot#73730 gets merged, which is not guaranteed to make it in 4.3.

@mhilbrunner mhilbrunner merged commit 8334647 into godotengine:master Jan 30, 2024
1 check passed
@mhilbrunner
Copy link
Member

Thank you!

@31 31 deleted the dev/31/signal-connect branch January 31, 2024 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:manual Issues and PRs related to the Manual/Tutorials section of the documentation cherrypick:4.2 enhancement topic:dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants