Page MenuHomeWildfire Games

Splash Damage shape tooltip
ClosedPublic

Authored by Imarok on Feb 13 2017, 3:58 PM.

Details

Reviewers
elexis
Commits
rP19229: Splash Damage shape tooltip
Trac Tickets
#4328
Summary

As we now have linear and circular splash damage in the game, this should be shown in the tooltip.

Test Plan

Check for Linear/Circular Splash Damage in:

  • structree tooltip ingame/mainmenu
  • trainging panel
  • single selection details panel

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

Imarok created this revision.Feb 13 2017, 3:58 PM
Vulcan added a subscriber: Vulcan.Feb 13 2017, 4:42 PM

Build is green

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

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

fatherbushido added inline comments.
binaries/data/mods/public/gui/common/tooltips.js
209 ↗(On Diff #522)

(perhaps missing spaces)

fatherbushido added inline comments.Feb 13 2017, 7:18 PM
binaries/data/mods/public/globalscripts/Templates.js
140 ↗(On Diff #522)

ok

binaries/data/mods/public/simulation/components/Attack.js
390 ↗(On Diff #522)

ok needed for GuiInterface

fatherbushido added inline comments.Feb 13 2017, 7:32 PM
binaries/data/mods/public/gui/common/tooltips.js
209 ↗(On Diff #522)

I am not able to review that part :/

Imarok added inline comments.Feb 13 2017, 7:37 PM
binaries/data/mods/public/gui/common/tooltips.js
209 ↗(On Diff #522)

I should use a global object for {"Circular": translate("Circular"), "Linear": translate("Linear")}

elexis added a subscriber: elexis.Feb 13 2017, 9:28 PM
elexis added inline comments.
binaries/data/mods/public/gui/common/tooltips.js
209 ↗(On Diff #522)

We were wondering whether the array becomes constructed again with each call (onTick) (old leper argument), but spidermonkey seems to be smart enough to cache that. However having a global object means that modders can modify that array externally outside of this file (old sanderd17 argument). Besides I believe that {"Circular": translate("Circular"), "Linear": translate("Linear")}[splash.shape] } looks ugly.
Either way I wouldn't raise a concern if any of the versions would be committed, feel free to review.

elexis requested changes to this revision.Feb 14 2017, 10:31 AM
elexis added inline comments.
binaries/data/mods/public/gui/common/tooltips.js
207 ↗(On Diff #522)

Sometimes the colon is colorized in the file, other times it isn't.
Seems indeed better to slowly but surely remove "Label:" strings, as some language might want to print the value first followed by the label.

I wonder though why we need this new string when we could reuse the existing "%(label)s: %(value)s"

This revision now requires changes to proceed.Feb 14 2017, 10:31 AM
Imarok updated this revision to Diff 534.Feb 15 2017, 5:22 PM
Imarok edited edge metadata.
Imarok marked an inline comment as done.

Change label value string and put the shape translations in a global const var.

Build is green

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

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

elexis added inline comments.Feb 15 2017, 9:26 PM
binaries/data/mods/public/gui/common/tooltips.js
20 ↗(On Diff #534)

If the accepted argument is allowing modders to extend the attack types without inserting a line of code here, that shouldn't pretend to be unmodifiable (i.e. const -> var).

213 ↗(On Diff #534)

Sorry for not having it posted before, but I wouldn't be surprised if there is some language that has issues with that string,
"Linear Splash Damage" and "Circular Splash Damage" might have a different translation grammatically

Imarok added inline comments.Feb 15 2017, 10:24 PM
binaries/data/mods/public/gui/common/tooltips.js
20 ↗(On Diff #534)

I think modders should just change this code part.
(Otherwise the two consts above are "unmoddable" too^^)

213 ↗(On Diff #534)

I don't think so, but using their own string doesn't hurt.

Imarok updated this revision to Diff 535.Feb 15 2017, 10:31 PM

elexis' remarks and const -> var

Build is green

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

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

elexis accepted this revision.Feb 15 2017, 11:39 PM

Thanks for the patch, tested, works, addressed all remarks, yadda, yadda.

Wondering whether we should remove the friendly fire tooltip as we disabled all friendly fire.
Mods could still have it enabled... perhaps it would be the best to have a global boolean showFriendlyFireTooltip, so that we can change our minds without changing the code and allow modders to enable the tip from an external file?

This revision is now accepted and ready to land.Feb 15 2017, 11:39 PM

Wondering whether we should remove the friendly fire tooltip as we disabled all friendly fire.
Mods could still have it enabled... perhaps it would be the best to have a global boolean showFriendlyFireTooltip, so that we can change our minds without changing the code and allow modders to enable the tip from an external file?

(displaying only when true)

(When the splash damage tooltip and it's friendly fire part were added in rP18477, I thought about showing the string in both cases so as to avoid confusion about the value of the property when the value is hidden. You might be right though that hiding it if it's disabled is more appropriate to our use case.)

This revision was automatically updated to reflect the committed changes.