[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).

[elexis]

wrong ticket (meant #4661)

[elexis]

In 23279:

Clean Skirmish maps of default and invalid gamesetting values.

• The gamesetup class rewrite in D2483 will enable Skirmish and Random maps to fix AIs and Civs by specifying them. Hence remove them from all maps that don't intend to fix them (i.e. all except Egypt 3v3).
• Remove default values (and the few irrelevant non-default values) so that the player-chosen settings may be persisted between Skirmish map selection changes, refs #3120, D2483.
• Remove invalid settings: mapType, AISeed and random map settings Script, Size, Seed, Nomad, BaseHeight, BaseTerrain, refs rP20726, rP21274.
• Remove StartingCamera values since they are useless, refs #4839, D1098.
• Add the "new" keyword to Atlas Valleys and Vesuvius from rP23278, so they appear in the "new" filter.
• Unhide Barcania (3)

[elexis]

In 23374:

Gamesetup class rewrite, fixes #5322, refs #5387.

• Decouples settings logically which in turn allows fixing many problems arising from previous coupling.
• Fixes the persist-match-settings feature, refs #2963, refs #3049.
• Improves performance of the matchsetup by rebuilding GUI objects only when necessary.

Provides groundwork for:

• UI to control per-player handicap, such as StartingResources, PopulationCap, StartingTechnologies, DisabledTechnologies, DisabledTemplates, ..., refs #812.
• Map specific settings (onMapChange event), refs #4838.
• Chat notifications announcing which settings changed, refs D1195,
• Multiple controllers setting up the game (since setting types can check for permissions in onUpdateGameAttributes without the need for a new data model or a second gamesetup data network message type), refs #3806, subsequently dedicated server, refs #3556.
• MapBrowser (MapCache, MapTypes, onUpdateGameAttributes interface), refs D1703 and D1777,
• Multiplayer saved games (decoupling and setting dependent unique logic), refs #1088.

Refs ​https://wildfiregames.com/forum/index.php?/topic/20787-paid-development-2016/ ​https://wildfiregames.com/forum/index.php?/topic/20789-paid-development-2016/

Enable maps to restrict setting values:

• If a map specifies an AI or Civs for a playerslot, the controller can't assign a player/other AI or Civ to that slot, refs #3049, #3013.

Fix per player StartingResources, PopulationCap, StartingTechnologies, DisabledTechnologies, DisabledTemplates following rP12756, refs #812, fixes #4504. Use this for DisabledTechnologies on Polar Sea.

Persist user settings for Skirmish maps:

• All user chosen settings are persisted when changing the selected map or maptype, except where the selected map overwrites the setting value and except for Scenario maps which still use the default value where the map doesn't specify the setting value.

• Tickets relating to that Skirmish mapchange user setting persistance:
  • Selecting a map doesn't change the selected civilizations, fixes #3120 (together with r23279 removing map specified Civs).
  • Selecting a map type doesn't reset the selected settings, fixes #5372.
  • Selecting a map doesn't change the selected victory conditions, unless the map specifies those, refs #4661, #3209. (Atlas still writes VictoryConditions to every map.)
  • Consume the player color palette from Skirmish maps, refs rP17040 / #1580. Preserve the selected playercolors when switching the Skirmish/Random map by chosing the most similar colors if the map comes with a different palette.

Rated games:

• Hide and disable Rated game setting unless there are exactly two players, fixes #3950, supersedes D2117.
• Display conspicuous warning if the game is rated, so players are perfectly aware.

Autostarted games:

• Allow using the gamesetup page to autostart matches with arbitrary maps, not only this one tutorial, as reported in D194 and rP19599, refs D11.

Networking:

• Keep gamesetup page open after disconnect, allowing players to read chat messages indicating why the host stopped the server, fixes #4114.
• The message subscription system allows new and mod settings to run custom logic on arbitrary setting changes (most importantly on map change). This removes hardcoded logic restrictions from the gamesetup option unification rewrite in rP19504/D322, refs #3994, such as the hardcoding of setting references in selectMap to biomes from rP20115/D852 and the difficulty from rP20760/D1189, RelicDuration, WonderDuration, LastManStanding, RegicideGarrison, TriggerScripts, CircularMap, Garrison, DisabledTemplates.

Checkboxes:

• Display values of disabled checkboxes with Yes/No labels, fixes D2349, reviewed by nani.

Clean g_GameAttributes of invalid values and gamesetup GUI temporaries, refs #3049, #3883:

• Delete useless values:
  • VictoryScripts, because they are redundant with TriggerScripts, introduced in rP15427.
  • mapType which was written twice to g_GameAttributes following rP12756
  • Description, Keywords, Preview since that doesn't impact simulation and can be loaded from the MapCache
  • mapFilter, mapPath, SupportedBiomes, SupportedTriggerDifficulties since they are only used in the gamesetup
• Delete conditional values if the condition is not met:
  • AIDiff, AIBehavior if there is no AI in that slot
  • Nomad and Size if the maptype is not Random
  • Biome, TriggerDifficulty if the map doesn't support that
  • WonderDuration, RegicideGarrison, RelicCount, RelicDuration if the according VictoryConditions are not enabled
  • LastManStanding if TeamsLocked
  • Rating if there are more than 2 players

MapCache:

• Refactor to MapCache class, store maps of all types and use it in the replaymenu, lobby and session as well.

SettingTabsPanel:

• Remove hardcodings and coupling of the SettingTabsPanel with biomes/difficulties/chat UI from D1027/rP20945.

GamesetupPage.xml:

• Restructure the page to use hierarchical object organization (topPanel, centerPanel, centerLeftPanel, bottomPanel, centerCenterPanel, centerRightPanel, bottomLeftPanel, bottomRightPanel), allowing to deduplicate object position margins and size math and ease navigation.

New defaults:

• Check LockedTeams default in multiplayer (not only rated games).
• Persist the rated game setting instead of defaulting to true when restarting a match, which often lead to unintentional rated games when rehosting.
• 60 FPS in menus since they are animated

Autocomplete sorting fixed (playernames should be completed first).
Refactoring encompasses the one proposed in Polakrity and bb D1651.

Differential Revision: ​https://code.wildfiregames.com/D2483
Tested by: nani
Discussed with:

• nani for blackbox testing, code architecture, performance and MapBrowser in PMs on 2019-12-19, 2019-12-31, 2020-01-06
• Angen for the simulation diff on ​http://irclogs.wildfiregames.com/2020-01/2020-01-03-QuakeNet-%230ad-dev.log
• bb on SettingsTabPanel on ​http://irclogs.wildfiregames.com/2020-01/2020-01-05-QuakeNet-%230ad-dev.log
• Imarok on data model and revised multi-controller plans for #3806 on ​http://irclogs.wildfiregames.com/2020-01/2020-01-07-QuakeNet-%230ad-dev.log

Emojis by: asterix, Imarok, fpre, nani, Krinkle, Stan, Angen, Freagarach