Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#4421 closed enhancement (fixed)

[PATCH] Minimum attack distance and spread tooltip

Reported by: elexis Owned by:
Priority: Nice to Have Milestone: Alpha 22
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by elexis)

In case a unit (f.e. ranged siege, towers without murderholes) requires a minimum distance to attack enemies, this should be added to the attack tooltip (and avoided if 0).

Furthermore in Alpha 21, the spread factor has become relevant since units don't deal damage anymore if they miss (#4241) (and they miss more than 50% depending on the distance), hence should be shown.

The schema in Attack.js defines the spread:

Radius over which missiles will tend to land (when shooting to the maximum range). Roughly 2/3 will land inside this radius (in metres).

Reported by defenderbenny and Grugnas.

Patch: https://github.com/elexis1/0ad/tree/spread_tooltip

http://trac.wildfiregames.com/raw-attachment/ticket/4421/sample.jpg

Attachments (4)

sample.jpg (45.3 KB ) - added by elexis 7 years ago.
spread_alpha21.diff (8.1 KB ) - added by elexis 7 years ago.
For alpha 21 players only.
spread_100m_comparison.html (25.8 KB ) - added by elexis 7 years ago.
d.diff (5.8 KB ) - added by fpre_O_O_O_O_O_O 6 years ago.
a23 port

Download all attachments as: .zip

Change History (27)

by elexis, 7 years ago

Attachment: sample.jpg added

comment:1 by elexis, 7 years ago

Description: modified (diff)

comment:2 by fatherbushido, 7 years ago

Requested comments:

  • sure it's a good idea
  • ok for 0 min range in templates (no techs for those ones)
  • patch looks ok.
    • we can notice that we will display the spread at max range. Perhaps we can instead display the angular or to avoid computation the spread per meters (or at 100 meters)
    • I understand why you use that GetSpread function but I m not really convinced by it (I think if we have another kind of attack with a spread entry). (I mean use GetSpread(type) instead)

