Page MenuHomeWildfire Games

Gamesetup Option Unification
ClosedPublic

Authored by elexis on Apr 12 2017, 4:31 PM.

Details

Summary

This diff tries to address two infamously annoying aspects of the gamesetup:
(1) Despite the 50+ cleanup commits in december 2015, a significant portion of it's code is still duplicated.
(2) If a developer wants to add a new gamesetup option, he has to edit both the XML document and the JS code in various places whose logic is puzzling to those who didn't get their handy dirty before.

As topping on the cake, the diff also implements:
(3) Singleplayer games with bots only
(4) Autocompleting of option titles and values (Victory Condition Regicide please!)

The diff isn't a jack of all trades device ("eierlegende Wollmilchsau") and does not fix the persisting of setting bugs (not only the loading of that one file but also occuring when changing random maps) and does not implement settings that are only valid on some maps. However this diff is a crucial prepration of implementing that and already very large to review.

(1) For an example of duplication, consider the initMapTypes, initMapFilters, initNumberOfPlayers, initGameSpeed, initPopulationCaps, initStartingResources, initCeasefire, initVictoryConditions, initVictoryDurations, initMapSizes functions who all do the same initialization of a dropdown GUI object.
But there is a typical duplication catch. Due to the selected / onSelectionChange order, some of these dropdowns set defaults, others don't.

(2) For an example of having to add ugly gamesetup diffs when adding a new gamesetup option, consider the capture the relic dropdown https://code.wildfiregames.com/D152#455a9afc or the disable spies checkbox from https://code.wildfiregames.com/D117#455a9afc
With this proposed diff, the XML doesn't have to be changed anymore at all and the JS code will look like this:

	"disableSpies": {
		"title": () => translate("Disable Spies"),
		"tooltip": () => translate("Disable spies during the game."),
		"default": () => false,
		"defined": () => g_GameAttributes.settings.DisableSpies !== undefined,
		"get": () => g_GameAttributes.settings.DisableSpies,
		"set": checked => {
			g_GameAttributes.settings.DisableSpies = checked;
		},
		"enabled": () => g_GameAttributes.mapType != "scenario",
	},
	"victoryDuration": {
		"title": () => translate("Victory Duration"),
		"tooltip": () => translate("Number of minutes until the player has won."),
		"labels": () => g_VictoryDurations.Title,
		"ids": () => g_VictoryDurations.Duration,
		"default": () => g_VictoryDurations.Default,
		"defined": () => g_GameAttributes.settings.VictoryDuration !== undefined,
		"get": () => g_GameAttributes.settings.VictoryDuration,
		"select": (idx) => {
			g_GameAttributes.settings.VictoryDuration = g_VictoryDurations.Duration[idx];
		},
		"hidden": () =>
			g_GameAttributes.settings.GameType != "wonder" &&
			g_GameAttributes.settings.GameType != "capture_the_relic",
		"enabled": () => g_GameAttributes.mapType != "scenario",
	},

Plus one line in the g_OptionOrderGUI stating where this object should appear.

To tackle the persist-match-settings issue, we should likely not save nor broadcast the g_GameAttributes array at all, only broadcast the chosen settings (and derive inherited settings like VictoryScripts on the clientside), so that map specific attributes like the disabledTemplates are not remembered in the first place. However it seems this rewrite would rather suite in a separate diff, so that reviewers don't have to look at (1) and (2) in the same diff.

Test Plan

There is really a lot to verify if one wants to do a complete test.
The entire codebase should be understood ideally.
Since some options depend on other options, one ought to test most combinations of settings.

Regressions with the persisting of settings should be tested. For example entering a non-lobbied MP game, disabling locked teams, then opening a lobby MP gamesetup and checking that teams are locked.

Test that all maptypes and the random map choice work as expected.
Test changing the number of players.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Apr 12 2017, 4:31 PM
wraitii added a subscriber: wraitii.

Gonna try to review this one since it has some relevance to the campaign patch.

wraitii edited edge metadata.Apr 15 2017, 9:58 AM

@elexis : Went through the code once, the whole rewrite seems good/interesting to me. Ideally we should try to make some of those functions more functional I think, it'd be easier to understand the side-effects across the board. But obviously gotta start somewhere.

