Page MenuHomeWildfire Games

Correct resource order in the GUI
ClosedPublic

Authored by elexis on Dec 29 2016, 7:30 PM.

Details

Summary

While the removed check in Templates.js in rP18977 indeed covered disabled resources,
it was also useful to ensure the resource order.

There might be other places where the resource order is incorrect. But after reading the resource agnostic commits in #3934 again, I can't spot any other issues easily.
There is no clear rule when we enforce the order (simulation? globalscripts? gui?).

Test Plan

When looking at the recently changed cost of the Slinger mercenary available to Carthaginians,
we observe an inconsistent resource cost order.
The applied patch fixes that.

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

elexis updated this revision to Diff 73.Dec 29 2016, 7:30 PM
elexis retitled this revision from to Correct resource order in the GUI.
elexis updated this object.
elexis edited the test plan for this revision. (Show Details)
Vulcan added a subscriber: Vulcan.Dec 29 2016, 8:13 PM

Build is green

Updating workspaces.
Build (release)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
../../../source/lib/tex/tex_png.cpp: In member function ‘virtual Status TexCodecPng::encode(Tex*, DynArray*) const’:
../../../source/lib/tex/tex_png.cpp:309:9: warning: variable ‘ret’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
  Status ret = ERR::FAIL;
         ^
Build (debug)...
../../../source/gui/CChart.cpp:38:41: warning: unused parameter ‘Message’ [-Wunused-parameter]
 void CChart::HandleMessage(SGUIMessage& Message)
                                         ^
Running debug tests...
Running cxxtest tests (302 tests)..............................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/44/ for more details.

fatherbushido accepted this revision.EditedDec 29 2016, 11:07 PM
fatherbushido added a reviewer: fatherbushido.
fatherbushido added a subscriber: fatherbushido.

Patch tested. Solve the issue (order was messed also for some structure like military colony...).

This revision is now accepted and ready to land.Dec 29 2016, 11:07 PM
elexis added a comment.Jan 1 2017, 7:19 PM

Still don't know if it's complete, if we just want a quick bugfix or ensure order internally in the simulation in all occurances or only the GUI. Could rename this post to adapt the scope.

elexis added a comment.Jan 2 2017, 3:40 PM

Guess reviewers might not look at the open question if the differential says ready to land.
It were bad if there is some other occurances, for example in the tooltips where the order is wrong due to the resource agnostic patches. Then we'd have to add yet another fix to the fix.

fatherbushido resigned from this revision.Jan 2 2017, 5:10 PM
fatherbushido removed a reviewer: fatherbushido.
This revision now requires review to proceed.Jan 2 2017, 5:10 PM
elexis updated the Trac tickets for this revision.Jan 5 2017, 11:28 AM
elexis added subscribers: Restricted Owners Package, Restricted Owners Package.Jan 16 2017, 3:20 PM

I guess it's worse to not fix this one known occurance, so accepting the acceptance.

elexis changed the visibility from "All Users" to "Public (No Login Required)".Jan 29 2017, 7:17 AM
fatherbushido edited edge metadata.Jan 29 2017, 9:41 AM

