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

Server can transmit the same graph nodes twice #80

Open
Ralith opened this issue May 17, 2020 · 3 comments
Open

Server can transmit the same graph nodes twice #80

Ralith opened this issue May 17, 2020 · 3 comments
Labels
bug Something isn't working

Comments

@Ralith
Copy link
Owner

Ralith commented May 17, 2020

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 Ralith added the bug Something isn't working label May 17, 2020
@Ralith
Copy link
Owner Author

Ralith commented Jan 12, 2024

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
Copy link
Collaborator

patowen commented Jan 12, 2024

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
Copy link
Collaborator

patowen commented Aug 12, 2024

The most recent occurrence of this bug was fixed in #427.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants