Opened 12 years ago

Closed 10 years ago

Last modified 10 years ago

#1496 closed defect (fixed)

[PATCH] Ranged attack bugs when targeting garrisoning, formation, or dead units

Reported by: historic_bruno Owned by: Josh
Priority: Should Have Milestone: Alpha 15
Component: UI & Simulation Keywords:
Cc: Jonathan Waller Patch:

Description (last modified by Jonathan Waller)

There's a few bugs related to #18 / r11886, reproducible as follows:

  • Error: a unit is fired upon while garrisoning. Ranged enemy units approach and attack some of your units. Meanwhile garrison your units in e.g. towers. If there's enough attackers and enough of your units, by chance a missile will be fired while your units are garrisoning, when it "hits" the entity has gone off-world (into the tower).
  • Error: units are fired upon while in formation. Select several units and move them around an enemy ranged unit (towers work well) without stopping, until they get hit. This causes the null footprint error: the target entity is a "special/formation".
  • Attack fails: a unit dies while being fired upon. To reproduce this it helps to have several towers with units garrisoned inside, firing upon numerous enemy units.

The first and third cases need a solution where the target calculation is independent of whether the target entity still exists in-world (perhaps with a temporary entity created for that purpose as Philip originally suggested?) Otherwise we potentially ignore friendly fire and splash damage.

The second case should never happen, it's not possible for a player to target a formation but only a single entity in a formation. So something is going wrong with the target selection.

Attachments (6)

Damagev2.diff (17.6 KB ) - added by Josh 11 years ago.
Diff of all changes to hopefully fix the last problem (including test functions)
Damagev2_notest.diff (15.5 KB ) - added by Josh 11 years ago.
Diff of all changes to hopefully fix the last problem (without test functions)
Damagev2_Updated.diff (16.9 KB ) - added by Josh 11 years ago.
Diff updated to latest SVN + small fixes
Damagev3.diff (14.1 KB ) - added by Josh 11 years ago.
Latest version; no recompile needed.
Damagev4.diff (14.0 KB ) - added by Josh 11 years ago.
Latest version, fixed issues found by alpha123.
Damagev5.diff (23.1 KB ) - added by Josh 11 years ago.
Latest version, fixed issues found by historic_bruno and fixed some whitespace issues in Attack.js.

Download all attachments as: .zip

Change History (24)

comment:1 by historic_bruno, 12 years ago

Description: modified (diff)

comment:2 by Jonathan Waller, 12 years ago

In 11966:

Fix the attack error due to no footprint. Refs #1496.

comment:3 by Jonathan Waller, 12 years ago

In 11967:

Added IsInWorld check to attack code. Refs #1496

comment:4 by Jonathan Waller, 12 years ago

Description: modified (diff)

The second occurred because the code checks all enemy units near where the arrow hit for collisions, so non targeted entities like the formation controller get tested.

comment:5 by Kieran P, 12 years ago

Milestone: Alpha 11Alpha 12

comment:6 by Kieran P, 11 years ago

Milestone: Alpha 12Alpha 13

comment:7 by Kieran P, 11 years ago

Milestone: Alpha 13Backlog

comment:8 by Josh, 11 years ago

Owner: set to Josh
Status: newassigned

comment:9 by Josh, 11 years ago

Milestone: BacklogAlpha 14

I'm hoping to fix this by alpha 14, see #1910 for the new component which abstracts the splash origin from targeted units. I should be able to adapt the code to use this new interface and fix the remaining bugs on this ticket.

by Josh, 11 years ago

Attachment: Damagev2.diff added

Diff of all changes to hopefully fix the last problem (including test functions)

by Josh, 11 years ago

Attachment: Damagev2_notest.diff added

Diff of all changes to hopefully fix the last problem (without test functions)

comment:10 by sanderd17, 11 years ago

Keywords: patch review added
Summary: Ranged attack bugs when targeting garrisoning, formation, or dead units[PATCH] Ranged attack bugs when targeting garrisoning, formation, or dead units

by Josh, 11 years ago

Attachment: Damagev2_Updated.diff added

Diff updated to latest SVN + small fixes

by Josh, 11 years ago

Attachment: Damagev3.diff added

Latest version; no recompile needed.

comment:11 by alpha123, 11 years ago

I haven't actually tried this patch (I can't really find a way to reproduce this ticket's issues and I've been busy trying to try the multiplayer lobby), but after a brief code review:

  • We use {} instead of new Object(), so the first line should be var Damage = {}; (You were also missing a semicolon.)
  • You have inconsistent spacing around operators, sometimes you use var pony='waffles'; while other times you have spaces around it. The object literal on line 80 of Damagev3.diff has odd spacing as well.
  • Superfluous parenthesis on line 27, only data.radius * data.radius needs parens.
  • Correct me if I'm wrong which is quite possible here, but shouldn't the comment on line 39 read, "z axis?"
  • You don't need the == true on line 76.
  • There's a spelling error on line 131; you missed an apostrophe (sorry, this is a pet peeve of mine).
  • In the 3 vector functions at the end you don't need the parenthesis around the return value (i.e. return (p1.x * p2.z - p1.z * p2.x); should be return p1.x * p2.z - p1.z * p2.x;).

So mostly just fix the wildly inconsistent spacing all over the place and a few other minor errors. I only have style complaints, the code itself looks good.

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

by Josh, 11 years ago

Attachment: Damagev4.diff added

Latest version, fixed issues found by alpha123.

comment:12 by historic_bruno, 11 years ago

Damage.EntitiesNearPoint doesn't have equivalent behavior to CmpAttack.GetNearbyEntities, the latter only queried all (non-gaia) players if a friendlyFire parameter was true, otherwise it would query enemies. The friendlyFire property is apparently ignored by the patch. Should gaia be included in the query?

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

comment:13 by Josh, 11 years ago

historic_bruno: It should honor the friendlyFire XML value in Attack.js now. Unfortunately though, I think the existing code discards that data anyway (I can't remember exactly where).

Edit: Changeset changeset #12829 seems to be the culprit.

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

comment:14 by leper, 11 years ago

r12829 only prevents units damaged by splash damage (with friendly fire) to attack back.

by Josh, 11 years ago

Attachment: Damagev5.diff added

Latest version, fixed issues found by historic_bruno and fixed some whitespace issues in Attack.js.

comment:15 by historic_bruno, 11 years ago

The latest patch still doesn't appear to work correctly, it sets playersToDamage if friendly fire is false but never passes that variable to CauseSplashDamage. Also I think it's more logically consistent to test !friendlyFire than to test if it equals false.

The comment explaining Player.GetEnemies() isn't correct, and is it an intentional change to make gaia entities susceptible to splash damage?

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

comment:16 by Josh, 11 years ago

Keywords: patch review removed
Milestone: Alpha 14Backlog

My current patch seems to have some severe bugs relating to UnitAI behavior that I haven't been able to figure out. Therefore I'm moving this to the backlog.

comment:17 by JoshuaJB, 10 years ago

Resolution: fixed
Status: assignedclosed

In 14231:

Fix #1496 with a revised version of my patch. Also moves damage-related functions from Attack.js to a seperate file in the global namespace.

comment:18 by Josh, 10 years ago

Milestone: BacklogAlpha 15
Note: See TracTickets for help on using tickets.