Deduplicate effect list verification

Drop Potion-specific range check, it's irrelevant

apps/opencs/CMakeLists.txt

@@ -42,7 +42,7 @@ opencs_units (model/tools
     mandatoryid skillcheck classcheck factioncheck racecheck soundcheck regioncheck
     birthsigncheck spellcheck referencecheck referenceablecheck scriptcheck bodypartcheck
     startscriptcheck search searchoperation searchstage pathgridcheck soundgencheck magiceffectcheck
-    mergestages gmstcheck topicinfocheck journalcheck enchantmentcheck
+    mergestages gmstcheck topicinfocheck journalcheck enchantmentcheck effectlistcheck
     )
 
 opencs_hdrs (model/tools

apps/opencs/model/tools/effectlistcheck.cpp

@@ -0,0 +1,88 @@
+#include "effectlistcheck.hpp"
+
+#include <components/esm/attr.hpp>
+#include <components/esm3/effectlist.hpp>
+#include <components/esm3/loadingr.hpp>
+#include <components/esm3/loadmgef.hpp>
+#include <components/esm3/loadskil.hpp>
+
+#include <apps/opencs/model/doc/messages.hpp>
+#include <apps/opencs/model/world/universalid.hpp>
+
+namespace CSMTools
+{
+    void effectListCheck(
+        const std::vector<ESM::IndexedENAMstruct>& list, CSMDoc::Messages& messages, const CSMWorld::UniversalId& id)
+    {
+        if (list.empty())
+        {
+            messages.add(id, "No magic effects", "", CSMDoc::Message::Severity_Warning);
+            return;
+        }
+
+        size_t i = 1;
+        for (const ESM::IndexedENAMstruct& effect : list)
+        {
+            const std::string number = std::to_string(i);
+
+            // At the time of writing this effects, attributes and skills are mostly hardcoded
+            if (effect.mData.mEffectID < 0 || effect.mData.mEffectID >= ESM::MagicEffect::Length)
+                messages.add(id, "Effect #" + number + ": invalid effect ID", "", CSMDoc::Message::Severity_Error);
+            if (effect.mData.mSkill < -1 || effect.mData.mSkill >= ESM::Skill::Length)
+                messages.add(id, "Effect #" + number + ": invalid skill", "", CSMDoc::Message::Severity_Error);
+            if (effect.mData.mAttribute < -1 || effect.mData.mAttribute >= ESM::Attribute::Length)
+                messages.add(id, "Effect #" + number + ": invalid attribute", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mRange < ESM::RT_Self || effect.mData.mRange > ESM::RT_Target)
+                messages.add(id, "Effect #" + number + ": invalid range", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mArea < 0)
+                messages.add(id, "Effect #" + number + ": negative area", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mDuration < 0)
+                messages.add(id, "Effect #" + number + ": negative duration", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mMagnMin < 0)
+                messages.add(
+                    id, "Effect #" + number + ": negative minimum magnitude", "", CSMDoc::Message::Severity_Error);
+
+            if (effect.mData.mMagnMax < 0)
+                messages.add(
+                    id, "Effect #" + number + ": negative maximum magnitude", "", CSMDoc::Message::Severity_Error);
+            else if (effect.mData.mMagnMax == 0)
+                messages.add(
+                    id, "Effect #" + number + ": zero maximum magnitude", "", CSMDoc::Message::Severity_Warning);
+
+            if (effect.mData.mMagnMin > effect.mData.mMagnMax)
+                messages.add(id, "Effect #" + number + ": minimum magnitude is higher than maximum magnitude", "",
+                    CSMDoc::Message::Severity_Error);
+
+            ++i;
+        }
+    }
+
+    void ingredientEffectListCheck(
+        const ESM::Ingredient& ingredient, CSMDoc::Messages& messages, const CSMWorld::UniversalId& id)
+    {
+        bool hasEffects = false;
+
+        for (size_t i = 0; i < 4; i++)
+        {
+            if (ingredient.mData.mEffectID[i] == -1)
+                continue;
+
+            hasEffects = true;
+
+            const std::string number = std::to_string(i + 1);
+            if (ingredient.mData.mEffectID[i] < -1 || ingredient.mData.mEffectID[i] >= ESM::MagicEffect::Length)
+                messages.add(id, "Effect #" + number + ": invalid effect ID", "", CSMDoc::Message::Severity_Error);
+            if (ingredient.mData.mSkills[i] < -1 || ingredient.mData.mSkills[i] >= ESM::Skill::Length)
+                messages.add(id, "Effect #" + number + ": invalid skill", "", CSMDoc::Message::Severity_Error);
+            if (ingredient.mData.mAttributes[i] < -1 || ingredient.mData.mAttributes[i] >= ESM::Attribute::Length)
+                messages.add(id, "Effect #" + number + ": invalid attribute", "", CSMDoc::Message::Severity_Error);
+        }
+
+        if (!hasEffects)
+            messages.add(id, "No magic effects", "", CSMDoc::Message::Severity_Warning);
+    }
+}

apps/opencs/model/tools/effectlistcheck.hpp

@@ -0,0 +1,30 @@
+#ifndef CSM_TOOLS_EFFECTLISTCHECK_H
+#define CSM_TOOLS_EFFECTLISTCHECK_H
+
+#include <vector>
+
+namespace ESM
+{
+    struct IndexedENAMstruct;
+    struct Ingredient;
+}
+
+namespace CSMDoc
+{
+    class Messages;
+}
+
+namespace CSMWorld
+{
+    class UniversalId;
+}
+
+namespace CSMTools
+{
+    void effectListCheck(
+        const std::vector<ESM::IndexedENAMstruct>& list, CSMDoc::Messages& messages, const CSMWorld::UniversalId& id);
+    void ingredientEffectListCheck(
+        const ESM::Ingredient& ingredient, CSMDoc::Messages& messages, const CSMWorld::UniversalId& id);
+}
+
+#endif

apps/opencs/model/tools/enchantmentcheck.cpp

@@ -10,14 +10,14 @@
 #include <apps/opencs/model/world/idcollection.hpp>
 #include <apps/opencs/model/world/record.hpp>
 
-#include <components/esm/attr.hpp>
-#include <components/esm3/effectlist.hpp>
 #include <components/esm3/loadench.hpp>
 
 #include "../prefs/state.hpp"
 
 #include "../world/universalid.hpp"
 
+#include "effectlistcheck.hpp"
+
 CSMTools::EnchantmentCheckStage::EnchantmentCheckStage(const CSMWorld::IdCollection<ESM::Enchantment>& enchantments)
     : mEnchantments(enchantments)
 {
@@ -55,45 +55,5 @@ void CSMTools::EnchantmentCheckStage::perform(int stage, CSMDoc::Messages& messa
     if (enchantment.mData.mCost > enchantment.mData.mCharge)
         messages.add(id, "Cost is higher than charge", "", CSMDoc::Message::Severity_Error);
 
-    if (enchantment.mEffects.mList.empty())
+    effectListCheck(enchantment.mEffects.mList, messages, id);
-    {
-        messages.add(id, "Enchantment doesn't have any magic effects", "", CSMDoc::Message::Severity_Warning);
-    }
-    else
-    {
-        std::vector<ESM::IndexedENAMstruct>::const_iterator effect = enchantment.mEffects.mList.begin();
-
-        for (size_t i = 1; i <= enchantment.mEffects.mList.size(); i++)
-        {
-            const std::string number = std::to_string(i);
-            // At the time of writing this effects, attributes and skills are mostly hardcoded
-            if (effect->mData.mEffectID < 0 || effect->mData.mEffectID > ESM::MagicEffect::Length)
-                messages.add(id, "Effect #" + number + " is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mSkill < -1 || effect->mData.mSkill > ESM::Skill::Length)
-                messages.add(
-                    id, "Effect #" + number + " affected skill is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mAttribute < -1 || effect->mData.mAttribute > ESM::Attribute::Length)
-                messages.add(
-                    id, "Effect #" + number + " affected attribute is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mRange < 0 || effect->mData.mRange > 2)
-                messages.add(id, "Effect #" + number + " range is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mArea < 0)
-                messages.add(id, "Effect #" + number + " area is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mDuration < 0)
-                messages.add(id, "Effect #" + number + " duration is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mMagnMin < 0)
-                messages.add(
-                    id, "Effect #" + number + " minimum magnitude is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mMagnMax < 0)
-                messages.add(
-                    id, "Effect #" + number + " maximum magnitude is negative", "", CSMDoc::Message::Severity_Error);
-            else if (effect->mData.mMagnMax == 0)
-                messages.add(
-                    id, "Effect #" + number + " maximum magnitude is zero", "", CSMDoc::Message::Severity_Warning);
-            if (effect->mData.mMagnMin > effect->mData.mMagnMax)
-                messages.add(id, "Effect #" + number + " minimum magnitude is higher than maximum magnitude", "",
-                    CSMDoc::Message::Severity_Error);
-            ++effect;
-        }
-    }
 }

apps/opencs/model/tools/referenceablecheck.cpp

@@ -38,6 +38,8 @@
 #include "../world/record.hpp"
 #include "../world/universalid.hpp"
 
+#include "effectlistcheck.hpp"
+
 namespace ESM
 {
     class Script;
@@ -331,47 +333,7 @@ void CSMTools::ReferenceableCheckStage::potionCheck(
 
     inventoryItemCheck<ESM::Potion>(potion, messages, id.toString());
 
-    if (potion.mEffects.mList.empty())
+    effectListCheck(potion.mEffects.mList, messages, id);
-    {
-        messages.add(id, "Potion doesn't have any magic effects", "", CSMDoc::Message::Severity_Warning);
-    }
-    else
-    {
-        std::vector<ESM::IndexedENAMstruct>::const_iterator effect = potion.mEffects.mList.begin();
-
-        for (size_t i = 1; i <= potion.mEffects.mList.size(); i++)
-        {
-            const std::string number = std::to_string(i);
-            // At the time of writing this effects, attributes and skills are mostly hardcoded
-            if (effect->mData.mEffectID < 0 || effect->mData.mEffectID > ESM::MagicEffect::Length)
-                messages.add(id, "Effect #" + number + " is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mSkill < -1 || effect->mData.mSkill > ESM::Skill::Length)
-                messages.add(
-                    id, "Effect #" + number + " affected skill is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mAttribute < -1 || effect->mData.mAttribute > ESM::Attribute::Length)
-                messages.add(
-                    id, "Effect #" + number + " affected attribute is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mRange != ESM::RT_Self)
-                messages.add(id, "Effect #" + number + " range is not Self", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mArea < 0)
-                messages.add(id, "Effect #" + number + " area is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mDuration < 0)
-                messages.add(id, "Effect #" + number + " duration is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mMagnMin < 0)
-                messages.add(
-                    id, "Effect #" + number + " minimum magnitude is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mMagnMax < 0)
-                messages.add(
-                    id, "Effect #" + number + " maximum magnitude is negative", "", CSMDoc::Message::Severity_Error);
-            else if (effect->mData.mMagnMax == 0)
-                messages.add(
-                    id, "Effect #" + number + " maximum magnitude is zero", "", CSMDoc::Message::Severity_Warning);
-            if (effect->mData.mMagnMin > effect->mData.mMagnMax)
-                messages.add(id, "Effect #" + number + " minimum magnitude is higher than maximum magnitude", "",
-                    CSMDoc::Message::Severity_Error);
-            ++effect;
-        }
-    }
 
     // Check that mentioned scripts exist
     scriptCheck<ESM::Potion>(potion, messages, id.toString());
@@ -607,27 +569,7 @@ void CSMTools::ReferenceableCheckStage::ingredientCheck(
     // Check that mentioned scripts exist
     scriptCheck<ESM::Ingredient>(ingredient, messages, id.toString());
 
-    bool hasEffects = false;
+    ingredientEffectListCheck(ingredient, messages, id);
-    for (size_t i = 0; i < 4; i++)
-    {
-        if (ingredient.mData.mEffectID[i] == -1)
-            continue;
-
-        hasEffects = true;
-
-        const std::string number = std::to_string(i + 1);
-        // At the time of writing this effects, attributes and skills are mostly hardcoded
-        if (ingredient.mData.mEffectID[i] < -1 || ingredient.mData.mEffectID[i] > ESM::MagicEffect::Length)
-            messages.add(id, "Effect #" + number + " is invalid", "", CSMDoc::Message::Severity_Error);
-        if (ingredient.mData.mSkills[i] < -1 || ingredient.mData.mSkills[i] > ESM::Skill::Length)
-            messages.add(id, "Effect #" + number + " affected skill is invalid", "", CSMDoc::Message::Severity_Error);
-        if (ingredient.mData.mAttributes[i] < -1 || ingredient.mData.mAttributes[i] > ESM::Attribute::Length)
-            messages.add(
-                id, "Effect #" + number + " affected attribute is invalid", "", CSMDoc::Message::Severity_Error);
-    }
-
-    if (!hasEffects)
-        messages.add(id, "Ingredient doesn't have any magic effects", "", CSMDoc::Message::Severity_Warning);
 }
 
 void CSMTools::ReferenceableCheckStage::creaturesLevListCheck(

apps/opencs/model/tools/spellcheck.cpp

@@ -9,11 +9,12 @@
 #include <apps/opencs/model/world/record.hpp>
 #include <apps/opencs/model/world/universalid.hpp>
 
-#include <components/esm/attr.hpp>
 #include <components/esm3/loadspel.hpp>
 
 #include "../prefs/state.hpp"
 
+#include "effectlistcheck.hpp"
+
 CSMTools::SpellCheckStage::SpellCheckStage(const CSMWorld::IdCollection<ESM::Spell>& spells)
     : mSpells(spells)
 {
@@ -47,45 +48,5 @@ void CSMTools::SpellCheckStage::perform(int stage, CSMDoc::Messages& messages)
     if (spell.mData.mCost < 0)
         messages.add(id, "Spell cost is negative", "", CSMDoc::Message::Severity_Error);
 
-    if (spell.mEffects.mList.empty())
+    effectListCheck(spell.mEffects.mList, messages, id);
-    {
-        messages.add(id, "Spell doesn't have any magic effects", "", CSMDoc::Message::Severity_Warning);
-    }
-    else
-    {
-        std::vector<ESM::IndexedENAMstruct>::const_iterator effect = spell.mEffects.mList.begin();
-
-        for (size_t i = 1; i <= spell.mEffects.mList.size(); i++)
-        {
-            const std::string number = std::to_string(i);
-            // At the time of writing this effects, attributes and skills are mostly hardcoded
-            if (effect->mData.mEffectID < 0 || effect->mData.mEffectID > ESM::MagicEffect::Length)
-                messages.add(id, "Effect #" + number + " is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mSkill < -1 || effect->mData.mSkill > ESM::Skill::Length)
-                messages.add(
-                    id, "Effect #" + number + " affected skill is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mAttribute < -1 || effect->mData.mAttribute > ESM::Attribute::Length)
-                messages.add(
-                    id, "Effect #" + number + " affected attribute is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mRange < 0 || effect->mData.mRange > 2)
-                messages.add(id, "Effect #" + number + " range is invalid", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mArea < 0)
-                messages.add(id, "Effect #" + number + " area is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mDuration < 0)
-                messages.add(id, "Effect #" + number + " duration is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mMagnMin < 0)
-                messages.add(
-                    id, "Effect #" + number + " minimum magnitude is negative", "", CSMDoc::Message::Severity_Error);
-            if (effect->mData.mMagnMax < 0)
-                messages.add(
-                    id, "Effect #" + number + " maximum magnitude is negative", "", CSMDoc::Message::Severity_Error);
-            else if (effect->mData.mMagnMax == 0)
-                messages.add(
-                    id, "Effect #" + number + " maximum magnitude is zero", "", CSMDoc::Message::Severity_Warning);
-            if (effect->mData.mMagnMin > effect->mData.mMagnMax)
-                messages.add(id, "Effect #" + number + " minimum magnitude is higher than maximum magnitude", "",
-                    CSMDoc::Message::Severity_Error);
-            ++effect;
-        }
-    }
 }