Page MenuHomeWildfire Games

Display which clients are still in the loading screen
ClosedPublic

Authored by Imarok on Feb 2 2017, 7:44 PM.

Details

Summary

Shows which clients are still loading after gamestart.

Test Plan

Start a game with multiple clients with different loading speed.
Ensure, the right clients are displayed as waiting for.

Simulating different loading speeds:

  1. Apply the following patch on top of the proposed one:
  2. Add the "setlag" function to your .bashrc file (described in http://trac.wildfiregames.com/ticket/3264#comment:5) . The tool comes with the iproute tool and should be installed already on unix systems.
  3. Start a networked gamesetup and chose a random map script
  4. Join via the global IP address and call setlag 800 to prevent becoming disconnected (refs #3700). As the lag is only applied to eth0, only the non-hosting client is affected.
  5. Start the game and observe the window of the hosting client

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Imarok created this revision.Feb 2 2017, 7:44 PM
Imarok set the repository for this revision to rP 0 A.D. Public Repository.Feb 2 2017, 8:54 PM
Imarok updated this revision to Diff 437.Feb 2 2017, 10:06 PM

Retry to trigger Vulcan

Vulcan added a subscriber: Vulcan.Feb 2 2017, 10:49 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/304/ for more details.

elexis retitled this revision from Display which clients are still in the loading screen to Display which clients are still in the loading screen .Feb 3 2017, 1:19 AM
Imarok updated this revision to Diff 468.Feb 6 2017, 9:12 PM

Changes discussed in IRC

Owners added a subscriber: Restricted Owners Package.Feb 6 2017, 9:12 PM
Vulcan added a comment.Feb 6 2017, 9:56 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/324/ for more details.

elexis edited the test plan for this revision. (Show Details)Feb 28 2017, 2:12 PM
elexis edited reviewers, added: elexis; removed: echotangoecho.
Imarok updated this revision to Diff 641.Feb 28 2017, 2:48 PM
Imarok retitled this revision from Display which clients are still in the loading screen to Display which clients are still in the loading screen.

Fix the thing

Imarok updated this revision to Diff 642.Feb 28 2017, 2:50 PM

Remove whitespace

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/430/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/431/ for more details.

elexis requested changes to this revision.Mar 1 2017, 5:05 PM

Tested with a sample size of 2 and works fine after your fix yesterday.
In particular works when the remaining client disconnects instead of finishing the loading screen.
Just few style optimizations recommended.

binaries/data/mods/public/gui/session/messages.js
140 ↗(On Diff #642)

(Okay to use players instead of clients here, as observers can be considered players that don't participate in the current match too and we don't want to bother our users with technical terms. Also afaics the list can never be empty, so ending with a colon is ok)

597 ↗(On Diff #642)

This looks like it assumes no other netstatus messages while in that waiting period.
Considering that both GUI obejcts have a text proeprty that we change, the naming is suboptimal.
How about LoadingClientsText and having that visible on init until the game starts?

613 ↗(On Diff #642)

Since all other occurances don't have translation context either, seems ok to keep it consistent

binaries/data/mods/public/gui/session/session.xml
162 ↗(On Diff #642)

ghost props needed? For the Exit button likely, wondering why the label of the button needs to be a custom text object in the first place.

source/network/NetClient.cpp
129 ↗(On Diff #642)

Can't think of a missing transition, as the synchronizing state should be ruled out as we don't have people rejoining after the first turn

789 ↗(On Diff #642)

guids is okay (I can imagine us replacing all guids with GUIDs and findGuidForPlayerID with findGUIDForPlayerID some day)

790 ↗(On Diff #642)

const &

source/network/NetMessages.h
197 ↗(On Diff #642)

GUIDs
because -> so that
GUI pages
Also this isnt the best location for a comment as there are no others here.
You could move it to the place where the CClientsLoadingMessage is constructed

source/network/NetServer.cpp
1184 ↗(On Diff #642)

player -> client

1188 ↗(On Diff #642)

2017-02-06-QuakeNet-#0ad-dev.log:17:59 < elexis> CheckGameLoadStatus could return true, so you dont send the message if its the last client that loaded
I see that issue being addressed, good

1189 ↗(On Diff #642)

// Inform clients that another client has finished the loading screen and who's missing or something to that effect

1199 ↗(On Diff #642)

// Send to the loaded client who is still in the NSS_PREGAME state?

source/network/NetServer.h
258 ↗(On Diff #642)

/**
blabla returns true if blabla
*/

This revision now requires changes to proceed.Mar 1 2017, 5:05 PM
elexis added inline comments.Mar 1 2017, 5:50 PM
source/network/NetClient.cpp
789 ↗(On Diff #642)

guids.reserve(message->m_Clients.size()); doesn't matter much here, but just to get used to it, refs D105#inline-1973

Imarok added inline comments.Mar 2 2017, 1:44 PM
binaries/data/mods/public/gui/session/messages.js
597 ↗(On Diff #642)

"This looks like it assumes no other netstatus messages while in that waiting period." Sure, as that is totally correct. Which other message should arrive during waiting period?

elexis added inline comments.Mar 2 2017, 1:46 PM
binaries/data/mods/public/gui/session/messages.js
597 ↗(On Diff #642)

Lag warnings? Kick? Player Assignments?

elexis added inline comments.Mar 2 2017, 2:01 PM
binaries/data/mods/public/gui/session/messages.js
597 ↗(On Diff #642)

Ah yes, confused g_StatusMessageTypes with g_NetMessageTypes. If the renaming proposed is done, that should be more self-evident

Imarok updated this revision to Diff 687.Mar 2 2017, 8:45 PM
Imarok edited edge metadata.
Imarok marked 9 inline comments as done.

style things suggested by elexis

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/455/ for more details.

Imarok added inline comments.Mar 3 2017, 8:18 PM
binaries/data/mods/public/gui/session/session.xml
162 ↗(On Diff #642)

yeah, the extra label is strange

source/network/NetServer.cpp
1199 ↗(On Diff #642)

sure what else?

elexis added inline comments.Mar 3 2017, 8:19 PM
source/network/NetServer.cpp
1199 ↗(On Diff #642)

As in add that comment?

Imarok added inline comments.Mar 3 2017, 8:20 PM
source/network/NetServer.cpp
1199 ↗(On Diff #642)

Yeah, I did!?

elexis accepted this revision.Mar 17 2017, 4:11 PM
elexis added inline comments.
binaries/data/mods/public/gui/session/messages.js
589 ↗(On Diff #687)

remove that false &&.
also much better with the rename (precise language ftw)

source/network/NetClient.cpp
790 ↗(On Diff #642)

thx

source/network/NetServer.cpp
1191 ↗(On Diff #687)

all GUIDs of clients in the loading state

1200 ↗(On Diff #687)

// Send to the client who has loaded the game but did not reach the NSS_INGAME state yet?

source/network/NetServer.h
259 ↗(On Diff #687)

whether. maybe.

262 ↗(On Diff #687)

capital r, period

This revision is now accepted and ready to land.Mar 17 2017, 4:11 PM
Imarok updated this revision to Diff 880.Mar 21 2017, 7:40 PM

Apply elexis remarks

This revision was automatically updated to reflect the committed changes.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/563/ for more details.