[PATCH] Remove hardcoded resources

[Imarok]

Mostly everywhere in the code the resources are hardcoded. Using one constant for the resources everywhere, would increase the moddability of the game.

Branches: ​https://github.com/s0600204/0ad/tree/resource_agnostic ​https://github.com/elexis1/0ad/tree/3934-resource-agnostic

[s0600204]

Potential patch. It's big.

[s0600204]

Instead of using a single constant, resources are defined in separate .json files in simulation/data/resources.

Still to do:

• Check all vars are lets where applicable
• Test with Petra AI

[sanderd17]

For the schemas (Cost, Loot, ...), it would be better to generate a stricter schema.

Now, people can make spelling mistakes in the templates that won't be noticed, this should be avoided when possible. As you already have access to the global scripts.

It should be possible to generate the schema with the resources that exist in the mod. This would be better than the warns you added to f.e. the productionQueue.

[s0600204]

Stricter schema and sorting out some L10n issues.

[sanderd17]

Small review, didn't look at everything yet:

• functions_repeat_positioning.js: a for loop like L49 is ugly, butter use a while(true) IMO
• I don't see any translations of resource subtypes (Grain, Fish, Meat, ...). And you should keep a distinction between the first word of a sentence and a different word. That distinction wasn't made for the fun of making it.
• It would be nice to keep the resource types cached as the Object.keys somewhere, so you don't have to do that ugly call every time.
• Cost.js: the checks for invalid resource names around L73 are no longer needed with a stricter schema.

[elexis]

