Opened 7 years ago

Last modified 4 years ago

#4595 new defect

Promotion while dying

Reported by: elexis Owned by:
Priority: Should Have Milestone: Backlog
Component: Simulation Keywords:
Cc: Patch:

Description

In this replay sent by borg-, one of his cavalry unit dies at 6m 10s and is promoted to the advanced rank simultaneously. So the old entity played the death animation and the new entity idles in the same place. In this case the promotion animation was not played.

A similar/same bug was reported and reportedly fixed in #3544.

Attachments (5)

commands.txt (271.6 KB ) - added by elexis 7 years ago.
ticket4595.patch (595 bytes ) - added by mimo 7 years ago.
possible fix
ticket4595-A22.patch (617 bytes ) - added by mimo 7 years ago.
version of the patch for A22 as the timer internal structure changed (array -> object)
commands.2.txt (4.7 KB ) - added by elexis 7 years ago.
Minimal replay for svn. (easy to reproduce by clicking on the promotion button when that unit is killed with a missile)
debug_output.patch (3.6 KB ) - added by elexis 7 years ago.
(To reproduce above annotation)

Download all attachments as: .zip

Change History (23)

by elexis, 7 years ago

Attachment: commands.txt added

comment:1 by fatherbushido, 7 years ago

That's strange, the first time I launch it, I got (on player perspective) I got an oos on turn 33 and that

ERROR: JavaScript error: simulation/helpers/Commands.js line 335
TypeError: cmpTechnologyManager is null
  g_Commands.research@simulation/helpers/Commands.js:335:7
  ProcessCommand@simulation/helpers/Commands.js:47:3
ERROR: Failed to call ProcessCommand() global script function

on turn 58

(In fact I get oos on different turns when I change perspective to player)

The replay shows well the issue (entity 1758 which is promote to 1959 if I checked well).

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:2 by mimo, 7 years ago

I've just tried it, without getting any error? The problem is in fact because of the delay due to the MissileHit: the arrow is sent on 1758 before the promotion, and reach it after.

Then the fact that the unit stays idle is only because we don't have the promotion animation for cav.

by mimo, 7 years ago

Attachment: ticket4595.patch added

possible fix

comment:3 by elexis, 7 years ago

(The error fatherbushido had was caused by revealing the map with a simulation command from the dev overlay).

The function looks good.

Sould it be moved to the most recent component that manages the arrow and saved that timer? (Still seems to be the Attack entity component, so it could just save the timer ID in the SetTimeout call afaics).

Adding subscription might have a performance impact, but maybe we can't get around it, maybe it's not that drastic (worst case are probably those unit techs where a class of units promotes, like macedonian champion infantry).

in reply to:  3 comment:4 by mimo, 7 years ago

Replying to elexis:

Sould it be moved to the most recent component that manages the arrow and saved that timer? (Still seems to be the Attack entity component, so it could just save the timer ID in the SetTimeout call afaics).

If doing something like that, the timer id should be saved in a component linked to the target so that only a OnEntityRenamed would be enough, otherwise all entities would need to listen to all EntityRenamed with a OnGlobalEntityRenamed.

In addition, we would need an array of timer id (not really needed currently, but in case somebody make a mod with machine guns).

Adding subscription might have a performance impact, but maybe we can't get around it, maybe it's not that drastic (worst case are probably those unit techs where a class of units promotes, like macedonian champion infantry).

I agree that we may have a perf impact, but as Timer is a system component and we don't have so many renamed, that should be very small. And this change is much cleaner than what is discussed above.

comment:5 by fatherbushido, 7 years ago

