Page MenuHomeWildfire Games

Allows trigger script with different difficulty levels
ClosedPublic

Authored by mimo on Jan 2 2018, 4:56 PM.

Details

Summary

For scenarios to be enjoyed by most players, the trigger scripts must have several difficulty levels settable through gamesetup. This is allowed by this patch.
The changes in Arcadia map are only an example of what should be provided by maps supporting such different levels.

Test Plan

Check that the patch works as expected.

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

mimo created this revision.Jan 2 2018, 4:56 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jan 2 2018, 4:56 PM
elexis added a subscriber: elexis.Jan 2 2018, 5:43 PM

Antithesis:
Map specific options would be more versatile. #4838
This way we could have more than one setting influencing the trigger script and more than the given choices.

For instance on extinct volcano one could set the time when the water rises.
On survival of the fittest, one could set the time between waves and the strength of the waves or the attacking civ.
On danubius the number of ships, the champion ratio increase time or whether there are gaia civic centers.
On the Unknown, one could chose which of the 10+ types will be generated.

Thesis:
Difficulties have a generic use case that is found in at least two maps already committed in svn (danubius, survival).
Even if all maps would use different types of difficulty settings, it would in essence still be a difficulty setting and thus contain some level logical duplication.
The proposed patch is the correct approach to not introduce partial duplication and minimize the code.

Synthesis: I guess it's ok. If it's ok, then it's good even.

Like the biome setter, this is inevitably affected by the reset-upon-mapselection phenomenon while all other gamesettings are persisted when switching the map.
But these two settings are at least reset upon each map selection, not by chance.
(You wouldn't want the reveal map flag to be set while browsing through map previews with the more-options dialog closed, would you? I'm asking for a friend)

