[Patch] Loot tooltips

[elexis]

Looting is a relatively unknown feature. In particular it is completely intransparent how many resources are earned when killing enemies or destroying buildings.

For example destroying a temple yields 500 metal! Killing a trade cart or a gathering unit yields the resources the unit carried.

The sum of these resources should be shown where all the other tooltips are shown, for every unit.

[elexis]

• The loot tooltip should show the sum of all loot, which consists of:
  • The loot as hardcoded in the template
  • The resources that the killed units had carried
  • The resources that the killed merchants traded
  • The experience points gained when killing those enemies

• Similar to r18154 it should show the sum of all loot when selecting multiple units

• The tooltip should be shown in the tech tree (since this is the go-to place when trying to find any information on unit stats)

• The tooltip should be shown in the session (if showdetailedtooltips is enabled)

• The tooltip shouldn't be shown in the training / construction panel, since that might be confusing (one won't get the loot after building / training)

---

This way one can select a group of gathering units and see how much resources they are worth. This applies to observermode and players selecting their own units (since one can't select multiple enemy units currently).

[fatherbushido]

There is still an annoying thing (not so annoying). You ll not always see the loot you'll get (because some units can have a modified Looter: the loot you get depends of the plunder too).

[Mate-86]

Attached proof of concept how the new field is displayed (with dummy values):

1. in structure tree each unit and building has the base loot value (hardcoded in template) in the tooltip
2. during the game each unit and building has an enhanced value (template + carried + traded + xp)

Questions: a; Shall I add the loot to the template or there is another ticket for that? => clarified, I need to check the template xml and not template.js b; Is the lootable XP a fix value and part of the template? Does it depends on the XP of the killed unit? => clarified: defined in loot.js and looter.js c; In which tooltip is the loot for multiple units displayed? In the attached 3rd poc the tooltip currently shows the name of the unit. => clarified: the sum of all units

[fatherbushido]

Replying to Mate-86:

Questions: a; Shall I add the loot to the template or there is another ticket for that? => clarified, I need to check the template xml and not template.js

You'll need to add that to template.js and perhaps to GuiInterface.js (in the extended state part), take care to add it in the same way for the two files. (And to add it to GuiInterface tests.)

b; Is the lootable XP a fix value and part of the template? Does it depends on the XP of the killed unit? => clarified: defined in loot.js and looter.js

Defined in Loot. (Looter is for the entity wich will take the loot.)

[Mate-86]

First version of the patch uploaded:

1. Tech tree and multiple unit selection tooltips are enriched with looting information
2. I need help for the single unit tool tip view: How to access entity information in getLootTooltip? Enity contains the resource carried by the worker. Currently the entity paramater gives back undefined. => elexis clarified I should add that calculation to Loot.js which is accessible though GuiInterface and template.js (but not mess up the behavior of Looter.js)
3. I have to write the GuiInterface tests.

[fatherbushido]

Replying to Mate-86:

First version of the patch uploaded:

1. Tech tree and multiple unit selection tooltips are enriched with looting information

Thx for working on that. You decided to replace the carried ressources (in the gui) by the loot, that makes sense, ask elexis if he's ok with that.

2. I need help for the single unit tool tip view: How to access entity information in getLootTooltip? Enity contains the resource carried by the worker. Currently the entity paramater gives back undefined. => elexis clarified I should add that calculation to Loot.js which is accessible though GuiInterface and template.js (but not mess up the behavior of Looter.js)

I m not fully convinced you should do that, it needs to be discussed. Perhaps you can then open a ticket suggesting this task.

3. I have to write the GuiInterface tests.

You've just to add the stuff you added in GuiInterface in the test (so that the object has all the keys).

• tooltips.js L579: template is the only param, isn't it ?
• selection_details: you can use for (let foo of arrayFoo) (for arrays)

for (let i in lootTypes) 
{ 
    let type = lootTypes[i]; 
    totalLoot[type] = (totalLoot[type] || 0) + extEntState.loot[type]; 
}

can be replaced by

for (let type of lootTypes) 
    totalLoot[type] = (totalLoot[type] || 0) + extEntState.loot[type]; 

(wich can be perahps refined too)

and same above

• selection_details: you can perhaps (needs to be checked) add all when constructing the totalLoot object or something else.

[Imarok]

Replying to fatherbushido:

Replying to Mate-86:

First version of the patch uploaded:

1. Tech tree and multiple unit selection tooltips are enriched with looting information

Thx for working on that. You decided to replace the carried ressources (in the gui) by the loot, that makes sense, ask elexis if he's ok with that.

So how does the player knows how many resources his units are carrying?

[elexis]

Indeed, the tooltip shouldn't be replaced, but extended, like

Carrying: 123 food
Loot: 234 food

[fatherbushido]

@Mate-86: So you have your answer. And it's one more good reason to not move the computation to the Loot component.

[elexis]

We discussed a bit more. L13-L42 would be code that is shared between gui/ and simulation/. Moving it to the Loot component were bad, since that component doesn't use that code at all. Instead a function should be added to globalscripts/`.

In order to merge duplicate code, it would have to receive an array of entityIDs (of units that are being looted/plundered) and optionally an attacker/looter entityID that if given, calls ApplyValueModificationsToEntity (which is only available to simulation/).

Notice #3934 will fix that TODO in the Looter code very soon.

[Mate-86]

Issue fixed:

• Loot and carried res is displayed when multiple units selected
• "let type of lootTypes" applied

Issues remained:

• GuiInterface.js test updated but still fails (executed with Cygwin but still no info what is the issue)
• for a single unit only the hard-coded loot is displyed
• carried goods are displayed in another tooltip (mouseover the unit icon) for multiple units
• traded goods are not displayed separately in tooltip

Please advice which one I should fix or will be solved in another ticket!

[Mate-86]

I'm stuck. I've created the file LootHelper.js in globalscripts/ and copied there L13-L42 from Looter.js to eliminate code duplication. Now I get an error: "ReferenceError: IID_Loot is not defined". What to do?

[fatherbushido]

Mate-86: can you upload a patch to see what's wrong ?

[Mate-86]

patch uploaded containing LootHelper

[fatherbushido]

in fact you use sim methods outside of the sim. you have to write a function in wich you parse getcarriedressources and getgoods and getressources objects

[Mate-86]

Does this mean that I have to pass these objects as arguments to the calculator function in globalscripts/LootHelper.js instead of the looter and target entities ids?

[fatherbushido]

yes, because you can't query those components in this globalscript

[Mate-86]

LootHelper updated based on the feedback from fatherbushido. Locally tested successfully.

[fatherbushido]

nice! In Templates.js you can use perhaps for (let type in template.loot) to skip that hardcoded loot types array (see costs in the same function). Else there is your other work (siege in summary) in this patch.

[Mate-86]

I've removed the siege related changes from the patch. I tried "for (let type in template.loot)" in template.js but it resulted "undefined property template.loot" error when I navigated to the techtree.

[fatherbushido]

Thanks for your work! it seems we are close to the goal. Here are my review.

test_GuiInterface.js:

• L18

add that: Engine.LoadComponentScript("interfaces/Loot.js");

Template.js:

• L252

remove L1 and replace

for (let type of lootTypes)
  if (template.Loot[type])
    ret.loot[type] = getEntityValue("Loot/" + type);

by

for (let type in template.Loot)
  ret.loot[type] = getEntityValue("Loot/"+ type);

(no error here) (note for myself : perhaps, we should make all loot types mandatory)

selection_details.js:

• L344 - 350

you would need to add that lootTypes array or instead (better) use a for (let type in objectFoo)

• At last, there are still broken stuff in the different tooltips. Iirc elexis asked:
  • loot tooltip in the structure tree -> done
  • loot tooltip in the individual unit tooltip taking account of ressources carried or goods traded (but not deleting those tooltips) -> seems broken
  • loot tooltip in the multiple selections unit (same as previous) -> seems broken

-> Ask elexis some hints about that.

• Btw i got that error in a test game (only with the patch)

  ERROR: Error in timer on entity 1, IID 57, function MissileHit: TypeError: carriedResources[Symbol.iterator] is not a function
    calculateResourcesCarried@globalscripts/LootHelper.js:10:1
    Looter.prototype.Collect@simulation/components/Looter.js:21:25
    Damage.prototype.TargetKilled@simulation/components/Damage.js:270:3
    Damage.prototype.CauseDamage@simulation/components/Damage.js:224:3
    Damage.prototype.MissileHit@simulation/components/Damage.js:126:1
    Timer.prototype.OnUpdate@simulation/components/Timer.js:100:4
  

[Mate-86]

1. loot tooltip in the multiple selections unit (same as previous) -> seems broken

works for me locally => ​http://i.imgur.com/wqsPgLk.png could you please post a screenshot?

2. for the single unit Loot tooltip I need help because I cannot access entity information in gui/common/tooltip.js -> getLootTooltip() => @elexis could you give some hint please?

Taking care of the rest.

[Mate-86]

Latest patch eliminates the problem about "TypeError: carriedResources[Symbol.iterator] is not a function" or at least I could not reproduce it with the attached replay anymore (before the fix I could reproduce it). I've added the lootTypes array to L344-350 in selection_details.js. For setting a "for (let type in objectFoo)" the keys of the objects should be merged which seems to be complicated. Eg. baseLoot of a unit is XP+food but it carries stone.

[fatherbushido]

• your screenshot is ok,I have missed something
• for 2, what s your problem ? when getLootTooltip is called from selections details, the template arg is entState.
• after discussion it seems that the looterhelper new file is not really fully satisfacting. perhaps you can revert that, we would merge what is mergeable later in a better way.

[fatherbushido]

ok nice for the fix, i see also what s your issue with object keys.

[Mate-86]

a, The single unit display issue fixed as well: ​http://i.imgur.com/z8R5eW1.png b, LooterHelper is used in 3 different places now:

• tooltips.js/getLootTooltip
• Looter.js
• selection_details.js/displayMultiple

Do you want me to copy the same code (calculateResourcesCarried) in all of these files? If you think it's less risky/nicer not to use the shared code of course I'll revert it. c, question: some of the resource types may have 0 value, shall I eliminate those from the tooltips? eg. stone/metal 0 here: ​http://i.imgur.com/wqsPgLk.png

[fatherbushido]

a) nice b) ask elexis, he will decide (I have no strong opinion about that) c) perhaps it's simpler to let them, (or do like it's done for costs in selection_panels).

