-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(Core/Creature): SelectLevel
hooks return IsDead() = True
during respawn
#17712
base: master
Are you sure you want to change the base?
Conversation
SelectLevel
hooks returning IsDead() = True
during respawnSelectLevel
hooks return IsDead() = True
during respawn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Good discussion, thank you for replying. The issue in the core is that 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 |
I am starting to test with that I'll keep a running list of things I've tested here:
|
What triggered this in the initial issue was a
It's the forced respawn of an alive creature that causes this. Would you prefer I find a way to keep the hooks in 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. |
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 |
setDeathState(DeathState::Respawning); | ||
m_respawnTime = 0; | ||
ResetPickPocketLootTime(); | ||
loot.clear(); | ||
SelectLevel(); | ||
|
||
SelectLevel(); | ||
setDeathState(DeathState::JustRespawned); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this would just be a hackfix to be fair
This happens during a forced respawn, even if the creature is alive. The next Update cycle will transition them to azerothcore-wotlk/src/server/game/Entities/Creature/Creature.cpp Lines 2011 to 2014 in e90d7a2
Regardless, SelectLevel (IMO) shouldn't see different
Which alternative would you like me to persue?
|
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. |
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. |
Honestly, I dont know, maybe @Nyeriah got a suggestion? |
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 |
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 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. |
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. |
Changes Proposed:
This PR proposes changes to:
When a
Respawn()
occurs,SelectLevel()
is currently called before the creature'sDeathState
is updated toDeathState::JustRespawned
. This has the strange effect of calling theOnBeforeCreatureSelectLevel()
andCreature_SelectLevel()
hooks when thecreature->IsDead()
function returnsTrue
. 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 withDeathState::JustRespawned
aren't due to happen yet.This solution should be fairly low-risk since it only affects the
DeathState
duringSelectLevel()
and some smaller functions, and I don't see anything in there that relies onDeathState
. The hooks would be affected, but they should see improved accuracy withIsDead()
.Another option I considered was moving
DeathState::JustRespawned()
to beforeSelectLevel()
. While it seemed to work okay, I am concerned about some of the function calls insetDeathState
when the state isDeathState::JustRespawned
and how they'll react ifSelectLevel()
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:
SOURCE:
The changes have been validated through:
Tests Performed:
This PR has been:
How to Test the Changes:
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.