Page MenuHomeWildfire Games

Generalise hard-counter tooltips.
AbandonedPublic

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

Details

Reviewers
elexis
Feldfeld
Stan
Trac Tickets
#4130
Summary

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

Test Plan

Verify that the tooltips for counters are displayed correctly.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 8915
Build 14629: Vulcan BuildJenkins

Unit TestsFailed

TimeTest
0 msJenkins > cxxtest-debug-gcc6.xml::[failed-to-read]
Failed to read test report file /zpool0/trunk/cxxtest-debug-gcc6.xml org.dom4j.DocumentException: Error on line 1 of document : Content is not allowed in prolog. Nested exception: Content is not allowed in prolog. at org.dom4j.io.SAXReader.read(SAXReader.java:482)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
wraitii commandeered this revision.Sep 1 2019, 11:45 AM
wraitii updated this revision to Diff 9572.
wraitii edited reviewers, added: Stan; removed: wraitii.

Rebase on top of D2092, which passed attack bonuses as parts of attack effects.

I've profiled this on Combat-Demo Huge, by selecting the maximum amount of units, it takes 100ms the first time, and then an average of 5ms per Gui update.

This is not negligible, but it's right in that region where maybe it doesn't actually need to be cached ?

Anybody else - feel free to commandeer this.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/31/display/redirect

wraitii added inline comments.Sep 1 2019, 11:50 AM
binaries/data/mods/public/gui/common/tooltips.js
824

