Core/Spells: rework part 1: Improved positive detection logic

5 files changed, 390 insertions, 229 deletions

diff --git a/src/server/game/Entities/Unit/Unit.cpp b/src/server/game/Entities/Unit/Unit.cpp
index d27d6b89917..d6322aed39c 100644
--- a/src/server/game/Entities/Unit/Unit.cpp
+++ b/src/server/game/Entities/Unit/Unit.cpp
@@ -9608,7 +9608,7 @@ void Unit::ModSpellCastTime(SpellInfo const* spellInfo, int32 & castTime, Spell*
         castTime = 500;
 }
 
-void Unit::ModSpellDurationTime(SpellInfo const* spellInfo, int32 & duration, Spell* spell)
+void Unit::ModSpellDurationTime(SpellInfo const* spellInfo, int32& duration, Spell* spell)
 {
     if (!spellInfo || duration < 0)
         return;
diff --git a/src/server/game/Spells/Spell.cpp b/src/server/game/Spells/Spell.cpp
index 918887fb131..3e22493cbce 100644
--- a/src/server/game/Spells/Spell.cpp
+++ b/src/server/game/Spells/Spell.cpp
@@ -2671,7 +2671,8 @@ SpellMissInfo Spell::DoSpellHitOnUnit(Unit* unit, uint32 effectMask, bool scaleA
                 {
                     for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
                     {
-                        if ((effectMask & (1 << i)) && !aurSpellInfo->IsPositiveEffect(i))
+                        // mod duration only for effects applying aura!
+                        if ((aura_effmask & (1 << i)) && !aurSpellInfo->IsPositiveEffect(i))
                         {
                             positive = false;
                             break;
diff --git a/src/server/game/Spells/SpellInfo.cpp b/src/server/game/Spells/SpellInfo.cpp
index 88958e97fd3..41ca8d54cb5 100644
--- a/src/server/game/Spells/SpellInfo.cpp
+++ b/src/server/game/Spells/SpellInfo.cpp
@@ -3357,22 +3357,40 @@ void SpellInfo::_InitializeExplicitTargetMask()
     ExplicitTargetMask = targetMask;
 }
 
-bool SpellInfo::_IsPositiveEffect(uint8 effIndex, bool deep) const
+inline bool _isPositiveTarget(SpellInfo const* spellInfo, uint8 effIndex)
 {
+    if (!spellInfo->Effects[effIndex].IsEffect())
+        return true;
+
+    return (spellInfo->Effects[effIndex].TargetA.GetCheckType() != TARGET_CHECK_ENEMY &&
+        spellInfo->Effects[effIndex].TargetB.GetCheckType() != TARGET_CHECK_ENEMY);
+}
+
+bool _isPositiveEffectImpl(SpellInfo const* spellInfo, uint8 effIndex, std::unordered_set<std::pair<uint32, uint8>>& visited)
+{
+    if (!spellInfo->Effects[effIndex].IsEffect())
+        return true;
+
+    // attribute may be already set in DB
+    if (!spellInfo->IsPositiveEffect(effIndex))
+        return false;
+
+    // passive auras like talents are all positive
+    if (spellInfo->IsPassive())
+        return true;
+
     // not found a single positive spell with this attribute
-    if (HasAttribute(SPELL_ATTR0_NEGATIVE_1))
+    if (spellInfo->HasAttribute(SPELL_ATTR0_NEGATIVE_1))
         return false;
 
-    switch (SpellFamilyName)
+    visited.insert({ spellInfo->Id, effIndex });
+
+    int32 bp = spellInfo->Effects[effIndex].CalcValue();
+    switch (spellInfo->SpellFamilyName)
     {
         case SPELLFAMILY_GENERIC:
-            switch (Id)
+            switch (spellInfo->Id)
             {
-                case 29214: // Wrath of the Plaguebringer
-                case 34700: // Allergic Reaction
-                case 41914: // Parasitic Shadowfiend (Illidan)
-                case 41917: // Parasitic Shadowfiend (Illidan)
-                case 54836: // Wrath of the Plaguebringer
                 case 61987: // Avenging Wrath Marker
                 case 61988: // Divine Shield exclude aura
                     return false;
@@ -3389,56 +3407,36 @@ bool SpellInfo::_IsPositiveEffect(uint8 effIndex, bool deep) const
             break;
         case SPELLFAMILY_MAGE:
             // Amplify Magic, Dampen Magic
-            if (SpellFamilyFlags[0] == 0x00002000)
+            if (spellInfo->SpellFamilyFlags[0] == 0x00002000)
                 return true;
-            // Impact
-            if (SpellIconID == 45)
-                return true;
-            // Arcane Missiles
-            if (SpellFamilyFlags[0] == 0x00000800)
+            // Permafrost (due to zero basepoint)
+            if (spellInfo->SpellFamilyFlags[2] == 0x00000010)
                 return false;
             break;
-        case SPELLFAMILY_PRIEST:
-            switch (Id)
-            {
-                case 64844: // Divine Hymn
-                case 64904: // Hymn of Hope
-                case 47585: // Dispersion
-                    return true;
-                default:
-                    break;
-            }
+        case SPELLFAMILY_WARRIOR:
+            // Slam, Execute
+            if ((spellInfo->SpellFamilyFlags[0] & 0x20200000) != 0)
+                return false;
+            break;
+        case SPELLFAMILY_PALADIN:
+            // assortment of judgement related spells
+            if (spellInfo->SpellFamilyFlags & flag96(0x208C0000, 0x00000208, 0x00000008))
+                return false;
             break;
         case SPELLFAMILY_HUNTER:
             // Aspect of the Viper
-            if (Id == 34074)
+            if (spellInfo->Id == 34074)
                 return true;
             break;
-        case SPELLFAMILY_SHAMAN:
-            if (Id == 30708)
+        case SPELLFAMILY_DRUID:
+            // Starfall
+            if (spellInfo->SpellFamilyFlags[2] == 0x00000100)
                 return false;
-            break;
-        case SPELLFAMILY_ROGUE:
-            switch (Id)
-            {
-                // Envenom must be considered as a positive effect even though it deals damage
-                case 32645:     // Envenom (Rank 1)
-                case 32684:     // Envenom (Rank 2)
-                case 57992:     // Envenom (Rank 3)
-                case 57993:     // Envenom (Rank 4)
-                // Slice and Dice. Prevents breaking Stealth
-                case 5171:      // Slice and Dice (Rank 1)
-                case 6774:      // Slice and Dice (Rank 2)
-                    return true;
-                default:
-                    break;
-            }
-            break;
         default:
             break;
     }
 
-    switch (Mechanic)
+    switch (spellInfo->Mechanic)
     {
         case MECHANIC_IMMUNE_SHIELD:
             return true;
@@ -3447,221 +3445,370 @@ bool SpellInfo::_IsPositiveEffect(uint8 effIndex, bool deep) const
     }
 
     // Special case: effects which determine positivity of whole spell
+    if (spellInfo->HasAttribute(SPELL_ATTR1_UNK11))
+    {
+        // check for targets, there seems to be an assortment of dummy triggering spells that should be negative
+        for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
+            if (!_isPositiveTarget(spellInfo, i))
+                return false;
+    }
+
+
     for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
     {
-        if (Effects[i].IsAura())
+        switch (spellInfo->Effects[i].Effect)
         {
-            switch (Effects[i].ApplyAuraName)
+            case SPELL_EFFECT_HEAL:
+            case SPELL_EFFECT_LEARN_SPELL:
+            case SPELL_EFFECT_SKILL_STEP:
+            case SPELL_EFFECT_HEAL_PCT:
+            case SPELL_EFFECT_ENERGIZE_PCT:
+                return true;
+            case SPELL_EFFECT_INSTAKILL:
+                if (i != effIndex && // for spells like 38044: instakill effect is negative but auras on target must count as buff
+                    spellInfo->Effects[i].TargetA.GetTarget() == spellInfo->Effects[effIndex].TargetA.GetTarget() &&
+                    spellInfo->Effects[i].TargetB.GetTarget() == spellInfo->Effects[effIndex].TargetB.GetTarget())
+                return false;
+            default:
+                break;
+        }
+
+        if (spellInfo->Effects[i].IsAura())
+        {
+            switch (spellInfo->Effects[i].ApplyAuraName)
             {
                 case SPELL_AURA_MOD_STEALTH:
+                case SPELL_AURA_MOD_UNATTACKABLE:
                     return true;
+                case SPELL_AURA_SCHOOL_HEAL_ABSORB:
                 case SPELL_AURA_CHANNEL_DEATH_ITEM:
                 case SPELL_AURA_EMPATHY:
                     return false;
+                default:
+                    break;
             }
         }
     }
 
-    switch (Effects[effIndex].Effect)
-    {
-        case SPELL_EFFECT_DUMMY:
-            // some explicitly required dummy effect sets
-            switch (Id)
+    switch (spellInfo->Effects[effIndex].Effect)
+    {
+        case SPELL_EFFECT_WEAPON_DAMAGE:
+        case SPELL_EFFECT_WEAPON_DAMAGE_NOSCHOOL:
+        case SPELL_EFFECT_NORMALIZED_WEAPON_DMG:
+        case SPELL_EFFECT_WEAPON_PERCENT_DAMAGE:
+        case SPELL_EFFECT_SCHOOL_DAMAGE:
+        case SPELL_EFFECT_ENVIRONMENTAL_DAMAGE:
+        case SPELL_EFFECT_HEALTH_LEECH:
+        case SPELL_EFFECT_INSTAKILL:
+        case SPELL_EFFECT_POWER_DRAIN:
+        case SPELL_EFFECT_STEAL_BENEFICIAL_BUFF:
+        case SPELL_EFFECT_INTERRUPT_CAST:
+        case SPELL_EFFECT_PICKPOCKET:
+        case SPELL_EFFECT_GAMEOBJECT_DAMAGE:
+        case SPELL_EFFECT_DURABILITY_DAMAGE:
+        case SPELL_EFFECT_DURABILITY_DAMAGE_PCT:
+        case SPELL_EFFECT_APPLY_AREA_AURA_ENEMY:
+        case SPELL_EFFECT_TAMECREATURE:
+        case SPELL_EFFECT_DISTRACT:
+            return false;
+        case SPELL_EFFECT_ENERGIZE:
+        case SPELL_EFFECT_ENERGIZE_PCT:
+        case SPELL_EFFECT_HEAL_PCT:
+        case SPELL_EFFECT_HEAL_MAX_HEALTH:
+        case SPELL_EFFECT_HEAL_MECHANICAL:
+            return true;
+        case SPELL_EFFECT_KNOCK_BACK:
+        case SPELL_EFFECT_CHARGE:
+        case SPELL_EFFECT_PERSISTENT_AREA_AURA:
+        case SPELL_EFFECT_ATTACK_ME:
+        case SPELL_EFFECT_POWER_BURN:
+            // check targets
+            if (!_isPositiveTarget(spellInfo, effIndex))
+                return false;
+            break;
+        case SPELL_EFFECT_DISPEL:
+            // non-positive dispel
+            switch (spellInfo->Effects[effIndex].MiscValue)
             {
-                case 28441:
-                    return false; // AB Effect 000
+                case DISPEL_STEALTH:
+                case DISPEL_INVISIBILITY:
+                case DISPEL_ENRAGE:
+                    return false;
                 default:
                     break;
             }
-            break;
-        // always positive effects (check before target checks that provided non-positive result in some case for positive effects)
-        case SPELL_EFFECT_HEAL:
-        case SPELL_EFFECT_LEARN_SPELL:
-        case SPELL_EFFECT_SKILL_STEP:
-        case SPELL_EFFECT_HEAL_PCT:
-        case SPELL_EFFECT_ENERGIZE_PCT:
-            return true;
-        case SPELL_EFFECT_APPLY_AREA_AURA_ENEMY:
-            return false;
 
-            // non-positive aura use
-        case SPELL_EFFECT_APPLY_AURA:
-        case SPELL_EFFECT_APPLY_AREA_AURA_FRIEND:
-        {
-            switch (Effects[effIndex].ApplyAuraName)
+            // also check targets
+            if (!_isPositiveTarget(spellInfo, effIndex))
+                return false;
+            break;
+        case SPELL_EFFECT_DISPEL_MECHANIC:
+            if (!_isPositiveTarget(spellInfo, effIndex))
             {
-                case SPELL_AURA_MOD_DAMAGE_DONE:            // dependent from bas point sign (negative -> negative)
-                case SPELL_AURA_MOD_STAT:
-                case SPELL_AURA_MOD_SKILL:
-                case SPELL_AURA_MOD_DODGE_PERCENT:
-                case SPELL_AURA_MOD_HEALING_PCT:
-                case SPELL_AURA_MOD_HEALING_DONE:
-                case SPELL_AURA_MOD_DAMAGE_PERCENT_DONE:
-                    if (Effects[effIndex].CalcValue() < 0)
-                        return false;
-                    break;
-                case SPELL_AURA_MOD_DAMAGE_TAKEN:           // dependent from bas point sign (positive -> negative)
-                    if (Effects[effIndex].CalcValue() > 0)
+                // non-positive mechanic dispel on negative target
+                switch (spellInfo->Effects[effIndex].MiscValue)
+                {
+                    case MECHANIC_BANDAGE:
+                    case MECHANIC_SHIELD:
+                    case MECHANIC_MOUNT:
+                    case MECHANIC_INVULNERABILITY:
                         return false;
-                    break;
-                case SPELL_AURA_MOD_CRIT_PCT:
-                case SPELL_AURA_MOD_SPELL_CRIT_CHANCE:
-                    if (Effects[effIndex].CalcValue() > 0)
-                        return true;                        // some expected positive spells have SPELL_ATTR1_NEGATIVE
-                    break;
-                case SPELL_AURA_ADD_TARGET_TRIGGER:
-                    return true;
-                case SPELL_AURA_PERIODIC_TRIGGER_SPELL_WITH_VALUE:
-                case SPELL_AURA_PERIODIC_TRIGGER_SPELL:
-                    if (!deep)
+                    default:
+                        break;
+                }
+            }
+            break;
+        case SPELL_EFFECT_THREAT:
+        case SPELL_EFFECT_MODIFY_THREAT_PERCENT:
+            // check targets AND basepoints
+            if (!_isPositiveTarget(spellInfo, effIndex) && bp > 0)
+                return false;
+            break;
+        default:
+            break;
+    }
+
+    if (spellInfo->Effects[effIndex].IsAura())
+    {
+        // non-positive aura use
+        switch (spellInfo->Effects[effIndex].ApplyAuraName)
+        {
+            case SPELL_AURA_MOD_DAMAGE_DONE:            // dependent from basepoint sign (negative -> negative)
+            case SPELL_AURA_MOD_STAT:
+            case SPELL_AURA_MOD_SKILL:
+            case SPELL_AURA_MOD_DODGE_PERCENT:
+            case SPELL_AURA_MOD_HEALING_PCT:
+            case SPELL_AURA_MOD_HEALING_DONE:
+            case SPELL_AURA_MOD_DAMAGE_DONE_CREATURE:
+            case SPELL_AURA_OBS_MOD_HEALTH:
+            case SPELL_AURA_OBS_MOD_POWER:
+            case SPELL_AURA_MOD_CRIT_PCT:
+            case SPELL_AURA_MOD_HIT_CHANCE:
+            case SPELL_AURA_MOD_SPELL_HIT_CHANCE:
+            case SPELL_AURA_MOD_SPELL_CRIT_CHANCE:
+            case SPELL_AURA_MOD_RANGED_HASTE:
+            case SPELL_AURA_MOD_CASTING_SPEED_NOT_STACK:
+            case SPELL_AURA_HASTE_SPELLS:
+            case SPELL_AURA_MOD_RESISTANCE:
+            case SPELL_AURA_MOD_RESISTANCE_EXCLUSIVE:
+            case SPELL_AURA_MOD_DETECT_RANGE:
+            case SPELL_AURA_MOD_INCREASE_HEALTH_PERCENT:
+            case SPELL_AURA_MOD_TOTAL_STAT_PERCENTAGE:
+            case SPELL_AURA_MOD_INCREASE_SWIM_SPEED:
+                if (bp < 0)
+                    return false;
+                break;
+            case SPELL_AURA_MOD_ATTACKSPEED:            // some buffs have negative bp, check both target and bp
+            case SPELL_AURA_MOD_MELEE_HASTE:
+            case SPELL_AURA_MOD_RESISTANCE_PCT:
+            case SPELL_AURA_MOD_RATING:
+            case SPELL_AURA_MOD_ATTACK_POWER:
+            case SPELL_AURA_MOD_RANGED_ATTACK_POWER:
+            case SPELL_AURA_MOD_DAMAGE_PERCENT_DONE:
+                if (!_isPositiveTarget(spellInfo, effIndex) && bp < 0)
+                    return false;
+                break;
+            case SPELL_AURA_MOD_DAMAGE_TAKEN:           // dependent from basepoint sign (positive -> negative)
+            case SPELL_AURA_MOD_MELEE_DAMAGE_TAKEN:
+            case SPELL_AURA_MOD_MELEE_DAMAGE_TAKEN_PCT:
+            case SPELL_AURA_MOD_SCHOOL_CRIT_DMG_TAKEN:
+            case SPELL_AURA_MOD_ATTACKER_SPELL_CRIT_CHANCE:
+            case SPELL_AURA_MOD_POWER_COST_SCHOOL:
+            case SPELL_AURA_MOD_POWER_COST_SCHOOL_PCT:
+                if (bp > 0)
+                    return false;
+                break;
+            case SPELL_AURA_MOD_DAMAGE_PERCENT_TAKEN:   // check targets and basepoints (ex Recklessness)
+                if (!_isPositiveTarget(spellInfo, effIndex) && bp > 0)
+                    return false;
+                break;
+            case SPELL_AURA_ADD_TARGET_TRIGGER:
+                return true;
+            case SPELL_AURA_PERIODIC_TRIGGER_SPELL_WITH_VALUE:
+            case SPELL_AURA_PERIODIC_TRIGGER_SPELL:
+                if (!_isPositiveTarget(spellInfo, effIndex))
+                {
+                    if (SpellInfo const* spellTriggeredProto = sSpellMgr->GetSpellInfo(spellInfo->Effects[effIndex].TriggerSpell))
                     {
-                        if (SpellInfo const* spellTriggeredProto = sSpellMgr->GetSpellInfo(Effects[effIndex].TriggerSpell))
+                        // negative targets of main spell return early
+                        for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
                         {
-                            // negative targets of main spell return early
-                            for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
-                            {
-                                if (!spellTriggeredProto->Effects[i].Effect)
-                                    continue;
-                                // if non-positive trigger cast targeted to positive target this main cast is non-positive
-                                // this will place this spell auras as debuffs
-                                if (_IsPositiveTarget(spellTriggeredProto->Effects[i].TargetA.GetTarget(), spellTriggeredProto->Effects[i].TargetB.GetTarget()) && !spellTriggeredProto->_IsPositiveEffect(i, true))
-                                    return false;
-                            }
+                            // already seen this
+                            if (visited.count({ spellTriggeredProto->Id, i }) > 0)
+                                continue;
+
+                            if (!spellTriggeredProto->Effects[i].Effect)
+                                continue;
+
+                            // if non-positive trigger cast targeted to positive target this main cast is non-positive
+                            // this will place this spell auras as debuffs
+                            if (_isPositiveTarget(spellTriggeredProto, i) && !_isPositiveEffectImpl(spellTriggeredProto, i, visited))
+                                return false;
                         }
                     }
-                case SPELL_AURA_PROC_TRIGGER_SPELL:
-                    // many positive auras have negative triggered spells at damage for example and this not make it negative (it can be canceled for example)
-                    break;
-                case SPELL_AURA_MOD_STUN:                   //have positive and negative spells, we can't sort its correctly at this moment.
-                    if (effIndex == 0 && Effects[1].Effect == 0 && Effects[2].Effect == 0)
-                        return false;                       // but all single stun aura spells is negative
-                    break;
-                case SPELL_AURA_MOD_PACIFY_SILENCE:
-                    if (Id == 24740)             // Wisp Costume
-                        return true;
-                    return false;
-                case SPELL_AURA_MOD_ROOT:
-                case SPELL_AURA_MOD_SILENCE:
-                case SPELL_AURA_GHOST:
-                case SPELL_AURA_PERIODIC_LEECH:
-                case SPELL_AURA_MOD_STALKED:
-                case SPELL_AURA_PERIODIC_DAMAGE_PERCENT:
-                case SPELL_AURA_PREVENT_RESURRECTION:
+                }
+                break;
+            case SPELL_AURA_MOD_STUN:
+            case SPELL_AURA_TRANSFORM:
+            case SPELL_AURA_MOD_DECREASE_SPEED:
+            case SPELL_AURA_MOD_FEAR:
+            case SPELL_AURA_MOD_TAUNT:
+                // special auras: they may have non negative target but still need to be marked as debuff
+                // checked again after all effects (SpellInfo::_InitializeSpellPositivity)
+            case SPELL_AURA_MOD_PACIFY:
+            case SPELL_AURA_MOD_PACIFY_SILENCE:
+            case SPELL_AURA_MOD_DISARM:
+            case SPELL_AURA_MOD_DISARM_OFFHAND:
+            case SPELL_AURA_MOD_DISARM_RANGED:
+            case SPELL_AURA_MOD_CHARM:
+            case SPELL_AURA_AOE_CHARM:
+            case SPELL_AURA_MOD_POSSESS:
+            case SPELL_AURA_MOD_LANGUAGE:
+            case SPELL_AURA_DAMAGE_SHIELD:
+            case SPELL_AURA_PROC_TRIGGER_SPELL:
+            case SPELL_AURA_MOD_ATTACKER_MELEE_HIT_CHANCE:
+            case SPELL_AURA_MOD_ATTACKER_RANGED_HIT_CHANCE:
+            case SPELL_AURA_MOD_ATTACKER_SPELL_HIT_CHANCE:
+            case SPELL_AURA_MOD_ATTACKER_MELEE_CRIT_CHANCE:
+            case SPELL_AURA_MOD_ATTACKER_RANGED_CRIT_CHANCE:
+            case SPELL_AURA_MOD_ATTACKER_MELEE_CRIT_DAMAGE:
+            case SPELL_AURA_MOD_ATTACKER_RANGED_CRIT_DAMAGE:
+            case SPELL_AURA_MOD_ATTACKER_SPELL_AND_WEAPON_CRIT_CHANCE:
+                // have positive and negative spells, check target
+                if (!_isPositiveTarget(spellInfo, effIndex))
                     return false;
-                case SPELL_AURA_PERIODIC_DAMAGE:            // used in positive spells also.
-                    // part of negative spell if cast at self (prevent cancel)
-                    if (Effects[effIndex].TargetA.GetTarget() == TARGET_UNIT_CASTER)
-                        return false;
-                    break;
-                case SPELL_AURA_MOD_DECREASE_SPEED:         // used in positive spells also
-                    // part of positive spell if cast at self
-                    if (Effects[effIndex].TargetA.GetTarget() != TARGET_UNIT_CASTER)
-                        return false;
-                    // but not this if this first effect (didn't find better check)
-                    if (HasAttribute(SPELL_ATTR0_NEGATIVE_1) && effIndex == 0)
-                        return false;
-                    break;
-                case SPELL_AURA_MECHANIC_IMMUNITY:
+                break;
+            case SPELL_AURA_MOD_CONFUSE:
+            case SPELL_AURA_MOD_ROOT:
+            case SPELL_AURA_MOD_SILENCE:
+            case SPELL_AURA_MOD_DETAUNT:
+            case SPELL_AURA_GHOST:
+            case SPELL_AURA_PERIODIC_LEECH:
+            case SPELL_AURA_PERIODIC_MANA_LEECH:
+            case SPELL_AURA_MOD_STALKED:
+            case SPELL_AURA_PREVENT_RESURRECTION:
+            case SPELL_AURA_PERIODIC_DAMAGE:
+            case SPELL_AURA_PERIODIC_DAMAGE_PERCENT:
+            case SPELL_AURA_MELEE_ATTACK_POWER_ATTACKER_BONUS:
+            case SPELL_AURA_RANGED_ATTACK_POWER_ATTACKER_BONUS:
+                return false;
+            case SPELL_AURA_MECHANIC_IMMUNITY:
+            {
+                // non-positive immunities
+                switch (spellInfo->Effects[effIndex].MiscValue)
                 {
-                    // non-positive immunities
-                    switch (Effects[effIndex].MiscValue)
-                    {
-                        case MECHANIC_BANDAGE:
-                        case MECHANIC_SHIELD:
-                        case MECHANIC_MOUNT:
-                        case MECHANIC_INVULNERABILITY:
-                            return false;
-                        default:
-                            break;
-                    }
-                    break;
+                    case MECHANIC_BANDAGE:
+                    case MECHANIC_SHIELD:
+                    case MECHANIC_MOUNT:
+                    case MECHANIC_INVULNERABILITY:
+                        return false;
+                    default:
+                        break;
                 }
-                case SPELL_AURA_ADD_FLAT_MODIFIER:          // mods
-                case SPELL_AURA_ADD_PCT_MODIFIER:
+                break;
+            }
+            case SPELL_AURA_ADD_FLAT_MODIFIER:          // mods
+            case SPELL_AURA_ADD_PCT_MODIFIER:
+            {
+                switch (spellInfo->Effects[effIndex].MiscValue)
                 {
-                    // non-positive mods
-                    switch (Effects[effIndex].MiscValue)
-                    {
-                        case SPELLMOD_COST:                 // dependent from bas point sign (negative -> positive)
-                            if (Effects[effIndex].CalcValue() > 0)
-                            {
-                                if (!deep)
-                                {
-                                    bool negative = true;
-                                    for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
-                                    {
-                                        if (i != effIndex)
-                                            if (_IsPositiveEffect(i, true))
-                                            {
-                                                negative = false;
-                                                break;
-                                            }
-                                    }
-                                    if (negative)
-                                        return false;
-                                }
-                            }
-                            break;
-                        default:
-                            break;
-                    }
-                    break;
+                    case SPELLMOD_CASTING_TIME:             // dependent from basepoint sign (positive -> negative)
+                    case SPELLMOD_ACTIVATION_TIME:
+                    case SPELLMOD_SPELL_COST_REFUND_ON_FAIL:
+                    case SPELLMOD_GLOBAL_COOLDOWN:
+                        if (bp > 0)
+                            return false;
+                        break;
+                    case SPELLMOD_COOLDOWN:
+                    case SPELLMOD_COST:
+                        if (!spellInfo->IsPositive() && bp > 0) // dependent on prev effects too (ex Arcane Power)
+                            return false;
+                        break;
+                    case SPELLMOD_EFFECT1:                  // always positive
+                    case SPELLMOD_EFFECT2:
+                    case SPELLMOD_EFFECT3:
+                    case SPELLMOD_ALL_EFFECTS:
+                    case SPELLMOD_THREAT:
+                    case SPELLMOD_DAMAGE_MULTIPLIER:
+                    case SPELLMOD_VALUE_MULTIPLIER:
+                        return true;
+                    case SPELLMOD_DURATION:
+                    case SPELLMOD_CRITICAL_CHANCE:
+                    case SPELLMOD_DAMAGE:
+                    case SPELLMOD_JUMP_TARGETS:
+                        if (!spellInfo->IsPositive() && bp < 0) // dependent on prev effects too
+                            return false;
+                        break;
+                    default:                                // dependent from basepoint sign (negative -> negative)
+                        if (bp < 0)
+                            return false;
+                        break;
                 }
-                default:
-                    break;
+                break;
             }
-            break;
+            default:
+                break;
         }
-        default:
-            break;
     }
 
-    // non-positive targets
-    if (!_IsPositiveTarget(Effects[effIndex].TargetA.GetTarget(), Effects[effIndex].TargetB.GetTarget()))
-        return false;
-
     // negative spell if triggered spell is negative
-    if (!deep && !Effects[effIndex].ApplyAuraName && Effects[effIndex].TriggerSpell)
+    if (!spellInfo->Effects[effIndex].ApplyAuraName && spellInfo->Effects[effIndex].TriggerSpell)
     {
-        if (SpellInfo const* spellTriggeredProto = sSpellMgr->GetSpellInfo(Effects[effIndex].TriggerSpell))
-            if (!spellTriggeredProto->_IsPositiveSpell())
-                return false;
+        if (SpellInfo const* spellTriggeredProto = sSpellMgr->GetSpellInfo(spellInfo->Effects[effIndex].TriggerSpell))
+        {
+            // spells with at least one negative effect are considered negative
+            // some self-applied spells have negative effects but in self casting case negative check ignored.
+            for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
+            {
+                // already seen this
+                if (visited.count({ spellTriggeredProto->Id, i }) > 0)
+                    continue;
+
+                if (!spellTriggeredProto->Effects[i].IsEffect())
+                    continue;
+
+                if (!_isPositiveEffectImpl(spellTriggeredProto, i, visited))
+                    return false;
+            }
+        }
     }
 
     // ok, positive
     return true;
 }
 
-bool SpellInfo::_IsPositiveSpell() const
+void SpellInfo::_InitializeSpellPositivity()
 {
-    // spells with at least one negative effect are considered negative
-    // some self-applied spells have negative effects but in self casting case negative check ignored.
+    std::unordered_set<std::pair<uint32 /*spellId*/, uint8 /*effIndex*/>> visited;
+
     for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
-        if (!_IsPositiveEffect(i, true))
-            return false;
-    return true;
-}
+        if (!_isPositiveEffectImpl(this, i, visited))
+            AttributesCu |= (SPELL_ATTR0_CU_NEGATIVE_EFF0 << i);
 
-bool SpellInfo::_IsPositiveTarget(uint32 targetA, uint32 targetB)
-{
-    // non-positive targets
-    switch (targetA)
+    // additional checks after effects marked
+    for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
     {
-        case TARGET_UNIT_NEARBY_ENEMY:
-        case TARGET_UNIT_TARGET_ENEMY:
-        case TARGET_UNIT_SRC_AREA_ENEMY:
-        case TARGET_UNIT_DEST_AREA_ENEMY:
-        case TARGET_UNIT_CONE_ENEMY_24:
-        case TARGET_UNIT_CONE_ENEMY_104:
-        case TARGET_DEST_DYNOBJ_ENEMY:
-        case TARGET_DEST_TARGET_ENEMY:
-            return false;
-        default:
-            break;
+        if (!Effects[i].IsEffect() || !IsPositiveEffect(i))
+            continue;
+
+        switch (Effects[i].ApplyAuraName)
+        {
+            // has other non positive effect?
+            // then it should be marked negative despite of targets (ex 8510, 8511, 8893, 10267)
+            case SPELL_AURA_DUMMY:
+            case SPELL_AURA_MOD_STUN:
+            case SPELL_AURA_MOD_FEAR:
+            case SPELL_AURA_MOD_TAUNT:
+            case SPELL_AURA_TRANSFORM:
+            case SPELL_AURA_MOD_ATTACKSPEED:
+            case SPELL_AURA_MOD_DECREASE_SPEED:
+                if (!IsPositive())
+                    AttributesCu |= (SPELL_ATTR0_CU_NEGATIVE_EFF0 << i);
+                break;
+            default:
+                break;
+        }
     }
-    if (targetB)
-        return _IsPositiveTarget(targetB, 0);
-    return true;
 }
 
 void SpellInfo::_UnloadImplicitTargetConditionLists()
diff --git a/src/server/game/Spells/SpellInfo.h b/src/server/game/Spells/SpellInfo.h
index 5670a9052a2..cd86f8b9c9e 100644
--- a/src/server/game/Spells/SpellInfo.h
+++ b/src/server/game/Spells/SpellInfo.h
@@ -535,9 +535,7 @@ class TC_GAME_API SpellInfo
     private:
         // loading helpers
         void _InitializeExplicitTargetMask();
-        bool _IsPositiveEffect(uint8 effIndex, bool deep) const;
-        bool _IsPositiveSpell() const;
-        static bool _IsPositiveTarget(uint32 targetA, uint32 targetB);
+        void _InitializeSpellPositivity();
         void _LoadSpellSpecific();
         void _LoadAuraState();
         void _LoadSpellDiminishInfo();
diff --git a/src/server/game/Spells/SpellMgr.cpp b/src/server/game/Spells/SpellMgr.cpp
index aaeb40c7fc9..fd57fd5f8ba 100644
--- a/src/server/game/Spells/SpellMgr.cpp
+++ b/src/server/game/Spells/SpellMgr.cpp
@@ -2628,7 +2628,20 @@ void SpellMgr::LoadSpellInfoCustomAttributes()
                 continue;
             }
 
-            // TODO: validate attributes
+            if ((attributes & SPELL_ATTR0_CU_NEGATIVE) != 0)
+            {
+                for (uint8 i = 0; i < MAX_SPELL_EFFECTS; ++i)
+                {
+                    if (spellInfo->Effects[i].IsEffect())
+                        continue;
+
+                    if ((attributes & (SPELL_ATTR0_CU_NEGATIVE_EFF0 << i)) != 0)
+                    {
+                        TC_LOG_ERROR("sql.sql", "Table `spell_custom_attr` has attribute SPELL_ATTR0_CU_NEGATIVE_EFF%u for spell %u with no EFFECT_%u", uint32(i), spellId, uint32(i));
+                        continue;
+                    }
+                }
+            }
 
             spellInfo->AttributesCu |= attributes;
             ++count;
@@ -2829,14 +2842,7 @@ void SpellMgr::LoadSpellInfoCustomAttributes()
             spellInfo->AttributesCu |= SPELL_ATTR0_CU_SCHOOLMASK_NORMAL_WITH_MAGIC;
         }
 
-        if (!spellInfo->_IsPositiveEffect(EFFECT_0, false))
-            spellInfo->AttributesCu |= SPELL_ATTR0_CU_NEGATIVE_EFF0;
-
-        if (!spellInfo->_IsPositiveEffect(EFFECT_1, false))
-            spellInfo->AttributesCu |= SPELL_ATTR0_CU_NEGATIVE_EFF1;
-
-        if (!spellInfo->_IsPositiveEffect(EFFECT_2, false))
-            spellInfo->AttributesCu |= SPELL_ATTR0_CU_NEGATIVE_EFF2;
+        spellInfo->_InitializeSpellPositivity();
 
         if (spellInfo->SpellVisual[0] == 3879)
             spellInfo->AttributesCu |= SPELL_ATTR0_CU_CONE_BACK;
@@ -3122,6 +3128,15 @@ void SpellMgr::LoadSpellInfoCorrections()
         spellInfo->Effects[EFFECT_0].MiscValueB = 64;
     });
 
+    // Battlegear of Eternal Justice
+    ApplySpellFix({
+        26135, // Battlegear of Eternal Justice
+        37557  // Mark of Light
+    }, [](SpellInfo* spellInfo)
+    {
+        spellInfo->SpellFamilyFlags = flag96();
+    });
+
     ApplySpellFix({
         40244, // Simon Game Visual
         40245, // Simon Game Visual