Details

Reviewers

Commits

rP19679: Heal range visualization.

Summary

Now that the aura range visualization option has been added in rP19519, options for displaying heal range and attack range are planned. For attack range, it is mentioned in the trac ticket that it will be important to show the range in the building placement preview, and this will likely require more changes in CmpSelectable.

There needs to be new texture defaults for both of these new range visualizations; I've modified one of Pureon's textures, but different ones will likely be needed.

Also this diff removes a redundant check for this.enabled in RangeVisualization.js, and an unused var in toggleRangeOverlay() in session.js.

Test Plan

Test that the range of healers can be displayed in the game when these units are selected, and that pressing the hotkey toggles the display of the ranges of selected healers. Verify that the healer range is updated instantly (if a healer is selected and the option is enabled) when the techs that modify their ranges are researched.

This diff that modifies template_unit_support_healer.xml can be used to verify that specifying custom textures/line thickness works as expected (when the attached textures are put under binaries/data/mods/public/art/textures/selection/):

heal_range_texture.diff2 KBDownload

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Sandarac created this revision.May 6 2017, 8:16 AM

Sandarac edited the summary of this revision. (Show Details)May 6 2017, 8:25 AM

Build is green

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

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

Harbormaster completed remote builds in B1481: Diff 1680.May 6 2017, 9:03 AM

Using a heart-shaped texture/mask:

The overlay cannot be rendered as anything other than a line, and in order to have jagged/irregular shapes for SOverlayTexturedLine overlays (or rather just rendered overlays in general) there will need to be changes somewhere in the renderer code (or the issues with quad rendering need to be fixed (#1368)).

Move the range visualization options to "General" in the options menu, and use "display" instead of "rendering" in the comment in the config file.

Sandarac edited the test plan for this revision. (Show Details)May 11 2017, 1:55 AM

Build is green

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

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

Harbormaster completed remote builds in B1634: Diff 1838.May 11 2017, 7:21 AM

There is some odd bug. If we train the mauryan healer hero, the range visualization won't be shown until selecting him the first time. Then it will be shown forever.

binaries/data/mods/public/simulation/components/Heal.js
102 ↗	(On Diff #1838)	After getting used to female auras, `0.2` seems a bit ugly, 0.15 looks better IMO. (Apparently it's appealing if the thickness increases proportional with the range)
binaries/data/mods/public/simulation/components/RangeVisualization.js
59 ↗	(On Diff #1838)	The `enabled` property has no justification for existence, has it?
95 ↗	(On Diff #1838)	Why exactly did we add the argument if the information is already contained in the function name? Shouldn'we have only one function `GetVisualRanges(type)`? A getter function should get something, so should be called `Refresh`, `Reload`, `Update` or `Regenerate`

elexis added a reviewer: elexis.May 15 2017, 5:56 PM

Thanks for the comments.

That definitely shouldn't happen for the Mauryan healer hero (I tested with basic healer units, one would think the behavior would be the same, and this leads me to think there could be something askew in cmpSelectable, as the only difference I can see is that the Mauryan healer has m_AlwaysVisible as it is a hero.)

The healer bug seems to be independent of this patch. The enabled property too.

Actually fix the hero healer bug.

This revision is now accepted and ready to land.May 25 2017, 7:34 PM

Build is green

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

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

Harbormaster completed remote builds in B1955: Diff 2187.May 25 2017, 9:01 PM

elexis added inline comments.May 26 2017, 2:19 PM

binaries/data/mods/public/simulation/components/RangeVisualization.js
58 ↗	(On Diff #2187)	The component should be agnostic of the fact that there is a hotkey. Why doesn't the range vis renderer ignore the `m_AlwaysVisible` altogether?

Sandarac mentioned this in D563: Fix hero selection rings becoming unsaturated when moving.May 28 2017, 12:30 AM

Don't save a another var to the rangevis comp that doesn't need to be saved.

Thanks. Good that we can make use of this new system!

binaries/data/mods/public/gui/manual/intro.txt
112 ↗	(On Diff #2266)	You get the honor -> https://trac.wildfiregames.com/wiki/HotKeys
binaries/data/mods/public/gui/options/options.json
128 ↗	(On Diff #2266)	Didn't we want to add %(hotkey)s?
binaries/data/mods/public/gui/session/hotkeys/misc.xml
108 ↗	(On Diff #2266)	fuzzing failed. this hotkey should be below the other range vis hotkey
binaries/data/mods/public/gui/session/session.js
1291 ↗	(On Diff #2266)	By the looks of things we just have to call these statements below when closing the options page to make that one actually work.
binaries/data/mods/public/simulation/components/RangeVisualization.js
57 ↗	(On Diff #2266)	Better avoid defaults, so that callers are required to think about the value they want to pass / enforce people being conscious of the behavior triggered
73 ↗	(On Diff #2266)	I still have trouble understanding why this line has to be this way and why all of this can't be just more straight forward. But I can confirm that this is the only term of these booleans that works.
90 ↗	(On Diff #2266)	Always pass all arguments seems cleaner