#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 )
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)
Change History (24)
comment:1 by , 12 years ago
Description: | modified (diff) |
---|
comment:2 by , 12 years ago
comment:4 by , 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 , 12 years ago
Milestone: | Alpha 11 → Alpha 12 |
---|
comment:6 by , 11 years ago
Milestone: | Alpha 12 → Alpha 13 |
---|
comment:7 by , 11 years ago
Milestone: | Alpha 13 → Backlog |
---|
comment:8 by , 11 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:9 by , 11 years ago
Milestone: | Backlog → Alpha 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 , 11 years ago
Attachment: | Damagev2.diff added |
---|
Diff of all changes to hopefully fix the last problem (including test functions)
by , 11 years ago
Attachment: | Damagev2_notest.diff added |
---|
Diff of all changes to hopefully fix the last problem (without test functions)
comment:10 by , 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 |
comment:11 by , 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 ofnew Object()
, so the first line should bevar 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 bereturn 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.
comment:12 by , 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?
comment:13 by , 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.
comment:14 by , 11 years ago
r12829 only prevents units damaged by splash damage (with friendly fire) to attack back.
by , 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 , 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?
comment:16 by , 11 years ago
Keywords: | patch review removed |
---|---|
Milestone: | Alpha 14 → Backlog |
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:18 by , 10 years ago
Milestone: | Backlog → Alpha 15 |
---|
In 11966: