fix(Core/Spells): Fix proc phase ordering CAST before HIT for instant spells (#24942)

Co-authored-by: blinkysc <blinkysc@users.noreply.github.com> Co-authored-by: Shauren <297439+Shauren@users.noreply.github.com>
2026-03-08 02:00:29 +00:00 · 2026-02-28 15:27:05 -06:00
parent 92f12f3bce
commit eae14a58b3
4 changed files with 351 additions and 22 deletions
--- a/data/sql/updates/pending_db_world/rev_1772310582393914812.sql
+++ b/data/sql/updates/pending_db_world/rev_1772310582393914812.sql
@@ -0,0 +1,4 @@
+-- Fingers of Frost buff: change SpellPhaseMask from 3 (CAST|HIT) to 1 (CAST only).
+-- With !IsTriggered() removed from CAST proc blocks, triggered spells now fire
+-- CAST procs, so HIT phase is no longer needed for triggered spell consumption.
+UPDATE `spell_proc` SET `SpellPhaseMask` = 1 WHERE `SpellId` = 74396;
--- a/src/server/game/Spells/Spell.cpp
+++ b/src/server/game/Spells/Spell.cpp
@@ -3956,6 +3956,44 @@ void Spell::_cast(bool skipCheck)
    }
    else
    {
+        // CAST phase procs for non-channeled immediate spells
+        // (channeled spells handle this below to preserve spell mods)
+        // Note: triggered spells are allowed here; per-aura filtering via
+        // PROC_ATTR_TRIGGERED_CAN_PROC in SpellAuras.cpp handles rejection.
+        if (!m_spellInfo->IsChanneled() && m_originalCaster)
+        {
+            uint32 procAttacker = m_procAttacker;
+            if (!procAttacker)
+            {
+                bool IsPositive = m_spellInfo->IsPositive();
+                if (m_spellInfo->DmgClass == SPELL_DAMAGE_CLASS_MAGIC)
+                {
+                    procAttacker = IsPositive ? PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_POS : PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG;
+                }
+                else
+                {
+                    procAttacker = IsPositive ? PROC_FLAG_DONE_SPELL_NONE_DMG_CLASS_POS : PROC_FLAG_DONE_SPELL_NONE_DMG_CLASS_NEG;
+                }
+            }
+
+            uint32 hitMask = PROC_HIT_NORMAL;
+
+            for (std::list<TargetInfo>::iterator ihit = m_UniqueTargetInfo.begin(); ihit != m_UniqueTargetInfo.end(); ++ihit)
+            {
+                if (ihit->missCondition != SPELL_MISS_NONE)
+                    continue;
+
+                if (!ihit->crit)
+                    continue;
+
+                hitMask |= PROC_HIT_CRITICAL;
+                break;
+            }
+
+            Unit::ProcSkillsAndAuras(m_originalCaster, m_originalCaster, procAttacker, PROC_FLAG_NONE, hitMask, 1, BASE_ATTACK, m_spellInfo, m_triggeredByAuraSpell.spellInfo,
+                m_triggeredByAuraSpell.effectIndex, this, nullptr, nullptr, PROC_SPELL_PHASE_CAST);
+        }
+
        // Immediate spell, no big deal
        handle_immediate();
    }
@@ -3985,13 +4023,11 @@ void Spell::_cast(bool skipCheck)
    if (modOwner)
        modOwner->SetSpellModTakingSpell(this, false);

