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

rewrite multiplayer spawner and synchronizer, making it a module #62870

Closed
wants to merge 1 commit into from

Conversation

nathanfranke
Copy link
Contributor

@nathanfranke nathanfranke commented Jul 10, 2022

Fixes #62027
Fixes #61711
Fixes #62320

Testing project: test7.zip

Tests:

  • Custom Spawn with MultiplayerSynchronizer as child
  • Spawner inside another Spawner
  • Spawn with MultiplayerSynchronizer as child
  • Standalone MultiplayerSynchronizer

CC @Faless

Note for reviewers: replication_editor_plugin.* and scene_replication_config.* are not modified, except for #ifdef TOOLS_ENABLED and some imports.
multiplayer_spawner.* and multiplayer_synchronizer.* have changed considerably.


Note the use of RPCs. It isn't going to be perfectly space efficient, but the majority of the data being sent is Variants (which are encoded the same way regardless) so it's not a big deal. Only other issue is the methods are technically exposed to GDScript, allowing the user to override them. I worked around it by naming the methods _internal_rpc_..., but there should be a better solution. See #32585.

Edit: I am only naming them _rpc_ because thinking again, private methods should be inaccessible (un-overridable?) by default.

Edit 2: Just noting here for future reference. I do respect the way the previous spawner/synchronizer handles packets, it is more versatile for bare C++. However, given the current multiplayer system, I don't think it's possible for a module (or gdextension) to make a custom packet. If this is desired though, rather than moving it to core, it would be better to implement a way for any module/gdextension to make its own packets.


@jgillich
Copy link
Contributor

Nice work! I'm not qualified enough to review the code but I can confirm that it resolves the scene root syncing issue (#62027)

While you're at it, do you happen to have any ideas regarding #62320? That issue is still present here

@nathanfranke
Copy link
Contributor Author

nathanfranke commented Jul 10, 2022

I did take a look at #62320, but I don't think it'll be resolved here. I'll give it a test though! (Also thanks for taking a look 😃)


(Note GH actions, I have fixed them locally but I am also doing other fixes)

@nathanfranke nathanfranke force-pushed the multiplayer branch 5 times, most recently from 734a35a to 45ae023 Compare July 10, 2022 09:28
@nathanfranke
Copy link
Contributor Author

nathanfranke commented Jul 10, 2022

Well, that took quite a while! I believe I got #62320 resolved, and I also got spawner subchildren working.

Explanation:

Problem:
image

Previously, when a client joined, the server would receive the peer_connected signal and each Spawner would send the client the nodes that were needed. However, since signals are not called in a deterministic order, it was possible for the player spawner to send its RPC before the World spawner even created it.

Solution:

Now, the server doesn't listen to peer_connected, but rather the client requests the spawns/syncs when the spawner is initialized. This also prevents errors if for whatever reason the Spawner/Synchronizer exists on the server but not the client. This also paves the way for proposals such as godotengine/godot-proposals#3904, since the server no longer expects all clients to have the same Spawners and Synchronizers. (Lots of work is still needed, however, as there is nothing preventing a client from pretending to have those nodes)

Edit: Also updated OP with a testing project

@Faless
Copy link
Collaborator

Faless commented Jul 10, 2022

I wish you had written a proposal, or contacted me about this, I've been doing work on interest management that will collide with this PR.

@Faless
Copy link
Collaborator

Faless commented Jul 10, 2022

Also in general, using RPC this way is really not okay, since it causes each synchronizer to send it's own data alone, unlike the previous design where updates from multiple synchronizers would be bulked together saving a ton of bandwidth for small states.

@Faless
Copy link
Collaborator

Faless commented Jul 10, 2022

I wish you had written a proposal, or contacted me about this, I've been doing work on interest management that will collide with this PR.

To expand on this, I really appreciate your work in trying to implement those features, and I like the idea of clients requesting spawns, which is something likely needed, but there are some issues with the way this has been rewritten:

  • The MultiplayerReplicationInterface is there specifically to allow modules/extensions to implement the replication API, and should not be removed, to the contrary, it should be exposed to GDExtension once completed.
  • The MultiplayerSynchronizer and MultiplayerSpawner are "configuration" nodes, they implement their behaviors by calling into the replication interface (which needs a global overview to perform the appropriate optimizations).
  • As I mentioned above, RPCs for synchronizations are not okay, because we really need to pack updates together as much as possible or it will perform poorly as soon as the number of synchronized objects grow.

I am currently finishing my work on the interest management proposal, which should also solve some of the issues, I would be happy to discuss further changes on top of that if needed.

Again, I really appreciate you working on this, and I think there are good ideas, but it cannot be accepted as is, and I would advise against putting more work on this before I finish the work on interest management proposal, because that should solve most of the issues already, and would certainly conflict with this. This came at an unfortunate timing, I am sorry about it :/ .

@nathanfranke
Copy link
Contributor Author

nathanfranke commented Jul 10, 2022

Attempting some trials:

Specification:

  • Client connects to server.
  • Server spawns N nodes for client using MultiplayerSpawner.
  • After some idle time, client picks a random location and sends it directly to the server in an rpc. time starts now.
  • Server receives the RPC, sets the position of all nodes, and should eventually synchronize all positions.
  • Once ALL nodes are in the correct position, time stops and is recorded.

In my case, since Godot 4 doesn't yet have headless, I couldn't use my normal server, so instead I used my laptop at home. It was connected to ProtonVPN in Japan to simulate a relatively poor connection with high latency.

Task Synced 10 nodes Synced 100 nodes Synced 1000 nodes
Godot Master - Average latency after 10 trials 326ms 469ms 757ms
Pull request - Average latency after 10 trials 318ms 333ms 700ms - 3000ms

The fundamental problem with too many nodes:

  • Assume connection has arbitrary bandwidth limit of 200 unreliable packets per frame
  • Each node sends a packet, meaning 1000 unreliable packets
  • Assuming 200 packets make it through every frame
  • It will take 30-40 frames for all nodes to be synced.

Layman's terms:

  • Given a group of 1000 people, every timestep 200 random people are given a glass of water. After 30 timesteps, it is still likely that some particular person has never received their water.

I also had some issues where the peer would time out in the 1000 nodes trial. I am assuming this is from too many packets.

I think a great improvement would be bundling the data into a single packet


The MultiplayerReplicationInterface is there specifically to allow modules/extensions to implement the replication API, and should not be removed, to the contrary, it should be exposed to GDExtension once completed.

Hmm. I don't see what is particularly important about abstracting the replication interface. I'd imagine the biggest use case for multiplayer modules would be another multiplayer peer type (e.g. a Steamworks module that makes RPCs go through steam). Even if someone wants to modify the replication interface, they could disable the module and write their own (or copy some code).

The MultiplayerSynchronizer and MultiplayerSpawner are "configuration" nodes, they implement their behaviors by calling into the replication interface (which needs a global overview to perform the appropriate optimizations).

As I mentioned above, RPCs for synchronizations are not okay, because we really need to pack updates together as much as possible or it will perform poorly as soon as the number of synchronized objects grow.

I agree with both! How about, following your work with interest management and approval, I move all the functionality (e.g. networked spawning/syncing) to a singleton, which manages the spawners/syncs and bundles requests together?

@Diddykonga
Copy link
Contributor

Diddykonga commented Jul 11, 2022

The MultiplayerReplicationInterface is there specifically to allow modules/extensions to implement the replication API, and should not be removed, to the contrary, it should be exposed to GDExtension once completed.

Hmm. I don't see what is particularly important about abstracting the replication interface. I'd imagine the biggest use case for multiplayer modules would be another multiplayer peer type (e.g. a Steamworks module that makes RPCs go through steam). Even if someone wants to modify the replication interface, they could disable the module and write their own (or copy some code).

I just want to give my opinion in that, I believe every piece of code that can be made abstract/extendable should be, as that is one of the ultimate reasons why I enjoy Godot as an engine in the first place, and should hopefully remain so into the future.

Generality is slower then specialization, but can often support specialization while still being generic.

@nathanfranke
Copy link
Contributor Author

See #63049 ❤️

@nathanfranke nathanfranke deleted the multiplayer branch July 20, 2022 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants