Consistent starting resources and customizable population cap for scenarios
AbandonedPublic
Actions

Authored by Wundersam on Dec 25 2018, 5:39 PM.

Details

Reviewers

Trac Tickets

Summary

Changes will allow the population cap to be set when a scenario type map is selected. Before, in order to set the population cap, one would have to change to a non-scenario map, set the population cap and change back.
Will also hide starting resources from the game description of scenarios because they'd be ignore in InitGame.js:67

if (settings.mapType !== "scenario" && settings.StartingResources)
{
	let resourceCounts = cmpPlayer.GetResourceCounts();
	let newResourceCounts = {};
	for (let resouces in resourceCounts)
		newResourceCounts[resouces] = settings.StartingResources;
	cmpPlayer.SetResourceCounts(newResourceCounts);
}

I also added myself to the list of programmers as this is my first patch.

Test Plan

Manual testing was performed in the relevant game screen.
This revealed unexpected behaviour unrelated to the changes where game settings are reset when clicking on "random" in the map type dropdown; This will be adressed in another changeset.

Diff Detail

Repository

rP 0 A.D. Public Repository

Branch

/ps/trunk

Lint

Lint OK

Unit

No Unit Test Coverage

Build Status

Buildable 6517
Build 10779: arc lint + arc unit

Event Timeline

Wundersam created this revision.Dec 25 2018, 5:39 PM

Owners added a subscriber: Restricted Owners Package.Dec 25 2018, 5:39 PM

Harbormaster completed remote builds in B6517: Diff 7064.Dec 25 2018, 5:39 PM

Wundersam edited the summary of this revision. (Show Details)Dec 25 2018, 5:41 PM

As far as I know, scenario maps are supposed to be fixed in settings, skirmish maps changeable.

Well, they weren't fixed, it just wasn't possible to change them when scenario was selected but the last used values where used which made arbitrary settings possible.
I chose to allow setting population cap because that wasn't further prevented in the code and the default 300 population cap might be a performance barrier for some people with weaker hardware.

Goes against the design. The bug where settings persisted should be fixed though.
Requesting changes. Feel free to remove the status though.

This revision now requires changes to proceed.Dec 26 2018, 12:08 PM

Well, they weren't fixed, it just wasn't possible to change them when scenario was selected

= settings fixed for scenario, as intended as far as I can see from the code history

but the last used values where used which made arbitrary settings possible.

That's the classic persist-match-settings bug, that equally occurs when selecting a different map where the previous map had specified a default but the new map didn't.
This is a systematic error and needs a redesign of the implementation, which will become more feasible after having cleaned the code further, I have planned a branch for #5322 for this file.

binaries/data/mods/public/gui/common/gamedescription.js
404	There are starting resources for scenario maps, so the sentence is a bit wrong. Ultimately there should not be two ways to specify starting resources, or otherwise the two ways to define resources (resource per player vs. all resources of all players at a time) #4504.

I would also like to apologise for the misunderstanding earlier. It wasn’t meant to say “your patch sucks”. It was meant to point you at the underlying bug without conflicting with the game design.

elexis mentioned this in D2483: Gamesetup class rewrite.Dec 18 2019, 5:49 PM

This revision now requires review to proceed.Dec 18 2019, 5:49 PM

See inline comments. Thanks for the food for thought.

binaries/data/mods/public/gui/common/gamedescription.js
404	It should not become consistent with the simulation code since the simulation code is broken. Currently it copies the per-player values to the g_GameAttributes value but then ignores it for the skirmish map. But Atlas still provides a way to specify the per-player values, so map authors will think it has use for skirmish maps. And there is a considerable use case - why not have per-player population limits and starting resoures on skirmish (and random) maps. D2483 changes both places to consume the map-specified per-player limit and the global limit for players that don't have a specific value.
binaries/data/mods/public/gui/gamesetup/gamesetup.js
606	(This change is against the current scenario design (current scneario design = everything fixed, skirmish = most settings free)) There are many tickets asking for random and skirmish maps to become more restricted. This patch goes the other way and proposes to make scenario maps more unrestricted. Consequentially there is no more difference between Skirmish and Scenario maps and the two maptypes may be merged (wrote something about it in D2483), but not so soon.