fix(Core/Creature): `SelectLevel` hooks return `IsDead() = True` during respawn #17712

kjack9 · 2023-11-15T01:09:03Z

Changes Proposed:

This PR proposes changes to:

Core (units, players, creatures, game systems).
Scripts (bosses, spell scripts, creature scripts).
Database (SAI, creatures, etc).

When a Respawn() occurs, SelectLevel() is currently called before the creature's DeathState is updated to DeathState::JustRespawned. This has the strange effect of calling the OnBeforeCreatureSelectLevel() and Creature_SelectLevel() hooks when the creature->IsDead() function returns True. It also makes it very difficult for a module to determine what is a spawned-dead creature vs a respawning creature.

This PR creates a new DeathState::Respawning that is set when the respawn process has started but the actions associated with DeathState::JustRespawned aren't due to happen yet.

This solution should be fairly low-risk since it only affects the DeathState during SelectLevel() and some smaller functions, and I don't see anything in there that relies on DeathState. The hooks would be affected, but they should see improved accuracy with IsDead().

Another option I considered was moving DeathState::JustRespawned() to before SelectLevel(). While it seemed to work okay, I am concerned about some of the function calls in setDeathState when the state is DeathState::JustRespawned and how they'll react if SelectLevel() hasn't been called again.

Alternatively, we could just remove the SelectLevel() call entirely for respawning, since that function was presumably called when the creature was created the first time.

Interested in hearing what everyone thinks.

Issues Addressed:

Closes Bug: Kael'thas Sunstrider Advisors not scaling mod-autobalance#172

SOURCE:

The changes have been validated through:

Live research (checked on live servers, e.g Classic WotLK, Retail, etc.)
Sniffs (remember to share them with the open source community!)
Video evidence, knowledge databases or other public sources (e.g forums, Wowhead, etc.)
The changes promoted by this pull request come partially or entirely from another project (cherry-pick). Cherry-picks must be committed using the proper --author tag in order to be accepted, thus crediting the original authors, unless otherwise unable to be found

Tests Performed:

This PR has been:

Tested in-game by the author.
Tested in-game by other community members/someone else other than the author/has been live on production servers.
This pull request requires further testing and may have edge cases to be tested.

How to Test the Changes:

This pull request can be tested by following the reproduction steps provided in the linked issue
This pull request requires further testing. Provide steps to test your changes. If it requires any specific setup e.g multiple players please specify it as well.

Known Issues and TODO List:

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

Kitzunu

Everything about this feels weird.

DeathState::Respawning does nothing here. And if it were to be used it should be added into the switch at Creature.cpp L#610
If the module hook is the issue, then I would highly prefer you change where and how the hook is called instead of changing core functionality to fix a module.

kjack9 · 2023-11-15T18:07:28Z

Good discussion, thank you for replying.

The issue in the core is that SelectLevel() has different states for IsDead() depending on whether a creature is being respawned or not. When the creature is first created, the hooks see IsDead() = false but if the creature is respawned they see IsDead() = true, even if the creature never died (and was force respawned). As a module author, I cannot tell if this creature is actually dead or not until the first OnAllCreatureUpdate(). That seems like a core issue to me since the hooks are reporting inconsistent information to modules.

I think the hooks are in the right place - before and after stat generation from the creature's template. I'm not sure where they could be moved that they wouldn't be affected by this anymore.

What did you think of my other proposed solutions? I only picked this one because it had the lowest chance of breaking something else. Do we really need to run SelectLevel() and re-apply stats from the creature template during a respawn event? The creature already has its stats determined and applied, does it dying alter it in a way that those stats need to be recalculated? Any changes made by a module would be overwritten by this call, meaning that the same changes need to be made again. If a module/script really wanted that behavior they could call SelectLevel() themselves.

kjack9 · 2023-11-15T21:14:26Z

I am starting to test with that SelectLevel() removed entirely - I am not sure it needs to be there. I'd like to collect some cases where creatures might have their stats/characteristics changed during combat, and on respawn need to be reset to their template values.

I'll keep a running list of things I've tested here:

Raging Colossus (19188) - Respawn OK
Turgid the Vile (27808) - throw Seeds of Nature's Wrath (37887) at him to get Weakened Turgid the Vile (27809) - Respawn OK

kjack9 · 2023-11-16T05:07:28Z

What triggered this in the initial issue was a Respawn(force=True) being called here:

azerothcore-wotlk/src/server/scripts/Outland/TempestKeep/Eye/boss_kaelthas.cpp

Line 200 in bdc970b

advisor->Respawn(true);

It's the forced respawn of an alive creature that causes this. Would you prefer I find a way to keep the hooks in SelectLevel from firing if the creature was respawned while still alive? I understand that Respawn(true) is manipulating the death state to get its magic to happen.

I'm open to suggestion here, but from the module side I don't know how to handle this differently if the hook is giving me incorrect information. Let me know what you all think.

Kitzunu · 2023-11-18T14:36:54Z

But for isDead() to be true the creature would have DeathState::Dead or DeathState::Corpse which I don't see Respawn() ever setting unless the creature was not alive at first

Kitzunu · 2023-11-18T14:37:24Z

src/server/game/Entities/Creature/Creature.cpp

+        setDeathState(DeathState::Respawning);
        m_respawnTime = 0;
        ResetPickPocketLootTime();
        loot.clear();
-        SelectLevel();

+        SelectLevel();
        setDeathState(DeathState::JustRespawned);


and this would just be a hackfix to be fair

kjack9 · 2023-11-18T17:30:14Z

But for isDead() to be true the creature would have DeathState::Dead or DeathState::Corpse which I don't see Respawn() ever setting unless the creature was not alive at first

This happens during a forced respawn, even if the creature is alive. The next Update cycle will transition them to Dead/Corpse, which is what's causing the IsDead()=true problem.

azerothcore-wotlk/src/server/game/Entities/Creature/Creature.cpp

Lines 2011 to 2014 in e90d7a2

    
           if (force) 
        
           { 
        
               if (IsAlive()) 
        
                   setDeathState(DeathState::JustDied);

Regardless, SelectLevel (IMO) shouldn't see different IsDead() values depending on whether the creature is being spawned for the first time or is being respawned. That's the root of the issue here.

and this would just be a hackfix to be fair

Which alternative would you like me to persue?

don't call SelectLevel() again when respawning creatures since stats were determined upon initial creation
move setDeathState(DeathState::JustRespawned) to just before the SelectLevel() call
create an isRespawning bool that is passed down to the hooks so they can tell if IsDead()=true actually means Respawning. This is just a different implementation of the current solution but wouldn't require a new DeathState.

kjack9 · 2023-11-20T18:39:55Z

Wanted to follow up here. I still think that having a death state between "Dead/Corpse" and "JustRespawned" solves this problem nicely with zero side effects. I am also willing to pursue any of the other options I've laid out. Let me know which direction to run.

kjack9 · 2023-11-23T02:24:49Z

My next step forward is to do a core patch for AutoBalance to resolve this issue. The patch would be the changes in this PR. I don't want to do that, especially for a module that is as widely-used as AutoBalance, but I don't see any other path forward at this point. I'd much rather fix it in a way that is more acceptable to the core team, but I don't know what that looks like. Let me know if a core patch is the way I should go here.

Kitzunu · 2023-11-26T14:28:33Z

Honestly, I dont know, maybe @Nyeriah got a suggestion?

Nyeriah · 2023-11-26T14:37:59Z

Is this an issue anywhere else other than Kael's adds? Because his script is bad and needs to be rewritten to begin with. Just so we don't change something in the core in order to workaround a bad script that needs to be worked either way

kjack9 · 2023-11-26T14:38:21Z

Thank you for taking a look. Because of how a forced respawn works (telling the Update function that the unit is dead even though they never died) there needs to be some way to communicate to the hooks that the unit isn't actually dead. The cleanest way I could think of to do that is to make a new DeathState ("respawning") that allows the respawn logic to work but doesn't report the unit as dead. Seems reasonable to me that between "Dead" and "JustRespawned" could be a state called "Respawning".

That being said, I've proposed a couple of alternative solutions that may be more to taste, although my preference is still the original DeathState::Respawning.

I see that this PR has conflicts now due to my other PR merging - I'm out of town at a work conference this week, but will try to find time to resolve the conflicts when I can. In the meantime, would appreciate anyone's feedback on the current solution vs the alternatives so we can get this moved forward.

TL;DR - I'm motivated to fix it. I can accept that you don't like my preferred solution. I need input on how you'd like it to be fixed.

kjack9 · 2023-11-26T14:51:58Z

Is this an issue anywhere else other than Kael's adds? Because his script is bad and needs to be rewritten to begin with. Just so we don't change something in the core in order to workaround a bad script that needs to be worked either way

Yes, anywhere a force respawn is done on a creature that is alive. I'll see if I can find another replicatable scenario that shows the issue.

kjack9 added 2 commits November 14, 2023 19:19

fix: Respawn(): Update DeathState before SelectLevel

d7638cc

Create DeathState::Respawning

061101b

github-actions bot added CORE Related to the core file-cpp Used to trigger the matrix build labels Nov 15, 2023

Move DeathState::Respawning slightly farther down

bdc970b

kjack9 changed the title ~~fix(Core/Creature): Fix SelectLevel hooks returning IsDead() = True during respawn~~ fix(Core/Creature): SelectLevel hooks return IsDead() = True during respawn Nov 15, 2023

sudlud added the Ready to be Reviewed label Nov 15, 2023

Kitzunu requested changes Nov 15, 2023

View reviewed changes

Merge branch 'azerothcore:master' into fix-force-respawn

19d2a65

Kitzunu reviewed Nov 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Core/Creature): `SelectLevel` hooks return `IsDead() = True` during respawn #17712