See comments in irc (​http://irclogs.wildfiregames.com/2016-06-14-QuakeNet-%230ad-dev.log)

[s0600204]

More L10n work.

[s0600204]

...finally realised, after staring at the python script for way too long, that I can add a context key for the json extractor in messages.json (There's no mention of such a feature in the ​wiki)

As far as I know, resource subtypes are not shown at all in the gui, which is why it didn't occur to me that I'd forgotten to include them for the gui/translation. They should be being extracted for translation now. With context.

[s0600204]

AI compatibility

[mimo]

In 18478:

Implements the ai changes from s0600204 resource-agnostic patch, refs #3934

[elexis]

Only looked at the merge conflicts. Noticed that the summary screen part seems broken. The data would have to be saved too to the replay metadata file too. There are various parts that open the summary screen, search for page_summary.xml. But I'm not convinced that this data ought to be saved to the replay, it should be known from the mods loaded IMO. Various whitespace issues (trailing whitespace, whitespace in empty lines, missing spaces in objects { "foo": bar }). (Likely more as this wasn't a full review.)

[elexis]

rebased

[elexis]

I will start committing some things of that patch soon, so you should likely wait until I'm done with that and reviewing before reworking the patch.

Issues:

• Had a graphical issue when selecting a tree, perhaps I have introduced that.
• Summary screen still broken
• Breaks the tests
• Not fond of saving the resources in the sim but not in the gui, better use globalscripts to load and parse the JSON files and reuse that in Resources.js and the summary screen
• The title "resource agnostic" is a bit inaccurate, there are hardcoded checks left, like vegetarianFood in the summary screen or populationBonus.

Stuff, mostly style issues fixed in the v6:

• You didn't remove the old hideRemaining from the tech tree.
• Some unneeded parenthesis when using conditional and logic operators, check ​https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence
• Style: [ 0 ] -> [0] (we have those spaces mainly for objects)

  	let tradeProportions = [ 0, 0 ];
  	tradeProportions[0] = Math.floor(20 / resCodes.length);
  	tradeProportions[1] = 20 - resCodes.length * tradeProportions[0];
  

-> 0, 0 not needed. Should have just used [], even better do inline it right away like

	let tradeProportions = [
		Math.floor(20 / resCodes.length),
		20 - resCodes.length * tradeProportions[0]
	];

Then again we don't need that array at all, better 2 variable with readable names. That formula seems counterintuitive and is better of using the modulo operator 20 % resCodes.length to compute the remainder. Inline proportion and use a counted loop.

• if (this.resourceCount[type]), if (typeof this.resourcesUsed[type] === "number") why are those checks added? it should throw an error if an unknown resource type is treated
• Some trailing whitespace and whitespace in empty lines
• Spaces in objects {"population": 0, "populationBonus": 0, "time": 0}, -> { "population": 0, "populationBonus": 0, "time": 0 },`
• resHeads.push broken indentation.
• Attempt to stay under the 80 character limit, add linebreaks so every object property is stated in an individual line
• return data.find((resource) => { return resource.code == type; }) || false; no need for the { return ; } characters, nor that parenthesis around resource. Inline data. No need for GetResource to return false instead of undefined.
• Most function comments in Resources.js pointless.
• Not convinced we need "enabled". If some mod wants to disable, it can just delete the file.
• horizFitRepeatedObjects should work with non-repeated objects too. Rewrote that to use .children. The function should not return an argument, the caller should know that already.
• data.subtypeNames = data.subtypes; pointless
• ResourceDropsite.prototype.GetTypes -> .filter()
• objObj sounds like a bad name
• if (typeof GetSimState === "undefined" || GetSimState().resources.codes.indexOf(c) > -1 || c === "time") completely ugly, if people dont provide an icon let the error happen instead of doing half baked checks that sometimes work, sometimes dont and still have hardcoded res in it
• populationBonus being a cost seems to be a weird decision, that should go somewhere else sometime. sanderd17 suggested entityLimits, not sure if thats ideal.
• The !totalCosts[c] check can never be satisfied.
• wid -> width and why wouldn't the 260 be part of that

[elexis]

Reworked style issues.

[elexis]

In 18494:

Remove one array with hardcoded resources used for icons in tooltips. Patch by s0600204, refs #3934.

[elexis]

Remaining code

[elexis]

Review for r18494:

• Your patch didn't remove all references to g_CostDisplayIcons, you didn't take into account the new occurance in r18154.
• Better use type than c. One letter names should, if at all, only be used for integer counters in loops, not for strings.
• (The totalCosts[type] check was actually needed as a template could set the resource to 0 to disable a value of the parent template.)

Review remaining code:

• globalscripts should load and parse the resource types, so that the GUI can access too without having to rely on the simulation or saving that somewhere
• summary screen not updated in all places
• tests broken
• Strings used for chatting shouldn't have been moved to Shared.js in r18478 in my opinion. Strings used for chatting hould stay in chatHelper. If the AI needs to know which resources are available, it should use the Resources prototype or globalscripts thing. Those translate calls shouldn't stay like that, but just construct it by feeding it variables (as the strings are extracted by messages.json directly already).

[elexis]

And don't forget the tech tree:

	// TODO: It would be nice to use the gather rates present in the templates
	// instead of hard-coding the possible rates here.

	// We ignore ruins here, as those are not that common and would skew the results
	var types = {
		"food": ["food", "food.fish", "food.fruit", "food.grain", "food.meat", "food.milk"],
		"wood": ["wood", "wood.tree"],
		"stone": ["stone", "stone.rock"],
		"metal": ["metal", "metal.ore"]
	};

[elexis]

In 18533:

Ingame summary screen and allied resource / population tooltips for active players. Patch by Imarok, also reviewed by fatherbushido and discussed with mimo, fixes #3841.

Shows (only) the stats of the current player in the summary screen while playing.
Also shows those of mutual allies too if the shared vision tech is researched.
Display the resources and population counts of allies in the tooltips of the top panel.
Use a sprintf for the pop / limit ratio and remove hardcoded resource strings in the XML, refs #3934.

[s0600204]

Rebased and alterations (see below)

[s0600204]

v7:

• Rebased (r18569, ​e55e2f58)
• Code moved to globalscripts
• Used in structree
• Tests now clear
• Fixed elexis' breaking of
  • resource disabling
  • l10n (wrt resource subtype names)
  • in-game resource selection
  in his v6 and v6.1 patches

[s0600204]

As above, with some changes suggested by elexis on irc

[s0600204]

More suggestions from elexis, and adding precautions in session gui in case of more than eight resources

[s0600204]

Change how resource subtypes are parsed

[s0600204]

Make theUpgrade component resource agnostic. (It didn't exist when I started...)

[elexis]

I have imported your branch (​https://github.com/s0600204/0ad/tree/resource_agnostic) to ​https://github.com/elexis1/0ad/tree/3934-resource-agnostic and committed some changes.

Here some concerns:

• The order of resources should not change.
• The barter icons should keep the original size.

• For globalscripts/Resources.js: Why is this.resourceData.push(data); called even if we assume enabled = false? The filter of Resources.prototype.GetData could be removed otherwise. Is it so that mods can disable resources without having to remove them from the XML templates? Seems weird that it wouldn't complain about disabled resources but about unknown ones.
• For selection_panels.js: Why did you change withinSentence to firstWord for that getLocalizedResourceName call?

• messages.json The translation comments are gone now, so the translations have to be done again probably

[s0600204]

The order of resources should not change.

I've added an "order" attribute to the resource data files, and now sort the data on this attribute.

The barter icons should keep the original size.

I've made them bigger, but they can't be their original size because then they wouldn't fit vertically inside their panel. It is outside the purview of this ticket to redesign the session gui.

[...] Is it so that mods can disable resources without having to remove them from the XML templates? [...]

Yes. Many schema validation errors occur otherwise.

For selection_panels.js: Why did you change withinSentence to firstWord for that getLocalizedResourceName call?

Not intentional. Fixed.

messages.json The translation comments are gone now, so the translations have to be done again probably

Depending on how clever transifex is, most likely. To be fair, this is a fairly comprehensive overhaul of how resources are defined, so some translation work is inevitable.

[elexis]

• I'm still not too happy with the barter buttons, mostly with the fact that they are aligned at the top of the panel with a lot of unused space below it. Perhaps one could autoresize them depending on the number of resources? (Perhaps even squeeze them into one line if there are 8 res?)

• Not entirely sure if the equal horizontal distribution of the resources in the top panel is nice. Previously they had a fixed distance between each other. Perhaps you can just use the same offset like before and perhaps carve some pixels off to make it fit for 8? (There should be enough space between elements so that 1mio of every resource can be displaye wihtout overlap in 1024x768.)

[s0600204]

[...] Perhaps one could autoresize them depending on the number of resources? (Perhaps even squeeze them into one line if there are 8 res?)

Were you not just complaining that you wanted them to be bigger? I can, however, set it up so that they are vertically centred in their panel. See ​https://imgur.com/a/szna3, images 1 and 2

Perhaps you can just use the same offset like before and perhaps carve some pixels off to make it fit for 8? (There should be enough space between elements so that 1mio of every resource can be displaye wihtout overlap in 1024x768.)

Nope - see ​https://imgur.com/a/szna3 image 3. And that's with six, let alone eight. To be honest, maybe I was optimistic in wanting to support eight resources in the session gui. Clearly five is a better maximum for the top bar. Eight does work alright everywhere else, however.

[s0600204]

Rebased, and shifting of barter icons to the trade page

[Vladislav Belov]

After fast looking: patch looks good, few notes:

• why resSort not resourcesSort, as resourceData or resourceCodes? tribSize not tributeSize?
• empty line binaries/data/mods/public/gui/session/menu.js:562

[elexis]

Still changing few things here and there and didn't make it to read the entire patch from top to bottom while being fully aware of what every single line does. (Nearly there.) Committing such an ultrapatch 4 days before feature freeze without yielding any noticeable changes to the user however seems like a very risky undertaking without much advantage to the player (and I'm not sure if there are modders who want to add a new resource in alpha 21). I'd be much happier to commit it right after the release and have 3-5 months time to find the hidden oversights. Hope you agree!