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)
Change History (23)
by , 7 years ago
Attachment: | commands.txt added |
---|
comment:2 by , 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.
follow-up: 4 comment:3 by , 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).
comment:4 by , 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 , 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 , 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 , 7 years ago
Attachment: | ticket4595-A22.patch added |
---|
version of the patch for A22 as the timer internal structure changed (array -> object)
comment:8 by , 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 , 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 , 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 try to reach the closest target in damage code.
comment:11 by , 7 years ago
Ah I am actually not exactly right due to the L126-L128 block of Damage.MissileHit()
comment:12 by , 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)
by , 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 , 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.
comment:14 by , 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.)
comment:15 by , 7 years ago
Milestone: | Backlog → Work In Progress |
---|---|
Patch: | → Phab:D590 |
comment:17 by , 5 years ago
Component: | UI & Simulation → Simulation |
---|
Move tickets to Simulation
as UI & Simulation
got some sub components.
comment:18 by , 4 years ago
Milestone: | Work In Progress → Backlog |
---|---|
Patch: | Phab:D590 |
no one working on it currently nor opened diff
That's strange, the first time I launch it, I got (on player perspective) I got an oos on turn 33 and that
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).