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

[Net] MultiplayerReplicator state sync. #51788

Merged
merged 2 commits into from
Aug 29, 2021

Conversation

Faless
Copy link
Collaborator

@Faless Faless commented Aug 17, 2021

Like the spawn/despawn feature, it can be completely overridden with 2 custom callables.
The callables will be called with the list of tracked objects.
In SERVER mode, objects are automatically tracked, while in CUSTOM mode you can manually track them via track/untrack (but that's optional).
The default sync only happens from server to client, with batch updates, over unreliable channel (but with custom ordering).
The default sync will warn you if your state representation gets too big.

This PR is based on #51534 . Now merged, so based on master.

See attached project for an example: proj_spawn.zip.

Docs mostly done, will need proper tutorials soon.

@Faless Faless added this to the 4.0 milestone Aug 17, 2021
@Faless Faless added this to Feature or renaming PRs in Godot 4.0 alpha TODOs Aug 17, 2021
@Faless Faless marked this pull request as ready for review August 18, 2021 11:34
@Faless Faless requested review from a team as code owners August 18, 2021 11:34
Like the spawn/despawn feature, it can be completely overridden with 2
custom callables.
The callables will be called with the list of tracked objects.
In SERVER mode, objects are automatically tracked, while in CUSTOM mode
you can manually track them via `track`/`untrack` (but that's optional).
The default sync only happens from server to client, with batch updates,
over unreliable channel (but with custom ordering).
The default sync will warn you, if your state representation gets too
big.
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.

Looks good to me. Code looks fine too and I tested it locally.

Lets see what the others think. :)

@mhilbrunner
Copy link
Member

Opinions? :) @jonbonazza @AndreaCatania

Copy link
Contributor

@jordo jordo left a comment

Choose a reason for hiding this comment

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

Just a couple comments/questions regarding data structures, but otherwise looks solid! My opinion generally would be to use LocalVector (maybe even reserve a few elements) over Lists, unless there is going to be a lot of insertion and deletion from the middle of the collection.

@@ -63,31 +70,52 @@ class MultiplayerReplicator : public Object {
Vector<uint8_t> packet_cache;
Map<ResourceUID::ID, SceneConfig> replications;
Map<ObjectID, ResourceUID::ID> replicated_nodes;
Map<ResourceUID::ID, List<ObjectID>> tracked_objects;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is Map<> and a List<> the right data structure to be using here. Is there any reason to use a tree over HashMap or OAHashMap?

Copy link
Collaborator Author

@Faless Faless Aug 26, 2021

Choose a reason for hiding this comment

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

As mentioned above, the order is important, and must be consistent with the track/untrack order, and List ensures that.
Regarding Map, yes, tracked_objects could be HashMap<ResourceUID::ID, List<ObjectID>> as you say (given we don't care about the ResourceUID::ID order in this variable). I'll fix that. Thanks!

core/io/multiplayer_replicator.cpp Outdated Show resolved Hide resolved
core/io/multiplayer_replicator.cpp Outdated Show resolved Hide resolved
bool raw = false;
List<Variant> state;
};
Map<ObjectID, struct EncodeInfo> state;
Copy link
Contributor

Choose a reason for hiding this comment

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

OAHashMap here? Does ordering of the ObjectIDs matter in this tmp collection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, order is important, because we do not send the IDs to the clients (which saves us quite a few bytes).
So the first ObjectID tracked, will be the first element in the state update, and the last ObjectID tracked wil be the last.

@Faless
Copy link
Collaborator Author

Faless commented Aug 26, 2021

unless there is going to be a lot of insertion and deletion from the middle of the collection.

Well, there are likely going to be a lot of deletion in the middle (not insertions).
You might spawn 50 enemies, they will be inserted sequentially, but they will be deleted when they get destroyed, which will not be in the same order they get spawned.
So, I would expect most of the deletion to happen in the middle.

@AndreaCatania
Copy link
Contributor

The interface is great already, @Shatur proposed to use it for the net sync, and I think it's a great idea. Maybe we will need to expand the interface a bit. Would it be fine change it a bit (add few other APIs and in case change others) once we start using this?

@mhilbrunner
Copy link
Member

Sounds good, we can talk details in the next networking meet:
Sunday 29th August, 17:00 UTC

@AndreaCatania
Copy link
Contributor

Yup, I can show the NetSync interface.

@Faless
Copy link
Collaborator Author

Faless commented Aug 29, 2021

Merging as discussed in the #networking meeting of 29th Aug 2021.
Further work discussed: (for future PRs)

  • "standardized" API for "network object IDs" (optimize data structures, allows single object state sync).
  • Further configuration of "synced" variables (QoL for plugins, cleaner API, as discussed personally with @AndreaCatania who couldn't attend the meeting).

@Faless Faless changed the title [Net] [RFC] MultiplayerReplicator state sync. [Net] MultiplayerReplicator state sync. Aug 29, 2021
@Faless Faless merged commit 838a449 into godotengine:master Aug 29, 2021
Godot 4.0 alpha TODOs automation moved this from Feature or renaming PRs to Done Aug 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

4 participants