Opened 11 years ago

Closed 11 years ago

Last modified 8 years ago

#1960 closed enhancement (fixed)

[PATCH] Elevation advantage for ranged units

Reported by: sanderd17 Owned by: peter
Priority: Nice to Have Milestone: Alpha 14
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description

As talked about on the forum, elevation advantages for ranged units are nice to have.

http://www.wildfiregames.com/forum/index.php?showtopic=17358

Attachments (3)

elevation_advantage.diff (22.5 KB ) - added by sanderd17 11 years ago.
Diff with the complete code changes
elevation_advantage_with_gui.2.diff (39.4 KB ) - added by sanderd17 11 years ago.
elevation_advantage_with_gui.diff (40.0 KB ) - added by sanderd17 11 years ago.
update with svn

Download all attachments as: .zip

Change History (16)

comment:1 by Kieran P, 11 years ago

Keywords: patch,review → patch review
Milestone: BacklogAlpha 14
Summary: [PATCH]Elevation advantage for ranged units[PATCH] Elevation advantage for ranged units

comment:2 by sanderd17, 11 years ago

I noticed there are still some problems with it, at a sudden point in game, I got this error:

ERROR: CCmpPosition::GetPosition called on entity when IsInWorld is false
ERROR: CCmpPosition::GetPosition called on entity when IsInWorld is false
ERROR: CCmpPosition::GetPosition called on entity when IsInWorld is false
ERROR: CCmpPosition::GetPosition called on entity when IsInWorld is false

So that still needs to get fixed.

comment:3 by sanderd17, 11 years ago

Did some more modifications. I appended RangeManager to support parabolic queries. And I also introduced parabolic length comparison in the Vector3D file.

So now (with direct queries), the building range checking is a lot better.

I still have problems with the Unit range checking though. That code still contains sqrt-s, and is calculated completely on the JS side. Mainly because the MoveToTargetRange and CheckTargetRange take target outline into account. And I haven't found the way to change that.

Btw, looking at the UnitMovement code, that contains a lot of roots (with .Length() calculation), while some cases, it could also be compared with just length comparison, instead of length calculation. Which saves an sqrt.

The patches replace the old ones, and are split per directory.

comment:4 by historic_bruno, 11 years ago

Hi, in the future when you post patches, could you create only a single patch with all the related changes? It makes it easier to review and to track updates.

Last edited 11 years ago by historic_bruno (previous) (diff)

comment:5 by sanderd17, 11 years ago

I noticed. There's no way to remove those old, is there?

comment:6 by sanderd17, 11 years ago

Cleaned up the code

  • Replaced spaces with tabs where needed
  • decided not to change FixedVector3D, and instead added a static method to RangeManager
  • renamed some methods so they have more meaningful names

Next to that, range overlay in the developer mode now shows the correct ranges (with a precision of one tile to save CPU time)

For me, it's ready as I don't think I can improve it a lot further.

Getting rid of the SQRTs in UnitAI.js would be great, but requires deep changes in UnitMotion.cpp.

by sanderd17, 11 years ago

Attachment: elevation_advantage.diff added

Diff with the complete code changes

comment:7 by sanderd17, 11 years ago

Uploaded a new file. This one includes gui changes (showing the average advantage when placing a foundation in a tooltip, and adapting the range value in the GUI to something more comprehensive).

It also contains an extension to the ranged attack XML schema that can be used. Including an example for the defense towers. If this is good, some other buildings also can get this tag (wall towers, outposts, ...).

by sanderd17, 11 years ago

comment:8 by sanderd17, 11 years ago

The previous diff was wrong (cleaned it up a bit too much).

And I forgot to check the "overwrite" checkmark. But both are the same now.

comment:9 by sanderd17, 11 years ago

As this patch contained code that split ranged attacks from melee attacks, I took the opportunity to also fix #1731.

by sanderd17, 11 years ago

update with svn

comment:10 by sanderd17, 11 years ago

I reverted my latest change for the following reasons:

  • My original patch doesn't require re-balancing, while fixing #1731 requires rebalancing as relative ranges between units and buildings change (ballistas get in reach of fortresses and similar)
  • It's better to only address one issue at a time, so that solution can be tested completely, without interference of other changes.

comment:11 by peter, 11 years ago

Owner: set to peter
Resolution: fixed
Status: newclosed

In 13626:

Give an elevation advantage to ranged units. Patch by sanderd17. Fix #1960.

comment:12 by leper, 11 years ago

In 13632:

Fix tests. Refs #1960.

comment:13 by sanderd17, 8 years ago

Keywords: review removed
Note: See TracTickets for help on using tickets.