Enemy spawned with 0 HP are broken and undead / disagreements about what enemies exist/are alive
rversteegen opened this issue · 4 comments
If an enemy is spawned starting with 0 HP or die-without-boss, then loadfoe will partially initialise the enemy, then stops and calls dead_enemy before setting the sprite or many other things. As a result, if there was previously an enemy in the same slot it reappears with its old sprite. guo sent me a testcase (the battle ends the moment the enemy spawns, and it sticks around during victory) but we should be able to create a simpler one.
dead_enemy marks the formation slot as empty with formdata.slots(enemynum).id = -1, but few places in the code check slots(...).id to determine whether a hero/enemy exists, more commonly they check .vis and/or .dissolve. I can see a couple simple ways to fix this specific sprite problem, but that doesn't fix the underlying problem, that the code is incredibly inconsistent about which enemies exist or are alive, so there are probably other symptoms. Heroes are even worse!
SUB loadfoe(...)
loadenemydata .enemy, formdata.slots(slot).id, YES
setup_non_volatile_enemy_state bspr
reset_enemy_state bspr
'--Special handling for spawning already-dead enemies
IF allow_dead = NO THEN
'enemies which spawn already-dead should be killed off immediately
'die without boss or 0 hp?
IF dieWOboss(4 + slot, bslot()) OR .enemy.stat.hp <= 0 THEN
'rewards and spawn enemies on death
'enemy is only partially constructed, but already have everything needed.
dead_enemy 4 + slot, -1, bat, bslot(), formdata
EXIT SUB
END IF
END IF
setup_enemy_slice bspr, bat
'--Position
.basepos = formdata.slots(slot).pos
...
Related to #663 (that bug isn't about enemies which start dead), #975, #43, #1007
In particular, reset_enemy_state sets .vis = YES but check_death is one of those places that ignores slots with .id = -1, so vis = NO is never reached.
I've said before that .vis is a big problem, it has different meanings in different places. I think we should replace various state variables (which can have invalid combinations of assignments), with a single enumeration of possible states, probably being: Empty, Dead (in future, not just heroes but also enemies), Appearing, Dying (including fleeing), On_Death (reached 0 HP but waiting on an on-death attack), Pending_Death (has 0 HP but hasn't been processed for death yet), Alive.
The single enumeration is a good idea! I like it!
One other state that should possibly be part of the same enum might be Jump/Hide for when an enemy is untargetable because of a jump attack (I know that was the reason for at least some of the inconsistency of using .vis in place of death checks)
I was resisting putting Jump in there due to the existing jumping bug #1107. But thinking more carefully, I'm going to say that being hidden shouldn't be another state, because being hidden should be synonymous with the sprite slice being invisible (again, it's a bad idea to store the same state in multiple variables which can become inconsistent). And it could be possible for the sprite to be invisible as part of a Appear or Death animation, or as part of an on-death (even an on-death Jump attack!)
Also, to reduce bugs due to state not having been updated to Pending_Death yet after the HP reaches 0, maybe the state enumeration should be returned from a method call.
Giving it some actual testing, there's a host of things that go wrong with enemies spawned with 0 HP. First of all, they don't dissolve, unlike enemies in a formation which start with 0 HP at the start of battle. They're targetable although they're dead, though they don't prevent the battle from ending. Sometimes you can kill them (apparently if they spawned in a previously occupied slot; and then they do fade out) and sometimes not. Also, several times I saw that attacking one caused my attack targets to change to spread all, but then it stopped happening when I reduce my hero's Str (this was in a modified test.rpg).