Traverse spells in record order from content files. bronrod_the_roarer is perfect now. Other NPCs have some differences.

apps/openmw/mwclass/npc.cpp

@@ -221,33 +221,9 @@ namespace
         for (int i=0; i<ESM::Attribute::Length; ++i)
             attributes[i] = npcStats.getAttribute(i).getBase();
 
-        std::cout << npc->mId << std::endl;
-        std::cout << "Skills: " << std::endl;
-        for (int i=0; i<ESM::Skill::Length; ++i)
-            std::cout << skills[i] << std::endl;
-        std::cout << "Attributes: " << std::endl;
-        for (int i=0; i<ESM::Attribute::Length; ++i)
-            std::cout << attributes[i] << std::endl;
-        std::set<std::string> spells = MWMechanics::autoCalcNpcSpells(skills, attributes, race);
-        std::cout << "Spells: " << spells.size() << std::endl;
-        for (std::set<std::string>::iterator it = spells.begin(); it != spells.end(); ++it)
-        {
-            std::cout << *it << ", ";
+        std::vector<std::string> spells = MWMechanics::autoCalcNpcSpells(skills, attributes, race);
+        for (std::vector<std::string>::iterator it = spells.begin(); it != spells.end(); ++it)
             npcStats.getSpells().add(*it);
-        }
-        std::cout << std::endl;
-
-        const char* compare[] = { "weary","dire noise","reflect","weak spelldrinker","absorb endurance","absorb personality","absorb speed","absorb strength","absorb willpower","fortify alteration skill","fortify illusion skill","fortify unarmored skill","fortify mysticism skill","fortify restoration skill","assured sublime wisdom","surpassing sublime wisdom","surpassing golden wisdom","blood gift","surpassing silver wisdom","surpassing unseen wisdom","surpassing green wisdom","powerwell","orc's strength","surpassing fluid evasion","poet's whim","rapid regenerate","dispel","shadow weave" };
-        int n = sizeof(compare) / sizeof(compare[0]);
-        std::set<std::string> compareSet;
-        for (int i=0; i<n; ++i)
-            compareSet.insert(compare[i]);
-        std::cout << "Compare: " << n << std::endl;
-        for (std::set<std::string>::iterator it = compareSet.begin(); it != compareSet.end(); ++it)
-        {
-            std::cout << *it << ", ";
-        }
-        std::cout << std::endl;
     }
 }
 

apps/openmw/mwmechanics/autocalcspell.cpp

@@ -20,7 +20,7 @@ namespace MWMechanics
         std::string mWeakestSpell;
     };
 
-    std::set<std::string> autoCalcNpcSpells(const int *actorSkills, const int *actorAttributes, const ESM::Race* race)
+    std::vector<std::string> autoCalcNpcSpells(const int *actorSkills, const int *actorAttributes, const ESM::Race* race)
     {
         const MWWorld::Store<ESM::GameSetting>& gmst = MWBase::Environment::get().getWorld()->getStore().get<ESM::GameSetting>();
         static const float fNPCbaseMagickaMult = gmst.find("fNPCbaseMagickaMult")->getFloat();
@@ -53,10 +53,13 @@ namespace MWMechanics
             schoolCaps[i] = caps;
         }
 
-        std::set<std::string> selectedSpells;
+        std::vector<std::string> selectedSpells;
 
         const MWWorld::Store<ESM::Spell> &spells =
             MWBase::Environment::get().getWorld()->getStore().get<ESM::Spell>();
