Optimize MWMechanics::Spells

Use pointers as map keys instead of string IDs. Resolves a nasty performance bottleneck on functions like hasCommonDisease() that previously had to look up all contained spells from the ESM store on every call. hasCommonDisease() is called hundreds of times per frame by the AI target update since it's used to calculate target disposition.

The total cost of hasCommonDisease() was 2.7% of the frame loop, now it's negligible.

apps/openmw/mwgui/spellbuyingwindow.cpp

@@ -95,8 +95,7 @@ namespace MWGui
 
         for (MWMechanics::Spells::TIterator iter = merchantSpells.begin(); iter!=merchantSpells.end(); ++iter)
         {
-            const ESM::Spell* spell =
+            const ESM::Spell* spell = iter->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (iter->first);
 
             if (spell->mData.mType!=ESM::Spell::ST_Spell)
                 continue; // don't try to sell diseases, curses or powers
@@ -110,10 +109,10 @@ namespace MWGui
                     continue;
             }
 
-            if (playerHasSpell(iter->first))
+            if (playerHasSpell(iter->first->mId))
                 continue;
 
-            addSpell (iter->first);
+            addSpell (iter->first->mId);
         }
 
         updateLabels();

apps/openmw/mwgui/spellcreationdialog.cpp

@@ -516,8 +516,7 @@ namespace MWGui
 
         for (MWMechanics::Spells::TIterator it = spells.begin(); it != spells.end(); ++it)
         {
-            const ESM::Spell* spell =
+            const ESM::Spell* spell = it->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (it->first);
 
             // only normal spells count
             if (spell->mData.mType != ESM::Spell::ST_Spell)

apps/openmw/mwgui/spellmodel.cpp

@@ -52,7 +52,7 @@ namespace MWGui
 
         for (MWMechanics::Spells::TIterator it = spells.begin(); it != spells.end(); ++it)
         {
-            const ESM::Spell* spell = esmStore.get<ESM::Spell>().find(it->first);
+            const ESM::Spell* spell = it->first;
             if (spell->mData.mType != ESM::Spell::ST_Power && spell->mData.mType != ESM::Spell::ST_Spell)
                 continue;
 
@@ -67,9 +67,9 @@ namespace MWGui
             }
             else
                 newSpell.mType = Spell::Type_Power;
-            newSpell.mId = it->first;
+            newSpell.mId = spell->mId;
 
-            newSpell.mSelected = (MWBase::Environment::get().getWindowManager()->getSelectedSpell() == it->first);
+            newSpell.mSelected = (MWBase::Environment::get().getWindowManager()->getSelectedSpell() == spell->mId);
             newSpell.mActive = true;
             mSpells.push_back(newSpell);
         }

apps/openmw/mwmechanics/aicombataction.cpp

@@ -541,7 +541,7 @@ namespace MWMechanics
 
         for (Spells::TIterator it = spells.begin(); it != spells.end(); ++it)
         {
-            const ESM::Spell* spell = MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find(it->first);
+            const ESM::Spell* spell = it->first;
 
             float rating = rateSpell(spell, actor, target);
             if (rating > bestActionRating)

apps/openmw/mwmechanics/disease.hpp

@@ -34,8 +34,7 @@ namespace MWMechanics
         Spells& spells = carrier.getClass().getCreatureStats(carrier).getSpells();
         for (Spells::TIterator it = spells.begin(); it != spells.end(); ++it)
         {
-            const ESM::Spell* spell = MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find(it->first);
+            const ESM::Spell* spell = it->first;
-
             if (actor.getClass().getCreatureStats(actor).getSpells().hasSpell(spell->mId))
                 continue;
 

apps/openmw/mwmechanics/spellcasting.cpp

@@ -124,7 +124,7 @@ namespace MWMechanics
         }
 
         if (spell->mData.mType == ESM::Spell::ST_Power)
-            return stats.getSpells().canUsePower(spell->mId) ? 100 : 0;
+            return stats.getSpells().canUsePower(spell) ? 100 : 0;
 
         if (spell->mData.mType != ESM::Spell::ST_Spell)
             return 100;
@@ -823,7 +823,7 @@ namespace MWMechanics
 
             // A power can be used once per 24h
             if (spell->mData.mType == ESM::Spell::ST_Power)
-                stats.getSpells().usePower(spell->mId);
+                stats.getSpells().usePower(spell);
         }
 
         if (mCaster == getPlayer() && spellIncreasesSkill(spell))

