Page MenuHomeWildfire Games

Visual Aura Range
ClosedPublic

Authored by Sandarac on Mar 19 2017, 7:42 AM.

Details

Summary

This diff allows displaying the aura range of selected entities. A config option is included to allow disabling the feature, and a hotkey has been added to allow toggling the rendering of aura ranges in-game (without having to open the options menu). The line texture, mask, and line thickness used can be specified in the aura's JSON. If no rangeOverlay data is specified in the aura's JSON, then the default textured line will be rendered - the line used for territory boundaries and buildings selection overlays, outline_border.png.
It is possible for an entity to have more than one range aura; currently, this seems to only be the case for ptol_hero_cleopatra.xml, and there will need to be design decisions made regarding how to display multiple range overlays. This diff will render all aura ranges an entity has.

This diff refactors the UpdateStaticOverlay function (originally used just for rendering building outlines) into a more generic function that handles textured lines. It could eventually be adapted to allow rendering more types of line overlays for entities, for things like the range of defensive buildings like fortresses/defense towers (#3915).

SOverlayTexturedLine is used mainly because the rendered lines adapt smoothly to rough terrain. The other option would be to use quad texture overlays, which allow for unique textures that are not lines (and they use less memory), but they clip severely through uneven terrain. (#1368).

This feature can also be used to help with implementing the negative farm aura around Civic Centers, although there is uncertainty if this is something that is wanted (#4342).

Eventually aura range could be added to building previews.

See also lots of design discussions at https://code.wildfiregames.com/P25#164

Test Plan

When the config option is enabled, range auras will be rendered.

The file names of this spiral overlay (a rough version created by Pureon) and its mask can be added to the JSON of a range aura - as a demonstration of a non-default line texture.

These can be placed under "binaries\data\mods\public\art\textures\selection".

Then this diff can be applied giving the Macedonian hero Craterus the spiral texture to the aura range:

The result should look something like this:

The default texture looks more like this:

Diff Detail

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
elexis added inline comments.Apr 15 2017, 2:28 PM
binaries/data/mods/public/simulation/components/GuiInterface.js
900

Agree with what you @told me, that this is a counterintuitive place to do these player checks. It's to avoid displaying the range of selected enemies? So it's a design question. Notice we see all other information of enemies, all their attack stats and health bars, so maybe it's okay to also display the aura range when selecting the enemy unit. (Selecting it has the disadvantage that one can't command one's own units meanwhile). One might also argue that other information could be hid as well then.

Also I'm not sure whether we should display the aura range at all when hovering a unit with the cursor as opposed to only displaying it for selected units.

source/simulation2/components/CCmpSelectable.cpp
668

The other loops don't have the const

Sandarac updated this revision to Diff 1263.Apr 15 2017, 3:43 PM

Address elexis' comments.

Sandarac marked 6 inline comments as done.Apr 15 2017, 3:54 PM
Sandarac added inline comments.
binaries/data/mods/public/simulation/components/Auras.js
70

I'm not sure about that. I think the current name is okay.

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

I think the aura range could/should be shown when selecting non-allied entities, as the selection overlays are shown and so it might be counter-intuitive to not show the aura range. For similar reasons I think that the ranges should be shown when hovering over ents, not just when selecting them.

binaries/data/mods/public/simulation/components/RangeVisualization.js
16

A Map is used in a similar way to this in cmpStatusBars (which is what this new component is based on).

25

Init() is called in Deserialize() in components like StatusBars and Identity.

source/simulation2/components/CCmpSelectable.cpp
668

The other loops modify the range overlay data.

Build is green

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

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

mimo added inline comments.Apr 15 2017, 5:56 PM
binaries/data/mods/public/simulation/components/GuiInterface.js
900

I'd rather not show them for non-allied entities (as well as other stats currently shown as mentionned by elexis): from them, an experienced player would be able to deduce what are the techs the enemy has already researched and adapt its strategy to it. IMO that's cheating.

If we really want to show something for enemies, it should only be the raw stats and ranges (i.e. without applying the ValueModifications), but that may complicate the code with no real gain.

Sandarac marked an inline comment as done.Apr 15 2017, 6:13 PM
Sandarac added inline comments.
binaries/data/mods/public/simulation/components/GuiInterface.js
900

Well, then maybe another diff could be proposed that disables displaying the stats and range visualizations of enemy units. It would be counter-intuitive and inconsistent (at this point) to show all of the same stats of enemy entities as player-owned entities, but not the range visualization.

mimo added inline comments.Apr 15 2017, 6:35 PM
binaries/data/mods/public/simulation/components/GuiInterface.js
900

The point is to decide what we want (showing or not such info for the enmies), and this should be fast.
And then, either we decide to still show them and the patch is fine.
Or we decide not to show them, and we should disable them in this patch (+ creating a ticket to remove the enemy stats from tooltips which i agree should not be part of this ticket). In such a case, i don't think trying to be consistent with a behaviour we want to get rid of would be sane.

Sandarac planned changes to this revision.Apr 17 2017, 4:08 AM
fatherbushido resigned from this revision.Apr 23 2017, 6:08 PM

Some hotkey design evaluation:

@causative has proposed a patch to actually toggle the status bars on tab in #4534,
i.e. if tab is pressed once, it will show the status bars of all units owned, if pressed another time, it will display them as usual. It seems indeed more useful this way.

@mimo would this satisfy the usage you had in mind with regards to the hotkey (i.e. displaying auras only for short moments)?

The female inspiration aura is a bit noisy, the line thickness could be reduced - or (assuming the answer is yes) perhaps we could not display auras entirely unless the the hotkey boolean is toggled?

The idea to add a hotkey to only show aura ranges of selected units could equally be combined with a hotkey to show the health status bars of only the selected units (in another quite simple ticket).

Currently we only have heroes, relics, female inspiration and temple auras to display.
I agree with mimo that I don't want to see female inspiration aura for the most part of the game and thus would be forced to disable the option, while there are situations where I want to see the aura (so we can't just disable aura vis for females).
(So (if the answer to the above question is yes) then the config option would have to determine whether the aura range is seen when selecting units)

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

I don't mind if that one GUIInterface playerID check in L900 is added back again (even if it is not consistent with the Aura Icons), left as is or if a more broad check is added.

Wouldn't it be important to have a way to display auras even if the aura giver is not selected (like toggle tab)?

Was wondering whether the code should be in CCmpSelectable or whether it could be moved to a new overlay renderer component that might also be controlled by Selectable, perhaps making Selectable a JS component then even, but that is not in the scope of this diff.

I'd be willing to accept this iteration or a slightly advanced iteration and fix some things afterwards.

Sandarac updated this revision to Diff 1618.May 4 2017, 12:22 AM

Use a playerID check, use onSimulationUpdate() in session.js.

Vulcan added a comment.May 4 2017, 1:09 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/981/ for more details.

elexis requested changes to this revision.May 4 2017, 11:16 PM

When toggling the config option in the menu, it doesn't refresh the selection, but who cares, it's fixed with the next selection change.

About the hotkey discussion:

  • Testing D395 with this patch strengthened my impression that we need a hotkey to reveal the aura even if the unit is not selected.
  • Asked mimo in a PM since we disagreed on the hotkeys before:
I agree that I would disable the aura visualization if it was shown each time when I select a unit (especially annoying with females) and that there should be a hotkey to toggle the visualization.
Wouldn't it be useful if we could visualize auras of all units on the screen temporarily, even if not selected? This way we could also move the units into the range of the aura provider.
Independently, causative proposed to change the tab hotkey to actually toggle in #4534 (so pressing tab once means showing all health bars persistently until tab pressed again).
Wouldn't this perfectly cover all use cases if the tab hotkey would equally trigger aura visualization, i.e.

that would be fine, with possibly adding some combination for the (rare) cases we would need only one of them: for example tab+b would trigger only bars and tab+a would trigger only aura ranges.
But i would see that independant of the config which would then just be show or not aura when selecting
But i agree that other permutations are also possible and it is maybe now better to commit something (even if not our final choice) so that people can test and complain if not satisfied :) rather than discussing the choice at length.

So it seems it seems hotkeys should work this way to please everyone:

  • A hotkey to toggle visualization upon selection (as proposed currently)
  • A hotkey to toggle visualization of all units on the screen independent of selection and independently of the config option (a separate hotkey that just happens to be Tab by default, or not)

We already discussed and coded the solution

last night/morning
Now the only thing missing is the hotkey working correctly.

binaries/data/config/default.cfg
78

Since we only have to change 3 lines (default.cfg, options.json, session.js), I'd rather not add [renderer] here and have every other renderer option not be part of renderer.
Since we now have a hotkey to enable and disable it quickly it seems to more a user interface option than a renderer one

79

aurarange/ toggleauraoverlays
Better: aurarange/ toggleaurarange

167

"of selected units and structures", since it should be obvious to the user what it does (and since we don't show it for the building preview) and since it will be ignored for the other planned hotkey (that coincidentally will be on Tab by default, to show all auras on screen)

rendering -> display
Refs that not being a renderer option anymore

binaries/data/mods/public/gui/manual/intro.txt
109

" of selected units and structures"
remember to update the Hotkeys wiki page accordingly.

binaries/data/mods/public/gui/options/options.json
216

of selected units and structures

binaries/data/mods/public/gui/session/input.js
1844

Would put this above recalculateRangeOverlays in session.js. Additions to this messy file should be reserved to code that is either directly related to input processing (something with SDL keycodes or mousemovements) or functions that are very similar to the other ones.

Could have a JSdoc comment then

1847

Not doing the String() here means doing one less conversion summa summarum

1850

better two lines, at least we have it everywhere else in the GUI like it, makes it readable like a table and friendly to extend

binaries/data/mods/public/gui/session/session.js
328

Ok my assumption that this actually does make a difference if moved to OnTick was proven true, but no need to go for that, just costs performance for no reason, people can wait a turn (until it'unpaused in the worst case). People don't change the setting as often as the status bars appear

1242
  • no need for the default argument
  • The entire function is used only once, so could be inlined, especially since we can't really reuse it (the hotkey to toggle auras for specific units was discussed with causative, but if so, would have to be done differently, f.e. not take a function argument)
  • Function name could have been polished otherwise, also doesn't toggle but sets to enabled
  • After merging the two, we can do all 4 Config calls in one go
binaries/data/mods/public/simulation/components/Auras.js
88

This block looks really nice!

The defaults might or might not be moved to the aura templates someday

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

That does the same thing as EnableVisualRangeOverlayType and can be nuked. We thereby remove the way to delete entries, but that doesn't seem needed if we can disable them.

963

well it makes the line below 4 characters shorter, IMO looks nicer without the continue

binaries/data/mods/public/simulation/components/RangeVisualization.js
8

self-explanatory

66

better each argument on a separate line than 2 arguments on one line, 2 on another. Nice side effect of having them aligned

binaries/data/mods/public/simulation/templates/template_structure.xml
104

We could be more moderate and only add it to those that have an aura. But this going to be extended to attack and heal, so meh

binaries/data/mods/public/simulation/templates/template_unit.xml
68

source/simulation2/components/CCmpSelectable.cpp
69

fits better in the second line

80

trailing whitespace

146

196

Rename to AddRangeOverlay:

(03:12:34) elexis: renaming ResetRangeOverlays to DeleteRangeOverlays and SetRangeOverlay to AddRangeOverlay should make it more readable, right?
(03:14:21) Sandarac: I used Reset b/c that is the way it is worded in cmpoverlayrenderer
(03:15:08) elexis: that also has an AddSprite function :P
(03:15:18) elexis: -> ResetRangeOverlays AddRangeOverlay
(03:15:36) elexis: Set sounds like it mgiht modify an existing one
(03:15:41) Sandarac: ok

209

Guess m_RangeOverlayData.push_back({rangeOverlayDescriptor, rangeOverlay}); is nicer to read, since we're already constructing a temporary.

Some related discussion from 2015-08-29-QuakeNet-#0ad-dev.log

16:31 < Itms> elexis: #3293 is nice, just use emplace_back and I'll commit
16:53 < Philip`> Why would you ever want to use emplace_back?
16:53 < Philip`> All it saves you is the cost of a move constructor, if I remember correctly
16:58 < Philip`> It saves you nothing in almost all cases (since the move constructor is usually essentially free), and it's not supported by compilers that we probably care about, and its name is ugly

It landed with push_back in rP16963, nowadays we would add a reserve call (which isn't possible here)

http://en.cppreference.com/w/cpp/container/vector/emplace_back
http://www.cplusplus.com/reference/vector/vector/push_back/

271

(all range overlays or any range overlay)

345

unneeded comment

412

This is the hidden bug I've been looking for hours.
The removed comment + early return was rightfully there.
As the comment states, we don't need to update overlays of buildings each RenderSubmit call.
This can be trivially fixed by adding a pair of braces to the call:

case STATIC_OUTLINE:
if (!m_BuildingOverlay)
{
 	m_BuildingOverlay = new SOverlayTexturedLine;
 	UpdateTexturedLineOverlay(&m_OverlayDescriptor, *m_BuildingOverlay, 0, true);
}

Confirmed with a debug_sprintf on the rms_new_demo map that has only one entity (before and after the vis aura patch).

The performance impact would be entirely neglible since we never select more than a handful of buildings simultaneously, while we will now very often select many units with aura (females), but those braces are gratis this morning.

424

cmpTerrain was unused, so ok

449

A more specific name would help for example distinguishing it from the "fancy" aura rendering (where the texture spans the entire disk)

maybe UpdateTexturedLineOverlay ?

agree

465

Yep, we now need the GetInterpolatedPosition2D since units move

511

unsigned? not uint32 or similar?

source/simulation2/components/ICmpSelectable.h
46

Ok, we can reuse this indeed

65

Comment a bit pointless since it repeats the function name.
Some readers might not be aware of what a "Range Overlay" is, so adding an example for aura or attack range helps with both

70

Delete all range overlays to reduce the repetition

This revision now requires changes to proceed.May 4 2017, 11:16 PM
Sandarac updated this revision to Diff 1655.May 5 2017, 12:52 AM
Sandarac edited edge metadata.

Update with a version of the patch that actually includes the RangeVisualization component.

Sandarac updated this revision to Diff 1658.May 5 2017, 4:15 AM

Remove an uneeded continue in GuiInterface.SetRangeOverlays, remove a redundant comment in RangeVisualization.js and change the formatting of cmpSelectable.AddRangeOverlay.

Sandarac updated this revision to Diff 1659.May 5 2017, 7:57 AM

Add a missing check before the submit call in cmpselectable line 672; if the game was paused while hovering an entity with an aura overlay, then the cursor was moved away and then back onto the entity, it would crash. There are already equivalent checks for these null cases before submit calls in cmpselectable (f.e. line 663 if (m_UnitOverlay)).

Sandarac marked 16 inline comments as done.May 5 2017, 8:03 AM
Vulcan added a comment.May 5 2017, 8:30 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/1012/ for more details.

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/1014/ for more details.

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/1015/ for more details.

Sandarac updated this revision to Diff 1674.May 6 2017, 12:33 AM

Update after rP19517, and reduce the thickness for the inspiration aura so that it is not very visible unless zoomed in.

elexis accepted this revision.May 6 2017, 2:40 AM

Thanks for this highlight of alpha 22, the blood & tears and sleepless nights of design discussions since february (P25), the rewrites to change to a JS based structure as proposed by fatherbushido and sanderd17, the hotkey proposed by mimo, unifying the rendering code with the existing Selectable component code to avoid duplication and adapt the aura to terrain changes. It will be very useful to players in hero battles and we can now easily extend the code base to also display a healer and attack range. :-)

binaries/data/mods/public/gui/session/session.js
1242

{string} while atit

binaries/data/mods/public/simulation/components/RangeVisualization.js
71

thickness);, at least the other code does it like this

source/simulation2/components/CCmpSelectable.cpp
238–240

missing 1 space

262

missing space

670

That was the only null deref I could find too

This revision is now accepted and ready to land.May 6 2017, 2:40 AM
This revision was automatically updated to reflect the committed changes.
Vulcan added a comment.May 6 2017, 4: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/1027/ for more details.