binaries/data/mods/public/gui/gamesetup/gamesetup.js
657 ↗(On Diff #5036)

Just "Difficulty" to make it as simple as possible to the player?

659 ↗(On Diff #5036)

(Scenario is a reserved word, but it's ok to use it)

671 ↗(On Diff #5036)

(IIRC the more dialog was close to the 1024x768 limit. Then however bb has a patch again D1027)

1527 ↗(On Diff #5036)

Yep, that's definitely a thing, other gamesettings ought to be here too and then it would have to be moved to the g_DropDown arrays again to get rid of the hardcoding again sometime.

1575 ↗(On Diff #5036)

(the warning isn't worth the lines of code IMO. If we don't use the boolean SupportsDifficulties and someone provides a broken map, then the map creator should pay the cost for that, not the reader of this code. Providing a broken setting would be visible when opening the gamesetup then already. In the long run when we want to support broken maps without gamesetup errors we need to remove the lists from the map selection or show them as incompatible before processing, not while processing, IPO model)

1577 ↗(On Diff #5036)

= g_Settings.TriggerDifficulties.something.Default + warning becomes unneeded

1797 ↗(On Diff #5036)

(...this messy gameattributes code... just needs some more rewrites)

binaries/data/mods/public/maps/scenarios/Arcadia.xml
71 ↗(On Diff #5036)

Good reason for the restrictions or can that be simplified to a boolean SupportsDifficulties?

Shouldn't add an example for a map that doesn't have a trigger script. We can leave out the example and someone, possibly future me can take a look at danubius and survival.

The wiki trigger page(s) need to be updated for mods anyhow

binaries/data/mods/public/simulation/components/Trigger.js
38 ↗(On Diff #5036)

Would have to investigage, but wasn't there a policy or good reason to add all serialized properties to init? (this.difficulty = undefined;?)

binaries/data/mods/public/simulation/data/settings/trigger_difficulties.json
2 ↗(On Diff #5036)

needs a messages.json entry. (Perhaps the translatedkeys could become uniform eventually)

13 ↗(On Diff #5036)

Default = true

20 ↗(On Diff #5036)

More granularity, 5 levels?

binaries/data/mods/public/simulation/helpers/Setup.js
50 ↗(On Diff #5036)

can be inlined

Vulcan added a subscriber: Vulcan.Jan 2 2018, 6:27 PM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Vulcan added a comment.Jan 2 2018, 6:29 PM
Executing section Default...
Executing section Source...
Executing section JS...
mimo added inline comments.Jan 2 2018, 6:36 PM
binaries/data/mods/public/gui/gamesetup/gamesetup.js
1575 ↗(On Diff #5036)

I don't mind. I will remove the warning.

1577 ↗(On Diff #5036)

The idea is that not all scripts need (or can differentiate) all levels, so each map has only to support a sub-sample of all difficulties, and the default is recomputed dynamically.
For example, a map with only 2 levels would better call them "Easy" and "Hard" (rather than "Easy" and "Medium"), and then would not have any Default if we put globally Medium as Default.

binaries/data/mods/public/maps/scenarios/Arcadia.xml
71 ↗(On Diff #5036)

Yep, the fact that it is a subsample is wanted (see above), although here it may have been better as "Easy" and "Hard".
Of course, this is not supposed to be commited with the patch :)

binaries/data/mods/public/simulation/components/Trigger.js
38 ↗(On Diff #5036)

I was not sure, but i'll add it.

binaries/data/mods/public/simulation/data/settings/trigger_difficulties.json
2 ↗(On Diff #5036)

ok, i was not aware of it. I'll check.

13 ↗(On Diff #5036)

No as not all levels are not necessarily supported, the default is recomputed dynamically by taling the most median difficulty.

20 ↗(On Diff #5036)

Not sure it would be useful, but as script writers don't have to implement all levels and just give the supported ones, that's fine. I will add "Very Easy" and "Very Hard"

binaries/data/mods/public/simulation/helpers/Setup.js
50 ↗(On Diff #5036)

ok

mimo updated this revision to Diff 5046.Jan 2 2018, 6:59 PM
mimo edited the summary of this revision. (Show Details)

updated patch

elexis added a comment.Jan 2 2018, 7:35 PM

Remaining catch: Whenever adding a new gamesetup option, all gamesetup mechanisms would need to become aware of the new setting. Read: autostarted games and atlas.
Alltime favorite workaround: adding a fallback default (its "alpine" for randombiome.js. I guess that allows generating maps with alpine that nominally don't support it).
As far as I see this patch would result in GetDifficulty returning undefined when starting from atlas or command line.
I guess uhm, GetDifficulty could return 3 if it's undefined.
The correct solution would be to teach atlas (and Gamesetup.cpp) to parse the XML difficulties array and add a dropdown (arbitrary default).
The better solution would be unifying all 3 gamesetup mechanisms.

Otherwise this looks good to me. I'm all for it given the static code analysis. Can test if you want, but I expect that any possible bugs would be simple handcraft mistakes.

(wiki page reminder. Triggers should focus on using the getter. Manual_SettingUpAGame is outdated af. I'm missing some wiki page about map recommendations. You can search things on google using "keyword site:trac.wildfiregames.com/wiki/" and we have an index at TitleIndex)

binaries/data/mods/public/gui/gamesetup/gamesetup.js
1525 ↗(On Diff #5046)

.

1575 ↗(On Diff #5036)

(the early return was included in the argument, but seems ok as is)

1577 ↗(On Diff #5036)

triggerDifficultyList always true at this point

binaries/data/mods/public/simulation/components/Trigger.js
345 ↗(On Diff #5046)

(maybe the getter first)

38 ↗(On Diff #5036)

We ought to be confident about this. SimulationArchitecture doesn't state anything. Couldn't find anything in ComponentManager.cpp ComponentManagerSerialization.cpp and BinarySerializer.cpp quickly. Could do a quick test by not having the thing in init, playing a game and doing a textual oos dump from command line or a rejoin test from commandline or a MP match with a second client rejoining or cross fingers.

binaries/data/mods/public/simulation/data/settings/trigger_difficulties.json
31 ↗(On Diff #5046)

(eol property thing)

Vulcan added a comment.Jan 2 2018, 8:32 PM
Executing section Default...
Executing section Source...
Executing section JS...
mimo updated this revision to Diff 5054.Jan 2 2018, 9:38 PM

update followings latest elexis's comments (thanks for the fast feedback)

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Executing section Default...
Executing section Source...
Executing section JS...
Vulcan added a comment.Jan 3 2018, 1:39 AM

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
elexis accepted this revision.Jan 3 2018, 2:19 AM

see inline comment, this must be fixed

As you already noticed, the map also has the possibility to only specify true for the supported biomes instead of an array.
That might not hurt to add here too, so that one can avoid adding all difficulty names. But not crucial.
It's not strictly needed here as the list of difficulties is not currently intended to be exchangeable and extendible as the list of biomes.

Also, we should add a gamedescription.js entry to display the difficulty in running games / replays. Can be done afterwards.

Not the recommended coverage, but I did a quick test adding two options to survival and a warn() showing the difficulty and that worked.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
361 ↗(On Diff #5054)

Why isn't it in the more options dialog? There is too few space for another option in this part, so the last dropdown is rendered on top of the "More Options" button.

1566 ↗(On Diff #5054)

(Notice that we pass the supported difficulties and biomes to the simulation despite it only being used by the gamesetup. Don't see an easy way to fix without either significant structural or messy changes and it doesn't hurt anything.)

binaries/data/mods/public/simulation/components/Trigger.js
33 ↗(On Diff #5054)

Should probably state that this is a fallback for autostart games and atlas, as the reader might otherwise assume that it's used by some gamesetup route.

38 ↗(On Diff #5036)

I'm going with the latter and a hint that it's the same principle we use in the other simulation components too.

binaries/data/mods/public/simulation/data/settings/trigger_difficulties.json
31 ↗(On Diff #5046)

strings good.

Very hard danubius is something I see being played. (There is one guy in the lobby playing 50 pop low res regicide danubius each game)

This revision is now accepted and ready to land.Jan 3 2018, 2:19 AM
mimo added a comment.Jan 3 2018, 11:41 AM

After some more thinking, i will go with
"SupportedTriggerDifficulties": { "Values": ["Easy", "Hard"], "Default": "Easy" }
so that each map can define its own default, and this default value will be used for atlas or autostart.

When all difficulties are supported, this can simplified by
"SupportedTriggerDifficulties": { "Values": true, "Default": "Easy" }
by analogy with biome, but it may be clearer to replace true by "all" in the future for both cases.

binaries/data/mods/public/gui/gamesetup/gamesetup.js
361 ↗(On Diff #5054)

ok. I think it's an important parameter which should be directly visible, but i'm convinced by your screenshot. Hopefully that will be solved soon by the rearrangement of that panel.

This revision was automatically updated to reflect the committed changes.