[patch 303][7160] Profession Ability fix

[VladimirMangos]

Not sure that this is correct way. Why not use normal enable disabled spell functionality already existed in code for talent based learned spells?

In this case you know exactly known early spells.

As often i write totally wrong comment

But this comment i think more correct:

+ for (; i < PLAYER_MAX_SKILLS; i++)

+ if ( SKILL_VALUE(GetUInt32Value(PLAYER_SKILL_INDEX(i))) == skill_id ) break;

Why scan skill in inner loppp for another loop in in place when function called we know exactly skill_id that affected.

[yad02]

Why scan skill in inner loppp for another loop in in place when function called we know exactly skill_id that affected.

In fact I don't see how get "i" whitout use the for loop

[VladimirMangos]

You already have value in calling function:

new_value

[yad02]

You already have value in calling function:

Rewrite without for loop,

It works perfectly when remplacing this Player::learnSkillRewardedSpells(uint32 skill_id ) by Player::learnSkillRewardedSpells(uint32 skill_id, uint32 new_value )

[yuuzai]

will test this today...

thanks...

[VladimirMangos]

For proper work also need unlearn spell at newvalue < oldvalue.

[added]

I not see way how avoid used by you if < > for values. Ofc scan spells after each skill change not good way. But maybe move level values to array and replace if by search <> value in array.

[yad02]

I dont' see how remove old spell rank but I looking for

About hardcode "if <> tests" what do you think of this :

       uint32 SkillLevels[]={75,150,225,300,375,450};
       for(uint32 j = 0; j < sizeof(SkillLevels)/sizeof(uint32) - 1; j++)
       {
           if((SkillValue < SkillLevels[j] && new_value >= SkillLevels[j]))
           {
               learnSkillRewardedSpells( (GetUInt32Value(PLAYER_SKILL_INDEX(i)) & 0x0000FFFF), new_value);
               break;
           }
       }

I haven't test it but is it a better way ?

[VladimirMangos]

I think code more clear now

But for remove you need check reversed case also || newvalue < .... && SkillValue > ...

For remove spell can be used

removeSpell(spell_id), it will ignore unknown spells

[yad02]

Have you got an idea about how get all rank spell when you have just a spell ?

For example get the spells id for rank 1, 2, 3, when you know the spell id of the rank 4 ?

[VladimirMangos]

For example get the spells id for rank 1, 2, 3, when you know the spell id of the rank 4 ?

This possible (spellmgr.GetPrevSpellInChain(spell_id)) but why this need???

Clear you _don't_ must manually unlearn another ranks at rank learning....

Just do for example...

if (pAbility->req_skill_value > skill_value)

removeSpell(pAbility->spellId);

else

learnSpell(pAbility->spellId);

[yad02]

This possible (spellmgr.GetPrevSpellInChain(spell_id)) but why this need???

Sorry it's a mistake from me !

It should be this :

(I haven't test it, I do it in around 10hours)

Index: src/game/Player.cpp
===================================================================
--- src/game/Player.cpp    (revision 205)
+++ src/game/Player.cpp    (working copy)
@@ -4789,6 +4789,15 @@
            new_value = MaxValue;

        SetUInt32Value(PLAYER_SKILL_VALUE_INDEX(i),MAKE_SKILL_VALUE(new_value,MaxValue));
+        uint32 SkillLevels[]={75,150,225,300,375,450};
+        for(uint32 j = 0; j < sizeof(SkillLevels)/sizeof(uint32) - 1; j++)
+        {
+            if((SkillValue < SkillLevels[j] && new_value >= SkillLevels[j]))
+            {
+                learnSkillRewardedSpells( (GetUInt32Value(PLAYER_SKILL_INDEX(i)) & 0x0000FFFF), SkillLevels[j]);
+                break;
+            }
+        }        
        GetAchievementMgr().UpdateAchievementCriteria(ACHIEVEMENT_CRITERIA_TYPE_REACH_SKILL_LEVEL);
        sLog.outDebug("Player::UpdateSkillPro Chance=%3.1f%% taken", Chance/10.0);
        return true;
@@ -5046,7 +5055,7 @@
                    (*i)->ApplyModifier(true);

            // Learn all spells for skill
-            learnSkillRewardedSpells(id);
+            learnSkillRewardedSpells(id, 1);
            return;
        }
    }
@@ -17993,7 +18002,7 @@
    }
}

