Fix: several bugs related to joining a network server #8755
+64
−37
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Strictly seen, there are "N" people -waiting- in front of you in the queue, but it is nicer to show "N + 1" for the person that is currently downloading the map. Avoids it showing: "0 clients in front of you". That just feels a bit off.
TrueBrain
force-pushed
the
send-map-fixes
branch
2 times, most recently
from
7e446d2 to
f274b1b
Compare
ldpl
reviewed
Feb 27, 2021
Also terminate creating of the savegame, as the client is gone, there really is no need for that anymore.
TrueBrain
force-pushed
the
send-map-fixes
branch
from
f274b1b to
80cddee
Compare
Send all clients in the queue every game-day a packet that they are still in the queue.
…ading map When you are downloading a map, all the commands are queued up for you. Clients joining/leaving is done by the network protocol, and as such are processed immediately. This means that by the time you are processing the commands, a client that triggered it, might already have left. So, all commands that do something with ClientID, shouldn't error on an invalid ClientID when DC_EXEC is set, but gracefully handle the command anyway, to make sure the game-state is kept in sync with all the clients that did execute the DoCommand while the now-gone client was still there. Additionally, in the small chance a client disconnects between the server validating a DoCommand and the command being executed, also just process the command as if the client was still there. Otherwise, lag or latency can cause clients that did not receive the disconnect yet to desync.
TrueBrain
force-pushed
the
send-map-fixes
branch
from
80cddee to
d650020
Compare
|
Not sure why is it happening but two clients can't connect at the same time now if they do it with "new company" button. |
|
What was the "invalid or unexpected packet" that the client received? |
ldpl
approved these changes
Feb 27, 2021
LordAro
approved these changes
Feb 28, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8751
Motivation / Problem
There are four bugs mentioned in the above ticket:
Description
is a linguistic issue. "1 person waiting" means 1 is being served while there is 1 more waiting. So when you are the next in line, there are 0 people waiting, so 0 people in front of you. But this just looks odd and weird. So let's ignore "strictly seen correct" and go with the more expected version: 1 people in front of you means that person is downloading the map.
the state machine was poorly designed around unhappy flows. Now, if for what-ever reason the client disconnects while downloading the map, the savegame process is stopped and the next person is getting the map.
every second all people in queue now receive a packet, letting them know the server is alive.
after the server approved the CmdCompanyCtrl, clients and server could still reject it depending if they received that the client left the server. This is latency depending, and can cause desync. Introduced in f8f7feb, which originally avoided this situation, but stricter validation created the possibility to desync (the check changed from an out-of-bound check to a "does this still exist" check).
Limitations
Checklist for review
Some things are not automated, and forgotten often. This list is a reminder for the reviewers.