search

Ralith / hypermine

A hyperbolic voxel game
Apache License 2.0
160 stars 20 forks source link

Server can transmit the same graph nodes twice #80

Open Ralith opened 4 years ago

Ralith commented 4 years ago

When a client first connects, the graph is synchronized to them in full. Each timestep, newly introduced graph nodes from that step are broadcast to all clients. These sets can overlap, leading to the same nodes being added to the graph twice, which produces invalid topology and leads to panics when traversing the graph.

One solution could be to replace the first incremental update with a full sync directly, rather than having an independent initial sync.

Ralith commented 9 months ago

A more robust solution would be to ensure the initial sync reflects exactly the previous step, which by definition cannot overlap with new nodes introduced in the next step.

patowen commented 9 months ago

My most recent encounter of the bug was caused by ensure_nearby being called in the server's Sim::new without populating the nodes afterwards, followed by a client connecting before the server ran the first step. Because the nodes were not populated, they were still in the graph's fresh list, but they were also in the graph, causing the initial sync to include nodes that were not fully initialized.

I believe removing that call to ensure_nearby in PR #342 might have fixed this issue entirely, although I have not 100% confirmed that.

In case the above was confusing (since I don't think the vocabulary has been standardized), I should clarify that when I say "populated", I mean that a Node instance was added to the respective node of the graph, and it does not mean that terrain has been generated. To avoid bugs, "populating" a set of nodes should ideally occur immediately after these nodes are added to the graph, as otherwise, code elsewhere (such as in the character controller) has to handle the case of the character having incomplete information about the node it's in. Or, in this case, the server can get confused about which nodes it has transmitted to the client already.

I believe that if this bug is encountered, the client will immediately crash due to a debug_assert!.

patowen commented 2 months ago

The most recent occurrence of this bug was fixed in https://github.com/Ralith/hypermine/pull/427.