Validate enchantment records (bug #7654)

Clean up spell validation
Fix a flaw in spell effect tooltip code

CHANGELOG.md

@@ -90,6 +90,7 @@
     Bug #7639: NPCs don't use hand-to-hand if their other melee skills were damaged during combat
     Bug #7642: Items in repair and recharge menus aren't sorted alphabetically
     Bug #7647: NPC walk cycle bugs after greeting player
+    Bug #7654: Tooltips for enchantments with invalid effects cause crashes
     Bug #7660: Some inconsistencies regarding Invisibility breaking
     Feature #3537: Shader-based water ripples
     Feature #5492: Let rain and snow collide with statics

apps/openmw/mwgui/widgets.cpp

@@ -355,12 +355,10 @@ namespace MWGui::Widgets
 
         const MWWorld::ESMStore& store = *MWBase::Environment::get().getESMStore();
 
-        const ESM::MagicEffect* magicEffect = store.get<ESM::MagicEffect>().search(mEffectParams.mEffectID);
+        const ESM::MagicEffect* magicEffect = store.get<ESM::MagicEffect>().find(mEffectParams.mEffectID);
         const ESM::Attribute* attribute = store.get<ESM::Attribute>().search(mEffectParams.mAttribute);
         const ESM::Skill* skill = store.get<ESM::Skill>().search(mEffectParams.mSkill);
 
-        assert(magicEffect);
-
         auto windowManager = MWBase::Environment::get().getWindowManager();
 
         std::string_view pt = windowManager->getGameSettingString("spoint", {});

apps/openmw/mwworld/esmstore.cpp

@@ -138,6 +138,59 @@ namespace
         return npcsToReplace;
     }
 
+    template <class RecordType>
+    std::vector<RecordType> getSpellsToReplace(
+        const MWWorld::Store<RecordType>& spells, const MWWorld::Store<ESM::MagicEffect>& magicEffects)
+    {
+        std::vector<RecordType> spellsToReplace;
+
+        for (RecordType spell : spells)
+        {
+            if (spell.mEffects.mList.empty())
+                continue;
+
+            bool changed = false;
+            auto iter = spell.mEffects.mList.begin();
+            while (iter != spell.mEffects.mList.end())
+            {
+                const ESM::MagicEffect* mgef = magicEffects.search(iter->mEffectID);
+                if (!mgef)
+                {
+                    Log(Debug::Verbose) << RecordType::getRecordType() << " " << spell.mId
+                                        << ": dropping invalid effect (index " << iter->mEffectID << ")";
+                    iter = spell.mEffects.mList.erase(iter);
+                    changed = true;
+                    continue;
+                }
+
+                if (!(mgef->mData.mFlags & ESM::MagicEffect::TargetAttribute) && iter->mAttribute != -1)
+                {
+                    iter->mAttribute = -1;
+                    Log(Debug::Verbose) << RecordType::getRecordType() << " " << spell.mId
+                                        << ": dropping unexpected attribute argument of "
+                                        << ESM::MagicEffect::indexToGmstString(iter->mEffectID) << " effect";
+                    changed = true;
+                }
+
+                if (!(mgef->mData.mFlags & ESM::MagicEffect::TargetSkill) && iter->mSkill != -1)
+                {
+                    iter->mSkill = -1;
+                    Log(Debug::Verbose) << RecordType::getRecordType() << " " << spell.mId
+                                        << ": dropping unexpected skill argument of "
+                                        << ESM::MagicEffect::indexToGmstString(iter->mEffectID) << " effect";
+                    changed = true;
+                }
+
+                ++iter;
+            }
+
+            if (changed)
+                spellsToReplace.emplace_back(spell);
+        }
+
+        return spellsToReplace;
+    }
+
     // Custom enchanted items can reference scripts that no longer exist, this doesn't necessarily mean the base item no
     // longer exists however. So instead of removing the item altogether, we're only removing the script.
     template <class MapT>
@@ -538,71 +591,24 @@ namespace MWWorld
 
         removeMissingScripts(getWritable<ESM::Script>(), getWritable<ESM::Creature>().mStatic);
 
-        // Validate spell effects for invalid arguments
-        std::vector<ESM::Spell> spellsToReplace;
+        // Validate spell effects and enchantments for invalid arguments
         auto& spells = getWritable<ESM::Spell>();
-        for (ESM::Spell spell : spells)
-        {
-            if (spell.mEffects.mList.empty())
-                continue;
-
-            bool changed = false;
-            auto iter = spell.mEffects.mList.begin();
-            while (iter != spell.mEffects.mList.end())
-            {
-                const ESM::MagicEffect* mgef = getWritable<ESM::MagicEffect>().search(iter->mEffectID);
-                if (!mgef)
-                {
-                    Log(Debug::Verbose) << "Spell '" << spell.mId << "' has an invalid effect (index "
-                                        << iter->mEffectID << ") present. Dropping the effect.";
-                    iter = spell.mEffects.mList.erase(iter);
-                    changed = true;
-                    continue;
-                }
-
-                if (mgef->mData.mFlags & ESM::MagicEffect::TargetSkill)
-                {
-                    if (iter->mAttribute != -1)
-                    {
-                        iter->mAttribute = -1;
-                        Log(Debug::Verbose)
-                            << ESM::MagicEffect::indexToGmstString(iter->mEffectID) << " effect of spell '" << spell.mId
-                            << "' has an attribute argument present. Dropping the argument.";
-                        changed = true;
-                    }
-                }
-                else if (mgef->mData.mFlags & ESM::MagicEffect::TargetAttribute)
-                {
-                    if (iter->mSkill != -1)
-                    {
-                        iter->mSkill = -1;
-                        Log(Debug::Verbose)
-                            << ESM::MagicEffect::indexToGmstString(iter->mEffectID) << " effect of spell '" << spell.mId
-                            << "' has a skill argument present. Dropping the argument.";
-                        changed = true;
-                    }
-                }
-                else if (iter->mSkill != -1 || iter->mAttribute != -1)
-                {
-                    iter->mSkill = -1;
-                    iter->mAttribute = -1;
-                    Log(Debug::Verbose) << ESM::MagicEffect::indexToGmstString(iter->mEffectID) << " effect of spell '"
-                                        << spell.mId << "' has argument(s) present. Dropping the argument(s).";
-                    changed = true;
-                }
-
-                ++iter;
-            }
-
-            if (changed)
-                spellsToReplace.emplace_back(spell);
-        }
+        auto& enchantments = getWritable<ESM::Enchantment>();
+        auto& magicEffects = getWritable<ESM::MagicEffect>();
 
+        std::vector<ESM::Spell> spellsToReplace = getSpellsToReplace(spells, magicEffects);
         for (const ESM::Spell& spell : spellsToReplace)
         {
             spells.eraseStatic(spell.mId);
             spells.insertStatic(spell);
         }
+
+        std::vector<ESM::Enchantment> enchantmentsToReplace = getSpellsToReplace(enchantments, magicEffects);
+        for (const ESM::Enchantment& enchantment : enchantmentsToReplace)
+        {
+            enchantments.eraseStatic(enchantment.mId);
+            enchantments.insertStatic(enchantment);
+        }
     }
 
     void ESMStore::movePlayerRecord()