Improve ruleset validator (#13488)

* Mini-refactor: YearsPerTurn can be immutable, support destructuring

* Fixing pass over RulesetValidator

* Make builtin Ruleset clones keep their name and isBaseRuleset

* Prevent repeated cloning in Ruleset.load (borderline optimization)

* Teach AtlasPreview to load complex Rulesets

* Fix filtering Unique check

* Fix ModRequires on base rulesets check

* Add absolutely minimal ModConstants checking

core/src/com/unciv/models/ruleset/Ruleset.kt

@@ -148,6 +148,9 @@ class Ruleset {
     fun clone(): Ruleset {
         val newRuleset = Ruleset()
         newRuleset.add(this)
+        // Make sure the clone is recognizable - e.g. startNewGame fallback when a base mod was removed needs this
+        newRuleset.name = name
+        newRuleset.modOptions.isBaseRuleset = modOptions.isBaseRuleset
         return newRuleset
     }
 
@@ -439,25 +442,26 @@ class Ruleset {
 
         // Add objects that might not be present in base ruleset mods, but are required
         if (modOptions.isBaseRuleset) {
+            val fallbackRuleset by lazy { RulesetCache.getVanillaRuleset() } // clone at most once
             // This one should be temporary
             if (unitTypes.isEmpty()) {
-                unitTypes.putAll(RulesetCache.getVanillaRuleset().unitTypes)
+                unitTypes.putAll(fallbackRuleset.unitTypes)
             }
 
             // These should be permanent
             if (ruinRewards.isEmpty())
-                ruinRewards.putAll(RulesetCache.getVanillaRuleset().ruinRewards)
+                ruinRewards.putAll(fallbackRuleset.ruinRewards)
 
             if (globalUniques.uniques.isEmpty()) {
-                globalUniques = RulesetCache.getVanillaRuleset().globalUniques
+                globalUniques = fallbackRuleset.globalUniques
             }
             // If we have no victories, add all the default victories
-            if (victories.isEmpty()) victories.putAll(RulesetCache.getVanillaRuleset().victories)
+            if (victories.isEmpty()) victories.putAll(fallbackRuleset.victories)
 
-            if (speeds.isEmpty()) speeds.putAll(RulesetCache.getVanillaRuleset().speeds)
+            if (speeds.isEmpty()) speeds.putAll(fallbackRuleset.speeds)
 
             if (cityStateTypes.isEmpty())
-                for (cityStateType in RulesetCache.getVanillaRuleset().cityStateTypes.values)
+                for (cityStateType in fallbackRuleset.cityStateTypes.values)
                     cityStateTypes[cityStateType.name] = CityStateType().apply {
                         name = cityStateType.name
                         color = cityStateType.color

core/src/com/unciv/models/ruleset/Speed.kt

@@ -29,6 +29,7 @@ class Speed : RulesetObject(), IsPartOfGameInfoSerialization {
     var startYear: Float = -4000f
     var turns: ArrayList<HashMap<String, Float>> = ArrayList()
 
+    data class YearsPerTurn(val yearInterval: Float, val untilTurn: Int)
     val yearsPerTurn: ArrayList<YearsPerTurn> by lazy {
         ArrayList<YearsPerTurn>().apply {
             turns.forEach { this.add(YearsPerTurn(it["yearsPerTurn"]!!, it["untilTurn"]!!.toInt())) }
@@ -83,8 +84,3 @@ class Speed : RulesetObject(), IsPartOfGameInfoSerialization {
 
     fun numTotalTurns(): Int = yearsPerTurn.last().untilTurn
 }
-
-class YearsPerTurn(yearsPerTurn: Float, turnsPerIncrement: Int) {
-    var yearInterval: Float = yearsPerTurn
-    var untilTurn: Int = turnsPerIncrement
-}

core/src/com/unciv/models/ruleset/validation/BaseRulesetValidator.kt

@@ -1,19 +1,18 @@
 package com.unciv.models.ruleset.validation
 
 import com.unciv.Constants
-import com.unciv.logic.map.tile.RoadStatus
+import com.unciv.models.ruleset.Building
-import com.unciv.models.ruleset.BeliefType
 import com.unciv.models.ruleset.MilestoneType
 import com.unciv.models.ruleset.Policy
 import com.unciv.models.ruleset.Ruleset
 import com.unciv.models.ruleset.RulesetCache
+import com.unciv.models.ruleset.nation.Nation
 import com.unciv.models.ruleset.tile.TerrainType
 import com.unciv.models.ruleset.unique.IHasUniques
 import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.ruleset.unit.BaseUnit
 import com.unciv.models.ruleset.unit.Promotion
-import com.unciv.models.ruleset.unit.UnitMovementType
 import com.unciv.models.stats.Stats
 
 /**
@@ -32,55 +31,37 @@ internal class BaseRulesetValidator(
      *  value a Set of its prerequisites including indirect ones */
     private val prereqsHashMap = HashMap<String, HashSet<String>>()
 
-    override fun addBeliefErrors(lines: RulesetErrorList) {
+    init {
-        super.addBeliefErrors(lines)
+        // The `UniqueValidator.checkUntypedUnique` filtering Unique test ("X not found in Unciv's unique types, and is not used as a filtering unique")
-
+        // should not complain when running the RulesetInvariant version, because an Extension Mod may e.g. define additional "Aircraft" and the _use_ of the
-        for (belief in ruleset.beliefs.values) {
+        // filtering Unique only exists in the Base Ruleset. But here we *do* want the test, and it needs its cache filled, and that is not done automatically.
-            if (belief.type == BeliefType.Any || belief.type == BeliefType.None)
+        uniqueValidator.populateFilteringUniqueHashsets()
-                lines.add("${belief.name} type is ${belief.type}, which is not allowed!", sourceObject = belief)
-            uniqueValidator.checkUniques(belief, lines, true, tryFixUnknownUniques)
-        }
     }
 
-    override fun addBuildingErrors(lines: RulesetErrorList) {
+    override fun checkBuilding(building: Building, lines: RulesetErrorList) {
-        // No super.addBuildingErrors(lines): included in the loop below
+        super.checkBuilding(building, lines)
 
-        for (building in ruleset.buildings.values) {
+        for ((gppName, _) in building.greatPersonPoints)
-            addBuildingErrorRulesetInvariant(building, lines)
+            if (!ruleset.units.containsKey(gppName))
-
+                lines.add(
-            for (requiredTech: String in building.requiredTechs())
+                    "Building ${building.name} has greatPersonPoints for $gppName, which is not a unit in the ruleset!",
-                if (!ruleset.technologies.containsKey(requiredTech))
+                    RulesetErrorSeverity.Warning, building
-                    lines.add("${building.name} requires tech $requiredTech which does not exist!", sourceObject = building)
-            for (specialistName in building.specialistSlots.keys)
-                if (!ruleset.specialists.containsKey(specialistName))
-                    lines.add("${building.name} provides specialist $specialistName which does not exist!", sourceObject = building)
-            for (resource in building.getResourceRequirementsPerTurn(StateForConditionals.IgnoreConditionals).keys)
-                if (!ruleset.tileResources.containsKey(resource))
-                    lines.add("${building.name} requires resource $resource which does not exist!", sourceObject = building)
-            if (building.replaces != null && !ruleset.buildings.containsKey(building.replaces!!))
-                lines.add("${building.name} replaces ${building.replaces} which does not exist!", sourceObject = building)
-            if (building.requiredBuilding != null && !ruleset.buildings.containsKey(building.requiredBuilding!!))
-                lines.add("${building.name} requires ${building.requiredBuilding} which does not exist!", sourceObject = building)
-
-            checkUniqueToMisspelling(building, building.uniqueTo, lines)
-            uniqueValidator.checkUniques(building, lines, true, tryFixUnknownUniques)
-        }
-    }
-
-    override fun addCityStateTypeErrors(lines: RulesetErrorList) {
-        super.addCityStateTypeErrors(lines)
-
-        for (cityStateType in ruleset.cityStateTypes.values) {
-            for (unique in cityStateType.allyBonusUniqueMap.getAllUniques() + cityStateType.friendBonusUniqueMap.getAllUniques()) {
-                val errors = uniqueValidator.checkUnique(
-                    unique,
-                    tryFixUnknownUniques,
-                    null,
-                    true
                 )
-                lines.addAll(errors)
+        for (requiredTech: String in building.requiredTechs())
-            }
+            if (!ruleset.technologies.containsKey(requiredTech))
-        }
+                lines.add("${building.name} requires tech $requiredTech which does not exist!", sourceObject = building)
+        for (specialistName in building.specialistSlots.keys)
+            if (!ruleset.specialists.containsKey(specialistName))
+                lines.add("${building.name} provides specialist $specialistName which does not exist!", sourceObject = building)
+        for (resource in building.getResourceRequirementsPerTurn(StateForConditionals.IgnoreConditionals).keys)
+            if (!ruleset.tileResources.containsKey(resource))
+                lines.add("${building.name} requires resource $resource which does not exist!", sourceObject = building)
+        if (building.replaces != null && !ruleset.buildings.containsKey(building.replaces!!))
+            lines.add("${building.name} replaces ${building.replaces} which does not exist!", sourceObject = building)
+        if (building.requiredBuilding != null && !ruleset.buildings.containsKey(building.requiredBuilding!!))
+            lines.add("${building.name} requires ${building.requiredBuilding} which does not exist!", sourceObject = building)
+
+        checkUniqueToMisspelling(building, building.uniqueTo, lines)
     }
 
     override fun addDifficultyErrors(lines: RulesetErrorList) {
@@ -91,6 +72,12 @@ internal class BaseRulesetValidator(
             for (unitName in difficulty.aiCityStateBonusStartingUnits + difficulty.aiMajorCivBonusStartingUnits + difficulty.playerBonusStartingUnits)
                 if (unitName != Constants.eraSpecificUnit && !ruleset.units.containsKey(unitName))
                     lines.add("Difficulty ${difficulty.name} contains starting unit $unitName which does not exist!", sourceObject = null)
+            if (difficulty.aiDifficultyLevel != null && !ruleset.difficulties.containsKey(difficulty.aiDifficultyLevel))
+                lines.add("Difficulty ${difficulty.name} contains aiDifficultyLevel ${difficulty.aiDifficultyLevel} which does not exist!",
+                    RulesetErrorSeverity.Warning, sourceObject = null)
+            for (tech in difficulty.aiFreeTechs)
+                if (!ruleset.technologies.containsKey(tech))
+                    lines.add("Difficulty ${difficulty.name} contains AI free tech $tech which does not exist!", sourceObject = null)
         }
     }
 
@@ -116,9 +103,7 @@ internal class BaseRulesetValidator(
                 if (building !in ruleset.buildings)
                     lines.add("Nonexistent building $building built by settlers when starting in ${era.name}", sourceObject = era)
             // todo the whole 'starting unit' thing needs to be redone, there's no reason we can't have a single list containing all the starting units.
-            if (era.startingSettlerUnit !in ruleset.units
+            if (era.startingSettlerUnit !in ruleset.units && ruleset.units.values.none { it.isCityFounder() })
-                && ruleset.units.values.none { it.isCityFounder() }
-            )
                 lines.add("Nonexistent unit ${era.startingSettlerUnit} marked as starting unit when starting in ${era.name}", sourceObject = era)
             if (era.startingWorkerCount != 0 && era.startingWorkerUnit !in ruleset.units
                 && ruleset.units.values.none { it.hasUnique(UniqueType.BuildImprovements) }
@@ -129,35 +114,6 @@ internal class BaseRulesetValidator(
                 || allDifficultiesStartingUnits.contains(Constants.eraSpecificUnit)
             if (grantsStartingMilitaryUnit && era.startingMilitaryUnit !in ruleset.units)
                 lines.add("Nonexistent unit ${era.startingMilitaryUnit} marked as starting unit when starting in ${era.name}", sourceObject = era)
-            if (era.researchAgreementCost < 0 || era.startingSettlerCount < 0 || era.startingWorkerCount < 0 || era.startingMilitaryUnitCount < 0 || era.startingGold < 0 || era.startingCulture < 0)
-                lines.add("Unexpected negative number found while parsing era ${era.name}", sourceObject = era)
-            if (era.settlerPopulation <= 0)
-                lines.add("Population in cities from settlers must be strictly positive! Found value ${era.settlerPopulation} for era ${era.name}", sourceObject = era)
-
-            if (era.allyBonus.isNotEmpty())
-                lines.add(
-                    "Era ${era.name} contains city-state bonuses. City-state bonuses are now defined in CityStateType.json",
-                    RulesetErrorSeverity.WarningOptionsOnly, era
-                )
-            if (era.friendBonus.isNotEmpty())
-                lines.add(
-                    "Era ${era.name} contains city-state bonuses. City-state bonuses are now defined in CityStateType.json",
-                    RulesetErrorSeverity.WarningOptionsOnly, era
-                )
-
-            uniqueValidator.checkUniques(era, lines, true, tryFixUnknownUniques)
-        }
-    }
-
-    override fun addEventErrors(lines: RulesetErrorList) {
-        super.addEventErrors(lines)
-
-        // An Event is not a IHasUniques, so not suitable as sourceObject
-        for (event in ruleset.events.values) {
-            for (choice in event.choices) {
-                uniqueValidator.checkUniques(choice, lines, true, tryFixUnknownUniques)
-            }
-            uniqueValidator.checkUniques(event, lines, true, tryFixUnknownUniques)
         }
     }
 
@@ -169,68 +125,34 @@ internal class BaseRulesetValidator(
                 lines.add("${improvement.name} requires tech ${improvement.techRequired} which does not exist!", sourceObject = improvement)
             if (improvement.replaces != null && !ruleset.tileImprovements.containsKey(improvement.replaces))
                 lines.add("${improvement.name} replaces ${improvement.replaces} which does not exist!", sourceObject = improvement)
-            if (improvement.replaces != null && improvement.uniqueTo == null)
+
-                lines.add("${improvement.name} should replace ${improvement.replaces} but does not have uniqueTo assigned!")
             checkUniqueToMisspelling(improvement, improvement.uniqueTo, lines)
+
             for (terrain in improvement.terrainsCanBeBuiltOn)
                 if (!ruleset.terrains.containsKey(terrain) && terrain != "Land" && terrain != "Water")
                     lines.add("${improvement.name} can be built on terrain $terrain which does not exist!", sourceObject = improvement)
-            if (improvement.terrainsCanBeBuiltOn.isEmpty()
-                && !improvement.hasUnique(UniqueType.CanOnlyImproveResource)
-                && !improvement.hasUnique(UniqueType.Unbuildable)
-                && !improvement.name.startsWith(Constants.remove)
-                && improvement.name !in RoadStatus.entries.map { it.removeAction }
-                && improvement.name != Constants.cancelImprovementOrder
-            ) {
-                lines.add(
-                    "${improvement.name} has an empty `terrainsCanBeBuiltOn`, isn't allowed to only improve resources. As such it isn't buildable! Either give this the unique \"Unbuildable\", \"Can only be built to improve a resource\", or add \"Land\", \"Water\" or any other value to `terrainsCanBeBuiltOn`.",
-                    RulesetErrorSeverity.Warning, improvement
-                )
-            }
-            for (unique in improvement.uniqueObjects
-                .filter { it.type == UniqueType.PillageYieldRandom || it.type == UniqueType.PillageYieldFixed }) {
-                if (!Stats.isStats(unique.params[0])) continue
-                val params = Stats.parse(unique.params[0])
-                if (params.values.any { it < 0 }) lines.add(
-                    "${improvement.name} cannot have a negative value for a pillage yield!",
-                    RulesetErrorSeverity.Error, improvement
-                )
-            }
-
-            val hasPillageUnique = improvement.hasUnique(UniqueType.PillageYieldRandom, StateForConditionals.IgnoreConditionals)
-                || improvement.hasUnique(UniqueType.PillageYieldFixed, StateForConditionals.IgnoreConditionals)
-            if (hasPillageUnique && improvement.hasUnique(UniqueType.Unpillagable, StateForConditionals.IgnoreConditionals)) {
-                lines.add(
-                    "${improvement.name} has both an `Unpillagable` unique type and a `PillageYieldRandom` or `PillageYieldFixed` unique type!",
-                    RulesetErrorSeverity.Warning, improvement
-                )
-            }
-            uniqueValidator.checkUniques(improvement, lines, true, tryFixUnknownUniques)
         }
     }
 
     override fun addModOptionsErrors(lines: RulesetErrorList) {
         super.addModOptionsErrors(lines)
 
+        // `ruleset` can be a true base ruleset or a combined one when we're checking an extension mod together with a base.
+        // In the combined case, don't complain about ModRequires!
+        if (ruleset.name.isEmpty() && ruleset.mods.size > 1) return
+
         for (unique in ruleset.modOptions.getMatchingUniques(UniqueType.ModRequires)) {
             lines.add("Mod option '${unique.text}' is invalid for a base ruleset.", sourceObject = null)
         }
     }
 
-    override fun addNationErrors(lines: RulesetErrorList) {
+    override fun checkNation(nation: Nation, lines: RulesetErrorList) {
-        // No super.addNationErrors(lines), included in loop below
+        if (nation.preferredVictoryType != Constants.neutralVictoryType && nation.preferredVictoryType !in ruleset.victories)
-        for (nation in ruleset.nations.values) {
+            lines.add("${nation.name}'s preferredVictoryType is ${nation.preferredVictoryType} which does not exist!", sourceObject = nation)
-            addNationErrorRulesetInvariant(nation, lines)
+        if (nation.cityStateType != null && nation.cityStateType !in ruleset.cityStateTypes)
-
+            lines.add("${nation.name} is of city-state type ${nation.cityStateType} which does not exist!", sourceObject = nation)
-            uniqueValidator.checkUniques(nation, lines, true, tryFixUnknownUniques)
+        if (nation.favoredReligion != null && nation.favoredReligion !in ruleset.religions)
-
+            lines.add("${nation.name} has ${nation.favoredReligion} as their favored religion, which does not exist!", sourceObject = nation)
-            if (nation.preferredVictoryType != Constants.neutralVictoryType && nation.preferredVictoryType !in ruleset.victories)
-                lines.add("${nation.name}'s preferredVictoryType is ${nation.preferredVictoryType} which does not exist!", sourceObject = nation)
-            if (nation.cityStateType != null && nation.cityStateType !in ruleset.cityStateTypes)
-                lines.add("${nation.name} is of city-state type ${nation.cityStateType} which does not exist!", sourceObject = nation)
-            if (nation.favoredReligion != null && nation.favoredReligion !in ruleset.religions)
-                lines.add("${nation.name} has ${nation.favoredReligion} as their favored religion, which does not exist!", sourceObject = nation)
-        }
     }
 
     override fun addPersonalityErrors(lines: RulesetErrorList) {
@@ -240,7 +162,7 @@ internal class BaseRulesetValidator(
             if (personality.preferredVictoryType != Constants.neutralVictoryType
                 && personality.preferredVictoryType !in ruleset.victories) {
                 lines.add("Preferred victory type ${personality.preferredVictoryType} does not exist in ruleset",
-                    RulesetErrorSeverity.Warning, sourceObject = personality,)
+                    RulesetErrorSeverity.Warning, sourceObject = personality)
             }
         }
     }
@@ -252,8 +174,6 @@ internal class BaseRulesetValidator(
             for (prereq in policy.requires ?: emptyList())
                 if (!ruleset.policies.containsKey(prereq))
                     lines.add("${policy.name} requires policy $prereq which does not exist!", sourceObject = policy)
-
-            uniqueValidator.checkUniques(policy, lines, true, tryFixUnknownUniques)
         }
 
         for (branch in ruleset.policyBranches.values) {
@@ -271,37 +191,33 @@ internal class BaseRulesetValidator(
             }
         }
 
-
         for (policy in ruleset.policyBranches.values.flatMap { it.policies + it })
             if (policy != ruleset.policies[policy.name])
                 lines.add("More than one policy with the name ${policy.name} exists!", sourceObject = policy)
-
     }
 
     override fun addPromotionErrors(lines: RulesetErrorList) {
-        // No super.addPromotionErrors(lines): included below
+        super.addPromotionErrors(lines)
-        //TODO except the contrast check
-        for (promotion in ruleset.unitPromotions.values) {
-            addPromotionErrorRulesetInvariant(promotion, lines)
-
-            // These are warning as of 3.17.5 to not break existing mods and give them time to correct, should be upgraded to error in the future
-            for (prereq in promotion.prerequisites)
-                if (!ruleset.unitPromotions.containsKey(prereq))
-                    lines.add(
-                        "${promotion.name} requires promotion $prereq which does not exist!",
-                        RulesetErrorSeverity.Warning, promotion
-                    )
-            for (unitType in promotion.unitTypes) checkUnitType(unitType) {
-                lines.add(
-                    "${promotion.name} references unit type $unitType, which does not exist!",
-                    RulesetErrorSeverity.Warning, promotion
-                )
-            }
-            uniqueValidator.checkUniques(promotion, lines, true, tryFixUnknownUniques)
-        }
         checkPromotionCircularReferences(lines)
     }
 
+    override fun checkPromotion(promotion: Promotion, lines: RulesetErrorList) {
+        super.checkPromotion(promotion, lines)
+
+        for (prereq in promotion.prerequisites)
+            if (!ruleset.unitPromotions.containsKey(prereq))
+                lines.add(
+                    "${promotion.name} requires promotion $prereq which does not exist!",
+                    RulesetErrorSeverity.ErrorOptionsOnly, promotion
+                )
+        for (unitType in promotion.unitTypes) checkUnitType(unitType) {
+            lines.add(
+                "${promotion.name} references unit type $unitType, which does not exist!",
+                RulesetErrorSeverity.ErrorOptionsOnly, promotion
+            )
+        }
+    }
+
     private fun checkPromotionCircularReferences(lines: RulesetErrorList) {
         fun recursiveCheck(history: HashSet<Promotion>, promotion: Promotion, level: Int) {
             if (promotion in history) {
@@ -327,8 +243,6 @@ internal class BaseRulesetValidator(
     }
 
     override fun addResourceErrors(lines: RulesetErrorList) {
-        // No super.addResourceErrors(lines), included below
-
         for (resource in ruleset.tileResources.values) {
             if (resource.revealedBy != null && !ruleset.technologies.containsKey(resource.revealedBy!!))
                 lines.add("${resource.name} revealed by tech ${resource.revealedBy} which does not exist!", sourceObject = resource)
@@ -340,20 +254,17 @@ internal class BaseRulesetValidator(
             for (terrain in resource.terrainsCanBeFoundOn)
                 if (!ruleset.terrains.containsKey(terrain))
                     lines.add("${resource.name} can be found on terrain $terrain which does not exist!", sourceObject = resource)
-            uniqueValidator.checkUniques(resource, lines, true, tryFixUnknownUniques)
         }
+        super.addResourceErrors(lines)
     }
 
     override fun addRuinsErrors(lines: RulesetErrorList) {
         super.addRuinsErrors(lines)
 
         for (reward in ruleset.ruinRewards.values) {
-            @Suppress("KotlinConstantConditions") // data is read from json, so any assumptions may be wrong
-            if (reward.weight < 0) lines.add("${reward.name} has a negative weight, which is not allowed!", sourceObject = reward)
             for (difficulty in reward.excludedDifficulties)
                 if (!ruleset.difficulties.containsKey(difficulty))
                     lines.add("${reward.name} references difficulty ${difficulty}, which does not exist!", sourceObject = reward)
-            uniqueValidator.checkUniques(reward, lines, true, tryFixUnknownUniques)
         }
     }
 
@@ -362,29 +273,17 @@ internal class BaseRulesetValidator(
 
         // Specialist is not a IHasUniques and unsuitable as sourceObject
         for (specialist in ruleset.specialists.values) {
-            for (gpp in specialist.greatPersonPoints)
+            for ((gppName, _) in specialist.greatPersonPoints)
-                if (gpp.key !in ruleset.units)
+                if (gppName !in ruleset.units)
                     lines.add(
-                        "Specialist ${specialist.name} has greatPersonPoints for ${gpp.key}, which is not a unit in the ruleset!",
+                        "Specialist ${specialist.name} has greatPersonPoints for $gppName, which is not a unit in the ruleset!",
                         RulesetErrorSeverity.Warning, sourceObject = null
                     )
         }
     }
 
-    override fun addSpeedErrors(lines: RulesetErrorList) {
-        super.addSpeedErrors(lines)
-
-        for (speed in ruleset.speeds.values) {
-            if (speed.modifier < 0f)
-                lines.add("Negative speed modifier for game speed ${speed.name}", sourceObject = speed)
-            if (speed.yearsPerTurn.isEmpty())
-                lines.add("Empty turn increment list for game speed ${speed.name}", sourceObject = speed)
-        }
-    }
-
     override fun addTechErrors(lines: RulesetErrorList) {
-        // No super.addTechErrors(lines) or we would duplicate the checkUniques
+        super.addTechErrors(lines)
-        //TODO missing `row < 1` check -> unify
 
         for (tech in ruleset.technologies.values) {
             for (prereq in tech.prerequisites) {
@@ -394,7 +293,7 @@ internal class BaseRulesetValidator(
                 if (tech.prerequisites.any { it != prereq && getPrereqTree(it).contains(prereq) }) {
                     lines.add(
                         "No need to add $prereq as a prerequisite of ${tech.name} - it is already implicit from the other prerequisites!",
-                        RulesetErrorSeverity.Warning, tech
+                        RulesetErrorSeverity.WarningOptionsOnly, tech
                     )
                 }
 
@@ -403,13 +302,15 @@ internal class BaseRulesetValidator(
             }
             if (tech.era() !in ruleset.eras)
                 lines.add("Unknown era ${tech.era()} referenced in column of tech ${tech.name}", sourceObject = tech)
-            uniqueValidator.checkUniques(tech, lines, true, tryFixUnknownUniques)
+
+            for (otherTech in ruleset.technologies.values) {
+                if (tech.name > otherTech.name && otherTech.column?.columnNumber == tech.column?.columnNumber && otherTech.row == tech.row)
+                    lines.add("${tech.name} is in the same row and column as ${otherTech.name}!", sourceObject = tech)
+            }
         }
     }
 
     override fun addTerrainErrors(lines: RulesetErrorList) {
-        super.addTerrainErrors(lines)
-
         if (ruleset.terrains.values.none { it.type == TerrainType.Land && !it.impassable && !it.hasUnique(
                 UniqueType.NoNaturalGeneration) })
             lines.add("No passable land terrains exist!", sourceObject = null)
@@ -430,26 +331,21 @@ internal class BaseRulesetValidator(
                 // See https://github.com/hackedpassword/Z2/blob/main/HybridTileTech.md for a clever exploit
                     lines.add("${terrain.name} turns into terrain ${terrain.turnsInto} which is not a base terrain!", RulesetErrorSeverity.Warning, terrain)
             }
-            uniqueValidator.checkUniques(terrain, lines, true, tryFixUnknownUniques)
         }
+
+        super.addTerrainErrors(lines)
     }
 
     override fun addUnitErrors(lines: RulesetErrorList) {
-        // No super.addUnitErrors(lines), included below
-
         if (ruleset.units.values.none { it.isCityFounder() })
             lines.add("No city-founding units in ruleset!", sourceObject = null)
-
+        super.addUnitErrors(lines)
-        for (unit in ruleset.units.values) {
-            checkUnitRulesetInvariant(unit, lines)
-            checkUnit(unit, lines)
-            uniqueValidator.checkUniques(unit, lines, true, tryFixUnknownUniques)
-            checkUniqueToMisspelling(unit, unit.uniqueTo, lines)
-        }
     }
 
     /** Collects all RulesetSpecific checks for a BaseUnit */
-    private fun checkUnit(unit: BaseUnit, lines: RulesetErrorList) {
+    override fun checkUnit(unit: BaseUnit, lines: RulesetErrorList) {
+        super.checkUnit(unit, lines)
+
         for (requiredTech: String in unit.requiredTechs())
             if (!ruleset.technologies.containsKey(requiredTech))
                 lines.add("${unit.name} requires tech $requiredTech which does not exist!", sourceObject = unit)
@@ -498,17 +394,8 @@ internal class BaseRulesetValidator(
                     RulesetErrorSeverity.WarningOptionsOnly, unit)
             }
         }
-    }
 
-    override fun addUnitTypeErrors(lines: RulesetErrorList) {
+        checkUniqueToMisspelling(unit, unit.uniqueTo, lines)
-        super.addUnitTypeErrors(lines)
-
-        val unitMovementTypes = UnitMovementType.entries.map { it.name }.toSet()
-        for (unitType in ruleset.unitTypes.values) {
-            if (unitType.movementType !in unitMovementTypes)
-                lines.add("Unit type ${unitType.name} has an invalid movement type ${unitType.movementType}", sourceObject = unitType)
-            uniqueValidator.checkUniques(unitType, lines, true, tryFixUnknownUniques)
-        }
     }
 
     override fun addVictoryTypeErrors(lines: RulesetErrorList) {
@@ -524,11 +411,6 @@ internal class BaseRulesetValidator(
                     )
 
             for (milestone in victoryType.milestoneObjects) {
-                if (milestone.type == null)
-                    lines.add(
-                        "Victory type ${victoryType.name} has milestone \"${milestone.uniqueDescription}\" that is of an unknown type!",
-                        RulesetErrorSeverity.Error, sourceObject = null
-                    )
                 if (milestone.type in listOf(MilestoneType.BuiltBuilding, MilestoneType.BuildingBuiltGlobally)
                     && milestone.params[0] !in ruleset.buildings)
                     lines.add(
@@ -536,13 +418,6 @@ internal class BaseRulesetValidator(
                         RulesetErrorSeverity.Error,
                     )
             }
-
-            for (victory in ruleset.victories.values)
-                if (victory.name != victoryType.name && victory.milestones == victoryType.milestones)
-                    lines.add(
-                        "Victory types ${victoryType.name} and ${victory.name} have the same requirements!",
-                        RulesetErrorSeverity.Warning, sourceObject = null
-                    )
         }
     }
 

core/src/com/unciv/models/ruleset/validation/RulesetValidator.kt

@@ -7,6 +7,8 @@ import com.unciv.Constants
 import com.unciv.UncivGame
 import com.unciv.json.fromJsonFile
 import com.unciv.json.json
+import com.unciv.logic.map.tile.RoadStatus
+import com.unciv.models.ruleset.BeliefType
 import com.unciv.models.ruleset.Building
 import com.unciv.models.ruleset.IRulesetObject
 import com.unciv.models.ruleset.Ruleset
@@ -24,8 +26,10 @@ import com.unciv.models.ruleset.unique.UniqueTarget
 import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.ruleset.unit.BaseUnit
 import com.unciv.models.ruleset.unit.Promotion
+import com.unciv.models.ruleset.unit.UnitMovementType
 import com.unciv.models.ruleset.validation.RulesetValidator.Companion.create
 import com.unciv.models.stats.INamed
+import com.unciv.models.stats.Stats
 import com.unciv.models.tilesets.TileSetCache
 import com.unciv.models.tilesets.TileSetConfig
 import com.unciv.ui.images.AtlasPreview
@@ -56,7 +60,7 @@ import com.unciv.ui.images.PortraitPromotion
  */
 open class RulesetValidator protected constructor(
     protected val ruleset: Ruleset,
-    protected val tryFixUnknownUniques: Boolean
+    private val tryFixUnknownUniques: Boolean
 ) {
     /** `true` for a [BaseRulesetValidator] instance, `false` for a [RulesetValidator] instance. */
     private val reportRulesetSpecificErrors = ruleset.modOptions.isBaseRuleset
@@ -125,40 +129,82 @@ open class RulesetValidator protected constructor(
 
     //region RulesetObject-specific handlers
 
-    protected open fun addBeliefErrors(lines: RulesetErrorList) {}
+    protected open fun addBeliefErrors(lines: RulesetErrorList) {
-
+        for (belief in ruleset.beliefs.values) {
-    protected open fun addBuildingErrors(lines: RulesetErrorList) {
+            if (belief.type == BeliefType.Any || belief.type == BeliefType.None)
-        for (building in ruleset.buildings.values) {
+                lines.add("${belief.name} type is ${belief.type}, which is not allowed!", sourceObject = belief)
-            addBuildingErrorRulesetInvariant(building, lines)
+            uniqueValidator.checkUniques(belief, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
-            uniqueValidator.checkUniques(building, lines, false, tryFixUnknownUniques)
         }
     }
 
-    protected fun addBuildingErrorRulesetInvariant(building: Building, lines: RulesetErrorList) {
+    protected open fun addBuildingErrors(lines: RulesetErrorList) {
-        if (building.requiredTechs().none() && building.cost == -1 && !building.hasUnique(
+        for (building in ruleset.buildings.values) {
-                UniqueType.Unbuildable
+            checkBuilding(building, lines)
-            )
+            uniqueValidator.checkUniques(building, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
-        )
+        }
+    }
+
+    protected open fun checkBuilding(building: Building, lines: RulesetErrorList) {
+        if (building.requiredTechs().none() && building.cost == -1 && !building.hasUnique(UniqueType.Unbuildable))
             lines.add(
                 "${building.name} is buildable and therefore should either have an explicit cost or reference an existing tech!",
                 RulesetErrorSeverity.Warning, building
             )
 
-        for (gpp in building.greatPersonPoints)
-            if (gpp.key !in ruleset.units)
-                lines.add(
-                    "Building ${building.name} has greatPersonPoints for ${gpp.key}, which is not a unit in the ruleset!",
-                    RulesetErrorSeverity.Warning, building
-                )
-
         if (building.replaces != null && building.uniqueTo == null)
             lines.add("${building.name} should replace ${building.replaces} but does not have uniqueTo assigned!")
     }
 
-    protected open fun addCityStateTypeErrors(lines: RulesetErrorList) {}
+    protected open fun addCityStateTypeErrors(lines: RulesetErrorList) {
-    protected open fun addDifficultyErrors(lines: RulesetErrorList) {}
+        for (cityStateType in ruleset.cityStateTypes.values) {
-    protected open fun addEraErrors(lines: RulesetErrorList) {}
+            for (unique in cityStateType.allyBonusUniqueMap.getAllUniques() + cityStateType.friendBonusUniqueMap.getAllUniques()) {
-    protected open fun addEventErrors(lines: RulesetErrorList) {}
+                val errors = uniqueValidator.checkUnique(unique, tryFixUnknownUniques, null, reportRulesetSpecificErrors)
+                lines.addAll(errors)
+            }
+        }
+    }
+
+    protected open fun addDifficultyErrors(lines: RulesetErrorList) {
+        for (difficulty in ruleset.difficulties.values) {
+            if (difficulty.aiBuildingCostModifier < 0 || difficulty.aiBuildingMaintenanceModifier < 0 || difficulty.aiCityGrowthModifier < 0 ||
+                difficulty.aiUnhappinessModifier < 0 || difficulty.aiUnitCostModifier < 0 || difficulty.aiUnitMaintenanceModifier < 0 ||
+                difficulty.aiUnitSupplyModifier < 0 || difficulty.aiWonderCostModifier < 0 ||
+                difficulty.buildingCostModifier < 0 || difficulty.policyCostModifier < 0 || difficulty.researchCostModifier < 0 ||
+                difficulty.unhappinessModifier < 0 || difficulty.unitCostModifier < 0)
+                lines.add("Difficulty ${difficulty.name} contains one or more negative modifier(s)!", sourceObject = null)
+            if (difficulty.turnBarbariansCanEnterPlayerTiles < 0)
+                lines.add("Difficulty ${difficulty.name} has a negative turnBarbariansCanEnterPlayerTiles!",
+                    RulesetErrorSeverity.Warning, sourceObject = null)
+        }
+    }
+
+    protected open fun addEraErrors(lines: RulesetErrorList) {
+        for (era in ruleset.eras.values) {
+            if (era.researchAgreementCost < 0 || era.startingSettlerCount < 0 || era.startingWorkerCount < 0 || 
+                era.startingMilitaryUnitCount < 0 || era.startingGold < 0 || era.startingCulture < 0)
+                lines.add("Unexpected negative number found while parsing era ${era.name}", sourceObject = era)
+            if (era.settlerPopulation <= 0)
+                lines.add("Population in cities from settlers must be strictly positive! Found value ${era.settlerPopulation} for era ${era.name}", sourceObject = era)
+
+            if (era.allyBonus.isNotEmpty() || era.friendBonus.isNotEmpty())
+                lines.add(
+                    "Era ${era.name} contains city-state bonuses. City-state bonuses are now defined in CityStateType.json",
+                    RulesetErrorSeverity.WarningOptionsOnly, era
+                )
+
+            uniqueValidator.checkUniques(era, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
+        }
+    }
+
+    protected open fun addEventErrors(lines: RulesetErrorList) {
+        // An Event is not a IHasUniques, so not suitable as sourceObject
+        for (event in ruleset.events.values) {
+            for (choice in event.choices) {
+                uniqueValidator.checkUniques(choice, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
+            }
+            uniqueValidator.checkUniques(event, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
+        }
+    }
 
     protected open fun addGlobalUniqueErrors(lines: RulesetErrorList) {
         uniqueValidator.checkUniques(ruleset.globalUniques, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
@@ -183,13 +229,64 @@ open class RulesetValidator protected constructor(
         }
     }
 
-    protected open fun addImprovementErrors(lines: RulesetErrorList) {}
+    protected open fun addImprovementErrors(lines: RulesetErrorList) {
+        for (improvement in ruleset.tileImprovements.values) {
+            if (improvement.replaces != null && improvement.uniqueTo == null)
+                lines.add("${improvement.name} should replace ${improvement.replaces} but does not have uniqueTo assigned!")
+            if (improvement.terrainsCanBeBuiltOn.isEmpty()
+                && !improvement.hasUnique(UniqueType.CanOnlyImproveResource)
+                && !improvement.hasUnique(UniqueType.Unbuildable)
+                && !improvement.name.startsWith(Constants.remove)
+                && improvement.name !in RoadStatus.entries.map { it.removeAction }
+                && improvement.name != Constants.cancelImprovementOrder
+            ) {
+                lines.add(
+                    "${improvement.name} has an empty `terrainsCanBeBuiltOn`, isn't allowed to only improve resources. As such it isn't buildable! Either give this the unique \"Unbuildable\", \"Can only be built to improve a resource\", or add \"Land\", \"Water\" or any other value to `terrainsCanBeBuiltOn`.",
+                    RulesetErrorSeverity.Warning, improvement
+                )
+            }
+
+            for (unique in improvement.uniqueObjects
+                .filter { it.type == UniqueType.PillageYieldRandom || it.type == UniqueType.PillageYieldFixed }) {
+                if (!Stats.isStats(unique.params[0])) continue
+                val params = Stats.parse(unique.params[0])
+                if (params.values.any { it < 0 }) lines.add(
+                    "${improvement.name} cannot have a negative value for a pillage yield!",
+                    RulesetErrorSeverity.Error, improvement
+                )
+            }
+
+            val hasPillageUnique = improvement.hasUnique(UniqueType.PillageYieldRandom, StateForConditionals.IgnoreConditionals)
+                || improvement.hasUnique(UniqueType.PillageYieldFixed, StateForConditionals.IgnoreConditionals)
+            if (hasPillageUnique && improvement.hasUnique(UniqueType.Unpillagable, StateForConditionals.IgnoreConditionals)) {
+                lines.add(
+                    "${improvement.name} has both an `Unpillagable` unique type and a `PillageYieldRandom` or `PillageYieldFixed` unique type!",
+                    RulesetErrorSeverity.Warning, improvement
+                )
+            }
+
+            uniqueValidator.checkUniques(improvement, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
+        }
+    }
 
     protected open fun addModOptionsErrors(lines: RulesetErrorList) {
         // Basic Unique validation (type, target, parameters) should always run.
         // Using reportRulesetSpecificErrors=true as ModOptions never should use Uniques depending on objects from a base ruleset anyway.
         uniqueValidator.checkUniques(ruleset.modOptions, lines, reportRulesetSpecificErrors = true, tryFixUnknownUniques)
 
+        //TODO: More thorough checks. Here I picked just those where bad values might endanger stability.
+        val constants = ruleset.modOptions.constants
+        if (constants.cityExpandRange !in 1..100)
+            lines.add("Invalid ModConstant 'cityExpandRange'.", sourceObject = null)
+        if (constants.cityWorkRange !in 1..100)
+            lines.add("Invalid ModConstant 'cityWorkRange'.", sourceObject = null)
+        if (constants.minimalCityDistance < 1)
+            lines.add("Invalid ModConstant 'minimalCityDistance'.", sourceObject = null)
+        if (constants.minimalCityDistanceOnDifferentContinents < 1)
+            lines.add("Invalid ModConstant 'minimalCityDistanceOnDifferentContinents'.", sourceObject = null)
+        if (constants.baseCityBombardRange < 1)
+            lines.add("Invalid ModConstant 'baseCityBombardRange'.", sourceObject = null)
+
         if (ruleset.name.isBlank()) return // The rest of these tests don't make sense for combined rulesets
 
         val audioVisualUniqueTypes = setOf(
@@ -220,12 +317,12 @@ open class RulesetValidator protected constructor(
 
     protected open fun addNationErrors(lines: RulesetErrorList) {
         for (nation in ruleset.nations.values) {
-            addNationErrorRulesetInvariant(nation, lines)
+            checkNation(nation, lines)
-            uniqueValidator.checkUniques(nation, lines, false, tryFixUnknownUniques)
+            uniqueValidator.checkUniques(nation, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
         }
     }
 
-    protected fun addNationErrorRulesetInvariant(nation: Nation, lines: RulesetErrorList) {
+    protected open fun checkNation(nation: Nation, lines: RulesetErrorList) {
         if (nation.cities.isEmpty() && !nation.isSpectator && !nation.isBarbarian) {
             lines.add("${nation.name} can settle cities, but has no city names!", sourceObject = nation)
         }
@@ -233,19 +330,29 @@ open class RulesetValidator protected constructor(
         checkContrasts(nation.getInnerColor(), nation.getOuterColor(), nation, lines)
     }
 
-    protected open fun addPersonalityErrors(lines: RulesetErrorList) {}
+    protected open fun addPersonalityErrors(lines: RulesetErrorList) {
-    protected open fun addPolicyErrors(lines: RulesetErrorList) {}
+        for (personality in ruleset.personalities.values) {
-
+            if (personality.uniques.isNotEmpty())
-    protected open fun addPromotionErrors(lines: RulesetErrorList) {
+                lines.add("Personality Uniques are not supported", RulesetErrorSeverity.Warning, personality)
-        for (promotion in ruleset.unitPromotions.values) {
-            uniqueValidator.checkUniques(promotion, lines, false, tryFixUnknownUniques)
-            checkContrasts(promotion.innerColorObject ?: PortraitPromotion.defaultInnerColor,
-                promotion.outerColorObject ?: PortraitPromotion.defaultOuterColor, promotion, lines)
-            addPromotionErrorRulesetInvariant(promotion, lines)
         }
     }
 
-    protected fun addPromotionErrorRulesetInvariant(promotion: Promotion, lines: RulesetErrorList) {
+    protected open fun addPolicyErrors(lines: RulesetErrorList) {
+        for (policy in ruleset.policies.values) {
+            uniqueValidator.checkUniques(policy, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
+        }
+    }
+
+    protected open fun addPromotionErrors(lines: RulesetErrorList) {
+        for (promotion in ruleset.unitPromotions.values) {
+            uniqueValidator.checkUniques(promotion, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
+            checkContrasts(promotion.innerColorObject ?: PortraitPromotion.defaultInnerColor,
+                promotion.outerColorObject ?: PortraitPromotion.defaultOuterColor, promotion, lines)
+            checkPromotion(promotion, lines)
+        }
+    }
+
+    protected open fun checkPromotion(promotion: Promotion, lines: RulesetErrorList) {
         if (promotion.row < -1) lines.add("Promotion ${promotion.name} has invalid row value: ${promotion.row}", sourceObject = promotion)
         if (promotion.column < 0) lines.add("Promotion ${promotion.name} has invalid column value: ${promotion.column}", sourceObject = promotion)
         if (promotion.row == -1) return
@@ -256,18 +363,49 @@ open class RulesetValidator protected constructor(
 
     protected open fun addResourceErrors(lines: RulesetErrorList) {
         for (resource in ruleset.tileResources.values) {
-            uniqueValidator.checkUniques(resource, lines, false, tryFixUnknownUniques)
+            uniqueValidator.checkUniques(resource, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
+        }
+    }
+
+    protected open fun addRuinsErrors(lines: RulesetErrorList) {
+        for (reward in ruleset.ruinRewards.values) {
+            @Suppress("KotlinConstantConditions") // data is read from json, so any assumptions may be wrong
+            if (reward.weight < 0) lines.add("${reward.name} has a negative weight, which is not allowed!", sourceObject = reward)
+            uniqueValidator.checkUniques(reward, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
         }
     }
 
-    protected open fun addRuinsErrors(lines: RulesetErrorList) {}
     protected open fun addSpecialistErrors(lines: RulesetErrorList) {}
-    protected open fun addSpeedErrors(lines: RulesetErrorList) {}
+
+    protected open fun addSpeedErrors(lines: RulesetErrorList) {
+        for (speed in ruleset.speeds.values) {
+            if (speed.modifier < 0f || speed.barbarianModifier < 0f || speed.cultureCostModifier < 0f || speed.faithCostModifier < 0f ||
+                speed.goldCostModifier < 0f || speed.goldGiftModifier < 0f || speed.goldenAgeLengthModifier < 0f ||
+                speed.improvementBuildLengthModifier < 0f || speed.productionCostModifier < 0f || speed.scienceCostModifier < 0f)
+                lines.add("One or more negative speed modifier(s) for game speed ${speed.name}", sourceObject = speed)
+            if (speed.dealDuration < 1 || speed.peaceDealDuration < 1)
+                lines.add("Deal durations must be positive", sourceObject = speed)
+            if (speed.religiousPressureAdjacentCity < 0)
+                lines.add("'religiousPressureAdjacentCity' must not be negative", sourceObject = speed)
+            if (speed.yearsPerTurn.isEmpty())
+                lines.add("Empty turn increment list for game speed ${speed.name}", sourceObject = speed)
+            var lastTurn = 0
+            for ((yearInterval, untilTurn) in speed.yearsPerTurn) {
+                if (yearInterval <= 0f)
+                    lines.add("Negative year interval $yearInterval in turn increment list", sourceObject = speed)
+                if (untilTurn <= lastTurn)
+                    lines.add("The 'untilTurn' field in the turn increment list must be monotonously increasing, but $untilTurn is <= $lastTurn", sourceObject = speed)
+                lastTurn = untilTurn
+            }
+            if (speed.uniques.isNotEmpty())
+                lines.add("Speed Uniques are not supported", RulesetErrorSeverity.Warning, speed)
+        }
+    }
 
     protected open fun addTechErrors(lines: RulesetErrorList) {
         for (tech in ruleset.technologies.values) {
             if (tech.row < 1) lines.add("Tech ${tech.name} has a row value below 1: ${tech.row}", sourceObject = tech)
-            uniqueValidator.checkUniques(tech, lines, false, tryFixUnknownUniques)
+            uniqueValidator.checkUniques(tech, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
         }
     }
 
@@ -295,27 +433,24 @@ open class RulesetValidator protected constructor(
                     RulesetErrorSeverity.Warning, sourceObject = null
                 )
         }
-
-        for (tech in ruleset.technologies.values) {
-            for (otherTech in ruleset.technologies.values) {
-                if (tech != otherTech && otherTech.column?.columnNumber == tech.column?.columnNumber && otherTech.row == tech.row)
-                    lines.add("${tech.name} is in the same row and column as ${otherTech.name}!", sourceObject = tech)
-            }
-        }
     }
 
-    protected open fun addTerrainErrors(lines: RulesetErrorList) {}
+    protected open fun addTerrainErrors(lines: RulesetErrorList) {
+        for (terrain in ruleset.terrains.values) {
+            uniqueValidator.checkUniques(terrain, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
+        }
+    }
 
     protected open fun addUnitErrors(lines: RulesetErrorList) {
         for (unit in ruleset.units.values) {
-            checkUnitRulesetInvariant(unit, lines)
+            checkUnit(unit, lines)
-            uniqueValidator.checkUniques(unit, lines, false, tryFixUnknownUniques)
+            uniqueValidator.checkUniques(unit, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
         }
     }
 
-    protected fun checkUnitRulesetInvariant(unit: BaseUnit, lines: RulesetErrorList) {
+    protected open fun checkUnit(unit: BaseUnit, lines: RulesetErrorList) {
         for (upgradesTo in unit.getUpgradeUnits(StateForConditionals.IgnoreConditionals)) {
-            if (upgradesTo == unit.name || (upgradesTo == unit.replaces))
+            if (upgradesTo == unit.name || upgradesTo == unit.replaces)
                 lines.add("${unit.name} upgrades to itself!", sourceObject = unit)
         }
 
@@ -326,8 +461,34 @@ open class RulesetValidator protected constructor(
             lines.add("${unit.name} is a military unit but has no assigned strength!", sourceObject = unit)
     }
 
-    protected open fun addUnitTypeErrors(lines: RulesetErrorList) {}
+    protected open fun addUnitTypeErrors(lines: RulesetErrorList) {
-    protected open fun addVictoryTypeErrors(lines: RulesetErrorList) {}
+        val unitMovementTypes = UnitMovementType.entries.map { it.name }.toSet()
+        for (unitType in ruleset.unitTypes.values) {
+            if (unitType.movementType !in unitMovementTypes)
+                lines.add("Unit type ${unitType.name} has an invalid movement type ${unitType.movementType}", sourceObject = unitType)
+            uniqueValidator.checkUniques(unitType, lines, reportRulesetSpecificErrors, tryFixUnknownUniques)
+        }
+    }
+
+    protected open fun addVictoryTypeErrors(lines: RulesetErrorList) {
+        // Victory and Milestone aren't IHasUniques and are unsuitable as sourceObject
+        for (victoryType in ruleset.victories.values) {
+            for (milestone in victoryType.milestoneObjects) {
+                if (milestone.type == null)
+                    lines.add(
+                        "Victory type ${victoryType.name} has milestone \"${milestone.uniqueDescription}\" that is of an unknown type!",
+                        RulesetErrorSeverity.Error, sourceObject = null
+                    )
+            }
+
+            for (otherVictory in ruleset.victories.values)
+                if (otherVictory.name > victoryType.name && otherVictory.milestones == victoryType.milestones)
+                    lines.add(
+                        "Victory types ${victoryType.name} and ${otherVictory.name} have the same requirements!",
+                        RulesetErrorSeverity.Warning, sourceObject = null
+                    )
+        }
+    }
 
     //endregion
     //region General helpers
@@ -548,10 +709,7 @@ open class RulesetValidator protected constructor(
             .toSet()
 
     /* This is public because `FormattedLine` does its own checking and needs the textureNamesCache test */
-    fun uncachedImageExists(name: String): Boolean {
+    fun uncachedImageExists(name: String) = textureNamesCache.imageExists(name)
-        if (ruleset.folderLocation == null) return false // Can't check in this case
-        return textureNamesCache.imageExists(name)
-    }
 
     //endregion
 

core/src/com/unciv/models/ruleset/validation/UniqueValidator.kt

@@ -64,7 +64,7 @@ class UniqueValidator(val ruleset: Ruleset) {
         reportRulesetSpecificErrors: Boolean
     ): RulesetErrorList {
         val prefix by lazy { getUniqueContainerPrefix(uniqueContainer) + "\"${unique.text}\"" }
-        if (unique.type == null) return checkUntypedUnique(unique, tryFixUnknownUniques, uniqueContainer, prefix)
+        if (unique.type == null) return checkUntypedUnique(unique, tryFixUnknownUniques, uniqueContainer, prefix, reportRulesetSpecificErrors)
 
         val rulesetErrors = RulesetErrorList(ruleset)
 
@@ -342,7 +342,13 @@ class UniqueValidator(val ruleset: Ruleset) {
         return severity
     }
 
-    private fun checkUntypedUnique(unique: Unique, tryFixUnknownUniques: Boolean, uniqueContainer: IHasUniques?, prefix: String): RulesetErrorList {
+    private fun checkUntypedUnique(
+        unique: Unique,
+        tryFixUnknownUniques: Boolean,
+        uniqueContainer: IHasUniques?,
+        prefix: String,
+        reportRulesetSpecificErrors: Boolean
+    ): RulesetErrorList {
         // Malformed conditional is always bad
         if (unique.text.count { it == '<' } != unique.text.count { it == '>' })
             return RulesetErrorList.of(
@@ -351,7 +357,8 @@ class UniqueValidator(val ruleset: Ruleset) {
             )
 
         // Support purely filtering Uniques without actual implementation
-        if (isFilteringUniqueAllowed(unique)) return RulesetErrorList()
+        if (isFilteringUniqueAllowed(unique, reportRulesetSpecificErrors)) return RulesetErrorList()
+
         if (tryFixUnknownUniques) {
             val fixes = tryFixUnknownUnique(unique, uniqueContainer, prefix)
             if (fixes.isNotEmpty()) return fixes
@@ -364,10 +371,11 @@ class UniqueValidator(val ruleset: Ruleset) {
         )
     }
 
-    private fun isFilteringUniqueAllowed(unique: Unique): Boolean {
+    private fun isFilteringUniqueAllowed(unique: Unique, reportRulesetSpecificErrors: Boolean): Boolean {
         // Isolate this decision, to allow easy change of approach
         // This says: Must have no conditionals or parameters, and is used in any "filtering" parameter of another Unique
         if (unique.modifiers.isNotEmpty() || unique.params.isNotEmpty()) return false
+        if (!reportRulesetSpecificErrors) return true // Don't report unless checking a complete Ruleset
         return unique.text in allUniqueParameters // referenced at least once from elsewhere
     }
 

core/src/com/unciv/ui/images/AtlasPreview.kt

@@ -6,6 +6,7 @@ import com.badlogic.gdx.graphics.Pixmap
 import com.badlogic.gdx.graphics.g2d.TextureAtlas.TextureAtlasData
 import com.unciv.json.json
 import com.unciv.models.ruleset.Ruleset
+import com.unciv.models.ruleset.RulesetCache
 import com.unciv.models.ruleset.validation.RulesetErrorList
 import com.unciv.models.ruleset.validation.RulesetErrorSeverity
 import com.unciv.utils.Log
@@ -13,7 +14,7 @@ import java.io.File
 
 /**
  *  This extracts all texture names from all atlases of a Ruleset.
- *  - Weak point: For combined rulesets, this always loads the builtin assets.
+ *  - For combined rulesets, this loads assets for all component rulesets that are present in RulesetCache
  *  - Used by RulesetValidator to check texture names without relying on ImageGetter
  *  - Doubles as integrity checker and detects:
  *      - Atlases.json names an atlas that does not exist
@@ -28,6 +29,19 @@ class AtlasPreview(ruleset: Ruleset, errorList: RulesetErrorList) : Iterable<Str
     private val regionNames = mutableSetOf<String>()
 
     init {
+        if (ruleset.name.isNotEmpty()) loadSingleRuleset(ruleset, errorList)
+        else loadComplexRuleset(ruleset, errorList)
+        Log.debug("Atlas preview for $ruleset: ${regionNames.size} entries.")
+    }
+
+    private fun loadComplexRuleset(ruleset: Ruleset, errorList: RulesetErrorList) {
+        for (modName in ruleset.mods) {
+            val componentRuleset = RulesetCache[modName] ?: continue
+            loadSingleRuleset(componentRuleset, errorList)
+        }
+    }
+
+    private fun loadSingleRuleset(ruleset: Ruleset, errorList: RulesetErrorList) {
         // For builtin rulesets, the Atlases.json is right in internal root
         val folder = ruleset.folder()
         val controlFile = folder.child("Atlases.json")
@@ -53,7 +67,6 @@ class AtlasPreview(ruleset: Ruleset, errorList: RulesetErrorList) : Iterable<Str
                 errorList.add("${file.name()} contains no textures")
             data.regions.mapTo(regionNames) { it.name }
         }
-        Log.debug("Atlas preview for $ruleset: ${regionNames.size} entries.")
     }
 
     private fun getFileNames(