fix(Core): Small improvements for weapon/defence skill handling (#12253)

* Improve weaponskill handling

* replace pvp check TYPEID_PLAYER with IsCharmedOwnedByPlayerOrPlayer

* remove no longer needed pvp checks

* move pvp check back to ProcDamageAndSpellFor()

* rename plevel -> playerLevel, add LOG to verify increase chances

* fix issue due to negative value in uint

* revert change which allowed blocked attacks to increase weapon skill

Co-authored-by: schell244 <>

src/server/game/Entities/Player/PlayerUpdates.cpp

@@ -980,42 +980,56 @@ void Player::UpdateWeaponSkill(Unit* victim, WeaponAttackType attType, Item* ite
 
 void Player::UpdateCombatSkills(Unit* victim, WeaponAttackType attType, bool defence, Item* item /*= nullptr*/)
 {
-    uint8 plevel    = getLevel(); // if defense than victim == attacker
+    uint8  playerLevel = getLevel();
-    uint8 greylevel = Acore::XP::GetGrayLevel(plevel);
+    uint16 currentSkillValue = defence ? GetBaseDefenseSkillValue() : GetBaseWeaponSkillValue(attType);
-    uint8 moblevel  = victim->getLevelForTarget(this);
+    uint16 currentSkillMax = 5 * playerLevel;
+    int32  skillDiff = currentSkillMax - currentSkillValue;
+
+    // Max skill reached for level.
+    // Can in some cases be less than 0: having max skill and then .level -1 as example.
+    if (skillDiff <= 0)
+    {
+        return;
+    }
+
+    uint8 greylevel = Acore::XP::GetGrayLevel(playerLevel);
+    uint8 moblevel = defence ? victim->getLevelForTarget(this) : victim->getLevel(); // if defense than victim == attacker
     /*if (moblevel < greylevel)
         return;*/
     // Patch 3.0.8 (2009-01-20): You can no longer skill up weapons on mobs that are immune to damage.
 
-    if (moblevel > plevel + 5)
+    if (moblevel > playerLevel + 5)
-        moblevel = plevel + 5;
+    {
+        moblevel = playerLevel + 5;
+    }
 
-    uint8 lvldif = moblevel - greylevel;
+    int16 lvldif = moblevel - greylevel;
     if (lvldif < 3)
+    {
         lvldif = 3;
+    }
 
-    uint32 skilldif = 5 * plevel - (defence ? GetBaseDefenseSkillValue()
+    float chance = float(3 * lvldif * skillDiff) / playerLevel;
-                                            : GetBaseWeaponSkillValue(attType));
-    if (skilldif <= 0)
-        return;
-
-    float chance = float(3 * lvldif * skilldif) / plevel;
     if (!defence)
-        if (getClass() == CLASS_WARRIOR || getClass() == CLASS_ROGUE)
+    {
         chance += chance * 0.02f * GetStat(STAT_INTELLECT);
+    }
 
-    chance =
+    chance = chance < 1.0f ? 1.0f : chance; // minimum chance to increase skill is 1%
-        chance < 1.0f ? 1.0f : chance; // minimum chance to increase skill is 1%
+
+    LOG_DEBUG("entities.player", "Player::UpdateCombatSkills(defence:{}, playerLevel:{}, moblevel:{}) -> ({}/{}) chance to increase skill is {}\%", defence, playerLevel, moblevel, currentSkillValue, currentSkillMax, chance);
 
     if (roll_chance_f(chance))
     {
         if (defence)
+        {
             UpdateDefense();
-        else
-            UpdateWeaponSkill(victim, attType, item);
         }
         else
-        return;
+        {
+            UpdateWeaponSkill(victim, attType, item);
+        }
+    }
 }
 
 void Player::UpdateSkillsForLevel()

src/server/game/Entities/Unit/Unit.cpp

@@ -15742,22 +15742,19 @@ void Unit::ProcDamageAndSpellFor(bool isVictim, Unit* target, uint32 procFlag, u
 
         // Update skills here for players
         // only when you are not fighting other players or their pets/totems (pvp)
-        if (GetTypeId() == TYPEID_PLAYER &&
+        if (IsPlayer() && !target->IsCharmedOwnedByPlayerOrPlayer())
-                target->GetTypeId() != TYPEID_PLAYER &&
-                !(target->IsTotem() && target->ToTotem()->GetOwner()->IsPlayer()) &&
-                !target->IsPet()
-           )
         {
             // On melee based hit/miss/resist/parry/dodge need to update skill (for victim and attacker)
             if (procExtra & (PROC_EX_NORMAL_HIT | PROC_EX_MISS | PROC_EX_RESIST | PROC_EX_PARRY | PROC_EX_DODGE))
             {
-                if (target->GetTypeId() != TYPEID_PLAYER)
                 ToPlayer()->UpdateCombatSkills(target, attType, isVictim, procSpell ? procSpell->m_weaponItem : nullptr);
             }
-            // Update defence if player is victim and we block
+            // Update defence if player is victim and we block - TODO: confirm that blocked attacks only have a chance to increase defence skill
             else if (isVictim && procExtra & (PROC_EX_BLOCK))
+            {
                 ToPlayer()->UpdateCombatSkills(target, attType, true);
             }
+        }
         // If exist crit/parry/dodge/block need update aura state (for victim and attacker)
         if (procExtra & (PROC_EX_CRITICAL_HIT | PROC_EX_PARRY | PROC_EX_DODGE | PROC_EX_BLOCK))
         {