Page MenuHomeWildfire Games

Visual Attack Range
ClosedPublic

Authored by Sandarac on May 28 2017, 8:38 PM.

Details

Reviewers
elexis
Commits
rP20608: Attack Range Visualization.
Trac Tickets
#3915
Summary

D555 is required for the range to be shown when placing buildings.

The range will only be shown for ents that have a property in their xml (regardless of the config option), to limit the preview to structures, as showing the range of all units with an attack range when selected is not appealing.

Test Plan

Test that min and max attack range can be seen in-game for towers and fortresses, and that the ranges update when relevant techs are researched.

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

Sandarac created this revision.May 28 2017, 8:38 PM
elexis added a subscriber: elexis.May 29 2017, 12:50 AM

Really nice how there's about zero effort to add a new range visualization.

Does this also work for building previews?

Sentry Tower should be added.
Wall turret would seem reasonable, since it's only shown upon selection anyway.
Civic center would be really useful from a gameplay pov (for city planning and defending rushes in age 1, to plan expansions on mines),
but runs into artistical issues again.
The siege tower should have this range shown too.

Ships, archers, slingers, skirmishers are too frequently select. They would require some good hotkey to prevent the intrusiveness.

Eventually the hotkeys must become more accessible by setting some hotkeys to UNUSED in default.cfg that noone uses. (I have an idea what's missing).
Still think it were useful to have a hotkey to show them independent of selection, so that one can cycle through all visualizations with these 3 hotkeys without having to worry to select the right thing simultaneously.

binaries/data/mods/public/gui/session/session.js
807 ↗(On Diff #2290)

That's how loops with 2 elements pay off :->

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

It were nicer to read if we'd merge those 4 into one, so that we have one function returning the same format in each component, instead of 4 functions in each component.
Might render the redundancy of the RangeOverlay obsolete.

binaries/data/mods/public/simulation/components/RangeVisualization.js
33 ↗(On Diff #2290)

(If GetRange would return something falsy if there is no Ranged attack, then we could have rationalized the indexOf condition) by setting range = cmpAttack && cmpAttack.GetRange("Ranged"))

45 ↗(On Diff #2290)

Should that always return the texture for both ranges or shouldn't we pass min/max as an argument?
["min", "max"].map(m => ({ "prop1": foo(m)\n, ..., .. })) ?

46 ↗(On Diff #2290)

} as in L40, then
]);

Vulcan added a subscriber: Vulcan.May 29 2017, 2:37 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!
Checking XML files...

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

Sandarac updated this revision to Diff 2300.May 29 2017, 5:19 AM

Update and allow space in the options menu.

Sandarac updated this revision to Diff 2302.May 29 2017, 6:13 AM

Include wall tower.

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!
Checking XML files...

http://jw:8080/job/phabricator/1407/ 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!
Checking XML files...

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

elexis updated the Trac tickets for this revision.Nov 7 2017, 12:41 PM
elexis accepted this revision.Dec 7 2017, 5:19 PM

What a great patch and we didn't commit it since May!

Template changes are almost complete, I checked for all <Attack> instances in XML files.
Missed rome_army_camp.
It is a civil building, but as long as it fires 20 arrows or more, it is defacto tactically used as a military building. And in nearly everygame it shoots with it's full capacity. Players should be informed.
I still share the belief expressed in the comment from May.

About the order, I propose to make it the last property, since it only describes the presentation, not the simulation.

The female inspiration aura is a more problematic case than showing the ranges of all buildings with ranged attacks.
Perhaps we should just add a "female inspiration aura" config option ultimately and disable it by default but keep it visible to players who really want it.
Same could be added for the structures here.
I wonder if it should be settable for ships too! We can change templates to our preference easily the functional discussion of the simulation and serialization was closed.

In rP19861 we fixed a bug that healers had their unmodified ranges seen after deserialization which was due to the init order.

Since the RangeVisualization component is deserialized before the TechnologyManager, upgraded healers had shown a wrong range.

The same is true here!
But then again rP20299 removed the hardcoding, so the fix is already applied implicitly.

Here the rebased patch (tower template names changed) with the CC displayed, the range != 0 filter and the update only on range change fix:

I will leave the ticket open a bit in case there are complaints about visualizing the CC, or not visualizing ships or anything else about the code.

We actually should add textures so that we can easily distinguish attack from aura ranges. I have some zig-zag texture in mind.

Too bad that you Sandarac aren't here currently to witness the finishing of the project you had started long ago!

binaries/data/mods/public/gui/options/options.xml
17 ↗(On Diff #2302)

This hunk became unneeded with the tab buttons.

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

I guess that becomes self-evident when working with the range overlay code next time.

131 ↗(On Diff #2302)

That's a dependent clause without the main clause.

binaries/data/mods/public/simulation/components/RangeVisualization.js
40 ↗(On Diff #2302)

-1 tab
missed a filter for !range

112 ↗(On Diff #2302)

That should check for maxRange minRange, so that we ignore attack strength changes for instance.

This revision is now accepted and ready to land.Dec 7 2017, 5:19 PM

It would be great to have a different texture (ping @Stan @Nescio)

About the Civic Center, I believe visualizing it has two use cases:

  • planning where to build buildings to keep them covered by arrow fire
  • how to defend ideally

This is true at all stages of the game (rush in village phase, tower phases in age 2 and huge armies in age 3).
(But if someone disagrees we can maybe continue the discussion where we left off so long ago)
(I couldn't find the irclogs)

Thanks again for this awesome work!
Now I just have to find the building preview patch, if there is one (I recall you speaking of one).

binaries/data/mods/public/gui/options/options.json
123 ↗(On Diff #2302)

-parameters nowadays

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

If one doesn't want a range visualized, then there should be no RangeOverlay.
Disabling inherited templates can be done using disable="" or similar.
So this property shouldn't exist to begin with, it was only used to avoid specifying the defaults which shouldn't be in the JS component either.

459 ↗(On Diff #2302)

Using default images and widths in JS components is bad practice. Balancing and art must be removed from the components and specified in the templates.

The Aura and Heal component have it wrong too :-/

Those 4 functions can become one just returning the template.

In fact the object used in RangeVisualization.UpdateVisualAttackRanges can be constructed here.
Well, mess.

binaries/data/mods/public/simulation/components/RangeVisualization.js
40 ↗(On Diff #2302)

Seems better to only construct one array instead of 3 (min/max + filter + map) since this is likely performance critical

112 ↗(On Diff #2302)

Was wondering if the update calls should be moved to the individual components. But these functions don't particular ask to get this additional logic added and it seems nicely separated here.

This revision was automatically updated to reflect the committed changes.
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Dec 8 2017, 2:41 PM
elexis added a comment.Dec 8 2017, 5:52 PM

There it was D555.