Accidentally left this in

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 389| 389| function getRepairTimeTooltip(entState)
| 390| 390| {
| 391| 391| 	return sprintf(translate("%(label)s %(details)s"), {
| 392|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 392|+		"label": headerFont(translate("Number of repairers:")),
| 393| 393| 			"details": entState.repairable.numBuilders
| 394| 394| 		}) + "\n" + (entState.repairable.numBuilders ?
| 395| 395| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 390| 390| {
| 391| 391| 	return sprintf(translate("%(label)s %(details)s"), {
| 392| 392| 			"label": headerFont(translate("Number of repairers:")),
| 393|    |-			"details": entState.repairable.numBuilders
|    | 393|+		"details": entState.repairable.numBuilders
| 394| 394| 		}) + "\n" + (entState.repairable.numBuilders ?
| 395| 395| 		sprintf(translatePlural(
| 396| 396| 			"Add another worker to speed up the repairs by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 391| 391| 	return sprintf(translate("%(label)s %(details)s"), {
| 392| 392| 			"label": headerFont(translate("Number of repairers:")),
| 393| 393| 			"details": entState.repairable.numBuilders
| 394|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 394|+	}) + "\n" + (entState.repairable.numBuilders ?
| 395| 395| 		sprintf(translatePlural(
| 396| 396| 			"Add another worker to speed up the repairs by %(second)s second.",
| 397| 397| 			"Add another worker to speed up the repairs by %(second)s seconds.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 411| 411| function getBuildTimeTooltip(entState)
| 412| 412| {
| 413| 413| 	return sprintf(translate("%(label)s %(details)s"), {
| 414|    |-			"label": headerFont(translate("Number of builders:")),
|    | 414|+		"label": headerFont(translate("Number of builders:")),
| 415| 415| 			"details": entState.foundation.numBuilders
| 416| 416| 		}) + "\n" + (entState.foundation.numBuilders ?
| 417| 417| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 412| 412| {
| 413| 413| 	return sprintf(translate("%(label)s %(details)s"), {
| 414| 414| 			"label": headerFont(translate("Number of builders:")),
| 415|    |-			"details": entState.foundation.numBuilders
|    | 415|+		"details": entState.foundation.numBuilders
| 416| 416| 		}) + "\n" + (entState.foundation.numBuilders ?
| 417| 417| 		sprintf(translatePlural(
| 418| 418| 			"Add another worker to speed up the construction by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 413| 413| 	return sprintf(translate("%(label)s %(details)s"), {
| 414| 414| 			"label": headerFont(translate("Number of builders:")),
| 415| 415| 			"details": entState.foundation.numBuilders
| 416|    |-		}) + "\n" + (entState.foundation.numBuilders ?
|    | 416|+	}) + "\n" + (entState.foundation.numBuilders ?
| 417| 417| 		sprintf(translatePlural(
| 418| 418| 			"Add another worker to speed up the construction by %(second)s second.",
| 419| 419| 			"Add another worker to speed up the construction by %(second)s seconds.",
|    | [NORMAL] ESLintBear (space-before-function-paren):
|    | Unexpected space before function parentheses.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/session/selection_panels.js
| 763| 763| 				addResearchToQueue(data.item.researchFacilityId, t);
| 764| 764| 			})(tech);
| 765| 765| 
| 766|    |-			button.onPressRight = (t => function () {
|    | 766|+			button.onPressRight = (t => function() {
| 767| 767| 				showTemplateDetails(
| 768| 768| 					t,
| 769| 769| 					GetTemplateData(data.unitEntStates.find(state => state.id == data.item.researchFacilityId).template).nativeCiv);

binaries/data/mods/public/gui/session/selection_panels.js
|  48| »   »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
|  59| »   »   switch·(data.item)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.

binaries/data/mods/public/gui/session/selection_panels.js
| 731| »   »   »   »   »   »   switch·(entity.check)
|    | [NORMAL] ESLintBear (default-case):
|    | Expected a default case.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/540/display/redirect

Nescio removed a subscriber: Nescio.Sep 1 2019, 11:53 AM

Given the work at D2138 / D2218, which will add even more stuff to the attack tooltips, I think we have to consider redoing them entirely in context, sometimes after this patch.
It would probably be beneficial to take more whitespace for clarity, such that the tooltip would read something like:

Melee Attack:
   Damage: 5 Hack, 3 Pierce
   Rate: 0.8s
   Bonuses: 3x against Infantry, 4x against Whatever
Capture Attack:
   Capture: 4
   Rate: 0.8s
   Bonuses: 2x against Buildings

This might then accommodate icons instead of words, which would perhaps be more readable.
We could also write this shorter as such:

Attack:
   Melee: 5 Hack + 3 Pierce Damage, 5 Capture, Rate 0.3s Counters: Infantry (3x), Cavalry (2x)
   Capture: 3 Capture, Rate 0.8s, Counters: Building (1.5x)

Maybe we need shorter versions for the in-game tooltips. Bit of a conundrum.

Stan added a comment.Sep 1 2019, 12:06 PM

Didn't you like remove half the code and FeldFeld name from the credits ? Also you forgot the Kushite template and some other as well...

In D1707#93246, @Stan wrote:

Didn't you like remove half the code

Yes, it wasn't necessary after D2092

and FeldFeld name from the credits ?

FeldFeld name is still in there?

Also you forgot the Kushite template and some other as well...

I'm pretty sure I rebased from the latest diff.

Stan added a comment.Sep 1 2019, 12:41 PM

I guess the diff comparator failed then :D

Freagarach updated this revision to Diff 9632.Sep 5 2019, 9:23 AM
Freagarach retitled this revision from Hard-counter tooltips to Generalise hard-counter tooltips..
Freagarach edited the summary of this revision. (Show Details)
Freagarach edited the test plan for this revision. (Show Details)

Ugly-coded prove of concept for Melee Attack: X hack, Rate: Y second(s) (Zx vs Classes):


Code will be improved after D2138.

Freagarach commandeered this revision.Sep 5 2019, 9:23 AM
Freagarach added a reviewer: wraitii.
Vulcan added a comment.Sep 5 2019, 9:25 AM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/76/display/redirect

Vulcan added a comment.Sep 5 2019, 9:29 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 412| 412| function getRepairTimeTooltip(entState)
| 413| 413| {
| 414| 414| 	return sprintf(translate("%(label)s %(details)s"), {
| 415|    |-			"label": headerFont(translate("Number of repairers:")),
|    | 415|+		"label": headerFont(translate("Number of repairers:")),
| 416| 416| 			"details": entState.repairable.numBuilders
| 417| 417| 		}) + "\n" + (entState.repairable.numBuilders ?
| 418| 418| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 413| 413| {
| 414| 414| 	return sprintf(translate("%(label)s %(details)s"), {
| 415| 415| 			"label": headerFont(translate("Number of repairers:")),
| 416|    |-			"details": entState.repairable.numBuilders
|    | 416|+		"details": entState.repairable.numBuilders
| 417| 417| 		}) + "\n" + (entState.repairable.numBuilders ?
| 418| 418| 		sprintf(translatePlural(
| 419| 419| 			"Add another worker to speed up the repairs by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 414| 414| 	return sprintf(translate("%(label)s %(details)s"), {
| 415| 415| 			"label": headerFont(translate("Number of repairers:")),
| 416| 416| 			"details": entState.repairable.numBuilders
| 417|    |-		}) + "\n" + (entState.repairable.numBuilders ?
|    | 417|+	}) + "\n" + (entState.repairable.numBuilders ?
| 418| 418| 		sprintf(translatePlural(
| 419| 419| 			"Add another worker to speed up the repairs by %(second)s second.",
| 420| 420| 			"Add another worker to speed up the repairs by %(second)s seconds.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 434| 434| function getBuildTimeTooltip(entState)
| 435| 435| {
| 436| 436| 	return sprintf(translate("%(label)s %(details)s"), {
| 437|    |-			"label": headerFont(translate("Number of builders:")),
|    | 437|+		"label": headerFont(translate("Number of builders:")),
| 438| 438| 			"details": entState.foundation.numBuilders
| 439| 439| 		}) + "\n" + (entState.foundation.numBuilders ?
| 440| 440| 		sprintf(translatePlural(
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 2 tabs but found 3.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 435| 435| {
| 436| 436| 	return sprintf(translate("%(label)s %(details)s"), {
| 437| 437| 			"label": headerFont(translate("Number of builders:")),
| 438|    |-			"details": entState.foundation.numBuilders
|    | 438|+		"details": entState.foundation.numBuilders
| 439| 439| 		}) + "\n" + (entState.foundation.numBuilders ?
| 440| 440| 		sprintf(translatePlural(
| 441| 441| 			"Add another worker to speed up the construction by %(second)s second.",
|    | [NORMAL] ESLintBear (indent):
|    | Expected indentation of 1 tab but found 2.
|----|    | /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/gui/common/tooltips.js
| 436| 436| 	return sprintf(translate("%(label)s %(details)s"), {
| 437| 437| 			"label": headerFont(translate("Number of builders:")),
| 438| 438| 			"details": entState.foundation.numBuilders
| 439|    |-		}) + "\n" + (entState.foundation.numBuilders ?
|    | 439|+	}) + "\n" + (entState.foundation.numBuilders ?
| 440| 440| 		sprintf(translatePlural(
| 441| 441| 			"Add another worker to speed up the construction by %(second)s second.",
| 442| 442| 			"Add another worker to speed up the construction by %(second)s seconds.",
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/585/display/redirect

Freagarach updated this revision to Diff 9665.Sep 8 2019, 1:03 PM
Freagarach edited the summary of this revision. (Show Details)
Freagarach edited the test plan for this revision. (Show Details)

Rebased after D2138/rP22866.

Owners added a subscriber: Restricted Owners Package.Sep 8 2019, 1:03 PM
Vulcan added a comment.Sep 8 2019, 1:05 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/96/display/redirect

Vulcan added a comment.Sep 8 2019, 1:08 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/605/display/redirect

Stan added a comment.Oct 29 2019, 1:08 PM

Anything left to do here ?

Rebase and probably the inline is what this needs.

binaries/data/mods/public/gui/common/tooltips.js
280–284

It seems to me we need a nicer way to add variably present tooltips, like Status Effects and Counters.

Freagarach updated this revision to Diff 11253.Feb 2 2020, 8:50 AM
  • Rebased.
  • Generalises the tooltip creation a bit.
Vulcan added a comment.Feb 2 2020, 9:03 AM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1697/display/redirect

elexis requested changes to this revision.Feb 3 2020, 11:57 AM
elexis added inline comments.
binaries/data/mods/public/gui/common/tooltips.js
266

missing translate

267

"%(statusEffects)s" thats weird, either a missing translate or something else

276

missing translation

also one should not hardcode spaces or operators in english language, but use translated sprintf strings to concatenate strings (the one exception the code currently has are newlines).

350

The extractor won't be happy about translate("(" + otherstring), the expected input is translate("foo") or translate(expr), as in either a string literal or an expression without any string literals.

And thats not only an implementation bug, but there use case should be implemented using a translated sprintf strings. This way the ( and ) and order of arguments can be changed in the translation as well.

This revision now requires changes to proceed.Feb 3 2020, 11:57 AM
Freagarach updated this revision to Diff 11354.Feb 14 2020, 8:15 PM
Freagarach marked 4 inline comments as done.

Improve translations.

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...
Executing section JS...
|    | [NORMAL] ESLintBear (operator-linebreak):
|    | '||' should be placed at the end of the line.
|----|    | /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|    |++++| /zpool0/trunk/binaries/data/mods/public/globalscripts/Templates.js
|  87|  87| 		// If the elements are still strings, split them by space or by '+'
|  88|  88| 		if (typeof sublist == "string")
|  89|  89| 			sublist = sublist.split(/[+\s]+/);
|  90|    |-		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1)
|  91|    |-		                    || (c[0] != "!" && classes.indexOf(c) != -1)))
|    |  90|+		if (sublist.every(c => (c[0] == "!" && classes.indexOf(c.substr(1)) == -1) ||
|    |  91|+		                    (c[0] != "!" && classes.indexOf(c) != -1)))
|  92|  92| 			return true;
|  93|  93| 	}
|  94|  94| 

binaries/data/mods/public/globalscripts/Templates.js
|  91| »   »   ····················||·(c[0]·!=·"!"·&&·classes.indexOf(c)·!=·-1)))
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1761/display/redirect

Freagarach updated this revision to Diff 12715.Jul 16 2020, 1:09 PM
  • Rebased.
  • Get properly translated properties.
  • Properly translate classes. (Also multiple classes.)
Owners added a subscriber: Restricted Owners Package.Jul 16 2020, 1:09 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2662/display/redirect

Nescio added a comment.EditedJul 16 2020, 1:28 PM

How do you intend to render classes? Classes tend to be singular, but in tooltips they ought to be pluralized (e.g. Hero, Spearman, Siege, Structure → "Heroes", "Spearmen", "Siege Engines", "Structures"). Moreover, combinations can cause difficulty, e.g.:

  • Cavalry+Melee → "Melee Cavalry"
  • Champion Hero → "Champions or Heroes"
  • Unit+!Organic → "Ships and Siege Engines"

Have a look at https://trac.wildfiregames.com/wiki/EnglishStyleGuide#Terminology
Also iber_champion_cavalry.xml, mace_hero_alexander.xml (see D2610).

There is indeed no clever parsing. As is it is just the translated class, like the visible classes. With multiple classes, they are separated by a comma. E.g. 3x vs Cavalry, Structure.

Freagarach planned changes to this revision.Jul 16 2020, 1:38 PM

It didn't include my changes in Attacking.js.

Freagarach updated this revision to Diff 12718.Jul 16 2020, 1:48 PM

Include Attacking.js change.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2132/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2665/display/redirect

Freagarach updated this revision to Diff 12719.Jul 16 2020, 1:53 PM

Include bonus of Alex and iber champion cav.

It may prove difficult then to correctly translate classes on the fly.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/2133/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2666/display/redirect

Freagarach updated this revision to Diff 12720.Jul 16 2020, 1:57 PM

Fix tests.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2667/display/redirect

Freagarach updated this revision to Diff 12721.Jul 16 2020, 2:02 PM

Revert Iber champion cavalry, since those have no bonus.

There is indeed no clever parsing. As is it is just the translated class, like the visible classes.

In other words, the automatically generated tooltips will be inferior.

With multiple classes, they are separated by a comma. E.g. 3x vs Cavalry, Structure.

Let's have a look at a few different combinations of Cavalry and Archer:

<Bonuses>
  <A>
    <Classes>Cavalry Archer</Classes>
    <Multiplier>2</Multiplier>
  </A>
</Bonuses>

A means 2× vs Cavalry and Archers.

<Bonuses>
  <B>
    <Classes>Cavalry+Archer</Classes>
    <Multiplier>2</Multiplier>
  </B>
</Bonuses>

B means 2× vs Cavalry Archers.

<Bonuses>
  <C>
    <Classes>Cavalry+!Archer</Classes>
    <Multiplier>2</Multiplier>
  </C>
</Bonuses>

C means 2× vs non-Archer Cavalry.

<Bonuses>
  <D>
    <Classes>!Cavalry+Archer</Classes>
    <Multiplier>2</Multiplier>
  </D>
</Bonuses>

D means 2× vs non-Cavalry Archers.

<Bonuses>
  <E>
    <Classes>Cavalry</Classes>
    <Multiplier>2</Multiplier>
  </E>
  <F>
    <Classes>Archer</Classes>
    <Multiplier>2</Multiplier>
  </F>
</Bonuses>

E+F means 4× vs Cavalry Archers, 2× vs other Cavalry and Archers.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2668/display/redirect

Given the decrease in readability, I think we need a way to let the template overwrite the counter tooltip.

Ideally, we'd have a brilliant way to parse these classes things, but that sounds somewhat difficult.

As a stopgap measure, I'd suggest adding new files (possibly in template_helpers? Possibly elsewhere) that are basically a list of Class combinations and a human-readable name. This might ultimately replace our VisibleClasses logic somewhat.

If you somehow find a way to make it work, expand this patch to also generate tooltips for <RestrictedClasses>; see e.g. template_unit_dog.xml.

Given the decrease in readability, I think we need a way to let the template overwrite the counter tooltip.

Then why bother generating them automatically?

I like the idea behind this patch, but I believe there are simply too many conceivable cases and exceptions to make it work properly. The same is true for an earlier suggestion to automatically generate aura descriptions and technology tooltips. Adjusting tooltips every time modifications are altered is annoying (and a bit more work for translators), but at least they're straightforward to review and it's easy to explain what the tooltip ought to be (hence why we have an English style guide).

On a separate note, wouldn't it be better to insert a <Tooltip> inside the <Attack/*/Bonuses> or <Attack/*/Bonuses/*> node, instead of using the <Identity/Tooltip>? Likewise, auras have their own tooltips, and there is a separate <Upgrade/*/Tooltip> node in templates.

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1033/display/redirect

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/macos-differential/1034/display/redirect

wraitii added a subscriber: Nescio.Jul 16 2020, 2:52 PM

Then why bother generating them automatically?

Sane defaults are good... Then again, I'm not sure how sane these defaults are. But hiding information entirely is definitely bad, so I'm not opposed to just showing the 'math' of the classes somewhere.

On a separate note, wouldn't it be better to insert a <Tooltip> inside the <Attack/*/Bonuses> or <Attack/*/Bonuses/*> node, instead of using the <Identity/Tooltip>? Likewise, auras have their own tooltips, and there is a separate <Upgrade/*/Tooltip> node in templates.

This however seems a good idea regardless of the rest of the patch.

Sane defaults are good... Then again, I'm not sure how sane these defaults are. But hiding information entirely is definitely bad, so I'm not opposed to just showing the 'math' of the classes somewhere.

Doing a grep shows there are only eleven templates with a bonus attack in the public folder; ten of those have a tooltip reflecting that, only one is hidden: template_unit_fauna_hunt_defensive_elephant.xml, though that animal is not trainable, so I suppose it's not really important. For translators, there are only three different strings to translate: spearman, pikeman, and Alexander.
In other words, automatically generating bonus attack tooltips seem a lot of work for little benefit.

I think I'm voting towards abandoning this, but I'll let you decide.

Freagarach abandoned this revision.Aug 3 2020, 10:05 AM
Freagarach added a subscriber: wraitii.

In other words, automatically generating bonus attack tooltips seem a lot of work for little benefit.

I think I'm voting towards abandoning this, but I'll let you decide.

Agreed.

@Freagarach, care to check and commit D2610?
Also:

On a separate note, wouldn't it be better to insert a <Tooltip> inside the <Attack/*/Bonuses> or <Attack/*/Bonuses/*> node, instead of using the <Identity/Tooltip>? Likewise, auras have their own tooltips, and there is a separate <Upgrade/*/Tooltip> node in templates.

This however seems a good idea regardless of the rest of the patch.