So:

  • indeed we have that issue (structure tree, training and construction panels: resources or missing resources)
  • i don't see another bad sort in loot tooltips
  • (tech costs are ok)
  • (upgrade costs aren't, but it was yet the case before agnostic res patch)
  • patch fixes the order
  • (if we want to keep order we should avoid things like Object.keys() or for in)
  • we can fix the order here (in Template.js) and/or in tooltips too (we still do for (let type in template.cost) L332 of tooltips.js) (take care that there is time too at this place)

Consider the last point (you have a more global overview than me about those issues) and I'll accept it.

elexis planned changes to this revision.Jan 30 2017, 11:43 AM

You're right, there are some other occurances. For example the carrying resources tooltip:

			"details": bodyFont(Object.keys(totalCarrying).filter(
			"details": bodyFont(Object.keys(totalLoot).filter(

Thanks for the review! I'll be back!

Imarok added a subscriber: Imarok.Jan 30 2017, 5:32 PM
Imarok removed a subscriber: Imarok.
fatherbushido resigned from this revision.Feb 8 2017, 6:08 PM

@s0600204 mind taking this over?

I suppose I could, if you really want me to.

So I guess the question is where you wish the order enforced. My thinking:

  • Simulation: IMO, the order shouldn't matter in the simulation, and the player should never see that directly.
  • GUI: We'd have to ensure that all gui scripts enforce the order, either individually (such as in the session gui's top panel) or by through using common files (such as gui/common/tooltips.js when putting together tooltips). This would potentially include keeping an eye out for any gui added by modders.
  • Globalscripts: If the order is enforced in globalscripts, then it shouldn't matter what part of the gui prints it for the player - the resources should then be fed to the gui in the correct order.

I also note that technologies currently have their time cost listed before any other values. Is this wanted?

Other resource components that might need resource order enforcing:

  • Loot - The order is currently enforced in tooltips.js.
  • ResourceTrickle - Resource order not currently enforced (the Wonders have this component).
  • ResourceGatherer - Hard to achieve in globalscripts/Templates.js as the types specified in templates include minor types (ie. food.grain, wood.ruins) as well as the major types (food, wood, etc.). The order is currently enforced in tooltips.js.
  • ResourceDropsite - Simulation only - list of resources accepted is not displayed in gui, thus does not need sorting.
  • Upgrade (as mentioned by @fatherbushido) - Resource order not currently enforced anywhere.
temple added a subscriber: temple.Jun 7 2017, 3:23 AM

I hope this is included in a22. It's really annoying to see stone and metal listed before wood.

elexis edited the summary of this revision. (Show Details)Jun 9 2017, 4:23 PM

Broken resource order can be noticed in the current svn with messed up templates or with cost inheritance.
For example correcting the resource order in template_unit_infantry_ranged_slinger.xml still doesn't produce a correct resource order in the GUI.

summary/ and session/ seem to enforce the order already, but I could not finish to review the entire codebase. Other GUI pages (except the structree) don't refer to resources.
common/tooltips.js common/l10n.js are the only relevant files for resource order seen in tooltips as far as I can see.

Reading them from top to bottom reveals all places where the correct resource order is not enforced and thus was patched:

getLocalizedResourceName
multiplyEntityCosts
getEntityCostComponentsTooltipString
getResourceTrickleTooltip
getWallPieceTooltip

We observe that the other functions in these files work correctly.

Answering s0600204:

Simulation: IMO, the order shouldn't matter in the simulation, and the player should never see that directly.

Agree.
It might or might not be considered good practice to use the same order in all three directories.
But since there's no real reason to and in some cases we can avoid an if-statement when looping over object properties, it seems better to avoid that practice in the simulation.

Globalscripts: If the order is enforced in globalscripts, then it shouldn't matter what part of the gui prints it for the player - the resources should then be fed to the gui in the correct order.

Disagree.
globalscripts can keep it correct, but the GUIInterface still could fetch resource counts from a simulation component, thus it would have to be enforced everywhere or only in the GUI.

GUI: We'd have to ensure that all gui scripts enforce the order, either individually (such as in the session gui's top panel) or by through using common files (such as gui/common/tooltips.js when putting together tooltips).
This would potentially include keeping an eye out for any gui added by modders.

Notice that modders can also change globalscripts (for example by keeping outdated copies around for no reason) and the simulation directory anyway.

Agree.
It should be sufficient to enforce the order in gui/ since there is no reason to require a specific order other than for visual appearance to the enduser (at least for now).
Thus the patch enforces Correct resource order in the GUI (D23).

I also note that technologies currently have their time cost listed before any other values. Is this wanted?

Two code breakers broke that in rP18494.

Upgrade (as mentioned by @fatherbushido) - Resource order not currently enforced anywhere.

  • That the struct tree shows Upgrade costs (introduced by rP19600 / D92) using getEntityCostComponentsTooltipString is due to the fact that it uses drawProdIcon for upgrades in draw() (introduced by that commit)

and confirmed by appending this debug output to the function:

	if (template.name.generic.indexOf("Tower") != -1)
		warn(uneval(template.name.generic) + ": " + uneval(costs));
elexis updated this revision to Diff 2504.Jun 10 2017, 4:13 AM

See above

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/common/tooltips.js
| 566| »   »   '[/color][/font]'·+·"·"·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/common/tooltips.js
| 398| »   »   let·[rate,·count]·=·types.reduce((sum,·t)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/common/tooltips.js
| 578| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/common/tooltips.js
| 578| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/170/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1529/ for more details.

temple accepted this revision.Jun 10 2017, 5:36 PM
This revision is now accepted and ready to land.Jun 10 2017, 5:36 PM

GUI: We'd have to ensure that all gui scripts enforce the order, either individually (such as in the session gui's top panel) or by through using common files (such as gui/common/tooltips.js when putting together tooltips).
This would potentially include keeping an eye out for any gui added by modders.

Notice that modders can also change globalscripts (for example by keeping outdated copies around for no reason) and the simulation directory anyway.

Note that modders that keep broken things around are the only ones responsible for keeping broken things around. Especially things like globalscripts or simulation components (that are copies, not proper extensions to current components, or additions of new components) should not be copied, if someone decides to ignore all advise against doing that, tough luck and good riddance.

And if someone changes (instead of adding to) globalscripts in a mod they should be aware of the consequences, and debug those issues on their own.

s0600204 requested changes to this revision.Jun 10 2017, 7:15 PM

Reading through the changes proposed, I was initially concerned that the enforcing code seems to rely on the presence of a global (g_ResourceData) which is not declared in the common scripts, but separately in the structree and session GUI pages. However, as acking shows that the only place that the two files modified in this revision are included are within these two contexts, it seems to work. I suppose that as long as any gui page that seeks to include these files in the future also declares a global variable with this exact name, it shouldn't be an issue. (That said, modders are a law unto themselves.)

Testing in game, I notice that the changes break the {min} to {max} resource cost display for wallsets, affecting both the structree and session GUIs (image shows before and after):

This revision now requires changes to proceed.Jun 10 2017, 7:15 PM
elexis updated this revision to Diff 2517.Jun 10 2017, 7:38 PM
elexis edited edge metadata.

That wall loop needed getCostTypes(), not resource codes.

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/gui/common/tooltips.js
| 565| »   »   '[/color][/font]'·+·"·"·+
|    | [NORMAL] ESLintBear (no-useless-concat):
|    | Unexpected string concatenation of literals.

binaries/data/mods/public/gui/common/tooltips.js
| 398| »   »   let·[rate,·count]·=·types.reduce((sum,·t)·=>·{
|    | [NORMAL] JSHintBear:
|    | Don't make functions within a loop.

binaries/data/mods/public/gui/common/tooltips.js
| 577| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.

binaries/data/mods/public/gui/common/tooltips.js
| 577| »   if·(walk·==·0·&&·run·==·0)
|    | [NORMAL] JSHintBear:
|    | Use '===' to compare with '0'.
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jw:8080/job/phabricator_lint/172/ for more details.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

http://jw:8080/job/phabricator/1532/ for more details.

s0600204 accepted this revision.Jun 10 2017, 10:07 PM

That works, thank you.

This revision is now accepted and ready to land.Jun 10 2017, 10:07 PM
This revision was automatically updated to reflect the committed changes.