Opened 8 years ago

Closed 7 years ago

Last modified 4 years ago

#3994 closed task (fixed)

[PATCH] Gamesetup GUI handling rewrite

Reported by: elexis Owned by: elexis
Priority: Must Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Imarok Patch:

Description (last modified by elexis)

Aim of the ticket: In order to reduce code complexity (making things independent from each other) and to get maintainable code, it is required to

(1) unify duplicated functions in the gamesetup that initialize and update GUI objects and (2) separate the GUI methods (initializing GUI objects with userchoices, deciding when GUI obejcts are hidden/enabled/checked/selected) from the business logic (when and how settings can be changed).

Due to most gameattributes having their own peculiarities, code had been copy & pasted vastly, was changed slightly and then copied and changed again. This feedback loop resulted in several layers (orders of magnitude) of doing it wrong.

Why it is needed: Every addition to the gamesetup has to go through pain of reading hundreds of lines of unrelated code and in worst-case trial-n-error to come up with a patch. This is true regardless of the size of change. Recent examples include #3234 and ticket:2676#comment:4. #3049 and #3806 also directly require these changes.

Some peculiarities:

  • The initFoo functions called from initGUIObjects are nearly identical, but some set defaults, others don't.
  • the selectable items of all dropdowns are fixed, except the mapSelection dropdown which is reinitialized after changing mapType or mapFilter.
  • the rating setting is shown only for lobbied games
  • cheats can only be set in multiplayer
  • teams locked is only enabled if the rating setting is unchecked
  • some options (ceasefire, population capacity) can only be set on some "random" and "skirmish" maps. Others can only be set on "random" maps only (number of players, mapsize). Others can be set on all maps (gamespeed).
  • sometimes settings which can't be configured on the current maptype are shown but read-only (mapsize shows "Default" for non-random maps) while others are completely hidden (cheats)
  • the victory condition also updates the scripts variable
  • some GUI controls are set only once (initMultiplayerSettings), while others are updated regularly
  • changing the mapfilter, maptype or mapselections means nuking prior settings, loading the mapchoice and setting defaults otherwise
  • Don't get me started on the persistmatchsettings... #3883

Attachments (3)

t3994_gamesetup_GUI_rewrite_v1.patch (44.7 KB ) - added by elexis 8 years ago.
t3994_gamesetup_GUI_rewrite_v2.patch (41.4 KB ) - added by elexis 8 years ago.
Rebased.
t3994_gamesetup_GUI_rewrite_v3.patch (54.7 KB ) - added by elexis 8 years ago.
Committed some parts of the previous patch in r18315, r18299, r18298, r18297 and r18296. Rebased the patch after like 10 hunks with hundreds loc failed. g_DropdownArrays is half implemented half broken.

Download all attachments as: .zip

Change History (14)

comment:1 by elexis, 8 years ago

Contains style optimizations to be committed separately (setMapDescription). Contains a patch to remove an unneeded annoying breakpoint (not required for the patch, still). Delays the generation of the matchID and seeds to remove duplication and hiding the value until the game started. Deletes the mapsize "Default" placeholder for scenario/skirmish maps. Shows all instead of no maps if the mapFilter is broken. Displays "Unknown" if a setting (f.e. map) wasn't found.

comment:2 by elexis, 8 years ago

In 18227:

Gamesetup cleanup (game/map-description), refs #3994.

Add linebreaks.
Eliminate unneeded variables mapName and gameDescription.
Remove a check as g_GameAttributes.map is always defined.
Sort the two one-line statements above the helper variable declarations to make it more more obvious that the statements don't use them.

by elexis, 8 years ago

Rebased.

comment:3 by elexis, 8 years ago

Keywords: review removed

Broke the mapfilter when I overwrote the v1 patch. Also working on implementing the TODO about g_DropdownArrays for abstracting the player-assignments logic.

by elexis, 8 years ago

Committed some parts of the previous patch in r18315, r18299, r18298, r18297 and r18296. Rebased the patch after like 10 hunks with hundreds loc failed. g_DropdownArrays is half implemented half broken.

comment:4 by elexis, 8 years ago

Also discussed a different approach with Imarok and leper. Instead of using g_Checkboxes, g_Dropdowns and g_DropdownArrays one could use g_GameAttributesLogic and have that nested. This has some advantages (potentially removing a bit duplication) but also downsides (complexity parsing a nested object with potentially many edge cases, probably not really being needed).

I'm in favor of first merging the logic of the patch above once it's finished and then potentially switching to the nested object later if it becomes apparent that it'd be really useful.

The focus should lie on deleting the duplication and merging the similar yet slightly different logic first. Switching to a nested structure will be easier then too, though it would change many lines again.

comment:5 by elexis, 8 years ago

Note to self: Sometime this alpha there was a bug introduced: If the map changes AI players become unassigned.

comment:6 by elexis, 8 years ago

Milestone: Alpha 21Alpha 22

Too few time remaining for this release.

comment:7 by elexis, 7 years ago

Owner: elexis removed

comment:8 by elexis, 7 years ago

Milestone: Alpha 22Work In Progress

Moving to the Work In Progress milestone, since there is a patch asking for feedback, but since it is not strictly bound to a specific release.

comment:9 by elexis, 7 years ago

Owner: set to elexis
Resolution: fixed
Status: newclosed

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:10 by elexis, 7 years ago

Description: modified (diff)
Milestone: Work In ProgressAlpha 22
Priority: Should HaveMust Have

comment:11 by elexis, 4 years ago

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:

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:

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

Note: See TracTickets for help on using tickets.