Notification stating being attacked by an ally after capturing

[elexis]

Reproduce:

1) Start a game with one ally and one enemy 2) Open the developers overlay and change the perspective to an enemy 3) Attack your CC with that enemy ram 4) Change the perspective to your ally and capture the ram

The ally will receive a notification stating that he was attacked by you, which was triggered by the CC attacking the ram. Maybe the arrow was fired before the ram was captured, but hit the ram after the successful capture?

The attack stops immediately after that, so it is not really a problem. Unless we discover a more severe variety of this bug, the only reason worth to fix it is the player being confused by the notification.

---

Initially reported by Stan in a game on r17103, where he received the notification on turn 3630 after the ram of the purple bot (entity 7581) was captured by ffm. attachment:replay.7z:ticket:3481​

[elexis]

Minimal replay to reproduce the issue with r17103. Change the perspective to "You".

[elexis]

Alpha 21 replay where I get a message that I'm being attacked by siole at turn 2852 / 23m 46s. I captured an enemy barracks and sioles cavalry arrived, starting to capture when the ownership changed.

[elexis]

2 minute r19137 replay where that chat message occurs for the blue player after a blue player capturing a barracks finished the process while his allied cavalry reached the building

[elexis]

By changing skirmisher cavalry damage to a lot of crush damage and setting allowCapture to false, we can see that this bug can also inflict actual damage to the newly captured building (for example destroy a civic center that had only few HP left).

Executing turn 602 of 671
WARNING: "OnGlobalOwnershipChanged ({entity:558, from:3, to:1})"

Executing turn 603 of 671
Executing turn 604 of 671
WARNING: Attack.prototype.PerformAttack@simulation/components/Attack.js:448:7
UnitAI.prototype.UnitFsmSpec.INDIVIDUAL.COMBAT.ATTACKING.Timer@simulation/components/UnitAI.js:2017:8
FSM.prototype.ProcessMessage@simulation/helpers/FSM.js:274:12
UnitAI.prototype.TimerHandler@simulation/components/UnitAI.js:3976:2
Timer.prototype.OnUpdate@simulation/components/Timer.js:120:4

Executing turn 605 of 671
Executing turn 606 of 671
Executing turn 607 of 671
Executing turn 608 of 671
WARNING: AttackDetection.prototype.OnGlobalAttacked ({attacker:547, target:558, type:"Ranged", damage:36.950631100998635, attackerOwner:2})

An easy solution would be to add an ownership test to canAttack, but that sounds like performance heavy for a bug that occurs rarely. Reseting the Timer might be better.

[fatherbushido]

(situation in r19137 and now are a bit different as the way damage is taken into account is not the same).

The fact that the launched attack/missile hurt the unit captured meanwhile is delicate.

Maybe the arrow was fired before the ram was captured, but hit the ram after the successful capture?

Yes. There is the check when performing the attack but not for one case when dealing damage.

// Deal direct damage if we hit the main target
if (this.TestCollision(data.target, data.position, lateness))
{
  this.CauseDamage(data);
  cmpProjectileManager.RemoveProjectile(data.projectileId);
  return;
}

Here we must check if the attackerOwner is an ennemy of the data.target (or the converse) at this point of the simulation. But here it's a matter of design.

Then you should also decide if you want to be notify by ally damage (when friendly fire is enabled if the above is changed or in general if not) here (if the first is not changed then you should also pass that friendly fire boolean if you want to not be noticed in that case, but want to be in the friendly fire case).

// Don't register attacks dealt by myself
if (atkOwner == playerID)
  return;

[elexis]

I'm not sure whether my ram analysis above was correct, perhaps there was capturing instead of attacking going on. At least the one stack trace above handles both cases and was tested to produce the notification with capturing and attacking.

I'd say we should show that notification if and only if damage is dealt by the ally. Capturing by an ally doesn't sound like harm and is usually not notified.

The incentive to remove the notification in non-friendly-fire cases is to avoid confusion that was reported each time people became aware of the notification.

[fatherbushido]

elexis: so you think that the damage should not be dealt* (in that case nothing to change to notification, if you choose that in case of a true friendly fire, which we have not in the game, player should be notify).

*When we attack an ennemy wich is not an ennemy when we deal the damage.

So we'll add the check in first part of code above.

[elexis]

Yes, I'd rather add a simulation test to prevent units from attacking allies if friendly fire is disabled, as determined by the templates. Still consider the performance impact (as many units can fighting), perhaps we can reset some timer and abort things onownershipchange?

Also s/ennemy/player

[fatherbushido]

elexis: If I understand correctly (with the code) it seems the units *attack* his ennemy but *damage* his ally (can only happens with ranged units). (the ally and the ennemy is the same entity).

• edit: then the fix is simple.
• edit: in the log, the warn are added by you? (entity filtered?)
• edit: that one disturbs me ​http://trac.wildfiregames.com/ticket/3484?cnum_edit=6#comment:1 There wasn't PerformAttack before turn 602?

[fatherbushido]

Well I tested that one ​http://trac.wildfiregames.com/changeset/19137 and didn't see the issue with damage. (In fact there is no damage dealt here) So for that one it's not what I expected. That one is related to the capture part. (I will look but it seems ok to fix).

I will check for the stan issue for wich all my previous remarks was but it seems really improbable, so it should be fixed in a21. And I prefer that ;-)

[fatherbushido]

So for your 2 replays, when the unit captures back, the MT_attacked is sent. So before sending him, we should add a check. Edit: not exactly that. The not tested attached patch should fix that. (Notice that cmpCapturable.Reduce(strength, attackerOwner) perform the capture and return the taken cp).

[fatherbushido]

Forget all I wrote about the projectile stuff, I was a bit mislead by the reports.

[fatherbushido]

Well the attached patch fixes the issue but I'll look if it indeeds not hide something wrong with capture. You'll notice that in your first try you don't get the message and you get it in the second try. For the first one cp = [0,400,0,0] as it's cp = [0,395.5,0.5,0] for the second one.

EDIT: Executing turn 602 of 671 WARNING: cp [0, 399.5, 0, 0.5] taken cp: 0.5 Executing turn 603 of 671 Executing turn 604 of 671 Executing turn 605 of 671 Executing turn 606 of 671 WARNING: cp [0, 399.5, 0.5, 0] taken cp: 0.5 WARNING: attacker owner: 2 return of reduce function: 0.5

So as we have still the 0.5 point from player 3 (ennemy) remaining, the player 2 recapture them back and so the message is sent.

But for example if someone (ennemy) capture something then go away then someone (the same, or someone else, ally or ennemy) capture again then the MT_Attacked message is sent but AttackDetection doesn't spam us with another attack alert, so we (almost) never notice that when an ally capture back there is an attack notification.

Imo the patch above is so the expected fix.

[fatherbushido]

(If the first stan issue still exist, I can't reproduce it (the attack - damage issue) and it's unrelated to the one of the last replay.)

And independantly, in any case the MT_Attacked message should be sent we capture points back of an ally structure (wich is something *allowed*), whatever happened after in AttackDetection.

Or perhaps I missed something.

[elexis]

This replay shows that an attack notification can be triggered following a legitimate capture order and that these legitimate orders were just suppressed by the spamfilter before.

[fatherbushido]

Well I finally notice the other issue in the replay with changing the commands (the part I missed).

[fatherbushido]

(Well, it comes from the fact that sometimes we use CanCapture/CanAttack and sometimes only CanAttack (wich handles both case). Using a type argument could be something nice.)

So for our issue, with commands.3 modified with the allowCapture = false.

So: the missing check is in UnitAI.CanAttack

if (cmpCapturable && cmpCapturable.CanCapture(owner) && cmpAttack.GetAttackTypes().indexOf("Capture") != -1)
  return true;
if (IsOwnedByEnemyOfPlayer(owner, target))
  return true;

So in the replay, as the unit is "capturable" (it's a legal one), we fail in the first case.

Solution: parsing the allowCapture in UnitAI.Attack to UnitAI.CanAttack

(and also refs #4220).

EDIT: that modification should be done carefully as this.CanAttack is used many times in the UnitAI code. EDIT: really carefully. It needs to be well thought. Btw, it's perhaps better to wait a bit as there are some wip about unitMotion/unitAI rewrite. EDIT: refs also #252

[fatherbushido]

Refs r19148 for the first (in fact the second) bug.

[elexis]

Also I wouldn't be surprised if the bug in the stan replay was the bug fixed in that revision and didn't involve actual damage.

[Imarok]

Move tickets to Simulation as UI & Simulation got some sub components.

[elexis]

(In #5511 there was an actual attack with health reduction, I have not seen that prior to a24, perhaps it existed before)

[Freagarach]

In r25242: Fix entities being able to attack allied structures in rare cases.

Caused by not checking the attack type while performing an attack.

Closes: #5511
Differential revision: D3840
References #3484

(Which was reverted in r25246.)

[Freagarach]

In 25264:

Fix entities able to attack allied structures in rare cases (take 2).

Caused by not checking the attack type whilst performing an attack.

Fixes: #5511
Differential revision: D3845
Refs. #3484