talents not reseting correctly

[tibbi]

What bug does the patch fix? What features does the patch add?

There is a very old problem with talent reseting, pretty major one. Ill explain it on Blood Gorged, deep blood DK talent, spell id 61158 (not on wowhead). It has 2 spells filled in dbc, EffectTriggeredSpell0 and 2, which are also passive auras, increasing the DKs damage by 10% when he is above 75% hp. Now the problem comes, if the DK resets his talents, or its enough to switch a spec. Even tho 61158 is removed from him, those 2 triggered auras stay on him, and he still has that 10% dmg bonus. So this bug affects talents with something at EffectTriggeredSpell.

Adding the part of code that should be stick to normal talent reseting, spec switch, and also for reseting pets talents, even tho I honestly havent been searching for pet talents, that might be broke the same way. But Im pretty sure there are some.

For which repository revision was the patch created?

It was most likely always broken, and theres nothing special involved in the patch, so proly any revision.

Is there a thread in the bug report section or at lighthouse? If yes, please add a link to the thread.

I dont know about any thread about this.

Who has been writing this patch? Please include either forum user names or email addresses.

by me

diff --git a/src/game/Player.cpp b/src/game/Player.cpp
index d3ab26a..2c16667 100644
--- a/src/game/Player.cpp
+++ b/src/game/Player.cpp
@@ -23661,8 +23661,15 @@ void Player::ActivateSpec(uint8 specNum)

            for(int r = 0; r < MAX_TALENT_RANK; ++r)
                if (talentInfo->RankID[r])
+                {
                    removeSpell(talentInfo->RankID[r],!IsPassiveSpell(talentInfo->RankID[r]),false);

+                    SpellEntry const *spellInfo = sSpellStore.LookupEntry(talentInfo->RankID[r]);
+                    for (int k = 0; k < MAX_EFFECT_INDEX; ++k)
+                        if (spellInfo->EffectTriggerSpell[k])
+                            removeSpell(spellInfo->EffectTriggerSpell[k]);
+                }
+
            specIter = m_talents[m_activeSpec].begin();
        }
        else

diff --git a/src/game/Player.cpp b/src/game/Player.cpp
index 5fe16fd..d3ab26a 100644
--- a/src/game/Player.cpp
+++ b/src/game/Player.cpp
@@ -3853,8 +3853,15 @@ bool Player::resetTalents(bool no_cost, bool all_specs)

        for (int j = 0; j < MAX_TALENT_RANK; ++j)
            if (talentInfo->RankID[j])
+            {
                removeSpell(talentInfo->RankID[j],!IsPassiveSpell(talentInfo->RankID[j]),false);

+                SpellEntry const *spellInfo = sSpellStore.LookupEntry(talentInfo->RankID[j]);
+                for (int k = 0; k < MAX_EFFECT_INDEX; ++k)
+                    if (spellInfo->EffectTriggerSpell[k])
+                        removeSpell(spellInfo->EffectTriggerSpell[k]);
+            }
+
        iter = m_talents[m_activeSpec].begin();
    }


diff --git a/src/game/Pet.cpp b/src/game/Pet.cpp
index 0470378..0307cf6 100644
--- a/src/game/Pet.cpp
+++ b/src/game/Pet.cpp
@@ -1831,7 +1831,14 @@ bool Pet::resetTalents()

        for (int j = 0; j < MAX_TALENT_RANK; j++)
            if (talentInfo->RankID[j])
+            {
                removeSpell(talentInfo->RankID[j],!IsPassiveSpell(talentInfo->RankID[j]),false);
+
+                SpellEntry const *spellInfo = sSpellStore.LookupEntry(talentInfo->RankID[j]);
+                for (int k = 0; k < MAX_EFFECT_INDEX; ++k)
+                    if (spellInfo->EffectTriggerSpell[k])
+                        removeSpell(spellInfo->EffectTriggerSpell[k], false);
+            }
    }

    UpdateFreeTalentPoints(false);

[amaru]

Also spells like Primal Fury and Fiery Playback.

Players having those can reset the spec and still have their effects.

[Schmoozerd]

hm, nice catch.

Not entirely sure about the proper solution to unlearn the triggered (and their triggered) spells

For sake of recursivity I have the feeling that both Player::removeSpell and Pet::removeSpell are better places for this, but I am not familar with player spells at all.

and removeSpell is called in many places.

But as suggestion

[tibbi]

Im also not good at making proper solutions But Ive tested it, and works fine. Maybe the version of SeTM is better? https://gist.github.com/1644595

[VladimirMangos]

Expected that remove spell already remove dependent spells also... if bug exist then it in removespell code.

For normal cases i not remember like problem so in general dependent spells for like cases removed propertly.

Check .unlearn for same spells and you will see that dependent spells also unlearned.

[tibbi]

.unlearn works well, just tested. Its handled at https://github.com/mangos/mangos/blob/master/src/game/Level3.cpp#L1565 , but i dont see anything special there