Opened 9 years ago
Last modified 3 years ago
#3484 new defect
Notification stating being attacked by an ally after capturing
Reported by: | elexis | Owned by: | |
---|---|---|---|
Priority: | Nice to Have | Milestone: | Backlog |
Component: | Simulation | Keywords: | |
Cc: | Patch: |
Description (last modified by )
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
Attachments (5)
Change History (24)
by , 9 years ago
Attachment: | commands.txt added |
---|
by , 7 years ago
Attachment: | commands.2.txt added |
---|
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.
by , 7 years ago
Attachment: | commands.3.txt added |
---|
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
comment:1 by , 7 years ago
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.
comment:2 by , 7 years ago
(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 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;
comment:3 by , 7 years ago
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.
comment:4 by , 7 years ago
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.
comment:5 by , 7 years ago
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
comment:6 by , 7 years ago
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?
comment:7 by , 7 years ago
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 ;-)
by , 7 years ago
Attachment: | capture.diff added |
---|
comment:8 by , 7 years ago
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).
comment:9 by , 7 years ago
Forget all I wrote about the projectile stuff, I was a bit mislead by the reports.
comment:10 by , 7 years ago
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.
comment:11 by , 7 years ago
(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.
by , 7 years ago
Attachment: | commands_attack_notification_spamfilter.txt added |
---|
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.
comment:12 by , 7 years ago
Well I finally notice the other issue in the replay with changing the commands (the part I missed).
comment:13 by , 7 years ago
(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
comment:15 by , 7 years ago
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.
comment:16 by , 5 years ago
Component: | UI & Simulation → Simulation |
---|
Move tickets to Simulation
as UI & Simulation
got some sub components.
comment:17 by , 5 years ago
Description: | modified (diff) |
---|
(In #5511 there was an actual attack with health reduction, I have not seen that prior to a24, perhaps it existed before)
Minimal replay to reproduce the issue with r17103. Change the perspective to "You".