Opened 3 years ago

Last modified 4 months ago

#3883 new defect

[PATCH] Persist match settings should only save some settings

Reported by: elexis Owned by: Imarok
Priority: Must Have Milestone: Backlog
Component: UI – Game setup Keywords: patch
Cc: Patch:

Description (last modified by 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.

Attachments (11)

roughsketch.patch (2.0 KB) - added by elexis 3 years ago.
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.
persist_match_settings.patch (1.9 KB) - added by Imarok 3 years ago.
Only restore the settings the user can change (Based on elexis' roughsketch.patch)
persist_match_settings_v2.patch (1.7 KB) - added by Imarok 3 years ago.
omit duplicate array entries
persist_match_settings_v3.patch (1.4 KB) - added by Imarok 3 years ago.
cleanup
persist_match_settings_v4.patch (1.4 KB) - added by Imarok 3 years ago.
added one parenthesis, because ! has a higher precedence then ... in ...
persist_match_settings_v5.patch (1.4 KB) - added by Imarok 3 years ago.
persist_match_settings_v6.patch (1.7 KB) - added by Imarok 3 years ago.
persist_match_settings_v7.patch (1.8 KB) - added by Imarok 3 years ago.
persist_match_settings_v8.patch (1.8 KB) - added by Imarok 3 years ago.
Removed an else and added the ugly !! ;)
persist_match_settings_v9.patch (2.6 KB) - added by Imarok 3 years ago.
remove the settings the user can not change also when changing the map
persist_match_settings_v9.1.patch (3.3 KB) - added by Imarok 3 years ago.
removed some unneeded code and renamed cleanUpSettings

Download all attachments as: .zip

Change History (31)

Changed 3 years ago by elexis

Attachment: roughsketch.patch added

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.

comment:1 Changed 3 years ago by 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.

comment:2 Changed 3 years ago by 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.

comment:3 Changed 3 years ago by Imarok

Owner: set to Imarok

Changed 3 years ago by Imarok

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

comment:4 Changed 3 years ago by Imarok

Keywords: review patch added
Summary: Persist match settings should only save some settings[PATCH] Persist match settings should only save some settings

comment:5 Changed 3 years ago by 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.

Changed 3 years ago by Imarok

omit duplicate array entries

Changed 3 years ago by Imarok

cleanup

Changed 3 years ago by Imarok

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

Changed 3 years ago by Imarok

Changed 3 years ago by Imarok

Changed 3 years ago by Imarok

comment:6 Changed 3 years ago by Imarok

Milestone: BacklogAlpha 21

Changed 3 years ago by Imarok

Removed an else and added the ugly !! ;)

comment:7 Changed 3 years ago by elexis

Keywords: review removed

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.

Changed 3 years ago by Imarok

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

comment:8 Changed 3 years ago by Imarok

Keywords: review added

Changed 3 years ago by Imarok

removed some unneeded code and renamed cleanUpSettings

comment:9 Changed 3 years ago by wraitii

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

comment:10 Changed 3 years ago by 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 !!.

comment:11 Changed 3 years ago by 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}].

comment:12 in reply to:  11 Changed 3 years ago by 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

comment:13 Changed 3 years ago by elexis

Keywords: review removed

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.

comment:14 Changed 3 years ago by elexis

Description: modified (diff)

Same as #2803

comment:15 Changed 3 years ago by elexis

Milestone: Alpha 21Alpha 22

Feature freeze in 2 days.

comment:16 Changed 2 years ago by elexis

Milestone: Alpha 22Backlog

Backlogging due to lack of progress

comment:17 Changed 2 years ago by 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

comment:18 Changed 2 years ago by 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

comment:19 Changed 13 months ago by elexis

Description: modified (diff)
Keywords: simple removed

comment:20 Changed 4 months ago by Imarok

Component: UI & SimulationGame setup

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

Note: See TracTickets for help on using tickets.