I wonder if we should keep the attack (I guess it won't do any damage anyway as the (new) target is cheering). It would be strictly wrong but in practice perhaps good enough.

(Moving the old unit out of world just before putting it in the destroy queue could not work too?)

comment:6 by mimo, 7 years ago

Yes for that specific case, but the problem is more general than that: any missile send just before the target rename is currently lost. When the rename is due to promotion, as you said it does not matter because of the cheering (except if some mods have missile delay longer than the cheering time). But the rename could also be a catapult pack/unpack, a structure upgrade or whatever else, and in that case, the missile should reach its target.

by mimo, 7 years ago

Attachment: ticket4595-A22.patch added

version of the patch for A22 as the timer internal structure changed (array -> object)

comment:7 by fatherbushido, 7 years ago

(Yes, it's best to do it strictly right)

comment:8 by fatherbushido, 7 years ago

I wonder if we shouldn't do also another thing (quelque chose en plus) (for example with splash damage, we could have that old unit with the dying animation too?)

comment:9 by mimo, 7 years ago

you mean if the old entity is selected in the range query in case of splash damage? I don't know if that could happen, but if yes, putting (in addition) the old entity out-of-world the time it is effectively destroyed, as you proposed above, could do it.

comment:10 by fatherbushido, 7 years ago

There is indeed two problems:

  • the old entity is still here (in world and in sim) for some times
  • the target of the attack is not moved from the old one to the new one

But solving the first one would in fact solve the second one. As if we miss the target (it will be the case as it's out of world), we will checked if we didn't hit another target at the impact location (in the damage code).

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:11 by fatherbushido, 7 years ago

Ah I am actually not exactly right due to the L126-L128 block of Damage.MissileHit()

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:12 by fatherbushido, 7 years ago

Though we should in fact get rid of that block and use another query radius at L132 of Damage.MissileHit()

let ents = this.EntitiesNearPoint(Vector2D.from3D(data.position), targetPosition.horizDistanceTo(data.position) * 2, cmpPlayer.GetEnemies());

(which was perhaps the meaning of that TODO: handle dead target properly)

Last edited 7 years ago by fatherbushido (previous) (diff)

by elexis, 7 years ago

Attachment: commands.2.txt added

Minimal replay for svn. (easy to reproduce by clicking on the promotion button when that unit is killed with a missile)

comment:13 by elexis, 7 years ago

I can confirm the observation:

Turn 130 (200)...
WARNING: SetTimeout MissileHit 197 with 24.397900282769513HP
WARNING: SetTimeout MissileHit 197 with 24.397900282769513HP
WARNING: Health.Reduce 197 amount 5.815546132094651 to 18.58235415067486HP
WARNING: Health.Reduce 197 amount 5.815546132094651 to 12.766808018580209HP
WARNING: Health.Reduce 197 amount 5.815546132094651 to 6.9512618864855575HP
Turn 131 (200)...
WARNING: Promoting 197 having 6.9512618864855575HP
WARNING: MT_EntityRenamed 197 to 476
WARNING: Destroy entity 197 due to promotion
WARNING: SetTimeout MissileHit 476 with 8HP
WARNING: Health.Reduce 197 amount 5.815546132094651 to 1.1357157543909064HP
WARNING: Health.Reduce 197 amount 5.815546132094651 to 0HP
WARNING: Health.Reduce creates corpse and deletes entity 197
WARNING: Health.Reduce 197 amount 5.815546132094651 to 0HP

So it's different from #3544 because here the unit isn't dead when being promoted.

It seems really weird that the simulation entity is around for any longer after DestroyEntity and we can expect it to be bugged in many more cases if it isn't cleaned instantly, right?

This reminds me of the siege engine unpack segfault where the status bars were generated for a destroyed entity.

About mimo's timer patch:

Agree that it's not a bad idea to change the missile hit target

  • because the other code should be agnostic of the fact whether or not the unit is invincible when promoting (that game design decision should be easily changeable ideally)
  • and because Damage.MissileHit distinguishes between arrows that hit the target and arrows that hit a non-targetted entity.

Still not fond of doing logic of other components in the timer (should remember the timer in Attack)

Also it might not solve all cases, because that 8HP unit is still vulnerable and standing around with 8HP, so it might die to any other reason easily and play that animation in that position again.

About fatherbushido's patch: See Phab:D590 (moving the entity out of world on rename could be done in the position cmp itself instead of in every caller).

Agree that the if (!targetPosition) early return in Damage.MissileHit would have to disappear.

I don't know whether/why we need to use a different radius in EntitiesNearPoint of Damage.MissileHit, but I assume we would want to hit the closest entity?

I guess it would also be possible to make that function agnostic of the actual target and just hit whatever first (or a random) of these entities that is in that position regardless.

by elexis, 7 years ago

Attachment: debug_output.patch added

(To reproduce above annotation)

comment:14 by fatherbushido, 7 years ago

I don't know whether/why we need to use a different radius in EntitiesNearPoint of >Damage.MissileHit, but I assume we would want to hit the closest entity? I guess it would also be possible to make that function agnostic of the actual target and just hit >whatever first (or a random) of these entities that is in that position regardless.

Sure that's what is currently done, just we must choose some (not so) arbitrary radius with pros and cons.

EDIT: To be more clear (I will ignore splash damage).

We check if we hit the target, if so we damage it.

Else we did a query around the arrow impact and did damage to the first encountered ent if there is one. (we need a radius for that query, currently it's twice the distance between the arrow impact and the target location, which is something with pros and cons. More cons than pros.)

Last edited 7 years ago by fatherbushido (previous) (diff)

comment:15 by elexis, 7 years ago

Milestone: BacklogWork In Progress
Patch: Phab:D590

comment:16 by fatherbushido, 6 years ago

In 20393:

Move out of world promoted, packed and upgraded entity as they are not destroyed immediately. So they don't interact with the 'physical' world. Refs #4595.
Differential Revision: https://code.wildfiregames.com/D590
Reviewed By: wraitii

comment:17 by Imarok, 5 years ago

Component: UI & SimulationSimulation

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

comment:18 by Silier, 4 years ago

Milestone: Work In ProgressBacklog
Patch: Phab:D590

no one working on it currently nor opened diff

Note: See TracTickets for help on using tickets.