Page MenuHomeWildfire Games

Update client loading state upon client disconnect too
AcceptedPublic

Authored by elexis on Oct 16 2018, 5:32 PM.

Details

Reviewers
Imarok
Summary

Wen the current player has finished the loading screen in a multiplayer match and waits for some other clients to load and there is much delay between the disconnect and the next player finishing the loading screen,
then the loading screen will show that it waits for one player that has disconnected long ago, which may mislead the host to try to kick the client if it is an observer.

The according message updating that screen is sent when a player finishes the loading screen, but not upon disconnect. The patch changes that.

Test Plan

Should be possible to make one client load slowly and another one quickly if one has two code checkouts.

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
/ps/trunk
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6392
Build 10594: Vulcan BuildJenkins
Build 10593: arc lint + arc unit

Event Timeline

elexis created this revision.Oct 16 2018, 5:32 PM
Vulcan added a subscriber: Vulcan.Oct 16 2018, 5:40 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 151| »   while·(true)
|    | [NORMAL] ESLintBear (no-constant-condition):
|    | Unexpected constant condition.

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
| 189| »   »   »   »   for·(let·guid·in·g_PlayerAssignments)
|    | [MAJOR] JSHintBear:
|    | Let declaration not directly within block.

Link to build: https://jenkins.wildfiregames.com/job/differential/742/display/redirect

May be considered an oversight in rP19320 (as that came after rP18858)

Besides the comments: Looks good.
Could reproduce and fixes. 👍

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
209

What is that needed for?

source/network/NetServer.cpp
1439–1450

Maybe a bit more straight forward to switch the if-else around. Then we'd first handle CClientsLoadingMessage and can forget about that before we create the loadedMessage.

1442

Shouldn't that be called loadedMessage?

source/network/NetServer.h
1

That

elexis marked 3 inline comments as done.Nov 12 2018, 4:43 PM
elexis added inline comments.
binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
209

I guess I had added it because we don't want an error message if a client rejoins before all clients finished the gamestate. There was a commit making that impossible, but I think that was a workaround which ought to be rewritten.
I guess I had added it here as opposed to the other network cleaning patches, because this patch relates to the clients-loading message.

source/network/NetServer.cpp
1439–1450

I bought my computer science teachers argument of seeing if (!x) Y else Z as an anti-pattern as it introduces an unneeded negation / double negation. Other people say the more frequent case should occur first, others say the intended case should occur first, it's arbitrary.

1442

variable rename ok

Imarok added inline comments.Nov 12 2018, 7:30 PM
binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
209

Different bug, different patch I'd say...

source/network/NetServer.cpp
1439–1450

In general I agree with you, but here we do:

construct loadingMessage

if(x)
  construct loadedMessage
  send loadedMessage
else
  send loadingMessage

That makes understanding the code a bit more difficult.
Would be better to first finish handling with loadingMessage

elexis marked an inline comment as done.Nov 12 2018, 9:54 PM
elexis added inline comments.
source/network/NetServer.cpp
1439–1450

Then we'd first handle CClientsLoadingMessage and can forget about that before we create the loadedMessage.

The message can be used in both parts of the if-else-statement, so one can't 'forget about it', but must read all the code in all cases to understand it.

The way I wrote it, it says:

  • If we don't have clients that are still loading, send the loaded message, else send the loading message.

The way you propose, it says:

  • If we don't have no clients that are still loading, send the loading message, else send the loaded message.

With your proposal it's more grouped, with my proposal it's less operations. Both can show arguments that their variant to be more straight forward. So I claim it's arbitrary.

Imarok added inline comments.Sat, Nov 17, 8:52 PM
source/network/NetServer.cpp
1439–1450

The message can be used in both parts of the if-else-statement

But they aren't.

With your proposal it's more grouped, with my proposal it's less operations. Both can show arguments that their variant to be more straight forward. So I claim it's arbitrary.

But yeah, that is nitpicking, so don't care.

elexis marked an inline comment as done.Sun, Nov 18, 10:28 AM
In D1649#66186, @Imarok wrote:

Besides the comments: Looks good.
Could reproduce and fixes. 👍

Anything more that needs review?

source/network/NetServer.cpp
1439–1450

Then we'd first handle CClientsLoadingMessage and can forget about that before we create the loadedMessage.

The message can be used in both parts of the if-else-statement

But they aren't.

Which one cannot find out if one forgets about it after reading it, right?

(I guess one could argue for finding out about that fact when reading it in the first step and then forgetting about it, and then only understanding the second part. But then again those are only 5 lines)

In D1649#66370, @elexis wrote:
In D1649#66186, @Imarok wrote:

Besides the comments: Looks good.
Could reproduce and fixes. 👍

Anything more that needs review?

As said, not besides the comments I wrote.

elexis marked an inline comment as done.Wed, Nov 21, 10:27 AM
In D1649#66373, @Imarok wrote:

Anything more that needs review?

As said, not besides the comments I wrote.

Then do you want to accept it formally?
(Otherwise what worth does it add for me to revert my local patches, compile it, wait for the compile to finish, apply this patch again, revert one file, upload the patch again, then apply my local patches again, compile again and have you read that I actually reverted one JS file; in comparison to me just not committing that one file change (assuming agreement on not cleaning that JS file along the way)? )

binaries/data/mods/public/gui/gamesetup_mp/gamesetup_mp.js
209

Same message being cleaned up though.
(I guess the problem was that noone informed you that this JS diff is not mandatory to make the C++ diff work, whereas one more line in the commit message would clarify that distinction.))
(I think this cleanup line was added to testify that I looked at every possible place where the message could be processed, finding this place where it's missing. In case the server does send the packet, regardless of our server implementation, it shouldn't throw this specific error, right?))

Imarok accepted this revision.Sat, Nov 24, 6:04 PM

Accept if proposed changes are followed.

This revision is now accepted and ready to land.Sat, Nov 24, 6:04 PM