From faedf206bf7d808092f3c2e8647a20eea361fc40 Mon Sep 17 00:00:00 2001 From: blinkysc <37940565+blinkysc@users.noreply.github.com> Date: Tue, 16 Jun 2026 01:53:08 -0500 Subject: [PATCH] fix(Core/Spells): Reorder melee/ranged spell crit to match SpellCriticalDamageBonus (#24979) Co-authored-by: blinkysc --- src/server/game/Entities/Unit/Unit.cpp | 33 ++-- .../Spells/SpellCritDamageBonusOrderTest.cpp | 160 ++++++++++++++++++ 2 files changed, 178 insertions(+), 15 deletions(-) create mode 100644 src/test/server/game/Spells/SpellCritDamageBonusOrderTest.cpp diff --git a/src/server/game/Entities/Unit/Unit.cpp b/src/server/game/Entities/Unit/Unit.cpp index 3115149357..3cbd7c241d 100644 --- a/src/server/game/Entities/Unit/Unit.cpp +++ b/src/server/game/Entities/Unit/Unit.cpp @@ -1555,28 +1555,32 @@ void Unit::CalculateSpellDamageTaken(SpellNonMeleeDamage* damageInfo, int32 dama { damageInfo->HitInfo |= SPELL_HIT_TYPE_CRIT; - // Calculate crit bonus - uint32 crit_bonus = damage; - // Apply crit_damage bonus for melee spells - if (Player* modOwner = GetSpellModOwner()) - modOwner->ApplySpellMod(spellInfo->Id, SPELLMOD_CRIT_DAMAGE_BONUS, crit_bonus); - damage += crit_bonus; + // Calculate crit bonus (100% for melee/ranged spells) + int32 crit_bonus = damage; + crit_bonus += damage; // Apply SPELL_AURA_MOD_ATTACKER_RANGED_CRIT_DAMAGE or SPELL_AURA_MOD_ATTACKER_MELEE_CRIT_DAMAGE - float critPctDamageMod = 0.0f; + float crit_mod = 0.0f; if (attackType == RANGED_ATTACK) - critPctDamageMod += victim->GetTotalAuraModifier(SPELL_AURA_MOD_ATTACKER_RANGED_CRIT_DAMAGE); + crit_mod += victim->GetTotalAuraModifier(SPELL_AURA_MOD_ATTACKER_RANGED_CRIT_DAMAGE); else - critPctDamageMod += victim->GetTotalAuraModifier(SPELL_AURA_MOD_ATTACKER_MELEE_CRIT_DAMAGE); + crit_mod += victim->GetTotalAuraModifier(SPELL_AURA_MOD_ATTACKER_MELEE_CRIT_DAMAGE); // Increase crit damage from SPELL_AURA_MOD_CRIT_DAMAGE_BONUS - critPctDamageMod += GetTotalAuraModifierByMiscMask(SPELL_AURA_MOD_CRIT_DAMAGE_BONUS, spellInfo->GetSchoolMask()); - + crit_mod += GetTotalAuraModifierByMiscMask(SPELL_AURA_MOD_CRIT_DAMAGE_BONUS, spellInfo->GetSchoolMask()); // Increase crit damage from SPELL_AURA_MOD_CRIT_PERCENT_VERSUS - critPctDamageMod += GetTotalAuraModifierByMiscMask(SPELL_AURA_MOD_CRIT_PERCENT_VERSUS, crTypeMask); + crit_mod += GetTotalAuraModifierByMiscMask(SPELL_AURA_MOD_CRIT_PERCENT_VERSUS, crTypeMask); - if (critPctDamageMod != 0) - AddPct(damage, critPctDamageMod); + if (crit_bonus != 0 && crit_mod != 0.0f) + AddPct(crit_bonus, crit_mod); + + crit_bonus -= damage; + + // adds additional damage to critBonus (from talents) + if (Player* modOwner = GetSpellModOwner()) + modOwner->ApplySpellMod(spellInfo->Id, SPELLMOD_CRIT_DAMAGE_BONUS, crit_bonus); + + damage = crit_bonus + damage; } // Spell weapon based damage CAN BE crit & blocked at same time @@ -9384,7 +9388,6 @@ uint32 Unit::SpellCriticalDamageBonus(Unit const* caster, SpellInfo const* spell { case SPELL_DAMAGE_CLASS_MELEE: // for melee based spells is 100% case SPELL_DAMAGE_CLASS_RANGED: - /// @todo: write here full calculation for melee/ranged spells crit_bonus += damage; break; default: diff --git a/src/test/server/game/Spells/SpellCritDamageBonusOrderTest.cpp b/src/test/server/game/Spells/SpellCritDamageBonusOrderTest.cpp new file mode 100644 index 0000000000..98c7e4df9e --- /dev/null +++ b/src/test/server/game/Spells/SpellCritDamageBonusOrderTest.cpp @@ -0,0 +1,160 @@ +/* + * This file is part of the AzerothCore Project. See AUTHORS file for Copyright information + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * You should have received a copy of the GNU General Public License along + * with this program. If not, see . + */ + +#include "Util.h" +#include "gtest/gtest.h" + +/** + * @brief Tests for melee/ranged spell crit damage ordering (issue #24976) + * + * The inline melee/ranged spell crit code in CalculateSpellDamageTaken applied + * SPELLMOD_CRIT_DAMAGE_BONUS BEFORE aura percentage modifiers, causing aura + * modifiers to amplify the already-talent-modified bonus. The correct order + * (matching SpellCriticalDamageBonus) is: + * + * 1. Double damage (100% crit bonus for melee/ranged) + * 2. Apply aura % modifiers to the full crit value + * 3. Extract the bonus portion + * 4. Apply SPELLMOD_CRIT_DAMAGE_BONUS to the bonus + * 5. Recombine + * + * From the issue's expected blizzlike calculation: + * 200% * 1.03 = 206% (aura % first) + * 206% - 100% = 106% (extract bonus) + * 106% * 140% = 148.4% (talent mod on bonus) + * 100% + 148.4% = 248.4% (recombine) + */ + +namespace +{ + +/** + * Replicates the FIXED inline crit code (correct ordering, + * matches SpellCriticalDamageBonus) for melee/ranged spells. + * + * @param damage Base spell damage before crit + * @param auraCritMod Aura crit % modifier (e.g. 3 for meta gem +3%) + * @param spellModPct SPELLMOD_CRIT_DAMAGE_BONUS % (e.g. 40 for Mortal Shots) + */ +int32 CorrectCritOrder(int32 damage, float auraCritMod, float spellModPct) +{ + // Step 1: crit_bonus = 2x damage (100% bonus for melee/ranged) + int32 crit_bonus = damage; + crit_bonus += damage; + + // Step 2: Apply aura % modifiers FIRST + if (crit_bonus != 0 && auraCritMod != 0.0f) + AddPct(crit_bonus, auraCritMod); + + // Step 3: Extract bonus + crit_bonus -= damage; + + // Step 4: Apply talent mod to the bonus + AddPct(crit_bonus, spellModPct); + + // Step 5: Recombine + return crit_bonus + damage; +} + +/** + * Replicates the OLD inline crit code (wrong ordering): + * SPELLMOD_CRIT_DAMAGE_BONUS applied BEFORE aura % modifiers. + */ +int32 OldWrongCritOrder(int32 damage, float auraCritMod, float spellModPct) +{ + // OLD Step 1: talent mod on base bonus FIRST (wrong) + uint32 crit_bonus = damage; + AddPct(crit_bonus, spellModPct); + int32 totalDamage = damage + static_cast(crit_bonus); + + // OLD Step 2: aura % on full total SECOND + if (auraCritMod != 0.0f) + AddPct(totalDamage, auraCritMod); + + return totalDamage; +} + +} // namespace + +class SpellCritDamageBonusOrderTest : public ::testing::Test {}; + +// No modifiers: both orderings produce 2x damage +TEST_F(SpellCritDamageBonusOrderTest, NoModifiers) +{ + EXPECT_EQ(CorrectCritOrder(1000, 0.0f, 0.0f), 2000); + EXPECT_EQ(OldWrongCritOrder(1000, 0.0f, 0.0f), 2000); +} + +// Only aura mod, no talent mod: both orderings match +TEST_F(SpellCritDamageBonusOrderTest, OnlyAuraMod) +{ + EXPECT_EQ(CorrectCritOrder(1000, 30.0f, 0.0f), 2600); + EXPECT_EQ(OldWrongCritOrder(1000, 30.0f, 0.0f), 2600); +} + +// Only talent mod, no aura mod: both orderings match +TEST_F(SpellCritDamageBonusOrderTest, OnlyTalentMod) +{ + // Correct: bonus = 2000-1000 = 1000, *1.03 = 1030, +1000 = 2030 + // Old: crit_bonus = 1000*1.03 = 1030, total = 2030 + EXPECT_EQ(CorrectCritOrder(1000, 0.0f, 3.0f), 2030); + EXPECT_EQ(OldWrongCritOrder(1000, 0.0f, 3.0f), 2030); +} + +// Issue #24976 exact scenario: Hunter Steady Shot with Fine Light Crossbow +// +// damage = 320, meta gem = 3% aura, Mortal Shots etc = 40% SPELLMOD +// +// Expected (blizzlike): +// 200% * 1.03 = 206% → 640 * 1.03 = 659 +// 206% - 100% = 106% → 659 - 320 = 339 +// 106% * 140% = 148.4% → 339 * 1.40 = 474 +// 100% + 148.4% → 320 + 474 = 794 +// (issue reports 795 from float math; integer truncation gives 794) +// +// Old (wrong): +// crit_bonus = 320 * 1.40 = 448 +// damage = 320 + 448 = 768 +// 768 * 1.03 = 791 +TEST_F(SpellCritDamageBonusOrderTest, Issue24976_HunterSteadyShot) +{ + int32 damage = 320; + float auraMod = 3.0f; // meta gem: +3% crit damage + float talentMod = 40.0f; // Mortal Shots etc: +40% SPELLMOD_CRIT_DAMAGE_BONUS + + EXPECT_EQ(OldWrongCritOrder(damage, auraMod, talentMod), 791); + EXPECT_EQ(CorrectCritOrder(damage, auraMod, talentMod), 794); + EXPECT_GT(CorrectCritOrder(damage, auraMod, talentMod), + OldWrongCritOrder(damage, auraMod, talentMod)); +} + +// Both modifiers with larger values: difference becomes more pronounced +TEST_F(SpellCritDamageBonusOrderTest, BothMods_LargerValues) +{ + int32 damage = 1000; + float auraMod = 3.0f; + float talentMod = 40.0f; + + // Correct: 2000*1.03=2060, bonus=1060, *1.40=1484, +1000=2484 + EXPECT_EQ(CorrectCritOrder(damage, auraMod, talentMod), 2484); + + // Wrong: 1000*1.40=1400, total=2400, *1.03=2472 + EXPECT_EQ(OldWrongCritOrder(damage, auraMod, talentMod), 2472); + + EXPECT_GT(CorrectCritOrder(damage, auraMod, talentMod), + OldWrongCritOrder(damage, auraMod, talentMod)); +}