The ready state needs to be reset, when non-observers join.
This fixes the unintended "Host is ready" chat message and the wrong label on the ready button.
Details
Details
- Reviewers
elexis - Commits
- rP19215: Reset ready state when non-observers join in gamesetup
- Trac Tickets
- #4452
- Start a host.
- Enter the gamesetup with a second client and press ready.
- Enter the gamesetup with a third client.
Diff Detail
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
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/111/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/112/ for more details.
Comment Actions
Thanks for finding out that it wasn't the networking rewrite but one of my commits from a day before!
See comment in #4452 for the commit that broke and how it broke it.
The suggested change resets often enough, but too often,
since it resets for non-controllers, even if a client rejoins that was an observer before and is not becoming a player, if all playerslots are assigned.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/135/ for more details.
Comment Actions
As discussed in IRC, this code calls resetReadyData if a client becomes assigned to freeSlot while this wasn't the case before rP18299. Thus the code doesn't exactly represent the required code flow anymore / leaves the wrong impression that we need to reset in that one case when we don't..
Hence suggesting this the following diff which also adds a comment removes and removes the second early return and the negation in the condition to make things easier to read.
Comment Actions
Call resetReadyData for controllers and non-controllers if a client joined the game that is already known to be a player
while not calling it if an observer joined the game that is becoming assigned to a playerslot.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/139/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/140/ for more details.
Comment Actions
Your patch introduces a new bug: Host a game and change no setting -> join with a client and set the client to ready -> the client will not see the notification: "myName is ready!"
Comment Actions
Somehow I could reproduce it the day you posted it, but now the same code works magically (perhaps I don't have a fitting persist-match-settings file? :P)
Comment Actions
to reproduce without the right persist-mapsettings: start the host -> set the second player slot to Unassigned -> go back to main menu -> host again -> join with a second client -> click "I'm ready"
Comment Actions
I further hunted down this bug:
the bug occurs when a client joins into an unassigned slot, because your patch doesn't resetReady for every player joining.
The bug doesn't occur when a client joins into an AI slot, because this triggers a gamesetup message which then resets ready.
Comment Actions
The bug you discovered is present at least in rP17428
Blame page: https://code.wildfiregames.com/source/0ad/browse/ps/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js;17428$443
I guess it might have been added when ready buttons were implemented in rP15006 (alpha 16)
The code we see in rP17428 however also reset if a client joins that is assigned to an unassigned playerslot, so that can't be the reasoning.
What completely boggles my mind is that it matters whether if the replaced playerslot was an AI or an unassigned player...
At least that seems like a good point to start investigating.
Also going to copy & paste said alpha 16 code as those ready functions dont interfer with all the other code that has changed.
Comment Actions
I guess you mean L473 in rP17428. Afaics, this only resets ready when a player rejoins.
What completely boggles my mind is that it matters whether if the replaced playerslot was an AI or an unassigned player...
At least that seems like a good point to start investigating.
As already said, this is the case, because joining into an unassigned slot triggers no game setup message, but joining into an AI slot triggers one. And a game setup message triggers resetReady.
Comment Actions
Ah,
onClientJoin
-> Engine.AssignNetworkPlayer
-> handlePlayerAssignmentMessage
-> updatePlayerList
-> // There was a human, so make sure we don't have any AI left
-> Engine.SetNetworkGameAttributes
-> handleGamesetupMessage
->updateGUIObjects
-> resetReadyData
...obviously.
My branch moves the removal of the AI slot to onClientJoin to reduce the indirection, so I guess that was the right move.
Confirmed with alpha 19, starting host, having another client join and ready up, then joining with another client to an unassigned slot does not reset the ready data!
Indeed that particular line only resets for rejoining clients (as the client is already present in the delivered player assignments).
So if I'm seeing this correctly, rP15171 is the revision that introduced assigning of joining clients that were not present before to (assumed, refs rP17443) unused player slots, but missed to resetReadyData, which was only incidentally the case when an AI got removed from the slot.
Please commandeer the revision, so I can accept your fix! :-)
With regards to your proposed patch (id 189), we still don't need to find a free slot if the client is already a rejoining player and some of the comments don't seem to add information.
Also make sure the commit message refers to both revisions and tickets.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/338/ for more details.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running debug tests... Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/344/ for more details.