-void Player::learnSkillRewardedSpells(uint32 skill_id )
+void Player::learnSkillRewardedSpells(uint32 skill_id, uint32 skill_value )
{
    uint32 raceMask  = getRaceMask();
    uint32 classMask = getClassMask();
@@ -18002,6 +18011,7 @@
        SkillLineAbilityEntry const *pAbility = sSkillLineAbilityStore.LookupEntry(j);
        if (!pAbility || pAbility->skillId!=skill_id || pAbility->learnOnGetSkill != ABILITY_LEARNED_ON_GET_PROFESSION_SKILL)
            continue;
        // Check race if set
        if (pAbility->racemask && !(pAbility->racemask & raceMask))
            continue;
@@ -18012,7 +18022,10 @@
        if (sSpellStore.LookupEntry(pAbility->spellId))
        {
            // Ok need learn spell
-            learnSpell(pAbility->spellId);
+            if(skill_value - pAbility->req_skill_value >= 75)
+            {
+                if(pAbility->req_skill_value > 1 && skill_value > 1)
+                    removeSpell(pAbility->spellId);
+            }
+            else
+                learnSpell(pAbility->spellId);
        }
    }
}
@@ -18026,7 +18039,7 @@

        uint32 pskill = GetUInt32Value(PLAYER_SKILL_INDEX(i)) & 0x0000FFFF;

-        learnSkillRewardedSpells(pskill);
+        learnSkillRewardedSpells(pskill, 1);
    }
}

Index: src/game/Player.h
===================================================================
--- src/game/Player.h    (revision 205)
+++ src/game/Player.h    (working copy)
@@ -1745,7 +1745,7 @@
        uint16 GetPureSkillValue(uint32 skill) const;       // skill value
        int16 GetSkillTempBonusValue(uint32 skill) const;
        bool HasSkill(uint32 skill) const;
-        void learnSkillRewardedSpells( uint32 id );
+        void learnSkillRewardedSpells( uint32 id, uint32 value );
        void learnSkillRewardedSpells();

        void SetDontMove(bool dontMove);

[yad02]

Tested works perfectly with a remove old rank spell system.

Someone can test if it's really Blizzlike ?

The order of chat message information is it good ?

Actually it's this oder

Learn Spell of Profession

Learn Profession

Skill = 2

Skill = 3

...

Skill = 73

Skill = 74

You learn Spell Rank 1

Skill = 75

Skill = 76

...

Skill = 148

Skill = 149

You unlearn Spell Rank 1

You learn Spell Rank 2

Skill = 150

Skill = 151

...

[VladimirMangos]

I think now all ok, possible, and will review but small note:

@@ -18002,6 +18011,7 @@

This part not have changes? Is it ok and it can be skipped or lost something...

[added]Found problems:

1) In fact you not implement removing skills at skill value drop.

2) you attempt in code remove low ranks. But for normal casted spells low ranks _never_ must unlearned. It must _always_ known if known higher rank.

Problem with spell listing in spellbook solved in more correct way just adding proper data to spell_chain.

3) for correct learning/remove expected spells in skill set and player loading must be used _proper current skill value instead 1.

I will fix and add required changes by self.

But main part work: Code called in proper cases and use proper DBC fields

[VladimirMangos]

This is version with my changes: http://paste2.org/p/124126

+ spell chain data:

/* Lifeblood (Herbalizm) */

DELETE FROM `spell_chain` WHERE `spell_id` IN (55428,55480,55500,55501,55502,55503);

INSERT INTO `spell_chain` VALUES

(55428,0,55428,1,0),

(55480,55428,55428,2,0),

(55500,55480,55428,3,0),

(55501,55500,55428,4,0),

(55502,55501,55428,5,0),

(55503,55502,55428,6,0);

/* Toughness (Mining) */

DELETE FROM `spell_chain` WHERE `spell_id` IN (53120,53121,53122,53123,53124,53040);

INSERT INTO `spell_chain` VALUES

(53120,0,53120,1,0),

(53121,53120,53120,2,0),

(53122,53121,53120,3,0),

(53123,53122,53120,4,0),

(53124,53123,53120,5,0),

(53040,53124,53120,6,0);

/* Master of Anatomy (Skinning) */

DELETE FROM `spell_chain` WHERE `spell_id` IN (53125,53662,53663,53664,53665,53666);

INSERT INTO `spell_chain` VALUES

(53125,0,53125,1,0),

(53662,53125,53125,2,0),

(53663,53662,53125,3,0),

(53664,53663,53125,4,0),

(53665,53664,53125,5,0),

(53666,53665,53125,6,0);

Only single problem not fixed:

If player have more 1 rank of spell then at loading it show each rank independently.

But at normal learning at skill grow correctly show high available rank in spell book.

I not find at this moment why

[yad02]

I clean my sources and I see it.

Thank you for help !

Now this code is correct !

[VladimirMangos]

??? In prev. post _your_ patch but with applied my changes for resolve problems that i list in #37 post.

