[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!

[elexis]

Rebased -> ​https://github.com/elexis1/0ad/tree/3934-resource-agnostic

[elexis]

• Removing the enabled property from resources, reasons:
  1. For a mod it is cleaner to remove all occurances of the old resource than setting it to disabled
  2. For a mod it is easy to search for all occurances of <food> etc.
  3. The implementation is incomplete, for example the tech tree still shows disabled resources, Player.js has checks missing
  4. The implementation does/would have to add checks to all places that receive resources as an argument, giving significant overhead.

A modder who frequently wants to change resources could just use "resource1", ... as keys, thus still being able to switch between resources quickly (if that can be considered a use case at all).

• Performance/Caching: Functions like GetResource or GetNames are called very frequently and as their result is known on init - it seems like a big mistake to not cache these values.

[elexis]

Surprisingly the l10n isn't broken, even with the subtypes. The "Treasure" string disappeared but is treated as a special case, trying to improve...

[elexis]

In 18964:

Merge resource agnostic branch by s0600204, fixes #3934.

Remove all occurances of hardcoded resources in the simulation, GUI and AI code by
specifying resources as JSON files in a new simulation subdirectory and
accessing them through a globally defined prototype.

[elexis]

Those were not less than 78 commits on the merged git branch, first commit being done 3 Apr 2015. I hope all bugs and occurances of hardcoding (that don't have a reason to be there) were identified and removed, that you could take something away from some of the rewrite commits (and that I can upload archived versions of the branches soon).

Thanks for all of your efforts!

[mimo]

Nice (huge) patch! I had a quick look at the ai: that looks fine, except that i don't see why you needed line 103 in researchManager? have you seen cases where that was needed? otherwise, i guess if a mod requires some non-existing resource in some cost, better let it crash here rather than silently continuing.

[elexis]

Thanks mimo for rereviewing that AI part (I recall you had looked at it months ago). Indeed this and three components should have been removed in ​c16124f, will fix later today. All these checks came from the idea to disable resources with a boolean flag in the resource JSON file. But that was incomplete and a bad design choice IMO.

(Also broke the wall-placement tooltip (not sure with which git commit))

[elexis]

In 18977:

Remove silencing of resource-not-found errors (that were introduced with an enabled property in the JSON resource files that got removed later). Refs #3934.

[elexis]

(Doesn't remove the now unused resources argument from GetTemplateDataHelper as it is not unlikely to be used in the future.)

[elexis]

In 18980:

Fix wall-placement cost tooltip, refs #3934.

[mimo]

In 19004:

fixes some leftover from r18964, refs #3934

[elexis]

Atlas C++ and CCmpMiniMap.cpp still contain hardcoded resoures: #4404

[elexis]

Backup of the 78 commits using git format-patch. The four merge commits are attached separately as they are ignored by format-patch. The patches don't apply as there were merge commits. The actually applicable backup using format-patch contains 200 more commits including an 100mb art commit, thus not uploaded.

[elexis]

In 19354:

Add barter buttons to the trade window for quicker access and to support mods with more than four resource types.
The dialog is resized automatically and can be opened with a new hotkey.

Patch By: s0600204
Differential Revision: ​https://code.wildfiregames.com/D88
Fixes #4366
Refs #3934

[s0600204]

In 19604:

Revert change to datatype of resource costs in Cost.js made in rP18964

Refs: #3934
Change agreed by: fatherbushido, wraitii

[elexis]

In 19770:

Remove an unneeded complicated resource-not-found warning in the ResourceSupply component which is already covered by template validation.
It was a leftover from s0600204's proposal from rP18964 (refs #3934) to disable resources with a boolean property.

DifferentialRevision: ​https://code.wildfiregames.com/D625
Reviewed By: s0600204

[elexis]

In r19771:

More comfortable and safe resource name translation without breaking translation freeze.

Throw a reference error instead of silently using an untranslated string if someone would have passed a non-existing translation context.
Throw a property undefined warning if someone passed a non-existing resource when trying to translate it.
Both warnings had been introduced in rP16127, were removed by rP18964 but now occur without adding code explicitly.

Differential Revision: ​https://code.wildfiregames.com/D625
Reviewed By: s0600204

[elexis]

In 20600:

Allow the AI to read JSON simulation files and use the Resources prototype from rP18964 for the AI directly.
Removes the workaround copy of the resources JSON each turn in GetSimulationState.

Refs #3934, #4868
Differential Revision: ​https://code.wildfiregames.com/D1119
Reviewed By: mimo

[elexis]

In 22836:

Remove two GetTemplateDataHelper unused arguments following rP18977 and rP22527, refs #3934, #4801 / D1938.

Reported By: Polakrity