#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 )
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 frominitGUIObjects
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 changingmapType
ormapFilter
. - 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)
Change History (14)
comment:1 by , 8 years ago
by , 8 years ago
Attachment: | t3994_gamesetup_GUI_rewrite_v1.patch added |
---|
comment:3 by , 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 , 8 years ago
Attachment: | t3994_gamesetup_GUI_rewrite_v3.patch added |
---|
comment:4 by , 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 , 8 years ago
Note to self: Sometime this alpha there was a bug introduced: If the map changes AI players become unassigned.
comment:7 by , 8 years ago
Owner: | removed |
---|
comment:8 by , 7 years ago
Milestone: | Alpha 22 → Work 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:10 by , 7 years ago
Description: | modified (diff) |
---|---|
Milestone: | Work In Progress → Alpha 22 |
Priority: | Should Have → Must Have |
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.