I clearly will attempt fix _this_ patch version and apply when found source of problem at player loading. Possible in 2 commits to make destination your changes and my additions.

[yad02]

void Player::learnSkillRewardedSpells(uint32 skill_id, uint32 skill_value )
{
   uint32 raceMask  = getRaceMask();
   uint32 classMask = getClassMask();
   for (uint32 j=0; j<sSkillLineAbilityStore.GetNumRows(); ++j)
   {
       SkillLineAbilityEntry const *pAbility = sSkillLineAbilityStore.LookupEntry(j);
       if (!pAbility || pAbility->skillId!=skill_id || pAbility->learnOnGetSkill != ABILITY_LEARNED_ON_GET_PROFESSION_SKILL)
           continue;
       // Check race if set
       if (pAbility->racemask && !(pAbility->racemask & raceMask))
           continue;
       // Check class if set
       if (pAbility->classmask && !(pAbility->classmask & classMask))
           continue;

       if (sSpellStore.LookupEntry(pAbility->spellId))
       {
           // Ok need learn spell
           if(skill_value < pAbility->req_skill_value || skill_value - pAbility->req_skill_value >= 75)
           {
               if(pAbility->req_skill_value > 1)
                   removeSpell(pAbility->spellId);
           }
           else if(pAbility->req_skill_value == 1 || skill_value - pAbility->req_skill_value < 75)
           {
               if(GetSession()->PlayerLoading())
                   addSpell(pAbility->spellId,true,true,true);
               else
                   learnSpell(pAbility->spellId);
           }
       }
   }
}

rewrite tests :

if(skill_value < pAbility->req_skill_value || skill_value - pAbility->req_skill_value >= 75)

else if(pAbility->req_skill_value == 1 || skill_value - pAbility->req_skill_value < 75)

It's better, in spellbook all is right

A little bug with .learn command :

when you do a .learn spell_rank_6 you learn rank 5 4 3 2 1 (spell_chain effect)

When you disconnect all rank are removed

Other "bug" with spell

for example when you get 300 skill

in chat client console :

rank 3 is removed

rank 2 is removed

rank 1 is removed

you learn rank 1

you learn rank 2

you learn rank 3

you learn rank 4

your profession skill is 300

[VladimirMangos]

As I write before: remove low rank spell casted from spellbook wrong and not acceptable way. Ofc, in this case you see only single rank but this is wrong way.

Low rank _must_ be known by player if known higher rank.

Other spell that show only last rank in spellbook _also_ have not only last rabk but all lesser ranks also.

I temporary stop work at this patch. Need finished another related to spell_chain table use.

[yad02]

"remove low rank spell casted from spellbook wrong and not acceptable way"

ok so the problem is this one :

"how to remove a spell in spell book without unlearn it ?"

[VladimirMangos]

This already work fine for other spells that required like rank replacing. You can see code in Player::addSpell

in part after

        // replace spells in action bars and spellbook to bigger rank if only one spell rank must be accessible
       if(newspell->active && !newspell->disabled && !SpellMgr::canStackSpellRanks(spellInfo) && spellmgr.GetSpellRank(spellInfo->Id) != 0)
       {

Need only have spell_chain data and proper result from SpellMgr::canStackSpellRanks.

This already implemented in my patch version. But as i write exist small bug in case player loading when this fail.

I plan find related bug and then will add patch.

[waza123]

+        uint32 SkillLevels[]={75,150,225,300,375,450};
+        for(uint32 j = 0; j < sizeof(SkillLevels)/sizeof(uint32) - 1; j++)
+        {

why do you get "sizeof array" every loop time ?

Don't know how in C++

but in PHP:

$s = sizeof(SkillLevels);

for($i= 0; $i < $s; $i++){

[Wyk3d]

why do you get "sizeof array" every loop time ?

Haven't checked the code but in general that construct is used when the array is static. The sizeof operator gets the size in bytes of whatever is passed to it. For a static array (and only for a static array) this is always equal to the number of elements times the size of one element, which is known at compile time, as well as the total size of the array, so the computation of the number of rows will result in a hardcoded constant number in the compiled code.

[marcelomax]

Any news on this patch to make some new tests??

[VladimirMangos]

I found source of rank show at loading problem in my version. It related to multiply spell learning from spell loading/skill set in active/non-active state. This need deep rewriting Player::AddSpell in cases attempt adding already known spell in different active-state.

[gygelly]

For those of us running servers, what would you recommend for the time being? Currently, I've disabled all learning of trade skills to preserve game balance.

Is a patch soon coming?

If not, should I just apply the code posted earlier in this thread?

Or should I simply remove these spells from the db?