From f75aceb9a9b45a97452aad5b658ac52042c7c8cf Mon Sep 17 00:00:00 2001
From: schell244 <Schell244@web.de>
Date: Wed, 21 Sep 2022 15:09:26 +0200
Subject: [PATCH] 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 <>
---
 .../game/Entities/Player/PlayerUpdates.cpp    | 50 ++++++++++++-------
 src/server/game/Entities/Unit/Unit.cpp        | 13 ++---
 2 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/src/server/game/Entities/Player/PlayerUpdates.cpp b/src/server/game/Entities/Player/PlayerUpdates.cpp
index 953c35e69..2993b7d58 100644
--- a/src/server/game/Entities/Player/PlayerUpdates.cpp
+++ b/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 greylevel = Acore::XP::GetGrayLevel(plevel);
-    uint8 moblevel  = victim->getLevelForTarget(this);
+    uint8  playerLevel = getLevel();
+    uint16 currentSkillValue = defence ? GetBaseDefenseSkillValue() : GetBaseWeaponSkillValue(attType);
+    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)
-        moblevel = plevel + 5;
+    if (moblevel > playerLevel + 5)
+    {
+        moblevel = playerLevel + 5;
+    }
 
-    uint8 lvldif = moblevel - greylevel;
+    int16 lvldif = moblevel - greylevel;
     if (lvldif < 3)
+    {
         lvldif = 3;
+    }
 
-    uint32 skilldif = 5 * plevel - (defence ? GetBaseDefenseSkillValue()
-                                            : GetBaseWeaponSkillValue(attType));
-    if (skilldif <= 0)
-        return;
-
-    float chance = float(3 * lvldif * skilldif) / plevel;
+    float chance = float(3 * lvldif * skillDiff) / playerLevel;
     if (!defence)
-        if (getClass() == CLASS_WARRIOR || getClass() == CLASS_ROGUE)
-            chance += chance * 0.02f * GetStat(STAT_INTELLECT);
+    {
+        chance += chance * 0.02f * GetStat(STAT_INTELLECT);
+    }
 
-    chance =
-        chance < 1.0f ? 1.0f : chance; // minimum chance to increase skill is 1%
+    chance = 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;
 }
 
 void Player::UpdateSkillsForLevel()
diff --git a/src/server/game/Entities/Unit/Unit.cpp b/src/server/game/Entities/Unit/Unit.cpp
index 33e7e4c0f..c2b03b3e4 100644
--- a/src/server/game/Entities/Unit/Unit.cpp
+++ b/src/server/game/Entities/Unit/Unit.cpp
@@ -15742,21 +15742,18 @@ 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 &&
-                target->GetTypeId() != TYPEID_PLAYER &&
-                !(target->IsTotem() && target->ToTotem()->GetOwner()->IsPlayer()) &&
-                !target->IsPet()
-           )
+        if (IsPlayer() && !target->IsCharmedOwnedByPlayerOrPlayer())
         {
             // 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);
+                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))