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

Allow virtual methods to be GdSelf in special cases #1048

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

snakefangox
Copy link
Contributor

@snakefangox snakefangox commented Feb 16, 2025

So, this allows virtual methods take Gd<Self> as a receiver. It also implements this as a special case for exactly one method, that being MultiplayerApiExtension::poll. This is useful because we want to call rpc functions out of poll and if that function calls back to any multiplayer function (which most rpc functions do, even if it's just is_server or to get the remote peer) then we get double bind exceptions. This way, the implementer can unbind, call, rebind to avoid that.

Not sure if this is a sensible way to allow this or if there are any other places it would be useful. It's a pretty breaking change for a pretty niche problem but it's working well for me.

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1048

@Bromeon
Copy link
Member

Bromeon commented Feb 16, 2025

Thanks for the PR!

Not sure if this is a sensible way to allow this or if there are any other places it would be useful.

The problem is definitely a bigger one, see #359.

Here, the main challenge is to provide custom receivers in a way that allows users to select either this: Gd<Self> or &self/&mut self. Always choosing the former is of course possible, but requiring extra binds is really not nice for ergonomics.

The only solution I can currently think of is to declare all virtual methods with this: Gd<Self> and let the proc-macro desugar the bind, but this is still not great for doc purposes, IDE static analysis/completion, etc...

@Bromeon Bromeon added feature Adds functionality to the library c: register Register classes, functions and other symbols to GDScript labels Feb 16, 2025
@Bromeon
Copy link
Member

Bromeon commented Feb 16, 2025

Re CI, seems like Godot broke something again 🙄

@snakefangox
Copy link
Contributor Author

snakefangox commented Feb 16, 2025

Ah, yeah looks like a much trickier problem than the one issue this patch solves 😅

I do have a couple ideas for making this something the user can choose, I'll have to do a proper run through the code and see if any of them are feasible.

@snakefangox snakefangox marked this pull request as draft February 16, 2025 11:35
@Bromeon Bromeon force-pushed the add-gdself-virtual-methods branch from 7d686b9 to 86c6e95 Compare February 16, 2025 13:55
@Bromeon
Copy link
Member

Bromeon commented Feb 16, 2025

Fixed CI and rebased, so remaining errors are specific to this PR.

But as you say, we should look into solving the problem on a bigger scale, so any ideas in that regard are very welcome! 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: register Register classes, functions and other symbols to GDScript feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants