[PATCH] Persist match settings should only save some settings

[elexis]

Currently the persist-matchsettings function (see gamesetup.js) store all mapsettings and load it the nexttime.

But some tags shouldn't be saved.

• For example this author tag removed in r17980 was present in every replay due to this bug once the map was started once.

• Another example is DisabledTemplates. Belgian Uplands is the only map that disables the ptolemian lighthouse. But the persistmatchsettings will save this value and when selecting a map that doesn't set it, it actually disables them due to the remains in the persistmatchsettings-file.

Only known settings (which can be changed by the host) should be restored - every other setting ignored (notice on skirmish and scenario maps, less settings can be changed).

As mentioned in comment:7, this issue is not limited to persist-matchsettings but also applies to selecting maps itself.

[elexis]

In order to remove unintended tags, loadPersistMatchSettings() in gamesetup.js needs to delete all properties of the attrs object which can't be changed by the user.

[Imarok]

Is this patch not working or is something missing? Are you working on this patch? If not I would try to make a patch/finish your patch.

[elexis]

The patch is only a rough sketch of how I imagine this specific thing to work. It should only load settings from that file that are allowed by a whitelist. The construction of the whitelist depends on the maptype (as the user can't change all settings on skirmish/scenario maps for example) and is not correct nor complete in that patch. Knock yourself out on that one.

[Imarok]

Only restore the settings the user can change (Based on elexis' roughsketch.patch)

[elexis]

Settings like ceasefire can be changed on both skirmish and random maps. So you should add those only once if mapType == "skirmish" || mapType == "random". This way you omit the duplicate array entries.

[Imarok]

omit duplicate array entries

[Imarok]

cleanup

[Imarok]

added one parenthesis, because ! has a higher precedence then ... in ...

[Imarok]

Removed an else and added the ugly !! ;)

[elexis]

For the persist-match-settings, the patch seems okay.

However the problem described in the ticket is bigger, as the examples mentioned also occur when changing the map. selectMap basically persists matchsettings as well.

For example pick the random map "belgian uplands". By typing warn(uneval(g_GameAttributes.settings.DisabledTemplate)); to the JS console, you will see that the ptolemian lighthouse is disabled. If you change to a different map, that attribute will still be remaining, even if the map does not specify it.

The whitelist-filter should become a global function that is used by both selectMap and loadPersistMatchSettings. Be careful as there might be some pitfalls.

[Imarok]

remove the settings the user can not change also when changing the map

[Imarok]

removed some unneeded code and renamed cleanUpSettings

[wraitii]

Any reason for the "load file" comment and the !! thing?

[elexis]

The idea behind the !! was to not have the RatingEnabled variable in the gameattributes if its not true. So it would be undefined and thus throw an error without !!.

[elexis]

• With the latest sketch of #3994 we can nicely decide whether a given setting can be changed on a maptype, so that one array would become obsolescent.

• I'm not completely excited about the fact that we check whether the option can be changed, but that we don't test whether the option can actually be changed to the given attribute.

The linked rewrite also doesn't provide a method to derive the changed attribute from the gameAttributes name (with which one could call the dropdown methods to verify the integrity of that setting). In #3806 you run into the exact same issue.

Using a mapping g_GameAttributes.settings.PlayerData[i].Color -> playerColorPicker to do that seems ugly.

A hopefully less ugly alternative is to change the persist-matchsettings to not save gameattributes directly but only the current GUI object names and the values it sets things to, for example `[{"mapType":"skirmish"}, {"playerColor": "red", "idx": 4}, {"mapSize": 320}].

[Imarok]

Replying to elexis:

A hopefully less ugly alternative is to change the persist-matchsettings to not save gameattributes directly but only the current GUI object names and the values it sets things to, for example `[{"mapType":"skirmish"}, {"playerColor": "red", "idx": 4}, {"mapSize": 320}].

This sounds like a very good idea. Also increases readability of stored settings

[elexis]

Removing review as we would have to rewrite the specific code again if we also want to check for the integrity of the values, though the patch probably works as advertized and solves the original issues reported in the ticket. If #3806 will fail for any reason, we can take another look, but the rewrite should be merged asap.

[elexis]

Same as #2803

[elexis]

Feature freeze in 2 days.

[elexis]

Backlogging due to lack of progress

[elexis]

In 19504:

Unify gamesetup option handling, fixes #3994, refs #3049.

Removes a lot of duplication and ugly GUI handling code with very similar, yet different code paths.
Move the setting specific logic to the functions of that setting and keep the universal logic in global atomic functions.
Make gamesetup.xml agnostic of all gamesetup settings.

Adding a new gamesetup option now only requires adding one hunk with the titles and values and one line in g_OptionOrderGUI.
Opens up the possibility to iterate over all settings, refs #3806, #3883.

Allow starting singleplayer games in observermode with only AIs assigned, fixes #4078.
Autocomplete translations of all setting titles and selected title values like playernames and victory conditions.

Transfer the AI difficulty when swapping with a player.
Move logic from onTick to the GUI handling functions.
Change some global consts to var, so that contributors become invited to change them from a different place.
Add missing startGameButton tooltip translation.

Differential Revision: ​https://code.wildfiregames.com/D322
Reviewed By: wraitii
Some strings reviewed By: leper

[elexis]

In 19573:

Fix the most annoying instance of the persist-matchsettings-bugs that prevented players to build the Ptolemian lighthouse in case of previously having selected a map where that template is disabled.
Most recently reported by nigel87.

Differential Revision: ​https://code.wildfiregames.com/D489
Reviewed By: Grugnas
Refs #3883

[Imarok]

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

[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