Skip to content

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

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
Faless:mp/cache_no_spam
Apr 23, 2024
Merged

[MP] Gracefully handle cache confirmation of deleted nodes#90027
akien-mga merged 1 commit into
godotengine:masterfrom
Faless:mp/cache_no_spam

Conversation

@Faless

@Faless Faless commented Mar 29, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

Faless commented Apr 9, 2024

Copy link
Copy Markdown
Collaborator Author

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

grossqx commented Apr 22, 2024

Copy link
Copy Markdown

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

@Faless

Faless commented Apr 22, 2024

Copy link
Copy Markdown
Collaborator Author

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
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

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.

RPC with QueueFree Gives "Node Not Found" on Server RPCs from client to server fail after node reparented on both ends

4 participants