apps/openmw/mwmechanics/spells.cpp

@@ -25,9 +25,24 @@ namespace MWMechanics
         return mSpells.end();
     }
 
+    const ESM::Spell* Spells::getSpell(const std::string& id) const
+    {
+        return MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find(id);
+    }
+
+    bool Spells::hasSpell(const std::string &spell) const
+    {
+        return hasSpell(getSpell(spell));
+    }
+
+    bool Spells::hasSpell(const ESM::Spell *spell) const
+    {
+        return mSpells.find(spell) != mSpells.end();
+    }
+
     void Spells::add (const ESM::Spell* spell)
     {
-        if (mSpells.find (spell->mId)==mSpells.end())
+        if (mSpells.find (spell)==mSpells.end())
         {
             std::map<const int, float> random;
 
@@ -48,32 +63,32 @@ namespace MWMechanics
                 corprus.mWorsenings = 0;
                 corprus.mNextWorsening = MWBase::Environment::get().getWorld()->getTimeStamp() + CorprusStats::sWorseningPeriod;
 
-                mCorprusSpells[spell->mId] = corprus;
+                mCorprusSpells[spell] = corprus;
             }
 
-            mSpells.insert (std::make_pair (spell->mId, random));
+            mSpells.insert (std::make_pair (spell, random));
         }
     }
 
     void Spells::add (const std::string& spellId)
     {
-        const ESM::Spell* spell = MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find(spellId);
+        add(getSpell(spellId));
-        add(spell);
     }
 
     void Spells::remove (const std::string& spellId)
     {
-        std::string lower = Misc::StringUtils::lowerCase(spellId);
+        const ESM::Spell* spell = getSpell(spellId);
-        TContainer::iterator iter = mSpells.find (lower);
+        TContainer::iterator iter = mSpells.find (spell);
-        std::map<std::string, CorprusStats>::iterator corprusIt = mCorprusSpells.find(lower);
+
+        std::map<SpellKey, CorprusStats>::iterator corprusIt = mCorprusSpells.find(spell);
 
         // if it's corprus, remove negative and keep positive effects
         if (corprusIt != mCorprusSpells.end())
         {
-            worsenCorprus(lower);
+            worsenCorprus(spell);
-            if (mPermanentSpellEffects.find(lower) != mPermanentSpellEffects.end())
+            if (mPermanentSpellEffects.find(spell) != mPermanentSpellEffects.end())
             {
-                MagicEffects & effects = mPermanentSpellEffects[lower];
+                MagicEffects & effects = mPermanentSpellEffects[spell];
                 for (MagicEffects::Collection::const_iterator effectIt = effects.begin(); effectIt != effects.end();)
                 {
                     const ESM::MagicEffect * magicEffect = MWBase::Environment::get().getWorld()->getStore().get<ESM::MagicEffect>().find(effectIt->first.mId);
@@ -101,8 +116,7 @@ namespace MWMechanics
 
         for (TIterator iter = mSpells.begin(); iter!=mSpells.end(); ++iter)
         {
-            const ESM::Spell *spell =
+            const ESM::Spell *spell = iter->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (iter->first);
 
             if (spell->mData.mType==ESM::Spell::ST_Ability || spell->mData.mType==ESM::Spell::ST_Blight ||
                 spell->mData.mType==ESM::Spell::ST_Disease || spell->mData.mType==ESM::Spell::ST_Curse)
@@ -120,7 +134,7 @@ namespace MWMechanics
             }
         }
 
-        for (std::map<std::string, MagicEffects>::const_iterator it = mPermanentSpellEffects.begin(); it != mPermanentSpellEffects.end(); ++it)
+        for (std::map<SpellKey, MagicEffects>::const_iterator it = mPermanentSpellEffects.begin(); it != mPermanentSpellEffects.end(); ++it)
         {
             effects += it->second;
         }
@@ -145,11 +159,10 @@ namespace MWMechanics
 
     bool Spells::isSpellActive(const std::string &id) const
     {
-        TContainer::const_iterator found = mSpells.find(id);
+        TContainer::const_iterator found = mSpells.find(getSpell(id));
         if (found != mSpells.end())
         {
-            const ESM::Spell *spell =
+            const ESM::Spell *spell = found->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (id);
 
             return (spell->mData.mType==ESM::Spell::ST_Ability || spell->mData.mType==ESM::Spell::ST_Blight ||
                 spell->mData.mType==ESM::Spell::ST_Disease || spell->mData.mType==ESM::Spell::ST_Curse);
@@ -161,9 +174,7 @@ namespace MWMechanics
     {
         for (TIterator iter = mSpells.begin(); iter!=mSpells.end(); ++iter)
         {
-            const ESM::Spell *spell =
+            const ESM::Spell *spell = iter->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (iter->first);
-
             if (spell->mData.mType == ESM::Spell::ST_Disease)
                 return true;
         }
@@ -175,9 +186,7 @@ namespace MWMechanics
     {
         for (TIterator iter = mSpells.begin(); iter!=mSpells.end(); ++iter)
         {
-            const ESM::Spell *spell =
+            const ESM::Spell *spell = iter->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (iter->first);
-
             if (spell->mData.mType == ESM::Spell::ST_Blight)
                 return true;
         }
@@ -189,9 +198,7 @@ namespace MWMechanics
     {
         for (TContainer::iterator iter = mSpells.begin(); iter!=mSpells.end();)
         {
-            const ESM::Spell *spell =
+            const ESM::Spell *spell = iter->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (iter->first);
-
             if (spell->mData.mType == ESM::Spell::ST_Disease)
                 mSpells.erase(iter++);
             else
@@ -203,9 +210,7 @@ namespace MWMechanics
     {
         for (TContainer::iterator iter = mSpells.begin(); iter!=mSpells.end();)
         {
-            const ESM::Spell *spell =
+            const ESM::Spell *spell = iter->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (iter->first);
-
             if (spell->mData.mType == ESM::Spell::ST_Blight && !hasCorprusEffect(spell))
                 mSpells.erase(iter++);
             else
@@ -217,9 +222,7 @@ namespace MWMechanics
     {
         for (TContainer::iterator iter = mSpells.begin(); iter!=mSpells.end();)
         {
-            const ESM::Spell *spell =
+            const ESM::Spell *spell = iter->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (iter->first);
-
             if (hasCorprusEffect(spell))
                 mSpells.erase(iter++);
             else
@@ -231,9 +234,7 @@ namespace MWMechanics
     {
         for (TContainer::iterator iter = mSpells.begin(); iter!=mSpells.end();)
         {
-            const ESM::Spell *spell =
+            const ESM::Spell *spell = iter->first;
-                MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find (iter->first);
-
             if (spell->mData.mType == ESM::Spell::ST_Curse)
                 mSpells.erase(iter++);
             else
@@ -245,7 +246,7 @@ namespace MWMechanics
     {
         for (TIterator it = begin(); it != end(); ++it)
         {
-            const ESM::Spell* spell = MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find(it->first);
+            const ESM::Spell* spell = it->first;
 
             // these are the spell types that are permanently in effect
             if (!(spell->mData.mType == ESM::Spell::ST_Ability)
@@ -268,14 +269,13 @@ namespace MWMechanics
         }
     }
 
-    void Spells::worsenCorprus(const std::string &corpSpellId)
+    void Spells::worsenCorprus(const ESM::Spell* spell)
     {
-        mCorprusSpells[corpSpellId].mNextWorsening = MWBase::Environment::get().getWorld()->getTimeStamp() + CorprusStats::sWorseningPeriod;
+        mCorprusSpells[spell].mNextWorsening = MWBase::Environment::get().getWorld()->getTimeStamp() + CorprusStats::sWorseningPeriod;
-        mCorprusSpells[corpSpellId].mWorsenings++;
+        mCorprusSpells[spell].mWorsenings++;
 
         // update worsened effects
-        mPermanentSpellEffects[corpSpellId] = MagicEffects();
+        mPermanentSpellEffects[spell] = MagicEffects();
-        const ESM::Spell * spell = MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().find(corpSpellId);
         int i=0;
         for (std::vector<ESM::ENAMstruct>::const_iterator effectIt = spell->mEffects.mList.begin(); effectIt != spell->mEffects.mList.end(); ++effectIt, ++i)
         {
@@ -283,12 +283,12 @@ namespace MWMechanics
             if ((effectIt->mEffectID != ESM::MagicEffect::Corprus) && (magicEffect->mData.mFlags & ESM::MagicEffect::UncappedDamage)) // APPLIED_ONCE
             {
                 float random = 1.f;
-                if (mSpells[corpSpellId].find(i) != mSpells[corpSpellId].end())
+                if (mSpells[spell].find(i) != mSpells[spell].end())
-                    random = mSpells[corpSpellId].at(i);
+                    random = mSpells[spell].at(i);
 
                 float magnitude = effectIt->mMagnMin + (effectIt->mMagnMax - effectIt->mMagnMin) * random;
-                magnitude *= std::max(1, mCorprusSpells[corpSpellId].mWorsenings);
+                magnitude *= std::max(1, mCorprusSpells[spell].mWorsenings);
-                mPermanentSpellEffects[corpSpellId].add(MWMechanics::EffectKey(*effectIt), MWMechanics::EffectParam(magnitude));
+                mPermanentSpellEffects[spell].add(MWMechanics::EffectKey(*effectIt), MWMechanics::EffectParam(magnitude));
             }
         }
     }
@@ -305,43 +305,47 @@ namespace MWMechanics
         return false;
     }
 
-    const std::map<std::string, Spells::CorprusStats> &Spells::getCorprusSpells() const
+    const std::map<Spells::SpellKey, Spells::CorprusStats> &Spells::getCorprusSpells() const
     {
         return mCorprusSpells;
     }
 
-    bool Spells::canUsePower(const std::string &power) const
+    bool Spells::canUsePower(const ESM::Spell* spell) const
     {
-        std::map<std::string, MWWorld::TimeStamp>::const_iterator it = mUsedPowers.find(Misc::StringUtils::lowerCase(power));
+        std::map<SpellKey, MWWorld::TimeStamp>::const_iterator it = mUsedPowers.find(spell);
         if (it == mUsedPowers.end() || it->second + 24 <= MWBase::Environment::get().getWorld()->getTimeStamp())
             return true;
         else
             return false;
     }
 
-    void Spells::usePower(const std::string &power)
+    void Spells::usePower(const ESM::Spell* spell)
     {
-        mUsedPowers[Misc::StringUtils::lowerCase(power)] = MWBase::Environment::get().getWorld()->getTimeStamp();
+        mUsedPowers[spell] = MWBase::Environment::get().getWorld()->getTimeStamp();
     }
 
     void Spells::readState(const ESM::SpellState &state)
     {
-        for (TContainer::const_iterator it = state.mSpells.begin(); it != state.mSpells.end(); ++it)
+        for (ESM::SpellState::TContainer::const_iterator it = state.mSpells.begin(); it != state.mSpells.end(); ++it)
         {
             // Discard spells that are no longer available due to changed content files
             const ESM::Spell* spell = MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().search(it->first);
             if (spell)
             {
-                mSpells[it->first] = it->second;
+                mSpells[spell] = it->second;
 
                 if (it->first == state.mSelectedSpell)
                     mSelectedSpell = it->first;
             }
         }
 
-        // No need to discard spells here (doesn't really matter if non existent ids are kept)
         for (std::map<std::string, ESM::TimeStamp>::const_iterator it = state.mUsedPowers.begin(); it != state.mUsedPowers.end(); ++it)
-            mUsedPowers[it->first] = MWWorld::TimeStamp(it->second);
+        {
+            const ESM::Spell* spell = MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().search(it->first);
+            if (!spell)
+                continue;
+            mUsedPowers[spell] = MWWorld::TimeStamp(it->second);
+        }
 
         for (std::map<std::string, std::vector<ESM::SpellState::PermanentSpellEffectInfo> >::const_iterator it =
             state.mPermanentSpellEffects.begin(); it != state.mPermanentSpellEffects.end(); ++it)
@@ -350,33 +354,35 @@ namespace MWMechanics
             if (!spell)
                 continue;
 
-            mPermanentSpellEffects[it->first] = MagicEffects();
+            mPermanentSpellEffects[spell] = MagicEffects();
             for (std::vector<ESM::SpellState::PermanentSpellEffectInfo>::const_iterator effectIt = it->second.begin(); effectIt != it->second.end(); ++effectIt)
             {
-                mPermanentSpellEffects[it->first].add(EffectKey(effectIt->mId, effectIt->mArg), effectIt->mMagnitude);
+                mPermanentSpellEffects[spell].add(EffectKey(effectIt->mId, effectIt->mArg), effectIt->mMagnitude);
             }
         }
 
         mCorprusSpells.clear();
         for (std::map<std::string, ESM::SpellState::CorprusStats>::const_iterator it = state.mCorprusSpells.begin(); it != state.mCorprusSpells.end(); ++it)
         {
-            if (mSpells.find(it->first) != mSpells.end()) // Discard unavailable corprus spells
+            const ESM::Spell* spell = MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>().search(it->first);
-            {
+            if (!spell) // Discard unavailable corprus spells
-                mCorprusSpells[it->first].mWorsenings = state.mCorprusSpells.at(it->first).mWorsenings;
+                continue;
-                mCorprusSpells[it->first].mNextWorsening = MWWorld::TimeStamp(state.mCorprusSpells.at(it->first).mNextWorsening);
+            mCorprusSpells[spell].mWorsenings = state.mCorprusSpells.at(it->first).mWorsenings;
-            }
+            mCorprusSpells[spell].mNextWorsening = MWWorld::TimeStamp(state.mCorprusSpells.at(it->first).mNextWorsening);
         }
     }
 
     void Spells::writeState(ESM::SpellState &state) const
     {
-        state.mSpells = mSpells;
+        for (TContainer::const_iterator it = mSpells.begin(); it != mSpells.end(); ++it)
+            state.mSpells.insert(std::make_pair(it->first->mId, it->second));
+
         state.mSelectedSpell = mSelectedSpell;
 
-        for (std::map<std::string, MWWorld::TimeStamp>::const_iterator it = mUsedPowers.begin(); it != mUsedPowers.end(); ++it)
+        for (std::map<SpellKey, MWWorld::TimeStamp>::const_iterator it = mUsedPowers.begin(); it != mUsedPowers.end(); ++it)
-            state.mUsedPowers[it->first] = it->second.toEsm();
+            state.mUsedPowers[it->first->mId] = it->second.toEsm();
 
-        for (std::map<std::string, MagicEffects>::const_iterator it = mPermanentSpellEffects.begin(); it != mPermanentSpellEffects.end(); ++it)
+        for (std::map<SpellKey, MagicEffects>::const_iterator it = mPermanentSpellEffects.begin(); it != mPermanentSpellEffects.end(); ++it)
         {
             std::vector<ESM::SpellState::PermanentSpellEffectInfo> effectList;
             for (MagicEffects::Collection::const_iterator effectIt = it->second.begin(); effectIt != it->second.end(); ++effectIt)
@@ -388,13 +394,13 @@ namespace MWMechanics
 
                 effectList.push_back(info);
             }
-            state.mPermanentSpellEffects[it->first] = effectList;
+            state.mPermanentSpellEffects[it->first->mId] = effectList;
         }
 
-        for (std::map<std::string, CorprusStats>::const_iterator it = mCorprusSpells.begin(); it != mCorprusSpells.end(); ++it)
+        for (std::map<SpellKey, CorprusStats>::const_iterator it = mCorprusSpells.begin(); it != mCorprusSpells.end(); ++it)
         {
-            state.mCorprusSpells[it->first].mWorsenings = mCorprusSpells.at(it->first).mWorsenings;
+            state.mCorprusSpells[it->first->mId].mWorsenings = mCorprusSpells.at(it->first).mWorsenings;
-            state.mCorprusSpells[it->first].mNextWorsening = mCorprusSpells.at(it->first).mNextWorsening.toEsm();
+            state.mCorprusSpells[it->first->mId].mNextWorsening = mCorprusSpells.at(it->first).mNextWorsening.toEsm();
         }
     }
 }

apps/openmw/mwmechanics/spells.hpp

@@ -31,7 +31,9 @@ namespace MWMechanics
     {
         public:
 
-            typedef std::map<std::string, std::map<const int, float> > TContainer; // ID, <effect index, normalised random magnitude>
+            typedef const ESM::Spell* SpellKey;
+
+            typedef std::map<SpellKey, std::map<const int, float> > TContainer; // ID, <effect index, normalised random magnitude>
             typedef TContainer::const_iterator TIterator;
 
             struct CorprusStats
@@ -47,23 +49,26 @@ namespace MWMechanics
             TContainer mSpells;
 
             // spell-tied effects that will be applied even after removing the spell (currently used to keep positive effects when corprus is removed)
-            std::map<std::string, MagicEffects> mPermanentSpellEffects;
+            std::map<SpellKey, MagicEffects> mPermanentSpellEffects;
 
             // Note: this is the spell that's about to be cast, *not* the spell selected in the GUI (which may be different)
             std::string mSelectedSpell;
 
-            std::map<std::string, MWWorld::TimeStamp> mUsedPowers;
+            std::map<SpellKey, MWWorld::TimeStamp> mUsedPowers;
 
-            std::map<std::string, CorprusStats> mCorprusSpells;
+            std::map<SpellKey, CorprusStats> mCorprusSpells;
+
+            /// Get spell from ID, throws exception if not found
+            const ESM::Spell* getSpell(const std::string& id) const;
 
         public:
 
-            void worsenCorprus(const std::string &corpSpellId);
+            void worsenCorprus(const ESM::Spell* spell);
             static bool hasCorprusEffect(const ESM::Spell *spell);
-            const std::map<std::string, CorprusStats> & getCorprusSpells() const;
+            const std::map<SpellKey, CorprusStats> & getCorprusSpells() const;
 
-            bool canUsePower (const std::string& power) const;
+            bool canUsePower (const ESM::Spell* spell) const;
-            void usePower (const std::string& power);
+            void usePower (const ESM::Spell* spell);
 
             void purgeCommonDisease();
             void purgeBlightDisease();
@@ -74,7 +79,8 @@ namespace MWMechanics
 
             TIterator end() const;
 
-            bool hasSpell(const std::string& spell) const { return mSpells.find(Misc::StringUtils::lowerCase(spell)) != mSpells.end(); }
+            bool hasSpell(const std::string& spell) const;
+            bool hasSpell(const ESM::Spell* spell) const;
 
             void add (const std::string& spell);
             ///< Adding a spell that is already listed in *this is a no-op.

apps/openmw/mwworld/worldimp.cpp

@@ -2581,7 +2581,7 @@ namespace MWWorld
             }
 
             // If this is a power, check if it was already used in the last 24h
-            if (!fail && spell->mData.mType == ESM::Spell::ST_Power && !stats.getSpells().canUsePower(spell->mId))
+            if (!fail && spell->mData.mType == ESM::Spell::ST_Power && !stats.getSpells().canUsePower(spell))
             {
                 message = "#{sPowerAlreadyUsed}";
                 fail = true;