This patch rewrites the gamesetup to use object orientation semantics using class notation, fixing #5322.
Resolves the persist-match-settings problem, fixing #3883.
Protects g_GameAttributes from receiving incompatible, invalid or outdated values, refs #3049.
Paves the way for map-specific setting types refs #4838, chat notifications announcing which settings changed refs D1195, multi-controller gamesetup refs #3806, dedicated server refs #3556, multiplayer-savegame refs #1088, refs https://wildfiregames.com/forum/index.php?/topic/20787-paid-development-2016/
Removes setting coupling that could not be removed with D322.
Also:
Empower map authors to fix a playerslot to an AI, refs #3013, #3049.
Persist all settings when selecting Skirmish maps except where the map determines a value, fixes #3120, #4661, #5372.
Rated games are now only available for 2-player lobby matches, fixes D2117, part of #3950.
Rated games are now indicated by a conspicuous bright red warning message.
Player color palette now read from Skirmish maps.
Values of disabled checkboxes are now indicated by Yes/No labels, fixes D2349, reviewed by nani.
Gamesetup page is kept open after disconnect, so that unread chat messages can still be read, fixes #4114.
Support autostart of matches that aren't the starting_economy_walkthrough, refs rP19599, D11.
Move initSPtips, fixes D1651.
LockedTeams is now enabled by default in multiplayer and persist rated game setting.
Table of Contents
Chapter 1: Analysis of the current gamesetup, refs D322
Chapter 2: Objectives of this gamesetup class rewrite, fixes #5322, refs #5387
Chapter 3: New code design highlight 1: g_GameAttributes Cleanse, refs #3049
Chapter 4: New code design highlight 2: Setting decoupling
Chapter 5: New code design highlight 3: Persist match settings solution, fixes #3883.
Chapter 6: New feature highlight 1: More setting persistence
Chapter 7: New feature highlight 2: More map-determined values
Chapter 8: New feature highlight 3: Gamesetup Usability improvements
Chapter 9: Upcoming features with now solved requirements
Chapter 10: Implementation Highlight 1: Class hierarchy
Chapter 11: Implementation Highlight 2: Deduplication using abstract classes
Chapter 12: Implementation Highlight 3: Recursive updateGameAttributes approach
Chapter 13: Transported problem 1: user-setting preservance and map determination of gamesettings
Chapter 14: Transported Problem 2: Broken "Skirmish" maptype
Chapter 15: Transported Problem 3: PopulationCapacity, StartingResources
Chapter 16: Performance evaluation
Chapter 17: Correctness evaluation considering planned Multi-controller gamesetup, refs #3806
Chapter 1: Analysis of the current gamesetup, refs D322
Objectives of the last gamesetup rewrite in D322:
Achieved:
- Deduplication: almost no line written twice
Partially achieved:
- Decoupling: have only 1 hunk per setting type
- Flexibility: Allow insertion of gamesettings by only specifying one new object
Problems with the D322 approach:
- One object per setting, but there is no "this" instance, thus having to resort to global variables when requiring a state per setting. For example g_MapFilterList, g_MapSelectionList, g_BiomeList.
- Some assumptions were made to further deduplication that are problematic for settings that only exist on one map
Chapter 2: Objectives of this gamesetup class rewrite, fixes #5322, refs #5387
- Transition to object semantics using class syntax. This gains the ability to implement unique setting logic utilizing object local state.
- Remove remaining coupling of settings logic. This gains the ability to implement new settings without having to modify existing settings.
- Remove global variables and procedures.
- Keep duplication minimal.
- Facilitate integration of planned features.
Chapter 3: New code design highlight 1: g_GameAttributes Cleanse, refs #3049
There are many values written to g_GameAttributes that are invalid or irrelevant.
This was reported in #3049 for instance:
The code could also use some improvement such that the persistent game settings can work with just storing the minimum of information needed to recreate the settings instead of saving everything and then hoping that it doesn't cause conflicts/issues later
The list of values that are conditionally invalid or entirely irrelevant:
- g_GameAttributes.settings.VictoryScripts had been useless and is deleted, introduced by rP15427
- WonderDuration, RelicDuration, RelicCount not defined anymore if the victory condition isn't met
- g_GameAttributes.mapType == g_GameAttributes.settings.mapType from rP12756 requiring simulation changes
- Nomad is not stored anymore if it is irrelevant (i.e. not a random map)
- Rating is not stored anymore if the match is not setup in the lobby or if it has more than 2 players.
- LastManStanding is not be stored anymore if LockedTeams is enabled
- SupportedTriggerDifficulties is never copied to g_GameAttributes anymore
- SupportedBiomes is never copied to g_GameAttributes anymore
- Biome is not stored anymore if the map doesn't support Biomes
- TriggerDifficulty is not stored anymore if the map has no trigger difficulties
- mapPath never stored anymore
- PlayerData not copying everything anymore, only whitelisted properties, and only on mapchange, thus not being needed to be deleted explicitly anymore between mapchanges (e.g. DisabledTechs, StartingResources, ...)
- Map Description never copied anymore
- MapName copied for now since it means the replay menu doesnt have to load all maps when opening the page and since various GUI pages (savegame, gamedescription.js), and rmgen (rmgenlogger) consume it currently.
The following diff illustrates the values that had been written before to g_GameAttributes and are removed in this patch:
--- before 2019-12-17 14:33:55.600037886 +0100 +++ after 2019-12-17 14:33:38.086704002 +0100 @@ -1,68 +1,54 @@ { "settings": { "PlayerData": [{ "Name": "elexis", - "AI": "", - "AIDiff": 3, - "AIBehavior": "random", + "AI": false "Civ": "maur", "Color": { "r": 21, "g": 55, "b": 149 }, "Team": -1 }, { "Name": "Marcus Vipsanius Agrippa", "AI": "petra", "AIDiff": 3, "AIBehavior": "random", "Civ": "rome", "Color": { "r": 150, "g": 20, "b": 20 }, "Team": -1 }], "AISeed": 2207312901, "Ceasefire": 0, "CheatsEnabled": true, "CircularMap": true, - "Description": "Each player starts the match atop a large flat plateau.\n\nTo the East lies a large bay with fishing opportunities. To the West is a rugged hinterland with an unclaimed plateau commanding the valley below.", "DisableSpies": false, "DisableTreasures": false, "ExploreMap": false, - "Keywords": [], "LastManStanding": false, "LockTeams": false, "Name": "Acropolis Bay (2)", - "Nomad": false, "PopulationCap": 300, - "Preview": "acropolis_bay.png", - "RegicideGarrison": false, - "RelicCount": 2, - "RelicDuration": 20, "RevealMap": false, "Seed": 813227136, - "Size": 256, "StartingResources": 300, "TriggerScripts": ["scripts/TriggerHelper.js", "scripts/ConquestCommon.js", "scripts/Conquest.js"], "VictoryConditions": ["conquest"], - "VictoryScripts": ["scripts/TriggerHelper.js", "scripts/ConquestCommon.js", "scripts/Conquest.js"], - "WonderDuration": 20, - "mapType": "skirmish" }, "engine_version": "0.0.24", "gameSpeed": 1, "map": "maps/skirmishes/Acropolis Bay (2)", - "mapFilter": "default", - "mapPath": "maps/skirmishes/", "mapType": "skirmish", "matchID": "CCA3D3B3ABCDA335", "mods": [ ["public", "0.0.24"] ], "timestamp": 1576589047 }
The gamesetup rewrite facilitates erasure of these irrelevant or invalid values by having one class per setting type, thus providing more room for code to detect whether a setting is invalid or not.
The new gamesetup avoids copying all settings of a selected map to g_GameAttributes, thus avoiding to write a value that has to be conditionally deleted later on, and avoiding to copy values that no consumer or handler exists for.
At last, some map specific values are only copied when the match starts, so that the values don't have to be deleted or reset between map selections.
Chapter 4: New code design highlight 2: Setting decoupling
While decoupling settings (1 setting = 1 hunk of code) was an objective of D322, it failed to achieve it completely due to the lack of a "this" reference (class semantics).
The functions reloadMapFilterList, reloadMapList, selectMap, sanitizePlayerData, swapPlayers, ensureUniquePlayerColors, nitDefaults, reloadMapSpecific, reloadTriggerDifficulties, reloadBiomeList, reloadGameSpeedChoices
were either called from setting functions that should be agnostic of these settings types or wrote setting values that they should be agnostic of (see https://en.wikipedia.org/wiki/Information_hiding).
This is not only ugly code, but it actually prevents gamesetup developers from introducing new settings without having to modify other settings,
which again prevents mod developers from inserting new gamesetup code without copying and modifying existing gamesetup code,
which again makes it more likely that two gamesetup mods inserting a feature become incompatible,
this ultimately makes it impossible for every map to introduce a map specific setting, as each of them would have to be hardcoded in the gamesetup.js code.
Consider these examples:
-function selectMap(name)
-{
- // Reset some map specific properties which are not necessarily redefined on each map
- for (let prop of ["TriggerScripts", "CircularMap", "Garrison", "DisabledTemplates", "Biome", "SupportedBiomes", "SupportedTriggerDifficulties", "TriggerDifficulty"])
- g_GameAttributes.settings[prop] = undefined;
-
- let mapData = loadMapData(name);
- let mapSettings = mapData && mapData.settings ? clone(mapData.settings) : {};
-
- if (g_GameAttributes.mapType != "random")
- delete g_GameAttributes.settings.Nomad;
-
- if (g_GameAttributes.mapType == "scenario")
- {
- delete g_GameAttributes.settings.RelicDuration;
- delete g_GameAttributes.settings.WonderDuration;
- delete g_GameAttributes.settings.LastManStanding;
- delete g_GameAttributes.settings.RegicideGarrison;
- }-function loadPersistMatchSettings()
-{
- ...
- if (!g_IsNetworked)
- mapSettings.CheatsEnabled = true;
-
- // Replace unselectable civs with random civ
- let playerData = mapSettings.PlayerData;
- if (playerData && g_GameAttributes.mapType != "scenario")
- for (let i in playerData)
- if (!g_CivData[playerData[i].Civ] || !g_CivData[playerData[i].Civ].SelectableInGameSetup)
- playerData[i].Civ = "random";
-
- if (mapSettings.PlayerData)
- sanitizePlayerData(mapSettings.PlayerData);
-
- reloadMapFilterList();
- reloadMapSpecific();
-
- g_GameAttributes.settings.RatingEnabled = Engine.HasXmppClient();
- Engine.SetRankedGame(g_GameAttributes.settings.RatingEnabled);
-
- supplementDefaults();
-
- ...
-}-function launchGame()
-{
- ...
-
- // Select random map
- if (g_GameAttributes.map == "random")
- selectMap(pickRandom(g_Dropdowns.mapSelection.ids().slice(1)));
-
- if (g_GameAttributes.settings.Biome == "random")
- g_GameAttributes.settings.Biome = pickRandom(
- typeof g_GameAttributes.settings.SupportedBiomes == "string" ?
- g_BiomeList.Id.slice(1).filter(biomeID => biomeID.startsWith(g_GameAttributes.settings.SupportedBiomes)) :
- g_GameAttributes.settings.SupportedBiomes);
-
- g_GameAttributes.settings.VictoryScripts = g_GameAttributes.settings.VictoryConditions.reduce(
- (scripts, victoryConditionName) => scripts.concat(g_VictoryConditions[g_VictoryConditions.map(data =>
- data.Name).indexOf(victoryConditionName)].Scripts.filter(script => scripts.indexOf(script) == -1)),
- []);
-
- g_GameAttributes.settings.TriggerScripts = g_GameAttributes.settings.VictoryScripts.concat(g_GameAttributes.settings.TriggerScripts || []);
-
- g_GameAttributes.settings.mapType = g_GameAttributes.mapType;
-
-
- // Determine random civs and botnames
- for (let i in g_GameAttributes.settings.PlayerData)
- {
- // Pick a random civ of a random culture
- if (chosenCiv == "random")
- ....
- g_GameAttributes.settings.PlayerData[i].Civ = chosenCiv;
-
- // Pick one of the available botnames for the chosen civ
- ...
- }
- ...
-}- "enableRating": {
- ...
- "set": checked => {
- ...
- if (checked)
- {
- g_Checkboxes.lockTeams.set(true);
- g_Checkboxes.enableCheats.set(false);
- }
- },-function launchTutorial()
-{
- g_GameAttributes.mapType = "scenario";
- selectMap("maps/tutorials/starting_economy_walkthrough");
- launchGame();
-}-function sanitizePlayerData(playerData)
-{
- // Remove gaia
- if (playerData.length && !playerData[0])
- playerData.shift();
-
- playerData.forEach((pData, index) => {
-
- // Use defaults if the map doesn't specify a value
- for (let prop in g_DefaultPlayerData[index])
- if (!(prop in pData))
- pData[prop] = clone(g_DefaultPlayerData[index][prop]);
-
- // Replace colors with the best matching color of PlayerDefaults
- if (g_GameAttributes.mapType != "scenario")
- {
- let colorDistances = g_PlayerColorPickerList.map(color => colorDistance(color, pData.Color));
- let smallestDistance = colorDistances.find(distance => colorDistances.every(distance2 => (distance2 >= distance)));
- pData.Color = g_PlayerColorPickerList.find(color => colorDistance(color, pData.Color) == smallestDistance);
- }
-
- // If there is a player in that slot, then there can't be an AI
- if (Object.keys(g_PlayerAssignments).some(guid => g_PlayerAssignments[guid].player == index + 1))
- pData.AI = "";
- });
-
- ensureUniquePlayerColors(playerData);
-}
-This antipattern is called https://en.wikipedia.org/wiki/Shotgun_surgery because introducing one new setting requires changing many places of the code, when it should only be necessary to insert one new class.
The gamesetup class rewrite introduces the event-subscription system known from the lobby rP23172, savegame rP22985 and session dialogs rP23076 class implementation to remove the remaining coupling of settings.
Hence without the gamesetup class rewrite it was unfeasible if not impossible for map makers to introduce map specific setting types, refs #4838,
thus also eases implementation of new features (as one doesnt have to learn all the other code) and completes the objective of D322 to decouple setting types.
Chapter 5: New code design highlight 3: Persist match settings solution, fixes #3883.
GameSettingsFile.js is the successor of the "persistmatchsettings" implementation from .
The persistent persist-match-settings problem is solved by the new updateGameAttributes approach and the concept change that every setting now only hardcodes its dependencies and self-corrects its own value.
The new gamesetup classes do not assume g_GameAttributes to have been formed correctly by the other gamesetting classes.
This way, if a broken gamesettings file is loaded (or received from a map, from a formerly selected map or from a malicious netclient in the future),
invalid values for that handlers exist will be deleted or defaulted instead of throwing errors.
The new persist-match-settings code only consists of writing g_GameAttributes to a JSON file, and loading that file and calling updateGameAttributes.
So the solution to fix persist-match-settings was to rewrite all code except the persist-match-settings code in fact.
There are only two exceptions where properties are copied without examination and whitelisting:
- Loading the settings file from disk assigns the entire object. It still yields a correct state after loading because it was saved as a correct state (following the mod check). Invalid files should not result in errors, in the worst case in dead properties in g_GameAttribute as before.
- Receiving the settings object from the server copies the entire object. When other clients become able to send g_GameAttribute settings (see #3806), then they might be able to insert dead properties to g_GameAttributes, unless that patch introduces a function that copies the properties individually in every setting class instance.
So these two remaining known cases of storing non-whitelisted property values can be addressed in the multi-controller patch by adding a new function to every class that copies whitelisted properties.
Other than that the problem is considered fixed by the setting decoupling and class semantics provided in this patch.
Chapter 6: New feature highlight 1: More setting persistence
- Fixes #3120 persist civ when switching skirmish map
- Fixes #4661 persist civctory condition when switching skirmish map
- Fixes #5372 persist settings when switching maptype to random
- ColorPicker now preserves the color setting between mapchanges
Chapter 7: New feature highlight 2: More map-determined values
- Map specified AI now means the playerslot cannot be assigned to a human player, fixes part of #3013, #3049.
- Map specified Civ now means the Civ is fixed for that slot.
- ColorPicker now supports the color palette of skirmish maps. ColorPicker preserves the user color choice on skirmish maps by selecting the most similar color that the map provides
Chapter 8: New feature highlight 3: Gamesetup Usability improvements
- Default change: LockedTeams is now enabled by default in multiplayer, instead of only in lobby matches by default
- Default change: Rated game setting is now persisted too instead of defaulting to true when starting a new lobby match
- Rated games are now only available for 2-player lobby matches, fixes D2117, part of #3950
- Rated games are now indicated by a conspicuous bright red warning message.
- Values of disabled checkboxes are now indicated by Yes/No labels, fixes D2349, reviewed by nani.
- Gamesetup page is kept open after disconnect, so that unread chat messages can still be read, fixes #4114.
Also Fixes D1651, move initSPtips, and support autostart of matches that aren't the starting_economy_walkthrough, refs rP19599, D11.
Chapter 9: Upcoming features with now solved requirements
The gamesetup class implementation rectifies conceptual gamesetup flaws that blocked implementation of these features:
- Chat notifications that announce which settings changed D1195
- Easier introduction of new generic types of setting controls (for example slider, refs #4381)
- Easier introduction of map specific gamesettings #4838 #3049
- Remote clients being selectively permitted to change gamesettings #3806 D431
- This in turn would allow to solve the rated-game 1v1 fakery problem by implementing a lobby bot hosting rated games
- refs #4838 map specific settings
refs #4379 Arbitrary starting resources in the gamesetup
refs #3049 Rewrite/restructure gamesetup
refs #3013 Gamesetup - Improve match setup support for special (trigger) map
refs #3806 Gamesetup - Optionally allow players to setup the game
refs #3556 Dedicated server
refs #1088 multiplayer-savegame loading (since individual gamesetting handlers are now more able to react to a savegame-loading matchsetup constraint than they were with the D322 approach)
Chapter 10: Implementation Highlight 1: Class hierarchy
As familiar from the previous GUI class rewrites (see #5387), there is now one handler class per logical setting.
This is one class per game setting value, a class per unique UI element that has some code.
As before, the only network synchronized state is g_GameAttributes and g_PlayerAssignments, the other states are local to the classes and are determined by these two global states.
Few classes like the GameSettingsControl, PlayerAssignmentsControl, StartGameControl are introduced that are interfaces to these data models and events and are accessed by all the individual UI and gamesetting handler classes.
gamesetup/ folder changes:
- Previously 67 global functions declarations were stored in one JS file, and all GUI objects in one XML file. Now it's stored in 75 js files, 21 xml files, 1 json file in 14 folders:
Controls/ GameSettings/ GameSettings/PerPlayer/ GameSettings/PerPlayer/Dropdowns/ GameSettings/PerPlayer/Buttons/ GameSettings/Single/ GameSettings/Single/Checkboxes/ GameSettings/Single/Dropdowns/ Panels/ Panels/Chat/ Panels/Chat/ChatMessages/ Panels/Buttons/ NetMessages/
This was the most intuitive approach with the greatest amount of decoupling.
The code was obtained by moving each logic brick into an own class.
If two different purposes of a class were identified, usually the class was split into two more simple classes.
This furthers understanding, modifying and extending individual components of the gamesetup implementation without having to understand or modify or extend the surrounding code and files.
For mods this has the advantage that they dont have to overwrite an entire GUI page implementation to copy & modify a part of it.
- gamesetup.js There were 59 global variable declarations using the var keyword in gamesetup.js. These were entirely removed in this patch for the introduction of one GamesetupPage class global that owns the individual handlers. There were 200 inline functions for gamesetting controls, 67 function declarations and 20 function expressions for netmessage event handlers. Now there 80 these classes:
ChatMessageEvents.ClientChat = class ChatMessageEvents.ClientConnection = class ChatMessageEvents.ClientKicked = class ChatMessageEvents.ClientReady = class ChatMessageEvents.GameSettingsChanged = class class CancelButton class ChatInputAutocomplete class ChatInputPanel class ChatMessageEvents class ChatMessagesPanel class ChatPanel class CivInfoButton class GameDescription class GameRegisterStanza class GameSettingControl class GameSettingControlCheckbox extends GameSettingControl class GameSettingControlDropdown extends GameSettingControl class GameSettingControlManager class GameSettingControls class GameSettingsControl class GameSettingsFile class GameSettingsPanel class GameSettingTabs class GameSettingWarning class GamesetupPage class LoadingWindow class LobbyButton class MapCache class MapPreview class NetMessages class PlayerAssignmentItem PlayerAssignmentItem.AI = class PlayerAssignmentItem.Client = class PlayerAssignmentItem.Unassigned = class class PlayerAssignmentsControl class PlayerConfigButton class PlayerFrame class PlayerName class PlayerSettingControlManager class PlayerSettingControls class ReadyButton class ReadyControl class ResetCivsButton class ResetTeamsButton class SoundNotification class StartGameButton class StartGameControl class StatusMessageFormat class TimestampWrapper class TipsPanel class Tooltip class VictoryConditionCheckbox extends GameSettingControlCheckbox GameSettingControls.Biome = class extends GameSettingControlDropdown GameSettingControls.Ceasefire = class extends GameSettingControlDropdown GameSettingControls.Cheats = class extends GameSettingControlCheckbox GameSettingControls.ExploredMap = class extends GameSettingControlCheckbox GameSettingControls.GameSpeed = class extends GameSettingControlDropdown GameSettingControls.LastManStanding = class extends GameSettingControlCheckbox GameSettingControls.LockedTeams = class extends GameSettingControlCheckbox GameSettingControls.MapFilter = class extends GameSettingControlDropdown GameSettingControls.MapSelection = class extends GameSettingControlDropdown GameSettingControls.MapSize = class extends GameSettingControlDropdown GameSettingControls.MapType = class extends GameSettingControlDropdown GameSettingControls.Nomad = class extends GameSettingControlCheckbox GameSettingControls.PlayerCount = class extends GameSettingControlDropdown GameSettingControls.PopulationCap = class extends GameSettingControlDropdown GameSettingControls.Rating = class extends GameSettingControlCheckbox GameSettingControls.RegicideGarrison = class extends GameSettingControlCheckbox GameSettingControls.RelicCount = class extends GameSettingControlDropdown GameSettingControls.RelicDuration = class extends GameSettingControlDropdown GameSettingControls.RevealedMap = class extends GameSettingControlCheckbox GameSettingControls.Spies = class extends GameSettingControlCheckbox GameSettingControls.StartingResources = class extends GameSettingControlDropdown GameSettingControls.Treasures = class extends GameSettingControlCheckbox GameSettingControls.TriggerDifficulty = class extends GameSettingControlDropdown GameSettingControls.WonderDuration = class extends GameSettingControlDropdown PlayerSettingControls.PlayerAssignment = class extends GameSettingControlDropdown PlayerSettingControls.PlayerCiv = class extends GameSettingControlDropdown PlayerSettingControls.PlayerColor = class extends GameSettingControlDropdown PlayerSettingControls.PlayerTeam = class extends GameSettingControlDropdown
Chapter 11: Implementation Highlight 2: Deduplication using abstract classes
Deduplication is an important milestone reached in D322.
It was reached using the g_Dropdowns, g_PlayerDropdowns and g_Checkboxes approach.
The deduplication is transported to classes using using GameSettingControl, GameSettingControlCheckbox, GameSettingControlDropdown abstract base classes.
The only de-deduplication that is introduced in this patch is that these classes now again check whether a g_GameAttributes property should exist and be defaulted or whether it should be deleted,
whereas the previous code just always defined it.
Notice that the code is so different per gamesetting in almost all cases that the few reintroduced lines of code (setting defaulting logic) is arguably not qualifying as duplication.
Aside from that having been necessary to achieve more setting control freedom / practical expressivity.
Chapter 12: Implementation Highlight 3: Recursive updateGameAttributes approach
In the previous chapters it was explained how the gamesetup class rewrite leverages decoupling by using class semantics and an event-subscription system.
This chapter explains how decoupling and setting sanitization is furthered beyond these two factors.
- updateGameAttributes:
- In the previous code, each time g_GameAttributes was changed, updateGUIObjects was called. updateGUIObjects has checked that every setting value is defined (regardless whether that makes sense or not) and updated the dropdown, checkbox and label objects. It did that in a hardcoded (see "initOrder" property), so it did O(n) function calls where n is the number of settings.
-var g_Dropdowns = {
- "mapType": {
- "initOrder": 1
- },
- "mapFilter": {
- "initOrder": 2
- },
- "mapSelection": {
- "initOrder": 3
- },
- "mapSize": {
- "initOrder": 1000
- },
- "biome": {
- "initOrder": 1000
- },
- ...It means:
The mapfilter values depend on the maptype.
The mapselection values depend on the maptype and mapfilter.
The biomelist values depend on the selected map.
Some gamesettings are only available if a specific victory condition is enabled.
This ordering is required by an initOrder approach to correspond to that, whereas in the new code there is no init order to take care of.
This approach is incompatible with introducing mod-specific, map-specific (#4838) and other setting types that may depend on each others values while remaining necessary to be unaware of each other code-wise (Shotgun surgery), because the initOrder number of a new setting cannot be determined if there is no knowledge of all settings and their initOrder numbers.
Especially if some settings only exist on some selected maps, then an initOrder number may in theory even be valid for one set of selected matchsettings and invalid for another set of selected settings.
- The successor of updateGUIObjects is the GameSettingsControl.updateGameAttributes function. The hardcoding of the initOrder is removed by calling updateGameAttributes every time g_GameAttributes is changed, i.e. it becomes something like O(n²) function calls in the worst case, when every gamesetting changes its value in reaction to a changed value once. This should not be a performance issue, since typically only one setting value changes at a type except upon mapchange and page load. The work that is performed if settings are changed (creating and passing many dropdown list arrays for instance) by far outweighs the cost of empty function calls (function calls that end with an immediate early return). The gain of removing the hardcoded order is that gamesettings become more unaware of each other, which enables developers to implement more gamesettings without having to modify the existing code to account for the changes.
- launchGame approach: When starting the match, random civs, random map, random biome selection etc. were resolved by picking a random item. This was hardcoded in the launchGame function, so it required authors of new gamesetting types to modify the gamesetup code instead of just adding new code. This is removed by splitting that function into handlers and letting each gamesetting apply its changes itself when the game is launched (i.e. its only one new class to add when adding a new gamesetting).
Chapter 13: Transported problem 1: user-setting preservance and map determination of gamesettings
Problem: There is a gaping contradiction between the persisting userchosen settings feature and the feature that maps may overwrite the userchosen settings.
As a result people want two features that break each other, thus not being able to have either of the features, thus having broken code for years and many trac tickets.
The two concepts that contradict:
A) User settings persist - Match settings defacto persist between gamesetup page access, random map selections. So settings never change when players change the random map, therefore they also expect the settings to not change. Therefore if one of the maps suddenly changes a setting behind their back, noone notices, the game is started with the wrong settings, needs to be abandoned and the matchsetup has to start from the beginning.
B) User settings do not persist - Map authors wish to be able to set map settings (on skirmish and random maps).
Most multiplayer matches take place on random maps because they adapt to any user setting, allowing players to shape the match according to their wishes, rather than the map shaping the match the players will experience.
When players join the match, they start negotiating the gamesettings, the teams, the map, the victory condition, dozens of settings.
When players change any setting, including the random map, all their settings will be persisted.
Since there are many gamesettings (30+?) and since there is no notice which setting changed upon mapselection(see D1195), players most often do not notice when one of the settings is changed 'behind their back'.
This expressed itself in many tickets:
Pro map-determined settings:
- In #1834 it is proposed to random maps should be able to restrict the mapsize (i.e. change user setting instead of adapting to it)
- In a rP19449 comment it is proposed that the wall demo map should change the RevealMap (instead of revealing that from a triggerscript)
- In #3049 it is proposed that some settings should be fixed while others can be changed (e.g. allowing the player to assign himself to slots 1 and 3 on a 4 player map"
- In #3013 it is proposed that the user should not be able to change Player Placement ("You" and "AI" type should not be changeable)
Pro user-setting-persistence:
- In #2963: Preserve game settings after launching/canceling a game
- In #2982: The "preserve match settings" should also save player-assignments
- In #3120: persist civ when switching skirmish map
- In #4587 someone is surprised that the map modifies RevealedMap but not ExploredMap
- In #4661: persist civctory condition when switching skirmish map
- In #5372: persist settings when switching maptype to random
- The random map type has a "Random map" item, i.e. a random map is chosen when the game starts. This can ONLY happen if the random map adapts to the user settings. If the player choses a tiny map size but the randomly chosen map doesnt support tiny maps (#1834) then the game cannot start or the game is started with settings that the users didnt agree on (surprise match, contradicting the ready button feature).
- refs #3165 persist rating bug
- Refs #3209 each mapselection all victory conditions were reset to default (including random maps, while all other settings persisted there)
This is only an excerpt of the manifestations of the contradiction.
So as a matter of fact one can't have both features completely at the same time and one can't tell either of the parties that their feature will be removed because it's perceived already halfway implemented, just defective.
So it seems one can only mitigate the consequences of the contradiction, which is that players should be better notified if the map changes the value.
In D1195 a feature is proposed that informs players which gamesetting changed.
This would at least increase the likelihood that players notice that a map has changed a value 'behind their back'.
But even ideally informed players will still have the advantage that the settings that they negotiated before are lost and have to be renegotiated.
So this is only diluting the problem, not fixing it.
How the contradicting features are handled in this patch:
As of this rewrite, settings are only reset when changing a scenario map.
This means all settings are persisted when one changes the maptype from Skirmish to Random or vice versa, and also preserved when selecting a different Skirmish map (fixing mentioned tickets).
Secondly, if the map specifies an AI player, it will now be considered a fixed assignment.
This will be a nuisance to players who browse through maps and have their assignments peristed now more often, but completely reset when they browsed one of the maps.
As you can see the conceptual flaw is continued in this patch, it cannot be fixed in this patch, but the patch can help to reduce the impact of the problem by easing implementation of the notification which gamesettings changed in D1195.
Chapter 14: Transported Problem 2: Broken "Skirmish" maptype
As mentioned in Chapter 4, there are for example #3049 and #3013 mandating that maps can determine some settings.
This must mean Skirmish and Random maps, because for Scenario maps every setting is fixed.
It seems reasonable, because it allows for the use case that two players are AI players that are tailorsuited to this map (for example Jebel Barkal or Danubius).
So it is asked that a Skirmish map can specify which values are free and which values are fixed.
But then Scenario maps are nothing but a special case of a Skirmish map, i.e. a Skirmish map that fixes many settings.
In D1246 it is proposed that Scenario maps don't fix the LockedTeams setting and allow players to change it.
In D1704 it is proposed that Scenario maps don't fix the PopulationCap setting and allow players to change it.
If these wishes are all followed, then there is no difference between a Scenario map and a Skirmish map anymore.
Conclusion: Allowing maps to selectively fix gamesettings obsoletes the "Skirmish" maptype.
How it relates to this patch:
This patch cannot fix the "gamesetup rewrite" ticket that is asking for maps to fix some gamesettings completely.
This would involve more changes:
- The g_GameAttributes format used by the map JSON data, gamesetup (and later used by rmgen and simulation) would be changed to not only store values but also whether the values are hidden or enabled on this specific map). For example { "RevealMap": { "hidden": false, "enabled": true, "value": true } }.
- Atlas should be able to determine which settings are fixed.
- Skirmish maptype may be deleted.
Chapter 15: Fixed Problem: Per-Player PopulationCapacity, StartingResources
The PopulationCapacity and StartingResources can be both specified per-player as well as global, but only the global limits end up in the game.
- The Atlas editor allows specifying starting resources and population per player for Skirmish and Scenario maps.
- Per-player population field is overwritten by the gamesetup following rP12756, #812.
This had been reported at #4504, #4379, #5371, D1704, rP12756 and more places.
This patch changes the simulation code to consume the per-player value if specified, otherwise consume the global value.
In the future, the AIConfig dialog may become extended to allow players to specify per-player StartingResources and PopulationLimit (and picking a Hero for Regicide).
Inline Comments
refs #4463 Prevent the server from breaking client approval
- MapCache.js: Previously maps were cached too, but only of the selected maptype, whereas now maps of all maptypes are cached (because why not). The old problem that some maps specify gaia and others dont is not addressed here to avoid having to change many maps to become consistent.
- Split ReadyButton and StartButton
default.cfg:
- dropdown menus, the GameSettingsPanel and the main menu background are animated, so 30 FPS in the menu are visibly noticeably less favorable than 60 FPS
sprite:
- Gamesettings panel sprite Z hack workaround is still weird and ugly. It looks now differently since GameSetup.xml was restructured to be hierarchical, but the workaround was inserted with a size greater than the new parent (100% of the screen).
gamedescription.js:
- filter is a gamesetup GUI setting only, hence not shown in the gamedescription (since the purpose of that is to inform about the game that is to come)
StartGameControl.js
- Documented the cheat-prevention principle of gamesetup and a related TODO, see #4463
Chapter 16: Performance evaluation
Gamesetting changes consume 30-60ms (Update: 10ms) depending on what changed.
While there is some new overhead introduced with the recursion, most calls should perform little to no work, and in particular not more than they did before (enabled/hidden/dropdown list value changes).
There is a performance benefit with this iteration of the gamesetup rewrites in comparison to the previous code (D322), because many of the dropdown list items are not rebuilt anymore (for example player civ or mapsize selection) and the ones that are rebuilt conditionally are not rebuilt everytime anymore, but only when their dependent condition changed (for example biomes only on mapchange, not every setting change).
For scenario and skirmish map selection changes the performance seems to be slower (Update: are much faster) than before (still in the range of 50ms (Update: 10ms) per selected map , so still negligible in comparison to the gained bugfixes).
A low hanging fruit for performance improvements would be to make IGUIObject.cpp properties ignore changes if the value didn't actually change. For example setting hidden=false or enabled=false when it already is false.
Without patch there are consecutive updateGUIObject calls on init:
With the (edit: first verison of the) patch we have a better chance to see that the order of magnitude of the updateGameAttributes performance depends primarily on the setting values themselves that changed:
Update 2017-01-07:
With the performance update from 2017-01-07, the GUI objects are only updated on onGameAttributesBatchChange instead of the recursive onGameAttributesChange.
This means GUI objects are updated only once when a new gamesettings message arrives or when the controler changed a setting (including implicit consecutive setting changes).
This means the code in this revision proposal will make the code more performant in any case.
If we take the numbers 50-60ms previously and 10ms now, it's a factor of 5-6 more performance (numbers can vary by a factor of 2 or something depending on settings).
Chapter 17: Correctness evaluation considering planned Multi-controller gamesetup, refs #3806
To determine the correctness of the gamesetup class rewrite, one has to consider what greater features are/were to come.
The dedicated server feature #3556 requires remote clients to be able to setup the game, see #3806.
This was proposed to be implemented by implementing a way other than sending g_GameAttributes over the networked, in https://github.com/0ad/0ad/compare/master...Imarok:just_another_gamesetup_rewrite
In the scope of that ticket it was also considered to change g_GameAttributes to become an array of settings, not a nested object.
It seems this second way to change g_GameAttributes is not necessary following the gamesetup class rewrite anymore,
as every setting handler may determine safely whether to accept or reject a clients g_GameAttributes selectively,
without having to restructure g_GameAttributes,
without having to add a new network message to change an individual setting.
Notice that the D1195 feature equally needs to examine the setting changes and can now perfectly do so without this restructuring.
Implementing a new interface (new gamesetup function and a new network message type) to only change one GameAttribute without restructuring g_GameAttributes to remove its nesting would mean that
the NetServer still has to broadcast g_GameAttributes occasioanlly (when first joining a match for instance).
This again means that the NetServer needs to receive full g_GameAttributes from a trusted NetClient.
This seems to indicate that nothing is gained in terms of security (as a malicious client could still change more settings) and
nothing gained in terms of code complexity since there are now 2 ways to test the data for changes.
So it seems the gamesetup class rewrite may facilitate implementing those new features (multi-controller match setup, dedicated server, multiplayer savegames).
The work on that will show.
In the worst case, when some g_GameAttributes handling has to be restructured, it would probably still be easier to following the gamesetup class rewrite due to the performed decoupling.
Addendum:
315 adjectives for the opposite of entertaining from https://www.wordhippo.com/what-is/the-opposite-of/entertaining.html
boring uninteresting drab dreary dull flat heavy humdrum jading leaden monotonous pedestrian pleasureless ponderous stodgy stuffy tedious tiresome tiring wearisome weary wearying bad depressing disagreeable dramatic laborious repellent repulsive sad serious tragic unaffecting unamusing unenjoyable unexciting unfunny unhappy unpleasant unstimulating plodding wearing exhausting trying mundane fatiguing unimaginative commonplace ordinary mind-numbing routine uneventful mediocre ho-hum uninspired prosaic unvaried soulless mindless run-of-the-mill burdensome unvarying soul-destroying banal dry unremarkable grinding enervating sapping draining jejune slow stressful uncreative crushing repetitious tough arid lifeless strenuous vapid long-drawn-out everyday monochrome soporific uninspiring colorlessUS colourlessUK exacting dreich lacking variety average gruelingUS gruellingUK stale punishing taxing rigorous demanding challenging arduous irksome workaday repetitive back-breaking difficult onerous common vanilla quotidian day-to-day bland overlong lacklusterUS lacklustreUK deadly dull hard crippling matter-of-fact unoriginal derivative turgid annoying exasperating troublesome oppressive uphill normal stupid banausic stringent typical customary usual hackneyed dusty trite regular insipid OK draggy drudging prolix interminable so-so prosy mechanical long-winded deadly endless bog-standard half-pie as dry as dust pestilential samey dullsville hacky murderous bothersome hellish vexatious enervative exigent strict killing garden variety plain vanilla not so hot common or garden no great shakes not up to much nothing to write home about unpalatable unwelcome clean unsavouryUK displeasing unsavoryUS moral offensive horrible dead foul distasteful nasty unsatisfying hateful spiritless unentertaining ugly lazy unfriendly solemn decent horrid upright modest idle rude obnoxious gloomy lethargic depressed realistic unacceptable discordant sour mean bitter awful filthy tired characterless disgusting sterile unsociable nowhere languid ho hum lackadaisical languorous listless inactive languishing inanimate limp unsocial insociable prim nongregarious reclusive introverted antisocial unattractive unlikable loathsome despicable repugnant apathetic staid blah unreadable flavourlessUK chaste displeasng flavorlessUS homely unsuitable unlikeable disappointing unimpressive pretentious badly-written predictable lousy humourlessUK witless unintelligible rotten objectionable woeful discouraging upsetting relaxing unkind too much irritating abhorrent undesirable unrelieved heavy going humorlessUS icky severe uncool yucky troubling unfun a bit much harsh hostile grody worrisome agitated spiteful nondescript incompatible boresome straight poisonous tame bromidic operose well-worn weariful unalluring lame dragging gross worried upset unpleasing troubled nothing miserable melancholic dissatisfying wretched yukky dismal joyless deplorable detestable lacking excitement lacking interest