fix(Core/Creature): `SelectLevel` hooks return `IsDead() = True` during respawn #17712

kjack9 commented Nov 15, 2023 •

edited

Loading

Kitzunu left a comment

kjack9 commented Nov 15, 2023 •

edited

Loading

kjack9 commented Nov 15, 2023 •

edited

Loading

kjack9 commented Nov 16, 2023

Kitzunu commented Nov 18, 2023

Kitzunu Nov 18, 2023

kjack9 commented Nov 18, 2023 •

edited

Loading

kjack9 commented Nov 20, 2023

kjack9 commented Nov 23, 2023

Kitzunu commented Nov 26, 2023

Nyeriah commented Nov 26, 2023

kjack9 commented Nov 26, 2023 •

edited

Loading

kjack9 commented Nov 26, 2023

fix(Core/Creature): SelectLevel hooks return IsDead() = True during respawn #17712

Are you sure you want to change the base?

fix(Core/Creature): SelectLevel hooks return IsDead() = True during respawn #17712

Conversation

kjack9 commented Nov 15, 2023 • edited Loading

Changes Proposed:

Issues Addressed:

SOURCE:

Tests Performed:

How to Test the Changes:

Known Issues and TODO List:

How to Test AzerothCore PRs

Kitzunu left a comment

Choose a reason for hiding this comment

kjack9 commented Nov 15, 2023 • edited Loading

kjack9 commented Nov 15, 2023 • edited Loading

kjack9 commented Nov 16, 2023

Kitzunu commented Nov 18, 2023

Kitzunu Nov 18, 2023

Choose a reason for hiding this comment

kjack9 commented Nov 18, 2023 • edited Loading

kjack9 commented Nov 20, 2023

kjack9 commented Nov 23, 2023

Kitzunu commented Nov 26, 2023

Nyeriah commented Nov 26, 2023

kjack9 commented Nov 26, 2023 • edited Loading

kjack9 commented Nov 26, 2023

fix(Core/Creature): `SelectLevel` hooks return `IsDead() = True` during respawn #17712

fix(Core/Creature): `SelectLevel` hooks return `IsDead() = True` during respawn #17712

kjack9 commented Nov 15, 2023 •

edited

Loading

kjack9 commented Nov 15, 2023 •

edited

Loading

kjack9 commented Nov 15, 2023 •

edited

Loading

kjack9 commented Nov 18, 2023 •

edited

Loading

kjack9 commented Nov 26, 2023 •

edited

Loading