Page MenuHomeWildfire Games

Persistmatchsettings bug that assigned both AI player and local player to the same slot
ClosedPublic

Authored by elexis on May 6 2017, 5:52 PM.

Details

Summary

Since rP19504, g_PlayerAssignments wasn't sanitized each updatePlayerList call anymore.
This means it is possible to have both a player and an AI assigned to playerslot 1 in single and multiplayer mode.

Test Plan
  1. Enable Persist Match Settings
  2. Open an SP or MP gamesetup page
  3. Assign an AI to the first playerslot
  4. Get back to the main menu
  5. Open the page again
  6. You're now player 1 + an AI is assigned to that slot. If you're starting a game, the AI will play in your place.

Notice it's because we set g_PlayerAssignments.local.player = 1 and then set AI = "petra" from the loadPersistMatchSettigns call without santizing it as we did before with the updatePlayerListCall (since we now only sanitize in onClientJoin and swapPlayer)
Do the same in MP and see that onClientJoin took care of assigning the player to an unassigned slot, so it only applies to SP.

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

elexis created this revision.May 6 2017, 5:52 PM
Vulcan added a subscriber: Vulcan.May 6 2017, 6:43 PM

Build is green

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

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

Sandarac accepted this revision.May 6 2017, 10:25 PM
Sandarac added a subscriber: Sandarac.

Tested, fixes the bug as advertised.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
941 ↗(On Diff #1690)

As you mentioned, this comment could possibly be more clear that this is about unassigning AIs.

This revision is now accepted and ready to land.May 6 2017, 10:25 PM
elexis edited the summary of this revision. (Show Details)May 12 2017, 12:57 PM
elexis edited the test plan for this revision. (Show Details)
elexis updated this revision to Diff 1891.May 13 2017, 1:40 AM

Actually it bugged in MP as well.
Resetting the AIDiff doesn't seem necessary.

In the MP case:

initGUIObjects calls loadPersistMatchSettings (which restores sg_GameAttributes.settings.PlayerData[1].AI = "petra") and
then calls updateGameAttributes (which tells the netserver to broadcast g_GameAttributes to all clients.

However, before this message arrives on the clients at handleGamesetupMessage, onClientJoin is called which sets AI = "" on the g_GameAttributes variable which will be overwritten by that handleGamesetupMessage call.

So the order of execution is:

loadPersistMatchSettings
onClientJoin
handleGamesetupMessage

while it might have been expected to be:

loadPersistMatchSettings
handleGamesetupMessage
onClientJoin

but since we can't really chose when the client joins, just sanitizing it every time we receive new assignments works (and the previous code worked like this and sanitized each time in updatePlayerList which was called very often).

The SP case is solved too, since sanitizePlayerData is also called from selectMap while the player is assigned to 1.

elexis requested review of this revision.May 13 2017, 1:41 AM
elexis edited edge metadata.

Build is green

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

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

Imarok edited edge metadata.May 17 2017, 9:08 PM

Looks good codewise

Imarok accepted this revision.May 17 2017, 10:01 PM
This revision is now accepted and ready to land.May 17 2017, 10:01 PM
This revision was automatically updated to reflect the committed changes.