[PATCH] In Match Setup screen, selecting a map shouldn't change the selected civilizations

[dg1727]

In the Match Setup screen, if you first select one or more civilizations, then change the map (even to one with the same # of players), the selected civilization(s) revert to Random.

Instead, the civilizations should stay at what you selected. This should include if you change to a map with a smaller number of players. If you change to a map with a larger number of players (e.g., from 2 to 4), then (e.g.) the first 2 civilizations should stay at what you selected.

This issue was pointed out in a video by YouTube user WretchedJ, although when he did the video, he didn't notice that the civs had reverted to Random - he was only frustrated that when he began the game, his civ was different than he had selected.

I suppose many players would prefer that if they select 4 specific civs, then mistakenly change to a 2-player map, then change to a different 4-player map, the 3rd & 4th civs should reappear as the values that were just selected, instead of reverting to Random.

[elexis]

Already the case for random maps, but not skrimish maps (scenario maps have fixed civs).

[Jared Ryan Bills]

first commit and attempt to fix the problem

[Jared Ryan Bills]

I attempted to fix the problem, please review.

[elexis]

SubmittingPatches

[Jared Ryan Bills]

There, I tried making a proper patch.

[Jared Ryan Bills]

One last detail

[Palaxin]

Regarding the credits: the names are sorted according to the nickname, not the real name. Thus you need to look for "O". However, if you don't provide a nickname, then sort according to your surname. Thx

[elexis]

Review:

• As mentioned on IRC: unneeded parnthesis, newlines after ? and : and don't create variables for the length properties. If you use pData as a variable name for the current player data, it will stay below the 80 character limit suggested in wiki:Coding_Conventions
• selectNumPlayers has the same logic, it should use a common function
• As mentioned by Palaxin the contributors entry should be sorted correctly.

If you fix these points, I would commit that.

The patch doesn't address the last point of the ticket description. In order to accomplish that, the civs would have to be saved in g_PlayerAssignments and copied to the player data when starting the game. This however will mean that a lot of things will have to be changed, even the network protocol. Alternatively the assignments could also be stored in a global player data array. This however will introduce a lot of duplicate logic, as that array will have to be updated with every change of the assignments and on join/leave etc. Not completely convinced we should do that, but if someone comes up with an elegant patch for that...

#4037 would help with not killing the currently selected players / civs / teams.

[Jared Ryan Bills]

Updated according to review

[Jared Ryan Bills]

Very well, I added the latest patch according to the latest criticism. I however contest that I should create variables for the .length properties because calling .length parses for string length each time. Though I didn't actually keep the variables to suit your demands.

[Lionkanzen]

You can't close the ticket. Need reviewed and committed , so the developer will close this.

[Jared Ryan Bills]

Thanks, that's why I reopened it.

[elexis]

• Since that code is only called when selecting a map and getting the string length takes some microseconds, the performance won't matter in comparison to readability.

• The code hasn't been merged with the similar code in selectNumPlayers (if you don't understand my comments, just ask instead of doing like that comment wasn't posted ;) ). source:ps/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js#L1054​ Even if it's just one statement on three lines, the reader would probably appreciate an abbreviation so that he has to understand that code only once. You could call that function resizePlayerData and pass the desired length and the replacement playerdata, which is the maps playerdata in one case and the player defaults in the other.

• Also a space after //

[Jared Ryan Bills]

Made function applying the fix and applied in 2 different places

[Jared Ryan Bills]

Fixed one last bug

[elexis]

Thanks for the update!

• Unneeded newline in resizePlayerData
• It seems cleaner to not use a default argument there, but just pass the length in both cases

• Notice everytime the playerdata is resized, some players might become unassigned. Both selectNumPlayers and selectMap have a check to unassign those players in multiplayer and in singleplayer reassign the player to slot 1 in case he became unassigned. So it would be really great for the code to also move that logic to the new function.

[Jared Ryan Bills]

Cleaned up code and moved player (un)assignment into function.

[Jared Ryan Bills]

Added changes from IRC review.

[Jared Ryan Bills]

Fixed bug related to gaia player in skirmish maps.

[elexis]

Nice, you found a non-obvious bug! :) Now take a look at what sanitizePlayerData does and notice you just have to move the resizePlayerData call one line below instead of duplicating the gaia check.

[Jared Ryan Bills]

Moved sanitizePlayerData above resizePlayerData.

[elexis]

In 18458:

Persist the the valid subset of chosen player settings when changing the skirmish map. Patch by Offensive ePeen, refs #3120.

[elexis]

Thanks for the patch!

• Always add the review keyword once you think the patch is ready to commit, otherwise it will not be considered.
• Trailing whitespace in 4 places
• Only reassigning the local player to 1 if it's outside of the valid range
• Renaming ply to maxPlayers and defPData to targetPlayerData and player to playerID, since those names are a bit more in line with the rest of the code.
• Removing one changes from the comment, that unneeded "reassign player 1" and "on map change" comment.

Keeping the ticket open in case someone comes up with an elegant approach to also solve the persisting of teams/civs in case the number of players is changed from M > N to N back to M. The g_PlayerAssignments change can just be moved to launchGame then, but the playerData settings would have to be cached somewhere (-> likely not elegant).

[elexis]

Notice the commit broke the gamesetup when someone with playerID N disconnected, the host reduced the number of players to M<N, and that client rejoining the gamesetup again: #4120.

[elexis]

In 18702:

Fix critical gamesetup bug again where the game can't be started after a previously assigned disconnected client rejoins a full game. Same as #3602, reintroduced by 18458, refs #3120. Fixes #4120.

[elexis]

In 18806:

Revert the persistance of playersettings for skirmish maps in r18458, because it overwrites potential team numbers suggested by the map. Tested by mimo, fixes #4248, refs #3120.
Keep the code simplification and removal of duplicate code.

[Imarok]

Move tickets to Game Setup as UI & Simulation got some sub components.

[elexis]

Atlas writes the VictoryConditions to all saved maps, and Atlas doesn't work correctly if they're not specified, so the issue is not limited to the gamesetup part (but the gamesetup part will be fixed by ​Phab:D2483).