Address akortunov's concerns regarding spell refactoring

Separate linked effect handling into linked effects header
Separate spell absorption handling into spell absorption header
Make armor disintegration loop a range-based for loop

apps/openmw/CMakeLists.txt

@@ -86,6 +86,7 @@ add_openmw_dir (mwmechanics
     aicast aiescort aiface aiactivate aicombat recharge repair enchanting pathfinding pathgrid security spellcasting spellresistance
     disease pickpocket levelledlist combat steering obstacle autocalcspell difficultyscaling aicombataction actor summoning
     character actors objects aistate coordinateconverter trading weaponpriority spellpriority weapontype spellutil tickableeffects
+    spellabsorption linkedeffects
     )
 
 add_openmw_dir (mwstate

apps/openmw/mwmechanics/linkedeffects.cpp

@@ -0,0 +1,71 @@
+#include "linkedeffects.hpp"
+
+#include <components/misc/rng.hpp>
+#include <components/settings/settings.hpp>
+
+#include "../mwbase/environment.hpp"
+#include "../mwbase/world.hpp"
+
+#include "../mwrender/animation.hpp"
+
+#include "../mwworld/class.hpp"
+#include "../mwworld/esmstore.hpp"
+
+#include "creaturestats.hpp"
+
+namespace MWMechanics
+{
+
+    bool reflectEffect(const ESM::ENAMstruct& effect, const ESM::MagicEffect* magicEffect,
+                       const MWWorld::Ptr& caster, const MWWorld::Ptr& target, ESM::EffectList& reflectedEffects)
+    {
+        if (caster.isEmpty() || caster == target || !target.getClass().isActor())
+            return false;
+
+        bool isHarmful = magicEffect->mData.mFlags & ESM::MagicEffect::Harmful;
+        bool isUnreflectable = magicEffect->mData.mFlags & ESM::MagicEffect::Unreflectable;
+        if (!isHarmful || isUnreflectable)
+            return false;
+
+        float reflect = target.getClass().getCreatureStats(target).getMagicEffects().get(ESM::MagicEffect::Reflect).getMagnitude();
+        if (Misc::Rng::roll0to99() >= reflect)
+            return false;
+
+        const ESM::Static* reflectStatic = MWBase::Environment::get().getWorld()->getStore().get<ESM::Static>().find ("VFX_Reflect");
+        MWRender::Animation* animation = MWBase::Environment::get().getWorld()->getAnimation(target);
+        if (animation && !reflectStatic->mModel.empty())
+            animation->addEffect("meshes\\" + reflectStatic->mModel, ESM::MagicEffect::Reflect, false, std::string());
+        reflectedEffects.mList.emplace_back(effect);
+        return true;
+    }
+
+    void absorbStat(const ESM::ENAMstruct& effect, const ESM::ActiveEffect& appliedEffect,
+                    const MWWorld::Ptr& caster, const MWWorld::Ptr& target, bool reflected, const std::string& source)
+    {
+        if (caster.isEmpty() || caster == target)
+            return;
+
+        if (!target.getClass().isActor() || !caster.getClass().isActor())
+            return;
+
+        // Make sure callers don't do something weird
+        if (effect.mEffectID < ESM::MagicEffect::AbsorbAttribute || effect.mEffectID > ESM::MagicEffect::AbsorbSkill)
+            throw std::runtime_error("invalid absorb stat effect");
+
+        std::vector<ActiveSpells::ActiveEffect> absorbEffects;
+        ActiveSpells::ActiveEffect absorbEffect = appliedEffect;
+        absorbEffect.mMagnitude *= -1;
+        absorbEffects.emplace_back(absorbEffect);
+
+        // Morrowind negates reflected Absorb spells so the original caster won't be harmed.
+        if (reflected && Settings::Manager::getBool("classic reflected absorb spells behavior", "Game"))
+        {
+            target.getClass().getCreatureStats(target).getActiveSpells().addSpell(std::string(), true,
+                            absorbEffects, source, caster.getClass().getCreatureStats(caster).getActorId());
+            return;
+        }
+
+        caster.getClass().getCreatureStats(caster).getActiveSpells().addSpell(std::string(), true,
+                        absorbEffects, source, target.getClass().getCreatureStats(target).getActorId());
+    }
+}

apps/openmw/mwmechanics/linkedeffects.hpp

@@ -0,0 +1,32 @@
+#ifndef MWMECHANICS_LINKEDEFFECTS_H
+#define MWMECHANICS_LINKEDEFFECTS_H
+
+#include <string>
+
+namespace ESM
+{
+    struct ActiveEffect;
+    struct EffectList;
+    struct ENAMstruct;
+    struct MagicEffect;
+    struct Spell;
+}
+
+namespace MWWorld
+{
+    class Ptr;
+}
+
+namespace MWMechanics
+{
+
+    // Try to reflect a spell effect. If it's reflected, it's also put into the passed reflected effects list.
+    bool reflectEffect(const ESM::ENAMstruct& effect, const ESM::MagicEffect* magicEffect,
+                       const MWWorld::Ptr& caster, const MWWorld::Ptr& target, ESM::EffectList& reflectedEffects);
+
+    // Try to absorb a stat (skill, attribute, etc.) from the target and transfer it to the caster.
+    void absorbStat(const ESM::ENAMstruct& effect, const ESM::ActiveEffect& appliedEffect,
+                    const MWWorld::Ptr& caster, const MWWorld::Ptr& target, bool reflected, const std::string& source);
+}
+
+#endif

apps/openmw/mwmechanics/spellabsorption.cpp

@@ -0,0 +1,76 @@
+#include "spellabsorption.hpp"
+
+#include <components/misc/rng.hpp>
+
+#include "../mwbase/environment.hpp"
+#include "../mwbase/world.hpp"
+
+#include "../mwrender/animation.hpp"
+
+#include "../mwworld/class.hpp"
+#include "../mwworld/esmstore.hpp"
+#include "../mwworld/inventorystore.hpp"
+
+#include "creaturestats.hpp"
+
+namespace MWMechanics
+{
+
+    class GetAbsorptionProbability : public MWMechanics::EffectSourceVisitor
+    {
+    public:
+        float mProbability{0.f};
+
+        GetAbsorptionProbability() = default;
+
+        virtual void visit (MWMechanics::EffectKey key,
+                                const std::string& /*sourceName*/, const std::string& /*sourceId*/, int /*casterActorId*/,
+                            float magnitude, float /*remainingTime*/, float /*totalTime*/)
+        {
+            if (key.mId == ESM::MagicEffect::SpellAbsorption)
+            {
+                if (mProbability == 0.f)
+                    mProbability = magnitude / 100;
+                else
+                {
+                    // If there are different sources of SpellAbsorption effect, multiply failing probability for all effects.
+                    // Real absorption probability will be the (1 - total fail chance) in this case.
+                    float failProbability = 1.f - mProbability;
+                    failProbability *= 1.f - magnitude / 100;
+                    mProbability = 1.f - failProbability;
+                }
+            }
+        }
+    };
+
+    bool absorbSpell (const ESM::Spell* spell, const MWWorld::Ptr& caster, const MWWorld::Ptr& target)
+    {
+        if (!spell || caster == target || !target.getClass().isActor())
+            return false;
+
+        CreatureStats& stats = target.getClass().getCreatureStats(target);
+        if (stats.getMagicEffects().get(ESM::MagicEffect::SpellAbsorption).getMagnitude() <= 0.f)
+            return false;
+
+        GetAbsorptionProbability check;
+        stats.getActiveSpells().visitEffectSources(check);
+        stats.getSpells().visitEffectSources(check);
+        if (target.getClass().hasInventoryStore(target))
+            target.getClass().getInventoryStore(target).visitEffectSources(check);
+
+        int chance = check.mProbability * 100;
+        if (Misc::Rng::roll0to99() >= chance)
+            return false;
+
+        const ESM::Static* absorbStatic = MWBase::Environment::get().getWorld()->getStore().get<ESM::Static>().find("VFX_Absorb");
+        MWRender::Animation* animation = MWBase::Environment::get().getWorld()->getAnimation(target);
+        if (animation && !absorbStatic->mModel.empty())
+            animation->addEffect( "meshes\\" + absorbStatic->mModel, ESM::MagicEffect::SpellAbsorption, false, std::string());
+        // Magicka is increased by the cost of the spell
+        DynamicStat<float> magicka = stats.getMagicka();
+        magicka.setCurrent(magicka.getCurrent() + spell->mData.mCost);
+        stats.setMagicka(magicka);
+        return true;
+    }
+
+}

apps/openmw/mwmechanics/spellabsorption.hpp

@@ -0,0 +1,20 @@
+#ifndef MWMECHANICS_SPELLABSORPTION_H
+#define MWMECHANICS_SPELLABSORPTION_H
+
+namespace ESM
+{
+    struct Spell;
+}
+
+namespace MWWorld
+{
+    class Ptr;
+}
+
+namespace MWMechanics
+{
+    // Try to absorb a spell based on the magnitude of every Spell Absorption effect source on the target.
+    bool absorbSpell(const ESM::Spell* spell, const MWWorld::Ptr& caster, const MWWorld::Ptr& target);
+}
+
+#endif

apps/openmw/mwmechanics/spellcasting.cpp

@@ -2,7 +2,6 @@
 
 #include <components/misc/constants.hpp>
 #include <components/misc/rng.hpp>
-#include <components/settings/settings.hpp>
 
 #include "../mwbase/windowmanager.hpp"
 #include "../mwbase/soundmanager.hpp"
@@ -20,44 +19,19 @@
 
 #include "../mwrender/animation.hpp"
 
-#include "creaturestats.hpp"
 #include "actorutil.hpp"
 #include "aifollow.hpp"
-#include "weapontype.hpp"
+#include "creaturestats.hpp"
-#include "summoning.hpp"
+#include "linkedeffects.hpp"
+#include "spellabsorption.hpp"
 #include "spellresistance.hpp"
 #include "spellutil.hpp"
+#include "summoning.hpp"
 #include "tickableeffects.hpp"
+#include "weapontype.hpp"
 
 namespace MWMechanics
 {
-    class GetAbsorptionProbability : public MWMechanics::EffectSourceVisitor
-    {
-    public:
-        float mProbability{0.f};
-
-        GetAbsorptionProbability() = default;
-
-        virtual void visit (MWMechanics::EffectKey key,
-                                const std::string& sourceName, const std::string& sourceId, int casterActorId,
-                            float magnitude, float remainingTime = -1, float totalTime = -1)
-        {
-            if (key.mId == ESM::MagicEffect::SpellAbsorption)
-            {
-                if (mProbability == 0.f)
-                    mProbability = magnitude / 100;
-                else
-                {
-                    // If there are different sources of SpellAbsorption effect, multiply failing probability for all effects.
-                    // Real absorption probability will be the (1 - total fail chance) in this case.
-                    float failProbability = 1.f - mProbability;
-                    failProbability *= 1.f - magnitude / 100;
-                    mProbability = 1.f - failProbability;
-                }
-            }
-        }
-    };
-
     CastSpell::CastSpell(const MWWorld::Ptr &caster, const MWWorld::Ptr &target, const bool fromProjectile, const bool manualSpell)
         : mCaster(caster)
         , mTarget(target)
@@ -172,57 +146,27 @@ namespace MWMechanics
                     && target.getClass().isActor())
                 MWBase::Environment::get().getWindowManager()->setEnemy(target);
 
-            // Try absorbing if it's a spell
+            // Try absorbing the spell
-            // Unlike Reflect, this is done once per spell absorption effect source
+            // FIXME: this should be done only once for the spell
             bool absorbed = false;
-            if (spell && caster != target && target.getClass().isActor())
+            if (absorbSpell(spell, caster, target))
             {
-                CreatureStats& stats = target.getClass().getCreatureStats(target);
+                absorbed = true;
-                if (stats.getMagicEffects().get(ESM::MagicEffect::SpellAbsorption).getMagnitude() > 0.f)
+                continue;
-                {
+            }
-                    GetAbsorptionProbability check;
-                    stats.getActiveSpells().visitEffectSources(check);
-                    stats.getSpells().visitEffectSources(check);
-                    if (target.getClass().hasInventoryStore(target))
-                        target.getClass().getInventoryStore(target).visitEffectSources(check);
 
-                    int absorb = check.mProbability * 100;
+            // Reflect harmful effects
-                    absorbed = (Misc::Rng::roll0to99() < absorb);
+            if (!reflected && reflectEffect(*effectIt, magicEffect, caster, target, reflectedEffects))
-                    if (absorbed)
+            {
-                    {
+                reflected = true;
-                        const ESM::Static* absorbStatic = MWBase::Environment::get().getWorld()->getStore().get<ESM::Static>().find ("VFX_Absorb");
+                continue;
-                        MWBase::Environment::get().getWorld()->getAnimation(target)->addEffect(
-                                    "meshes\\" + absorbStatic->mModel, ESM::MagicEffect::SpellAbsorption, false, "");
-                        // Magicka is increased by cost of spell
-                        DynamicStat<float> magicka = stats.getMagicka();
-                        magicka.setCurrent(magicka.getCurrent() + spell->mData.mCost);
-                        stats.setMagicka(magicka);
-                    }
-                }
             }
 
             float magnitudeMult = 1;
 
             if (target.getClass().isActor())
             {
-                if (absorbed)
-                    continue;
-
                 bool isHarmful = magicEffect->mData.mFlags & ESM::MagicEffect::Harmful;
-                // Reflect harmful effects
-                if (isHarmful && !reflected && !caster.isEmpty() && caster != target && !(magicEffect->mData.mFlags & ESM::MagicEffect::Unreflectable))
-                {
-                    float reflect = target.getClass().getCreatureStats(target).getMagicEffects().get(ESM::MagicEffect::Reflect).getMagnitude();
-                    bool isReflected = (Misc::Rng::roll0to99() < reflect);
-                    if (isReflected)
-                    {
-                        const ESM::Static* reflectStatic = MWBase::Environment::get().getWorld()->getStore().get<ESM::Static>().find ("VFX_Reflect");
-                        MWBase::Environment::get().getWorld()->getAnimation(target)->addEffect(
-                                    "meshes\\" + reflectStatic->mModel, ESM::MagicEffect::Reflect, false, "");
-                        reflectedEffects.mList.push_back(*effectIt);
-                        continue;
-                    }
-                }
 
                 // Try resisting
                 magnitudeMult = MWMechanics::getEffectMultiplier(effectIt->mEffectID, target, caster, spell, &targetEffects);
@@ -317,23 +261,8 @@ namespace MWMechanics
 
                         // For absorb effects, also apply the effect to the caster - but with a negative
                         // magnitude, since we're transferring stats from the target to the caster
-                        if (!caster.isEmpty() && caster != target && caster.getClass().isActor())
+                        if (effectIt->mEffectID >= ESM::MagicEffect::AbsorbAttribute && effectIt->mEffectID <= ESM::MagicEffect::AbsorbSkill)
-                        {
+                            absorbStat(*effectIt, effect, caster, target, reflected, mSourceName);
-                            if (effectIt->mEffectID >= ESM::MagicEffect::AbsorbAttribute &&
-                                effectIt->mEffectID <= ESM::MagicEffect::AbsorbSkill)
-                            {
-                                std::vector<ActiveSpells::ActiveEffect> absorbEffects;
-                                ActiveSpells::ActiveEffect effect_ = effect;
-                                effect_.mMagnitude *= -1;
-                                absorbEffects.push_back(effect_);
-                                if (reflected && Settings::Manager::getBool("classic reflected absorb spells behavior", "Game"))
-                                    target.getClass().getCreatureStats(target).getActiveSpells().addSpell("", true,
-                                        absorbEffects, mSourceName, caster.getClass().getCreatureStats(caster).getActorId());
-                                else
-                                    caster.getClass().getCreatureStats(caster).getActiveSpells().addSpell("", true,
-                                        absorbEffects, mSourceName, target.getClass().getCreatureStats(target).getActorId());
-                            }
-                        }
                     }
                 }
 

apps/openmw/mwmechanics/tickableeffects.cpp

@@ -117,8 +117,8 @@ namespace MWMechanics
 
         case ESM::MagicEffect::DisintegrateArmor:
         {
-            // According to UESP
+            static const std::array<int, 9>  priorities
-            int priorities[] = {
+            {
                 MWWorld::InventoryStore::Slot_CarriedLeft,
                 MWWorld::InventoryStore::Slot_Cuirass,
                 MWWorld::InventoryStore::Slot_LeftPauldron,
@@ -129,10 +129,9 @@ namespace MWMechanics
                 MWWorld::InventoryStore::Slot_Greaves,
                 MWWorld::InventoryStore::Slot_Boots
             };
-
+            for (const int priority : priorities)
-            for (unsigned int i=0; i<sizeof(priorities)/sizeof(int); ++i)
             {
-                if (disintegrateSlot(actor, priorities[i], magnitude))
+                if (disintegrateSlot(actor, priority, magnitude))
                     break;
             }