Else it can be moved in review queue (as in I'll do it).

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:3 by elexis, 7 years ago

  • Yes, makes sense to display a comparable value. It's a bit unexpected though that we display something totally different from the number defined in the template, complicating the change of templates from reading the tech tree. Should the number defined in the templates also be relative to 100m range instead of maxRange (so that changing the maxRange doesn't reduce the accuracy)?
  • Not really convinced to change it (the function would receive a type argument and then ensure that it's equal to Ranged)

Pushed according commits anyway.

by elexis, 7 years ago

Attachment: spread_alpha21.diff added

For alpha 21 players only.

comment:4 by fatherbushido, 7 years ago

Indeed, there is the same modification in the templateanalyzer tool (at first I wondered why I had different values in templates and in the html table given by the tool). I m ok with both options.

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:5 by elexis, 7 years ago

So we agree that the spread values in all templates should be divided by the current maxRange and multiplied with 100 (which would also be a good moment to reconsider the values, as the maxRange probably changed more often than the spread factor)?

Keeping the rfc keyword for the min-distance tooltip (i.e. c53353e and 3bd554f).

comment:6 by fatherbushido, 7 years ago

I agree with both options (1: dividing in tooltip, 2: dividing in templates). There is just a little problem with both solutions. Currently the spread is the spread at max range but the max range is sometimes different from the one in template. So if you choose to use the newSpread, you will have to choose between newSpread * horizDistance or newSpread * horizDistance * range.max / elevationAdaptedMaxRange (the 2nd if you want to keep the current formula). (I didn't use the factor 100).

comment:7 by elexis, 7 years ago

  • Spread depending on heightbonus seems weird: In one case the unit can be X meters away without height bonus and in another case X meters away with heightbonus. Intuitively the arrow has the same chance of missing at that distance (depending on the weapon used), but the formula says otherwise. So I would suggest fixing the spread to either the templates hardcoded maxRange or to a fixed distance (like 100 meters) (or use the angular spread).
  • The factor 100 comes from the fact that it yields human-readable numbers. The reader can more easily imagine two thirds of the arrows missing the target in 100m distance by 3.5 meters and stating that the angular spread is 0.035 (might be equivalent / subjective).

comment:8 by fatherbushido, 7 years ago

(for the secon point, sure, i just meant i forgot the factor in what i wrote)

for the first point, i agree. i just wanted notice it. (go with the slope in percent, forgot angle to avoid tan computation)

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:9 by fatherbushido, 7 years ago

​c53353e: reviewed. ​3bd554f: ok but perhaps not complete (outpost, wall turret, siege wall turret). any choice is ok for me

comment:10 by elexis, 7 years ago

Keywords: review added; rfc removed

Thanks for the review!

comment:11 by fatherbushido, 7 years ago

For the min distance: again, it's ok :) (perhaps minrange 0 for outpost too) For the spread one: dividing in template should be better indeed else the patch is ok (adding that in Attack schema help). I remove review (as in accepted).

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:12 by fatherbushido, 7 years ago

Keywords: review removed

by elexis, 7 years ago

Attachment: spread_100m_comparison.html added

comment:13 by elexis, 7 years ago

Keywords: review added

Following our recent discussions about spread balancing (see attachment), I'm not fully convinced about conforming the spread to 100m. It might be more important for balancing if it actually relates to the actual max range of that unit, rather than having a value that can be compared to the other units. For example skirmishers have 24m range, so the 100m range doesn't explain well by how many meters it misses at max range. Displaying the things in a spreadsheed form as you provided is much more useful. In such a document we can display both 100m and maxRange spread and thus come to a more informed decision.

For the same reason, I'm not sure whether the spread tooltip actually informs the reader. Furthermore it scrolls the the attack rate tooltip to the next line, despite it being more important than the spread (as it reveals the number of garrisoned arrows).

The min-distance tooltip is okay, so adding back the review keyword so that someone who didn't write the patch nor did the first review can commit it in accordance with the guidelines.

Was wondering whether we should not remove but increase the min-distance in accordance with the Murder Holes technology (in AoE2 fortresses also don't shoot arrows at units at it's footprint unless that tech is researched). But it seems that is not intended for 0 A.D., otherwise units could safely destroy Civic Centers once surrounded.

Yadda, yadda, ​c53353e and 3bd554f reviewed and commitable.

comment:14 by fatherbushido, 7 years ago

Perhaps the murderhole techs could affect fortress and towers. EDIT: In that case keeping mindist for fortress and towers but not for cc

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:15 by elexis, 7 years ago

As mentioned in the last paragraph, also thought about fortresses, but do we really want it? It would yield a window of opportunity where units can surround the building and destroy it while not being attacked. If we can accept that, then adding the mindist would be nice.

comment:16 by elexis, 7 years ago

MinRange patch: still reviewable

About the spread patch:

  • Testing this patch in alpha 21 revealed that is a big mistake to have the Interval field in the line below "Ranged Attack:". It consumes too much time to read the entire tooltip if one doesn't recall that peculiarity. The Interval is a multiplier of the attack damage, thus should probably be the second attack property listed, followed by the range and perhaps spread.
  • A long comma separated concatenation results in a mandatory linebreak.
    • Commas could be replaced with linebreaks, yields 4 lines instead of 2 though
    • Indentation would help distinguish component tooltips from it's properties
    • The properties likely shouldn't be bold
  • Spread value likely more readable if it mentions the actual meters missed at maxRange. It still can be compared in a way between archers and skirmishers, even if not directly. The current spread value (distance missed at max Range) can be grasped by anyone, while normalizing to 100m range isn't applicable to skirmishers with a small range for example.

comment:17 by fatherbushido, 7 years ago

The current spread value (distance missed at max Range) can be grasped by anyone, while normalizing to 100m range isn't applicable to skirmishers with a small range for example.

Don't see it like that but like a ratio in % (tan of the angle). But agree that both has pros and cons.

EDIT: I would bend to keeping the spread at max range

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:18 by elexis, 7 years ago

Keywords: review removed

MinRange increment + MinRange tooltip patch at https://code.wildfiregames.com/D111 Ack on not changing anything with regards to spread.

comment:19 by elexis, 7 years ago

In 19360:

Minimum attack range tooltip.

Differential Revision: https://code.wildfiregames.com/D267
Reviewed By: Gallaecio
Refs #4421 D111

comment:20 by elexis, 7 years ago

Description: modified (diff)

Maybe it is useful to display the accuracy tooltip too #4530, since it has been asked for recently.

comment:22 by elexis, 7 years ago

Milestone: Work In ProgressAlpha 22
Resolution: fixed
Status: newclosed

Still not convinced that the spread tooltip provides players with actionable intelligence.

comment:23 by fpre_O_O_O_O_O_O, 6 years ago

https://imgur.com/a/0o65y Spread Hit Radius: X.X at [Maximum Range]

by fpre_O_O_O_O_O_O, 6 years ago

Attachment: d.diff added

a23 port

Note: See TracTickets for help on using tickets.