That being said I have a few questions which I'd like you to answer before I get into actually testing this.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
398 ↗(On Diff #1221)

General comment here: the globals should in my opinion be called with the same convention, possibly [dropdownName]List, i.e. mapSelectionList, mapFilterList, …

This would make it a little easier to differentiate between the globals that are settings-related and the dropdown and the rest and might even help automatise this a little later on.

866 ↗(On Diff #1221)

Shouldn't this be called only once at the end of the game-setting rather? Or am I misunderstanding the purpose?

871 ↗(On Diff #1221)

Not clear to me while you set this when no dropdown or checkboxes seem to rely on it for side effects?

1775 ↗(On Diff #1221)

Bit of a poor name in my opinion, since all it does is send the game attributes across the network - unless I'm mistaken. You'd think this actually changes g_GameAttributes.

elexis marked an inline comment as done.Apr 15 2017, 5:37 PM
In D322#12534, @wraitii wrote:

It has some relevance to the campaign patch

Which?

try to make some of those functions more functional I think

Which and how?

binaries/data/mods/public/gui/gamesetup/gamesetup.js
398 ↗(On Diff #1221)

Rename ok

866 ↗(On Diff #1221)

When looking for it's calls you can find the purposes:

  • To have settings when having the (default) "random" map choice selected. For example having the 300 population item selected instead of having nothing selected. The current svn code already selected defaults accross the board, just in those init functions.
  • When selecting a map (since the map might not define all attributes)
  • When changing the number of players (so those new player slots have g_GameAttributes.settings.PlayerData entries)
871 ↗(On Diff #1221)

It is used to prevent recursion. Remove the early return and you will see the errors that occur when doing so.
The proposed flow is 1. install proper settings 2. install GUI objects in contrast to setting up one GUI object that relies on settings which are not initialized yet.

1775 ↗(On Diff #1221)

Also calls updateGUIObjects (directly in SP and indirectly in MP).
It does change g_GameAttributes on MP side.
Name suggestions? BroadcastGameAttributes would hide the fact that it updates the GUI objects in SP.

wraitii added a comment.EditedApr 15 2017, 6:51 PM

Regarding the changes to pure functions:

  • SupplementDefaults as I said inline
  • LaunchGame should be split in my opinion, and it's not very functional (for example why does Engine.StartNetworkedGame not take the g_GameAttributes as parameter? That's odd). But that's probably for another patch.

The others don't seem to have nearly that many side-effects overall (well UpdateGameAttributes, see inline).

In general I think it'd be good if functions that read from, but do not write-to GameAttributes took it as a read-only parameter for clarity, but that might be a little impractical.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
866 ↗(On Diff #1221)

Well here is what I'm wondering. Instead of calling this when a setting changes, which is prone to breaking (we can forget to call it…), we could call this when we need to access the game attributes. But make it purely functional, i.e. you'd call "supplementDefaults(g_GameAttributes)" and that would return a copy of that array with defaults swapped in. So g_GameAttributes only contains user-changes, and we don't have to worry about forgetting to supplement or recursion.

1775 ↗(On Diff #1221)

How does it change g_GameAttributes? At least not the local copy, does it?

My opinion is that this should really be BroadcastGameAttributes, and you should call updateGUIObjects separately, or something.

elexis marked 8 inline comments as done.Apr 21 2017, 4:51 PM

Fixed the mapfilter bug reported by you in 0cb72a9e since I used find instead of findIndex.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
398 ↗(On Diff #1221)
866 ↗(On Diff #1221)

Just calling it everytime something changes would make it simpler, yes.
Done that in edb7eeb3

Not only makes it simpler but also removes the fact that we need to read from g_GameAttributes, therefore it has become possible to remove that recursion prevention global in 71cc4f.
Nice one!

Passing the gameattributes as a variable seems ugly though and wouldn't have fixed the bug.

1775 ↗(On Diff #1221)

The Engine.SetNetworkGameAttributes(g_GameAttributes); call updates the game attributes for all (other) clients.
Calling updateGUIObjects would be bad IMO as we would have 2 calls in every call to this function. Don't really see the problem. Can update the JSdoc if that helps, but really, if someone wants to do anything in the gamesetup, they have to read the code.

elexis updated this revision to Diff 1404.Apr 21 2017, 4:57 PM
elexis marked 3 inline comments as done.
elexis edited the summary of this revision. (Show Details)
elexis edited the test plan for this revision. (Show Details)

Call supplementDefaults each time after selecting a dropdown value or toggling a checkbox as proposed by wraitii,
this allowed removal of the recursion prevention global too.
Fix mapfilter not working for non-random maps as reported by wraitii.
Fix a persist-matchsettings-bug :D
See git commits in the comment above for a more reviewable diff of the diff

Vulcan added a subscriber: Vulcan.Apr 21 2017, 6:56 PM

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/842/ for more details.

wraitii added inline comments.Apr 27 2017, 12:30 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
866 ↗(On Diff #1221)

Mh, this isn't exactly what I suggested but it does make the code simpler.

I'll try and do a proper review this WE with these changes, had missed that you updated it (I get too many notifications :p )

Issues:
-Revealed Map should probably only be available if Explored map is available too.

I had a weird trouble with the "Teams Locked" option but that might be a persistent settings issue.

Otherwise the settings all seem to work correctly, haven't encountered any bugs, and the code overall looks much better to me. Bot-only games work and put the player as observer, which is probably the right thing.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
18 ↗(On Diff #1404)

Isn't the Fill call redundant?

358 ↗(On Diff #1404)

this whitespace seems useless

1100 ↗(On Diff #1404)

Would be nice to sanitise the input here I guess, but that's probably for later.

elexis marked 3 inline comments as done.Apr 30 2017, 3:48 AM
In D322#16258, @wraitii wrote:

-Revealed Map should probably only be available if Explored map is available too.

15:17 < elexis> wraitii: you mean setting RevealMap to disabled if not having checked Explore Map?
15:17 < wraitii> yes
15:17 < wraitii> and reversing their order I suppose
15:17 < wraitii> strictly speaking I realize that one does not imply the other but the player should not know that
15:19 < elexis> wraitii: yes
15:46 < elexis> wraitii: perhaps it were better the other way around? like having both checkboxes enabled but if the reveal one is checked, the explore one will be checked and disabled?
15:48 < wraitii> mh
15:48 < wraitii> elexis: yes
15:48 < wraitii> sounds good

Implemented in ce8e6f0aac

I had a weird trouble with the "Teams Locked" option but that might be a persistent settings issue.

Which one?

14:45 < wraitii> elexis: testing the gamesetup. Can you remind me of the current state of the "persistence of settings" bugs?
14:45 < elexis> broken badly
14:46 < wraitii> same fthing for setting a ceasefire on an RM and then choosing a scenario right?
14:53 < elexis> wraitii: apparently so
14:53 < elexis> (reproduced without rewrite)
binaries/data/mods/public/gui/gamesetup/gamesetup.js
18 ↗(On Diff #1404)

Array() itself doesn't update the length of the array or something (I tried avoiding it when i was committed to svn one or two years ago)

1100 ↗(On Diff #1404)

Had already started working on it, but reverted everything (It would mean weeks more of work and the biggest problem currently are constant merge commits in which I have to rewrite the according gamesetup change from scratch too often, the current state seems to be functionally equivalent or improved. Also would allow other people to start working on gamesetup patches that are waiting for this merge.)

The rating check should actually be moved to the globals array.
We should probably not store g_GameAttributes but derive it from the new global array, so that we 'avoid dangling settings' and verify them logically.

elexis updated this revision to Diff 1548.Apr 30 2017, 4:08 AM
elexis marked 2 inline comments as done.

Disable Explore Map option if Reveal Map option is checked and reverse the order of these two settings as proposed by wraitii

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/933/ for more details.

IMO you should also enable "revealed map", on top of disabling it.

Managed to reproduce the "Teams locked" bug: delete your persistent settings, load a skirmish map, enable teams locked, switch map, you should be unable to disable it (click does nothing apparently).

With these changes I'd accept it, fine if you want separate testing too though.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
18 ↗(On Diff #1404)

https://developer.mozilla.org/fr/docs/Web/JavaScript/Reference/Objets_globaux/Array disagrees. Maybe you had trouble because it wasn't seen as a number or we have something weird?

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
363 ↗(On Diff #1548)

Also apparently unnecessary.

elexis marked 3 inline comments as done.Apr 30 2017, 4:33 PM
In D322#16380, @wraitii wrote:

IMO you should also enable "revealed map", on top of disabling it.

(I guess you mean checking or unchecking the Explore Map option if Reveal Map is checked).
I expected this actually, but I was reluctant to implement it due to side-effects:

  • Not setting other flags means the player choice is persistet, might make things easier to the player (but that's not really an argument since the buttons are right above each other)
  • Explore map is buggy #3202 (don't know if that bug appears if we reveal the map too)
  • Don't know if it's a performance difference since we will create all mirages even if the map is revealed. The map revelation can be disabled ingame later again with the dev overlay or some other sim command.

But you're right, it seems more intuitive if we set the checkbox and there is a ticket for that bug.

Managed to reproduce the "Teams locked" bug: delete your persistent settings, load a skirmish map, enable teams locked, switch map, you should be unable to disable it (click does nothing apparently).

Ah, thanks! That was a brainfart, missing an if statement in the rating setter.

Addressed all three remarks in https://github.com/0ad/0ad/commit/5b74404aecd01768f3a787a556c9b4c23ca7f6c2

binaries/data/mods/public/gui/gamesetup/gamesetup.js
18 ↗(On Diff #1404)

Try this:

warn(uneval(Array(g_MaxPlayers).map((v, i) => i + 1)));
warn(uneval(Array(g_MaxPlayers).fill(0).map((v, i) => i + 1)));

Results:

WARNING: [, , , , , , , ,]
WARNING: [1, 2, 3, 4, 5, 6, 7, 8]

I guess it's:

(Note: this implies an array of arrayLength empty slots, not slots with actual undefined values).

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array#Parameters

binaries/data/mods/public/gui/gamesetup/gamesetup.xml
363 ↗(On Diff #1548)

Changed the comments to make the whitespace choice more obvious

elexis updated this revision to Diff 1555.Apr 30 2017, 4:35 PM
elexis marked 2 inline comments as done.

Check explore map if reveal map is checked.
Fix enableRating brainfart.
Change order of two XML comments.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/940/ for more details.

wraitii accepted this revision.Apr 30 2017, 6:58 PM

In my testing the patch now works correctly and it would be nice to real-life test this for a few days, so I'd suggest committing this sooner rather than later (though feel free to ask for a second opinion).

This revision is now accepted and ready to land.Apr 30 2017, 6:58 PM
elexis added inline comments.May 1 2017, 7:56 AM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
763 ↗(On Diff #1555)

Pointless to have these 2 constants in JS, moving

elexis marked 4 inline comments as done.May 2 2017, 10:56 PM

Thanks for the proofreading, tests, bugreports and improvement proposals wraitii and the translation fails found by leper in the github branch!

Some final remarks:
Doesn't conflict with D364.
Very simplie merge conflict with D194 and that code will become nicer too with this one in (since it doesn't launch the tutorial from the second onTick anymore).

(Looks like the first iteration of this was uploaded 12 months ago to #3994 and received 80 noisy commits since -_-)

Tested the 23 svn commits to gamesetup.js from rP18692 (september 2016) onwards that often had to be rewritten to fit into this patch.
Also tested about every changed feature again, hope we have found everything :-)

binaries/data/mods/public/gui/gamesetup/gamesetup.js
16 ↗(On Diff #1555)

The color had to be moved here because it is used few lines below.

const -> var because we're already touching this line, mods should be able to change it and since most of the other things that are unlikely to change are already var for that matter. Constants should be avoided in general

170 ↗(On Diff #1555)

Might just as well move the colors to the other color too that had to be moved and while at it, change it to var for mods.

318 ↗(On Diff #1555)

While g_Dropdowns is certain to be extended very soon, this trailing comma in g_OptionOrderGUI seems truly unneeded

348 ↗(On Diff #1555)

The remaining properties should have a comment too for completeness

763 ↗(On Diff #1555)

Nope, those two were actually needed since all GUI objects in the "more options" window become hidden in updateGUIObjects unless there is a rule showing them and they are distributed vertically, so we can't just add a new GUI object containing only the options without adding annoying more hardcoded code. So adding a comment instead.

Also be possible to change the dropdown handler function, but not going for this now.

1240 ↗(On Diff #1555)

missing semicolon

1480 ↗(On Diff #1555)

Since the "dropdownArrays" have been renamed to "playerDropdowns" (same for checkboxArrays) (because we only use gui object arrays for players currently), this one could also be renamed to "isValidPlayer" for consistency potentially, but it also returns true if idx is undefined (i.e. not a player-repeated gui object but only a conventional one like victory condition), so actually seems good as is.

1832 ↗(On Diff #1555)

if we want to be super fancy: prepareForDropdown([...playerChoices, ...aiChoices, unassignedSlot])
missing semicolon

This revision was automatically updated to reflect the committed changes.
elexis marked an inline comment as done.