+
+        // Note: the algorithm heavily depends on the traversal order of the spells. For vanilla-compatible results the
+        // Store must preserve the record ordering as it was in the content files.
         for (MWWorld::Store<ESM::Spell>::iterator iter = spells.begin(); iter != spells.end(); ++iter)
         {
             const ESM::Spell* spell = &*iter;
@@ -88,18 +91,32 @@ namespace MWMechanics
             if (calcAutoCastChance(spell, actorSkills, actorAttributes, school) < fAutoSpellChance)
                 continue;
 
-            selectedSpells.insert(spell->mId);
+            selectedSpells.push_back(spell->mId);
 
             if (cap.mReachedLimit)
             {
-                selectedSpells.erase(cap.mWeakestSpell);
+                std::vector<std::string>::iterator found = std::find(selectedSpells.begin(), selectedSpells.end(), cap.mWeakestSpell);
+                if (found != selectedSpells.end())
+                    selectedSpells.erase(found);
 
-                // Note: not school specific
                 cap.mMinCost = INT_MAX;
-                for (std::set<std::string>::iterator weakIt = selectedSpells.begin(); weakIt != selectedSpells.end(); ++weakIt)
+                for (std::vector<std::string>::iterator weakIt = selectedSpells.begin(); weakIt != selectedSpells.end(); ++weakIt)
                 {
                     const ESM::Spell* testSpell = spells.find(*weakIt);
-                    if (testSpell->mData.mCost < cap.mMinCost) // XXX what if 2 candidates have the same cost?
+
+                    //int testSchool;
+                    //float dummySkillTerm;
+                    //calcWeakestSchool(testSpell, actorSkills, testSchool, dummySkillTerm);
+
+                    // Note: if there are multiple spells with the same cost, we pick the first one we found.
+                    // So the algorithm depends on the iteration order of the outer loop.
+                    if (
+                            // There is a huge bug here. It is not checked that weakestSpell is of the correct school.
+                            // As result multiple SchoolCaps could have the same mWeakestSpell. Erasing the weakest spell would then fail if another school
+                            // already erased it, and so the number of spells would often exceed the sum of limits.
+                            // This bug cannot be fixed without significantly changing the results of the spell autocalc, which will not have been playtested.
+                            //testSchool == school &&
+                            testSpell->mData.mCost < cap.mMinCost)
                     {
                         cap.mMinCost = testSpell->mData.mCost;
                         cap.mWeakestSpell = testSpell->mId;
@@ -119,6 +136,7 @@ namespace MWMechanics
                 }
             }
         }
+
         return selectedSpells;
     }
 

apps/openmw/mwmechanics/autocalcspell.hpp

@@ -14,7 +14,7 @@ namespace MWMechanics
 /// Contains algorithm for calculating an NPC's spells based on stats
 /// @note We might want to move this code to a component later, so the editor can use it for preview purposes
 
-std::set<std::string> autoCalcNpcSpells(const int* actorSkills, const int* actorAttributes, const ESM::Race* race);
+std::vector<std::string> autoCalcNpcSpells(const int* actorSkills, const int* actorAttributes, const ESM::Race* race);
 
 // Helpers
 

apps/openmw/mwworld/store.hpp

@@ -209,8 +209,6 @@ namespace MWWorld
         }
 
         void setUp() {
-            //std::sort(mStatic.begin(), mStatic.end(), RecordCmp());
-
             mShared.clear();
             mShared.reserve(mStatic.size());
             typename std::map<std::string, T>::iterator it = mStatic.begin();
@@ -675,18 +673,15 @@ namespace MWWorld
         }
 
         void setUp() {
-            //typedef std::vector<ESM::Cell>::iterator Iterator;
             typedef DynamicExt::iterator ExtIterator;
             typedef std::map<std::string, ESM::Cell>::iterator IntIterator;
 
-            //std::sort(mInt.begin(), mInt.end(), RecordCmp());
             mSharedInt.clear();
             mSharedInt.reserve(mInt.size());
             for (IntIterator it = mInt.begin(); it != mInt.end(); ++it) {
                 mSharedInt.push_back(&(it->second));
             }
 
-            //std::sort(mExt.begin(), mExt.end(), ExtCmp());
             mSharedExt.clear();
             mSharedExt.reserve(mExt.size());
             for (ExtIterator it = mExt.begin(); it != mExt.end(); ++it) {
@@ -1147,6 +1142,37 @@ namespace MWWorld
         }
     };
 
+
+    // Specialisation for ESM::Spell to preserve record order as it was in the content files.
+    // The NPC spell autocalc code heavily depends on this order.
+    // We could also do this in the base class, but it's usually not a good idea to depend on record order.
+    template<>
+    inline void Store<ESM::Spell>::clearDynamic()
+    {
+        // remove the dynamic part of mShared
+        mShared.erase(mShared.begin() + mStatic.size(), mShared.end());
+        mDynamic.clear();
+    }
+
+    template<>
+    inline void Store<ESM::Spell>::load(ESM::ESMReader &esm, const std::string &id) {
+        std::string idLower = Misc::StringUtils::lowerCase(id);
+
+        std::pair<Static::iterator, bool> inserted = mStatic.insert(std::make_pair(idLower, ESM::Spell()));
+        if (inserted.second)
+            mShared.push_back(&mStatic[idLower]);
+
+        inserted.first->second.mId = idLower;
+        inserted.first->second.load(esm);
+    }
+
+    template<>
+    inline void Store<ESM::Spell>::setUp()
+    {
+        // remove the dynamic part of mShared
+        mShared.erase(mShared.begin() + mStatic.size(), mShared.end());
+    }
+
 } //end namespace
 
 #endif