Resource Trickle Tooltip
ClosedPublic
Actions

Authored by elexis on Apr 5 2017, 7:29 AM.

Details

Reviewers

fatherbushido

Commits

rP19387: Resource Trickle tooltip.

Summary

Before Alpha 22, only the persian palace had a resource trickle. With this release, wonders and the macedonian catafalque (Philip V) also have it, so we should inform the player.
Also prevents empty loot tooltip in case of an entity having a loot component but no loot numbers.

Test Plan

Yo dawg, I heard you like sprintf. (So I put a sprintf into the sprintf and so on)
Open the tech tree and read the tooltip of the wonder and persian palace.
Start a capture the relic match and look for Philip V and the tooltip.
Remove ResourceGather disabled="" from special_catafalque.xml and check that there is no "Loot:" string.

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

elexis created this revision.Apr 5 2017, 7:29 AM

elexis edited a custom field.Apr 5 2017, 7:31 AM

Build has FAILED

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).ERROR: JavaScript error: simulation/components/GuiInterface.js line 587
ReferenceError: IID_ResourceTrickle is not defined
  GuiInterface.prototype.GetExtendedEntityState@simulation/components/GuiInterface.js:587:6
  @simulation/components/tests/test_GuiInterface.js:602:25

In TestComponentScripts::test_scripts:
/mnt/data/jenkins/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Test failed: L"Running script simulation/components/tests/test_GuiInterface.js"
/mnt/data/jenkins/workspace/phabricator/source/simulation2/components/tests/test_scripts.h:44: Error: Assertion failed: scriptInterface.LoadScript(pathname, content)
...............................................................................................................................................................................................................................................................................................................
Failed 1 and Skipped 0 of 305 tests
Success rate: 99%

Link to build: http://jw:8080/job/phabricator/698/
See console output for more information: http://jw:8080/job/phabricator/698/console

Harbormaster failed remote builds in B1017: Diff 1103!Apr 5 2017, 9:56 AM

Fix the tests as reported by fatherbushido and add an actual test.

Code:
Checked all the not sprintf stuff.
Test:
unit with Loot, without ResourceGatherer -> loot tooltip
unit without Loot, with ResourceGatherer -> loot tooltip when carrying
unit without Loot and withtout ResourceGatherer -> no tooltip
ResourceTrickle tooltip ok, take account of tech and aura modification too (see Templates.js)
The tooltip seems fine (but my opinion about that is not really valuable.
Attack rate tooltip not broken.
Checked stucttree and session.

This revision is now accepted and ready to land.Apr 5 2017, 6:12 PM

bb added a subscriber: bb.Apr 5 2017, 6:20 PM

bb added inline comments.

binaries/data/mods/public/globalscripts/Templates.js
257 ↗	(On Diff #1114)	missing space
binaries/data/mods/public/gui/common/tooltips.js
416 ↗	(On Diff #1114)	shouldn't here be a round/tofixed? (imagine a mod adding a tech for this) I guess stupid people adding a 0 rate should be punished with an ugly tooltip. Actually a tech could enable it, probably need a fix.
binaries/data/mods/public/gui/structree/draw.js
4–19 ↗	(On Diff #1114)	The order of these list could use some syncing, suggesting alphabetical order or same order as they are used. But out of scope. Same goes for function order.

fatherbushido added inline comments.Apr 5 2017, 6:54 PM

binaries/data/mods/public/gui/common/tooltips.js
416 ↗	(On Diff #1114)	Sounds like, it's the best place to do it. I did multiplication by 10 in my tests, so it doesn't look weird.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!

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

Harbormaster completed remote builds in B1021: Diff 1114.Apr 5 2017, 7:33 PM

fatherbushido added inline comments.Apr 7 2017, 5:47 AM

binaries/data/mods/public/gui/common/tooltips.js
416 ↗	(On Diff #1114)	The annoying thing is if we have for example 0.001 each 1000 ms, I don't really get how to round that. We can only display the by second gain (but we lost some information).

Thanks for the reviews! I've added that space and filtered 0 resource entries.

binaries/data/mods/public/gui/common/tooltips.js
416 ↗	(On Diff #1114)	We wanted to keep resources integer, so there shouldn't be a template value providing a float in the first place, nor an aura effecting the resource amount if it's not an integer number too (like 2x bonus). For the same reason we changed the interval for the relic to 1250. Having zeroes in there sounds more probable and I've had to add commits for special cases often enough to add a check preemptively now.
binaries/data/mods/public/gui/structree/draw.js
4–19 ↗	(On Diff #1114)	The order here in the order seen in the tooltips. It should be(come) the same in the session and tech tree indeed. Moving functions around can be done when changing them anyway, but doing an own commit for that seems a bit unneded, considering that they are all getFooTooltip and work individually and equaly.