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

[MP] Gracefully handle cache confirmation of deleted nodes #90027

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Mar 29, 2024

It's possible that after sending a cached node reference (e.g. RPC or static MultiplayerSynchronizer) the reference node is removed from tree before the remote peer(s) can confirm the referenced path.

To better detect that case, and avoid spamming errors when it happens, this commit modifies the multiplayer API caching protocol, to send the received ID instead of the Node path when sending the confirmation packet.

This is a breaking change because it makes the runtime multiplayer protocol incompatible with previous versions of Godot.

Fixes #85883
Fixes #86909

It's possible that after sending a cached node reference (e.g. RPC or
static MultiplayerSynchronizer) the reference node is removed from tree
before the remote peer(s) can confirm the referenced path.

To better detect that case, and avoid spamming errors when it happens,
this commit modifies the multiplayer API caching protocol, to send the
received ID instead of the Node path when sending the confirmation
packet.

**This is a breaking change** because it makes the runtime multiplayer
protocol incompatible with previous versions of Godot.
@mhilbrunner
Copy link
Member

This is a breaking change because it makes the runtime multiplayer protocol incompatible with previous versions of Godot.

I don't remember where, but didn't we decide that we don't expect different Godot versions to be MP protocol compatible anyway? So a networking breaking change is one that makes networking code written for a previous version not work anymore; that a new Godot version can't connect to an older version without issues is just expected?

I don't really know many MP games where you can connect to a server of a different game/engine version without issue.

The code changes here LGTM, going to test this with some projects to make sure nothing breaks.

@akien-mga akien-mga requested a review from a team April 8, 2024 16:33
@Faless
Copy link
Collaborator Author

Faless commented Apr 9, 2024

I don't remember where, but didn't we decide that we don't expect different Godot versions to be MP protocol compatible anyway?

Yes, we mentioned that multiple times, but I couldn't find a clear mention of it in the docs (something we might want to specify in the high-level multiplayer tutorial).

I don't really know many MP games where you can connect to a server of a different game/engine version without issue.

There's godotengine/godot-proposals#3533 which keeps popping to mind when I do these breaking changes.

I think we could provide an "auth_callback" that performs the version check as an addon at least.

The problem is that to cover all cases (or, well, most of the cases), it would probably need to be implemented at the mid-level abstraction layer (MultiplayerPeer).

The reason for the "breaks compat" label is more to ensure a mention in the release blog post, to avoid people getting surprised by it.

@grossqx
Copy link

grossqx commented Apr 22, 2024

Might this pr also fix this error on server when client kills the app without properly disconnecting the peer? (4.2.2)
image

@Faless
Copy link
Collaborator Author

Faless commented Apr 22, 2024

Might this pr also fix this error on server when client kills the app without properly disconnecting the peer? (4.2.2)

No, that appears to be a different problem, please open a separate issue so we can properly track it (EDIT: likely related to #91007). I think it should be an easy fix.

@akien-mga akien-mga merged commit ad4dff2 into godotengine:master Apr 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

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