-    // Handle procs on cast - only for non-triggered spells
-    // Triggered spells (from auras, items, etc.) should not fire CAST phase procs
-    // as they are not player-initiated casts. This prevents issues like Arcane Potency
-    // charges being consumed by periodic damage effects (e.g., Blizzard ticks).
-    // Must be called AFTER handle_immediate() so spell mods (like Missile Barrage's
-    // duration reduction) are applied before the aura is consumed by the proc.
-    if (m_originalCaster && !IsTriggered())
+    // CAST phase procs for delayed and channeled spells
+    // Note: triggered spells are allowed here; per-aura filtering via
+    // PROC_ATTR_TRIGGERED_CAN_PROC in SpellAuras.cpp handles rejection.
+    if ((m_spellState == SPELL_STATE_DELAYED || m_spellInfo->IsChanneled())
+        && m_originalCaster)
    {
        uint32 procAttacker = m_procAttacker;
        if (!procAttacker)
--- a/src/server/scripts/Spells/spell_mage.cpp
+++ b/src/server/scripts/Spells/spell_mage.cpp
@@ -958,23 +958,11 @@ class spell_mage_fingers_of_frost : public AuraScript

    void PrepareProc(ProcEventInfo& eventInfo)
    {
+        // Block channeled spells (e.g. Blizzard channel start) from consuming charges.
+        // All other filtering is handled by SpellPhaseMask=1 (CAST only) in spell_proc.
        if (Spell const* spell = eventInfo.GetProcSpell())
-        {
-            bool isTriggered = spell->IsTriggered();
-            bool isCastPhase = (eventInfo.GetSpellPhaseMask() & PROC_SPELL_PHASE_CAST) != 0;
-            bool isChanneled = spell->GetSpellInfo()->IsChanneled();
-            bool prevent = false;
-
-            if (isTriggered)
-                prevent = false;
-            else if (isChanneled)
-                prevent = true;
-            else if (!isCastPhase)
-                prevent = true;
-
-            if (prevent)
+            if (spell->GetSpellInfo()->IsChanneled())
                PreventDefaultAction();
-        }
    }

    void OnRemove(AuraEffect const* /*aurEff*/, AuraEffectHandleModes /*mode*/)
--- a/src/test/server/game/Spells/SpellProcPhaseOrderingTest.cpp
+++ b/src/test/server/game/Spells/SpellProcPhaseOrderingTest.cpp
@@ -0,0 +1,301 @@
+/*
+ * 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 <http://www.gnu.org/licenses/>.
+ */
+
+/**
+ * @file SpellProcPhaseOrderingTest.cpp
+ * @brief Tests for proc phase ordering: CAST -> HIT -> FINISH
+ *
+ * Validates that CAST-phase procs are isolated from HIT-phase events
+ * and vice versa. This is critical for correct behavior of spells like
+ * Arcane Potency (consumed on CAST) not being affected by HIT events
+ * from the same spell cast.
+ *
+ * Related fix: Non-channeled immediate spells fire CAST procs before
+ * handle_immediate() to ensure CAST -> HIT -> FINISH ordering.
+ */
+
+#include "ProcChanceTestHelper.h"
+#include "ProcEventInfoHelper.h"
+#include "AuraStub.h"
+#include "SpellInfoTestHelper.h"
+#include "gtest/gtest.h"
+
+using namespace testing;
+
+class SpellProcPhaseOrderingTest : public ::testing::Test
+{
+protected:
+    void SetUp() override {}
+
+    void TearDown() override
+    {
+        for (auto* si : _spellInfos)
+            delete si;
+        _spellInfos.clear();
+    }
+
+    SpellInfo* CreateSpellInfo(uint32 id)
+    {
+        auto* si = SpellInfoBuilder().WithId(id).Build();
+        _spellInfos.push_back(si);
+        return si;
+    }
+
+    std::vector<SpellInfo*> _spellInfos;
+};
+
+// =============================================================================
+// Phase Isolation: CAST-only procs must not trigger on HIT events
+// =============================================================================
+
+TEST_F(SpellProcPhaseOrderingTest, CastPhaseProc_TriggersOnCastEvent)
+{
+    auto* spellInfo = CreateSpellInfo(1);
+    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
+
+    auto procEntry = SpellProcEntryBuilder()
+        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .Build();
+
+    auto castEvent = ProcEventInfoBuilder()
+        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .WithHitMask(PROC_HIT_NORMAL)
+        .WithDamageInfo(&damageInfo)
+        .Build();
+
+    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
+}
+
+TEST_F(SpellProcPhaseOrderingTest, CastPhaseProc_DoesNotTriggerOnHitEvent)
+{
+    auto* spellInfo = CreateSpellInfo(1);
+    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
+
+    auto procEntry = SpellProcEntryBuilder()
+        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .Build();
+
+    auto hitEvent = ProcEventInfoBuilder()
+        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_HIT)
+        .WithHitMask(PROC_HIT_NORMAL)
+        .WithDamageInfo(&damageInfo)
+        .Build();
+
+    EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, hitEvent));
+}
+
+TEST_F(SpellProcPhaseOrderingTest, CastPhaseProc_DoesNotTriggerOnFinishEvent)
+{
+    auto* spellInfo = CreateSpellInfo(1);
+    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
+
+    auto procEntry = SpellProcEntryBuilder()
+        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .Build();
+
+    auto finishEvent = ProcEventInfoBuilder()
+        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_FINISH)
+        .WithHitMask(PROC_HIT_NORMAL)
+        .WithDamageInfo(&damageInfo)
+        .Build();
+
+    EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, finishEvent));
+}
+
+// =============================================================================
+// Phase Isolation: HIT-only procs must not trigger on CAST events
+// =============================================================================
+
+TEST_F(SpellProcPhaseOrderingTest, HitPhaseProc_DoesNotTriggerOnCastEvent)
+{
+    auto* spellInfo = CreateSpellInfo(1);
+    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
+
+    auto procEntry = SpellProcEntryBuilder()
+        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_HIT)
+        .Build();
+
+    auto castEvent = ProcEventInfoBuilder()
+        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .WithHitMask(PROC_HIT_NORMAL)
+        .WithDamageInfo(&damageInfo)
+        .Build();
+
+    EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
+}
+
+// =============================================================================
+// Charge consumption respects phase isolation
+// Simulates the Arcane Potency scenario: charges consumed on CAST phase
+// should not be consumed again when HIT phase fires later.
+// =============================================================================
+
+TEST_F(SpellProcPhaseOrderingTest, ChargeConsumedOnCast_NotConsumedAgainOnHit)
+{
+    auto* spellInfo = CreateSpellInfo(1);
+    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
+
+    // Proc entry configured for CAST phase only (like Arcane Potency)
+    auto procEntry = SpellProcEntryBuilder()
+        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .WithChance(100.0f)
+        .Build();
+
+    auto aura = AuraStubBuilder()
+        .WithId(12345)
+        .WithCharges(2)
+        .Build();
+
+    // CAST phase event fires first (correct ordering)
+    auto castEvent = ProcEventInfoBuilder()
+        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .WithHitMask(PROC_HIT_NORMAL)
+        .WithDamageInfo(&damageInfo)
+        .Build();
+
+    // CAST phase matches -> proc triggers, charge consumed
+    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
+    ProcChanceTestHelper::SimulateConsumeProcCharges(aura.get(), procEntry);
+    EXPECT_EQ(aura->GetCharges(), 1);
+
+    // HIT phase event fires second
+    auto hitEvent = ProcEventInfoBuilder()
+        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_HIT)
+        .WithHitMask(PROC_HIT_NORMAL)
+        .WithDamageInfo(&damageInfo)
+        .Build();
+
+    // HIT phase does NOT match CAST-only proc -> no trigger, no charge consumed
+    EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, hitEvent));
+    EXPECT_EQ(aura->GetCharges(), 1); // Still 1, not consumed
+}
+
+TEST_F(SpellProcPhaseOrderingTest, ChargeConsumedOnCast_AvailableForNextSpell)
+{
+    auto* spellInfo = CreateSpellInfo(1);
+    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
+
+    auto procEntry = SpellProcEntryBuilder()
+        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .WithChance(100.0f)
+        .Build();
+
+    auto aura = AuraStubBuilder()
+        .WithId(12345)
+        .WithCharges(2)
+        .Build();
+
+    auto castEvent = ProcEventInfoBuilder()
+        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .WithHitMask(PROC_HIT_NORMAL)
+        .WithDamageInfo(&damageInfo)
+        .Build();
+
+    // First spell cast consumes one charge
+    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
+    ProcChanceTestHelper::SimulateConsumeProcCharges(aura.get(), procEntry);
+    EXPECT_EQ(aura->GetCharges(), 1);
+    EXPECT_FALSE(aura->IsRemoved());
+
+    // Second spell cast consumes last charge
+    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
+    ProcChanceTestHelper::SimulateConsumeProcCharges(aura.get(), procEntry);
+    EXPECT_EQ(aura->GetCharges(), 0);
+    EXPECT_TRUE(aura->IsRemoved());
+}
+
+// =============================================================================
+// Multi-phase procs (CAST | HIT) trigger on both phases
+// =============================================================================
+
+TEST_F(SpellProcPhaseOrderingTest, MultiPhaseProc_TriggersOnBothCastAndHit)
+{
+    auto* spellInfo = CreateSpellInfo(1);
+    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
+
+    auto procEntry = SpellProcEntryBuilder()
+        .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST | PROC_SPELL_PHASE_HIT)
+        .Build();
+
+    auto castEvent = ProcEventInfoBuilder()
+        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_CAST)
+        .WithHitMask(PROC_HIT_NORMAL)
+        .WithDamageInfo(&damageInfo)
+        .Build();
+
+    auto hitEvent = ProcEventInfoBuilder()
+        .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+        .WithSpellPhaseMask(PROC_SPELL_PHASE_HIT)
+        .WithHitMask(PROC_HIT_NORMAL)
+        .WithDamageInfo(&damageInfo)
+        .Build();
+
+    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, castEvent));
+    EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, hitEvent));
+}
+
+// =============================================================================
+// All three phases are distinct
+// =============================================================================
+
+TEST_F(SpellProcPhaseOrderingTest, AllThreePhases_MutuallyExclusive)
+{
+    auto* spellInfo = CreateSpellInfo(1);
+    DamageInfo damageInfo(nullptr, nullptr, 100, spellInfo, SPELL_SCHOOL_MASK_ARCANE, SPELL_DIRECT_DAMAGE);
+
+    uint32 phases[] = { PROC_SPELL_PHASE_CAST, PROC_SPELL_PHASE_HIT, PROC_SPELL_PHASE_FINISH };
+
+    for (uint32 procPhase : phases)
+    {
+        auto procEntry = SpellProcEntryBuilder()
+            .WithProcFlags(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+            .WithSpellPhaseMask(procPhase)
+            .Build();
+
+        for (uint32 eventPhase : phases)
+        {
+            auto event = ProcEventInfoBuilder()
+                .WithTypeMask(PROC_FLAG_DONE_SPELL_MAGIC_DMG_CLASS_NEG)
+                .WithSpellPhaseMask(eventPhase)
+                .WithHitMask(PROC_HIT_NORMAL)
+                .WithDamageInfo(&damageInfo)
+                .Build();
+
+            if (procPhase == eventPhase)
+                EXPECT_TRUE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, event))
+                    << "Phase " << procPhase << " should match itself";
+            else
+                EXPECT_FALSE(sSpellMgr->CanSpellTriggerProcOnEvent(procEntry, event))
+                    << "Phase " << procPhase << " should not match phase " << eventPhase;
+        }
+    }
+}