Page MenuHomeWildfire Games

Hard-counter tooltips
Needs RevisionPublic

Authored by Feldfeld on Dec 28 2018, 9:21 AM.

Details

Reviewers
elexis
Stan
Trac Tickets
#4130
Summary

This adds units counter information in unit tooltip in structure tree and in game, for each types of attack a unit can have (currently no units have multiple counters for different attack types but just in case).
Adds a new line for each counter an attack can have.

Test Plan

An incoming hard counter mod by borg- should provide many test cases.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

Feldfeld created this revision.Dec 28 2018, 9:21 AM
Feldfeld updated this revision to Diff 7076.Dec 28 2018, 9:34 AM
elexis added a subscriber: elexis.Dec 28 2018, 10:10 AM
  • This should be a new function in gui/common/tooltips.js, and then used for both ingame tooltips and structree tooltips alike. (* The identity tooltip is already displayed in the tooltips I think? Oh, you're modifying that property, that's not good. The function should just return a new string.)
  • { on a separate line
  • kind should be a word that the rest of the code uses too, I guess that's attackType or something like that.
  • * Avoid untranslated strings like " :" and "x " - translations should be able to do the weirdest things, so we use sprintf(translate("Title: %(value)s), { value: "value" }) (or translate("%(multiplier)sx against %(class)s")) for such formatted strings
  • The spear infantry has a hardcoded tooltip to display the 3x bonus as I recall? It should be removed then
  • toLowerCase is not what is done elsewhere in the tooltips is it? (As usual it should be the translation that decides)
  • One may consider early returns (if !x then return; or if !x then continue) to reduce the scopes, but not inevitably necessary.
  • Add a programming.json so you appear in the credits for helping 0 A.D.!

Good addition, perhaps we already have a ticket on trac.wildfiregames.com for this.

Feldfeld updated this revision to Diff 7077.Dec 28 2018, 11:55 AM

Not sure I understood everything but there's what i modified.
I still (temporarily ?) modify the identity tooltip property, while i search for something better.

Feldfeld updated this revision to Diff 7078.Dec 28 2018, 12:00 PM

Okay, this looks much nicer.

  • if (bonus && bonus.Classes) could become a continue too, otherwise remove the braces { } if they only contain one statement.
  • Somewhere in gui/session/ and the structree folder, there is an array of tooltip function names. If you search for the tooltip function names in tooltips.js, you will find the places. You only need to insert your new function name there for the tooltip to appear.
  • the individual class names like Infantry should receive a translate call. So if we receive some sort of string encoded array, that might have to be split.
  • Template modification: That should never happen anyhow, because the functions processing the templates expect the model the way it is created. We had several bugs because tooltip functions changed the numbers in the templates for laziness. So depending on the entities and order of selection, the tooltips would show wrong numbers and it was extremely hard to notice. So we marked the templates as read-only using the deepfreeze function last release. So if there actually is a modification, it will throw errors rather than showing fake tooltips that entirely look authentic (#4257).

So basically you have found another place where a template should be marked as readonly using deepfreeze:data.Identity.Tooltip. But I suspect more code will have to be reworked, so meh. Perhaps we can add an annoying TODO comment above the variable then.

  • (translate("Counters %(attackType)s :") + somethingElse is a pattern we try to avoid in new code, but since every tooltip function uses this pattern, it would probably be better to go for consistency in this case, so nothing to be changed here.) (But in general another sprintf argument after the colon would allow translators to change the order of the words)

The only question remains how to have the tooltips information-complete without them becoming too noisy.
Intuitively one considers counters to apply to the entire damage output, not the per-attacktype one.
On the other hand there is more freedom in damage bonus expression if it's per type and mods might have unpredictable use cases.

Well, I added my function name in gui\reference\draw.js, gui\session\selection_details.js and gui\session\selection_panels.js which is where my file search lead me, but it doesn't seem to add the tooltip still. I will search some more.
Currently my code adds a new line for each attack type, but also for each counter contained in that attack type, it would maybe help to regroup counters in a line for each counters that share the same multiplier, but it would still remain quite noisy if someone wanted to regroup all possibilities in a single unit.

but it doesn't seem to add the tooltip still. I will search some more.

Did you check whehter the early return in the begin of your function is triggered?

Noisiness: One could group if all attack types have the same bonus against one class. It depends on how counters will be used whether that's worth to implement.
(I recall we had many consistency reworks of templates, techs and auras - giving the same bonus for every attack type because it was weird that one attack type was missing etc.
But I guess that's still not a sufficient argument to remove the per-attacktype distinction.)

Well, indeed it got triggered, now it's template.attack (attack in lowercase).
But after correcting this, it still doesn't work, after I try warn(uneval(template.attack)) I don't find any bonuses even if some units should have them

As mentioned few days ago, the data that is fed to the tooltip functions is the EntityState from GUIInterface.js for ingame tooltips and the template data parsed by globalscripts/ (Templates.js IIRC. Actually I looked it up now, it's in the GetTemplateDataHelper function).

The template data / entitystate constructed there only contains the information that is currently used by anything, so you will probably have to extend that place to add counter information. (Unfortunately that will make the game a bit more slower, as the data is copied to the AI for every unit and every selected unit to the GUI every turn)

(Perhaps, theoretically, the copying of the counter data through the GUIInterface for every unit could be avoided by just relying on the template data and assuming that no aura or tech can change the counter. But I have a suspicion that the code is not laid out for this.)

(In case these two files mentioned aren't clear enough already, one can also check the commit history to tooltips.js that added new tooltips for exampless where new data is pulled)

Unrelated with last comment but for the noisiness, perhaps counter could be added after the details of one AttackType, without adding new lines except if it goes out of bounds (which would most of the time).
Something like Melee attack: 3.0 Hack, 2.5 Pierce, Rate: 1.0 Seconds, Counters: 3.0 Cavalry
But the problem is that i don't see how to harmonize it aesthetically, it would certainly look weird if i want to put unit classes in small font yellow letters

Nescio added a subscriber: Nescio.Dec 29 2018, 11:27 AM

How does it show up in game? Could you post a screenshot?

Also, could you replace the x with the proper multiplication sign × (U+00D7; AltGr+= on my keyboard)? It looks better.

Special characters like × will probably fail entirely for translators, one could insert it using a sprintf argument %(x)s however (and add a translation comment what that x stands for).

i don't see how to harmonize it aesthetically

That's the story of tooltips.js. borg- proposed symbols on the forums last year, but I'm not sure if it would make it really easier in the end. It's a lot of relevant information, that's why it won't be easy for the reader regardless.

But if it's generated by script, then only the language-specific part has to be translated, right? E.g. for the Pikeman: "3.0× vs Cavalry", the "vs Cavalry" is English and has to be translated, but the "3.0×" part is probably the same in all languages.

How could you tell the probability without the knowledge about all languages?

Feldfeld updated this revision to Diff 7109.Dec 29 2018, 12:44 PM

Now it works like it should. Here are the screenshots (where i didn't bother putting my game in english). First one in vanilla, second one in Delenda Est where we can have multiple counters.



There i separate counters with ", ", i could use new lines too but that might take too much space.
Also I didn't understand for the translate calls. I know that unit classes should be translated separately (which I did in first version) since they are already translated, but then how do I do ? I suppose that putting the translate call in the object argument of sprintf is not good because it would get retranslated after (I suppose ?), and concatenating strings with sprintf is would not be good too ?

Also for the if(bonus.Classes) code, I put it there because before i saw it in DamageBonus.js code in simulation, but didn't understand why it was there, if there are Bonuses in the template there should always be at least a bonus, which should always contain Classes and Multiplier, so maybe it can just be removed, didn't try it.

Thanks for the screenshots. Perhaps you could put the counters line directly below the attack line, thus above the armour line?

The python translation script (somewhere in source/tools/l10n or i18n) scans through all source files, extracts all english strings that are marked with translate() or markForTranslation(), uploads them to transifex.com, then another script downloads the translations from transifex again and puts them into l10n/*.po files, they are loaded via C++ and the JS translate() function can then lookup the translations when the function is called for a given string. A bit more information here https://trac.wildfiregames.com/wiki/Implementation_of_Internationalization_and_Localization.
Anyhow, so, yes you can use translate(template.identity.VisibleClasses[0]) and so forth. The VisibleClasses should be translated already, so you can see if your diff got it right if they appear in french.

if there are Bonuses in the template there should always be at least a bonus

Whether an empty list of bonuses is allowed is determined by the Schema in the according simulation component. Check the first 5 lines of Attack.js, it says "zeroOrMore", so that should become 1+ (whatever syntax) if we want to change that.

Feldfeld updated this revision to Diff 7115.Dec 29 2018, 3:35 PM

Here is how it looks after moving the line down attack :


Perhaps if we want to keep it in this location we can remove word against and write unit classes in yellow ?
In Attack.js zeroOrMore applies for each bonus but i get the bonus by iterating on it so there should be no problem in removing that test, and "<element name='Classes' in Attack.js to me looks like this part is not optional if we get there in the hierarchy. So i removed the if and tried with a template that had <Bonuses></Bonuses> and it seemed to be just fine.
For translation what worried me the way I did is if translaters won't get semi-translated strings after ?

  • translateWithContext("separator for damage bonuses", ", ")
  • I suspect the "\n" ternary and the first variable can be avoided by creating an array and join() each. Array functions can be neat for this (someArray.map(item => someStringUsing(item)).join(separator)), but that's code fiddling which can be done when everything else works as intended.

About counters in general, I guess this will impact the effectiveness of petra AI...

Feldfeld updated this revision to Diff 7132.Dec 30 2018, 8:55 AM

What do you prefer ? As done in first 2 screenshots, or in these :



Or I try to mix with attack type stats, or other proposal ?

I didn't notice the difference in the two screenshots (it would help if they're in english btw).
I only noticed that the unitFont/color should only be used for units (like seconds, meters), not for Unit types.

binaries/data/mods/public/simulation/components/GuiInterface.js
412

missing semicolon
-1 tab

This is the default way, and the right way to do it assuming that auras/techs can change the counters, also the way I explained.
(The GetEntiyState function is a significant performance bottleneck, last addressed in #4322, in particular when selecting as many units as allowed (currently capped at 200)).
But perhaps one could also load the template data from the EntityState in tooltips.js to avoid this copy? Then again not sure which of the two variants will turn out to be the faster / feasible / cleaner mechanism. (So maybe we end up with this GUIInterface diff as is.)

elexis retitled this revision from Automatically updates unit tooltip to include hard counter information to Hard-counter tooltips.Dec 30 2018, 2:21 PM

Yes, classes and other texts ought to be white, not orange.

Feldfeld updated this revision to Diff 7165.Dec 31 2018, 9:14 AM

By the first 2 screenshots, i meant overall in this differential. I posted 5 in total.
If unit classes are not in yellow wouldn't it look weird if line is inbetween attack and armor tooltip, with nothing other than digits in white ? The first 2 screenshot show the result if i put it on the top, where i would say white font looks less weird but putting counters here is also not logically pertinent. In the third screenshot the line is after attack tooltip and in white(but with "against" that is unnecessary and removed now)
I didn't find the missing semi-colon.
For effectiveness of petra, true it would reduce it but it can be possible to make him train counters to his opponent units (AoM AI does it IIRC). Of course, that AI train units well doesn't mean it would use those units well. (also i think AI has a big room for improvement)
And now i just noticed i removed sprintf where i shouldn't

smiley added a subscriber: smiley.Dec 31 2018, 10:02 AM

Re: AI
With current behavior, not sure how such a thing could be done in an acceptable way. The quick and easy fix could end up being too unfair. People could attack the AI only to find it having the perfect army. An alternative option is to rely on past attack compositions. Which would of course allow people to “play” the AI. I guess the only sane option is to finally implement scouting.

Could also consider to delay reaction of the AI by a multiple of AI difficulty, with 0 for the very hard one

elexis added a subscriber: borg-.Dec 31 2018, 11:25 AM
  • The counters tooltip should be right below the attack stats probably, since it modifies the attack stats. I suppose the icons arent right to the thumbnail anymore and that's why you put it there? By the way if @borg- has an opinion he might voice it here too.
  • The word "Counters:" should be bold like the other captions.
In D1707#68254, @smiley wrote:

Re: AI

Not sure what you mean with the AI being to good, the problem I was refering to is that the AI has no maintainer anymore and won't take counters into account.

binaries/data/mods/public/globalscripts/Templates.js
220

{} can be removed since it contains only one statement
still mising semicolon x = y; (check what Vulcan posts or paste code to www.jshint.com for syntax checks (we use ES6 btw)))

binaries/data/mods/public/gui/common/tooltips.js
851

Looks nicer without the first and other variables, right?
Try map https://developer.mozilla.org/de/docs/Web/JavaScript/Reference/Global_Objects/Array/map

let bonusBody = template.attack[attackType].bonuses.map(bonus => bodyFont(...) + " " + ...)

(The bonus variable could be ommitted by using template.attack[attackType].bonuses[key] twice and adding a newline. The string variable can be inlined too with some \n and indentation).

multiplier + " " + classes should use a sprintf too I think, languages might reverse that or add characters somewhere, or more space or whatever.

The proposal is about adding a tooltip to something existing. If I was in position to attribute adjectives, I would qualify that of nice and desired (#4130).
I wonder what is the link between an AI and a tooltip.
Did the new AI read tooltip?

AI talk is about the eventuality that more hard counters are added
But the proposal itself doesn't affect AI yes

Feldfeld added inline comments.Dec 31 2018, 1:30 PM
binaries/data/mods/public/gui/common/tooltips.js
851

If I try map it fails (it says it's not a function), may it be because template.attack[attackType].bonuses is an object (not array) or i failed the syntax ?

elexis added inline comments.Dec 31 2018, 1:51 PM
binaries/data/mods/public/gui/common/tooltips.js
851

Oh, right, sorry for the wasted time. It is an object, not an array. One could use Object.keys(bonuses).map(bonusType => bonuses[bonusType].bla) but that will mean more objects are created, making this slower than your current function, so screw that array function.

elexis updated the Trac tickets for this revision.Dec 31 2018, 1:51 PM
Feldfeld updated this revision to Diff 7167.Dec 31 2018, 2:06 PM

Here's how it looks now :


Please remove the space before the colon.

Also, there is nothing between the value and the class. I'd prefer "3.0× vs Cavalry" instead of "3.0 Cavalry".

And how does it look like if you click on the tooltip "to view more information"?

There was a space in french translation so I missed it
In more information tooltip :

elexis accepted this revision.Jan 3 2019, 10:45 AM

The minimum number of bonuses should be 1, not 0, as you mentioned in the lobby. Can be done separately, but the patch implicitly relies on that change already.
I guess the evidence and me witnissing it force me to accept this and thank you for serving.

binaries/data/mods/public/gui/common/tooltips.js
838

-1 space

851

", " still needs a translateWithContext, but that can be fixed at committing stage too.
string variable can be inlined. Also makes it easier to change "Counters %(attackType)s : " to "Counters %(attackType)s : %(counters)s"

binaries/data/mods/public/simulation/components/GuiInterface.js
412

Given that we have cmpAttavck.GetBonusTemplate already, passing it through the GUIInterface is justified, despite the unfortunate performance impact. (The cmpAttack function may change the bonuses, if not now then in the future or for mods)

This revision is now accepted and ready to land.Jan 3 2019, 10:45 AM
Nescio added inline comments.Jan 12 2019, 3:19 PM
binaries/data/mods/public/gui/common/tooltips.js
838
let string = headerFont(sprintf(translate("%(attackType)s Counters: "), {
845
bonusesBody.push(sprintf(translate("%(multiplier)s× vs %(classes)s"), {
Feldfeld updated this revision to Diff 7514.Mar 3 2019, 9:03 PM

Tried to fix compatibility problem with tech but i didn't test it, also for some reasons the decimals .0 will not show up when the multiplier is an integer and this is not the unit card for some reasons

Stan added a subscriber: Stan.Mar 3 2019, 9:44 PM
Stan added inline comments.
binaries/data/mods/public/globalscripts/Templates.js
11

I didn't manage to get it work, can you try whether it works for you ?

binaries/data/mods/public/simulation/components/Attack.js
476

You forgot that change :)

Angen added a subscriber: Angen.Mar 17 2019, 5:28 PM
Angen added inline comments.
binaries/data/mods/public/simulation/components/Attack.js
476

let bonus creates local variable which is changed and forgot after every loop iteration. You are not changing nothing in template.Bonuses which you are returning later

Stan requested changes to this revision.Fri, May 10, 8:44 AM

The change in Attack.js shouldn't be there.

This revision now requires changes to proceed.Fri, May 10, 8:44 AM
smiley added inline comments.Fri, May 10, 9:33 AM
binaries/data/mods/public/simulation/components/Attack.js
476

let bonus creates local variable which is changed and forgot after every loop iteration.

Not necessarily.

var a = [{"a": 4, "b": 7}, {"a": 5, "b": 8}
for (let obj of a) obj.b += 10;
warn(uneval(a));

Objects are passed by reference. This would have been correct if you were not iterating the keys. If you print out bonus in the loop, you would only see the key names in the first object. Not the object the key corresponds to.
I have no clue about the semantics of this change. (Is this needed or not?)

Stan added inline comments.Fri, May 10, 9:37 AM
binaries/data/mods/public/simulation/components/Attack.js
476

Change is from another diff of mine :)
It allows bonus modifications from tech.

smiley added inline comments.Fri, May 10, 10:13 AM
binaries/data/mods/public/simulation/components/Attack.js
476

Ah, I see.

Stan added inline comments.Fri, May 10, 3:17 PM
binaries/data/mods/public/simulation/components/Attack.js
474

{ on new line.

476

Actually I was wrong, this is not from my patch, but it is indeed the reason why my patch and this one are not compatible. Since the multiplier do not get applied correctly here, the tooltip is always wrong.
This should do the trick I think.

if (template.Bonuses) 
{
	for (let bonusKey in template.Bonuses)
	{
		let bonus = template.Bonuses[bonusKey];
		bonus.Multiplier = ApplyValueModificationsToEntity("Attack/" + type + "/Bonuses/" + bonusKey + "/Multiplier", +bonus.Multiplier, this.entity);
	}

}