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] Implement "watched" properties (reliable/on change). #75467

Merged
merged 1 commit into from
May 24, 2023

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Mar 29, 2023

Tentative implementation for godotengine/godot-proposals#4429.

How it works

"Watched" properties are deep duplicated and then compared (Variant::hash_compare) according to the delta_interval property of the MultiplayerSynchronizer (default = every frame). Re-duplication only happens when a change is detected, and the time of change is also stored for each property.

Clients receives progressive (delta) changes which only includes the changed properties. The transmission happens reliably, so all changes "across frames" (delta_interval) are guaranteed to be received by the clients.

There is a hard limit of 64 watched properties per synchronizer.
There is a (quite arbitrary) limit of 64k bytes per delta packet which probably needs adjustment.

watch option

WIP / RFC:

  • More testing (e.g. comparison method). Seems to work fine in my tests, we can always fix that in a follow up PR.
  • More feedback (e.g. custom comparison function, comparison frequency, reliable vs unreliable + ack). Well, it's been a month, I think I can mark the PR as ready.
  • Discuss replication config editor redesign (e.g. make "Initial/Sync/Delta" exclusive, or use the current one in the proposal). Let's do that in a separate PR/proposal.
  • Some further cleanup / internal command ids change. Kinda unrelated to this PR.
  • Nice to have: Implement editor profiler (needs redesign proposal due to space constraints 😅 ). Needs design proposal.

@Faless Faless added this to the 4.x milestone Mar 29, 2023
@Faless Faless changed the title [RFC] [MP] Implement "watched" properties. [RFC] [MP] Implement "watched" properties (reliable/on change). Mar 29, 2023
@Faless Faless force-pushed the mp/4.x_watch branch 3 times, most recently from 9ee2269 to 570acdc Compare March 30, 2023 04:00
@mhilbrunner mhilbrunner modified the milestones: 4.x, 4.1 Apr 21, 2023
@Calinou
Copy link
Member

Calinou commented May 4, 2023

There is a hard limit of 64 watched properties per synchronizer.

If we go with this design, remember to make the editor UI show an error dialog when trying to make a property watchable when there are already 64 watched properties.

There is a (quite arbitrary) limit of 64k bytes per delta packet which probably needs adjustment.

I assume this is done to avoid UDP fragmentation, therefore reducing potential issues with latency?

@Faless
Copy link
Collaborator Author

Faless commented May 5, 2023

If we go with this design, remember to make the editor UI show an error dialog when trying to make a property watchable when there are already 64 watched properties.

Will do.

I assume this is done to avoid UDP fragmentation, therefore reducing potential issues with latency?

Well, yes and no, UDP fragmentation starts at around 1400 bytes, so a 64k packet will already be fragmented in ~64 packets.
The goal of the limit is to still keep the fragmentation to what I believe is a reasonable number and avoid clogging too much the underlying peer with huge packets.
But honestly it's pretty arbitrary as I mentioned, so it should probably be configurable. I'll look into that too.

@Faless Faless marked this pull request as ready for review May 5, 2023 09:03
@Faless
Copy link
Collaborator Author

Faless commented May 5, 2023

Updated:
Editor error when adding > 64 properties.
Expose sync_mtu/delta_mtu for configuration.
Use usec instead of msec in synchronizer/replicator.

@Faless Faless changed the title [RFC] [MP] Implement "watched" properties (reliable/on change). [MP] Implement "watched" properties (reliable/on change). May 5, 2023
Copy link
Contributor

@dsnopek dsnopek 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 great! Regarding the overall approach: it's hard to strike a balance between saving network bandwidth and the processing necessary to do so, and I think this does a pretty good job :-)

modules/multiplayer/editor/replication_editor.cpp Outdated Show resolved Hide resolved
modules/multiplayer/editor/replication_editor.cpp Outdated Show resolved Hide resolved
modules/multiplayer/multiplayer_synchronizer.cpp Outdated Show resolved Hide resolved
modules/multiplayer/multiplayer_synchronizer.cpp Outdated Show resolved Hide resolved
Copy link
Member

@mhilbrunner mhilbrunner left a comment

Choose a reason for hiding this comment

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

@Faless I found nothing besides what is already mentioned above, docs look good too, and the implementation is clean and minimal, nice work :)

I'll see if I can find some time to do some tests and play around with it, but once the above is addressed this IMO is good to merge. Should not impact anything else anyway.

@Faless Faless force-pushed the mp/4.x_watch branch 3 times, most recently from fbe5dc2 to 39acd97 Compare May 24, 2023 00:48
@Faless Faless marked this pull request as draft May 24, 2023 02:54
Checked at "delta_interval" (default = every frame), synchronized
(reliably) if changes are detected.
@Faless Faless marked this pull request as ready for review May 24, 2023 03:57
@Faless
Copy link
Collaborator Author

Faless commented May 24, 2023

Thanks everyone for the reviews ❤️ , I believe I've addressed all the comments and suggestions.

@akien-mga akien-mga merged commit 513f43e into godotengine:master May 24, 2023
@akien-mga
Copy link
Member

Thanks!

@FeralBytes
Copy link
Contributor

This is awesome but it is also causing the server side to crash often. I will add more details to this issue here.

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.

6 participants