I will do my final review this evening.

[elexis]

• b) Three occurances? getLootTooltip doesn't have one. Well, as long as the exact same code is shared in both places and not having to be extended to 14 nested layers, avoiding the duplicate sounds preferable.
• c) Yes, don't show the 0 entries. We don't show 0 entries anywhere so as to hide needless strings.
• calculateResourcesCarried should have some JSdoc style comment mentioning that the variables are either extendedEntityStates or
• The name calculateResourcesCarried seems misleading as it doesn't imply that it implies trading goods.
• Perhaps the .goods part can be moved to calculateResourcesCarried too (perhaps simplifying some of those ternaries.)
• Not convinced to add a new file for 10 lines. Templates.js could be extended instead.
• calculateResourcesCarried -> + should be at the end of the previous line. It's our coding convention (though missing in wiki:Coding_Conventions), and a common good practice, f.e. recommended by jshint.com.
• Icon: File not found error for art/textures/ui/session/icons/resources/xp_small.png
• Icon: If that icon from the screenshot is used, it should be transparent
• translate("Carrying") + ": " -> sprintf(translate("Carrying: %(resources)s"), { "resources": foo }, same for the other occurances of ": " (translators should be able to move that colon around. I guess the catch is that we need to colorize the label including the colon, which means "%(label)s%(colon)s %(value)s" would have to be used. But don't do that as it's not the task of this ticket to rewrite all tooltips to fix that issue. You can use translate(Carrying:) if you need that colorized colon, otherwise "Carrying: %(resources)s".
• .length > 0 -> .length
• Those "food", "wood",... will be rewritten following #3934, but that shouldn't be an issue now. (The objects will have to be constructed given a ["food", "wood", "metal", "stone"] array.)
• For the one committing the patch: New files added need the svn eol-style native property.
• Unused global lootTypes in Templates.js
• Temples give 500 metal loot. Too much, wtf. Even a civic center gives onyl 50 wood 50 metal 50 stone. I suggest 50 metal for a temple and 250 wood metal stone for a civic center. Broken in r11524. Seems they were forgotton in nearly every new structure ever since. A probably complete list can be found in IRC. Fixing the templates would be a nice addition following the GUI patch!
• That what the diff said...No newline at end of file (as in files should end with newlines)
• LootHelper.js L2 missing *
• // TODO: stop assuming that cmpLoot.GetResources() delivers all resource types (by defining them in a central location) removing that is wrong since you don't actually fix it. cmpLoot.GetResources can just return an array with only one resource type.
• cmpResourceGatherer ? cmpResourceGatherer.GetCarryingStatus() : false, could use foo.bar() || false instead I think
• As pointed out by Imarok, GetExtendedEntityState already calls GetEntityState (these proxy functions found in session.js). So there is no point in doing the GetEntityState call there at all. Should just replace it with Extended.
• (What I don't like about this comment is that many of these issues had been pointed out already.)

Patch looks ready to commit otherwise, good work.

[Mate-86]

@elexis: thanks fot the comments!

• I've attached a new patch and the icons with transparent background to the ticket (xp.png and xp_small.png)
• "Temples give 500 metal loot. Too much ..." => is this something I should fix or just an info?
• the rest should be ok but plz let me know if I missed out something

[elexis]

Patch looking VERY good :) (few comments in irc today that are fixed with the next upload)

About the messed up templates, if you get this one committed, I'd be happy to fix this now too. If we present players those interesting loot values, they should be proper values ideally. From yesterdays IRC log:

10:41 fatherbushido: temple metal loot 500 -> 50, civic center 50 50 50 -> 250 250 250?
10:41 broken in r11524
11:03 I'd pick 250 as its half of the build cost
11:30 fatherbushido: persian temple got no loot component
11:30 persian hall
11:30 wonder could have slightly more
11:30 fort too :P
11:31 blacksmith less
11:32 ptol lighthouse missing Loot
11:32 library missing
11:33 roman army camp missing
11:34 temple of vesta should use relative templates, +50% or something
11:34 barracks could give slightly more loot
11:35 tower more loot too
11:35 military colony is ok
11:36 theatron missing loot
11:36 athen gymnaseon missing loot
11:36 royal stoa might be a bit more
11:39 kennel missing loot
11:45 fatherbushido: rotary mill should have loot too, by inheriting template_structure_economic rather than template_structure_special, no?
11:45 naval shipyard has no loot
11:46 monuments dont have it
11:46 siege workshop neither, surprise
11:46 elephant stables
11:46 persian gate
11:47 lighthouse if I didnt mention that already

For most buildings it makes sense to give them 10-30% loot value.

[Mate-86]

Some more fixes uploaded. I could not eliminate the lootTypes array from tooltips.js/getLootTooltip because the keys returned by the template are sorted alphabetically (food, metal, ...) and 0 values are not filtered (xml templates contain 0 loot values too).

[Mate-86]

I've introduced regression in patch 9 so that Looter.js fails. I've added some guards around cmpResourceGatherer (Looter.js L22) and trader.goods (Template.js L407). Should be fixed now.

[elexis]

In 18733:

Loot tooltips. Patch by Mate-86, fixes #4193.

[elexis]

Thanks for the patch Mate-86! You have very bravely addressed all remarks and made a very clean patch solving this issue quickly. Happy to see more contributions :)

As mentioned above, we should also fix the templates now that everyone can see these values. Besides the given list above, only military structures, perhaps the wonder should give experience point loot (since one doesn't really learn fighting skills when kicking over a storehouse).

Some changes I had done to your patch:

• Had asked for these JSdoc comments that we have an exact description of what our input is for calculateResourcesAndGoodsCarried. The description the looted trader doesn't provide any useful information to the reader. Finding a better one revealed that the helper function accesses the goods variable of the Trader prototype directly. goods ought to be a local variable which can't be read or changed outside. Getter functions should be used for that (GetGoods). F.e. the prototype might want to use a differnt variable name or apply some value modifications when returning the value. Changing the new function to just receive the carried goods, thus having an exact definition of input too. (Notice the entState.trader && entState.trader.goods check is not needed, as GUIInterface.js always provides the latter.) The file Templates.js thus is also more legitimately extended, as the arguments follow the GetEntityState format like the other functions of that file.
• foo > 0 should be replaced by foo checks, if foo is non-negative. similar foo==0 should become !foo (here: loot > 0)
• getLootTooltip -> Merging those 2 loops: Unneeded variable loots. Unneeded term (loots[type] || 0) which will alwas be zero (since there is only 1 selected entity). Merging those:

  let templateLoot = 0;
  if (template.loot)
  	templateLoot = template.loot[type] || 0;
  

  Moving lootTypes definition down a bit.
• Renaming

totalResourcesAndGoodsCarried to totalCarrying resourcesAndGoodsCarried to carrying calculateResourcesAndGoodsCarried to calculateCarriedResources

• Replacing var with let
• Using && in the Looter component, since that returns the second value if the first one is true
• Removing Loot comment from calculateResourcesAndGoodsCarried since that is also used to display the non-loot carrying resources tooltips
• Removing two other unneeded comments (code became nice enough to read)
• Adding some spaces: {"foo": "bar"} -> { "foo": "bar" }
• Removed whitespace in emptylines and trailing whitespace.

[elexis]

In 18738:

Loot balancing overhaul and cleanup. Based on patch by fatherbushido, refs #4193.

Add missing loot entry to many new buildings that were added after r11524 and ships.
Give civic centers and wonders a significant amount.
Food related structures contain some food.
Temples somehow yielded 500 metal loot previously.
Remove experience point loot from non-defensive buildings.
Merge loot for cavalry and hero templates.
Swordsmen and sword cavalry loot 5 metal, slingers 5 stone, other units 5 wood.
Remove some duplicate Looter, MiniMap, Selectable and TrainingRestriction entries.
Remove pointless sentence "Special Building." from persian hall tooltip.