Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#3934 closed task (fixed)

[PATCH] Remove hardcoded resources

Reported by: Imarok Owned by: elexis
Priority: Should Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by elexis)

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

Attachments (15)

resource_agnostic.patch (81.9 KB ) - added by s0600204 8 years ago.
Potential patch. It's big.
resource_agnostic-v2.patch (93.5 KB ) - added by s0600204 8 years ago.
Stricter schema and sorting out some L10n issues.
resource_agnostic-v3.patch (92.7 KB ) - added by s0600204 8 years ago.
More L10n work.
resource_agnostic-v4.patch (100.6 KB ) - added by s0600204 8 years ago.
AI compatibility
resource_agnostic-v5.patch (89.9 KB ) - added by elexis 8 years ago.
rebased
resource_agnostic-v6.patch (86.0 KB ) - added by elexis 8 years ago.
Reworked style issues.
resource_agnostic-v6.1.patch (80.1 KB ) - added by elexis 8 years ago.
Remaining code
resource_agnostic-v7.diff (100.5 KB ) - added by s0600204 8 years ago.
Rebased and alterations (see below)
resource_agnostic-v7.1.patch (100.4 KB ) - added by s0600204 8 years ago.
As above, with some changes suggested by elexis on irc
resource_agnostic-v7.2.patch (109.1 KB ) - added by s0600204 8 years ago.
More suggestions from elexis, and adding precautions in session gui in case of more than eight resources
resource_agnostic-v7.3.patch (109.2 KB ) - added by s0600204 8 years ago.
Change how resource subtypes are parsed
resource_agnostic-v7.4.patch (110.3 KB ) - added by s0600204 8 years ago.
Make theUpgrade component resource agnostic. (It didn't exist when I started...)
resource_agnostic-v8.patch (121.8 KB ) - added by s0600204 8 years ago.
Rebased, and shifting of barter icons to the trade page
l10n.diff (10.4 KB ) - added by elexis 7 years ago.
Surprisingly the l10n isn't broken, even with the subtypes. The "Treasure" string disappeared but is treated as a special case, trying to improve…
git_commits_incomplete_but_readable_eed994f.zip (133.5 KB ) - added by elexis 7 years ago.
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.

Download all attachments as: .zip

Change History (52)

comment:1 by Imarok, 8 years ago

Keywords: simple added

by s0600204, 8 years ago

Attachment: resource_agnostic.patch added

Potential patch. It's big.

comment:2 by s0600204, 8 years ago

Keywords: patch review added; simple removed
Milestone: BacklogAlpha 21
Summary: Remove hardcoded resources[PATCH] Remove hardcoded resources

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

comment:3 by sanderd17, 8 years ago

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.

by s0600204, 8 years ago

Attachment: resource_agnostic-v2.patch added

Stricter schema and sorting out some L10n issues.

comment:4 by sanderd17, 8 years ago

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.

comment:5 by elexis, 8 years ago

Keywords: review removed

by s0600204, 8 years ago

Attachment: resource_agnostic-v3.patch added

More L10n work.

comment:6 by s0600204, 8 years ago

Keywords: review added

...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.

by s0600204, 8 years ago

Attachment: resource_agnostic-v4.patch added

AI compatibility

comment:7 by mimo, 8 years ago

In 18478:

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

comment:8 by elexis, 8 years ago

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.)

by elexis, 8 years ago

Attachment: resource_agnostic-v5.patch added

rebased

comment:9 by elexis, 8 years ago

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:

-> 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
Last edited 8 years ago by elexis (previous) (diff)

by elexis, 8 years ago

Attachment: resource_agnostic-v6.patch added

Reworked style issues.

comment:10 by elexis, 8 years ago

In 18494:

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

by elexis, 8 years ago

Remaining code

comment:11 by elexis, 8 years ago

Keywords: review removed

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).
Last edited 8 years ago by elexis (previous) (diff)

comment:12 by elexis, 8 years ago

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"]
	};

comment:13 by elexis, 8 years ago

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.

by s0600204, 8 years ago

Attachment: resource_agnostic-v7.diff added

Rebased and alterations (see below)

comment:14 by s0600204, 8 years ago

Keywords: rfc added

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

by s0600204, 8 years ago

As above, with some changes suggested by elexis on irc

by s0600204, 8 years ago

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

by s0600204, 8 years ago

Change how resource subtypes are parsed

by s0600204, 8 years ago

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

comment:15 by elexis, 8 years ago

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
Last edited 8 years ago by elexis (previous) (diff)

comment:16 by s0600204, 8 years ago

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.

comment:17 by elexis, 8 years ago

  • 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.)

comment:18 by s0600204, 8 years ago

[...] 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.

by s0600204, 8 years ago

Attachment: resource_agnostic-v8.patch added

Rebased, and shifting of barter icons to the trade page

comment:19 by Vladislav Belov, 8 years ago

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
Last edited 8 years ago by Vladislav Belov (previous) (diff)

comment:20 by elexis, 7 years ago

Description: modified (diff)
Keywords: review added; rfc removed
Milestone: Alpha 21Alpha 22

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!

comment:21 by elexis, 7 years ago

Priority: Nice to HaveShould Have

comment:22 by elexis, 7 years ago

  • 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.
Last edited 7 years ago by elexis (previous) (diff)

by elexis, 7 years ago

Attachment: l10n.diff added

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

comment:23 by elexis, 7 years ago

Owner: set to elexis
Resolution: fixed
Status: newclosed

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.

comment:24 by elexis, 7 years ago

Keywords: review removed

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!

comment:25 by mimo, 7 years ago

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.

comment:26 by elexis, 7 years ago

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))

comment:27 by elexis, 7 years ago

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.

comment:28 by elexis, 7 years ago

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

comment:29 by elexis, 7 years ago

In 18980:

Fix wall-placement cost tooltip, refs #3934.

comment:30 by mimo, 7 years ago

In 19004:

fixes some leftover from r18964, refs #3934

comment:31 by elexis, 7 years ago

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

by elexis, 7 years ago

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.

comment:32 by elexis, 7 years ago

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

comment:33 by s0600204, 7 years ago

In 19604:

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

Refs: #3934
Change agreed by: fatherbushido, wraitii

comment:34 by elexis, 7 years ago

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

comment:35 by elexis, 7 years ago

Description: modified (diff)

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

comment:36 by elexis, 6 years ago

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

comment:37 by elexis, 5 years ago

In 22836:

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

Reported By: Polakrity

Note: See TracTickets for help on using tickets.