[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