Moved the check for conditionals applying to getMatchingUniques functions; rewrote civInfo.getMatchingUniques (#5342)

* Moved the check for conditionals applying to `getMatchingUniques` functions. Rewrote `civInfo.getMatchingUniques`.

* Clarified comment

core/src/com/unciv/logic/city/CityConstructions.kt

@@ -6,6 +6,7 @@ import com.unciv.logic.civilization.NotificationIcon
 import com.unciv.logic.civilization.PopupAlert
 import com.unciv.models.ruleset.Building
 import com.unciv.models.ruleset.Ruleset
+import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unique.UniqueMap
 import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.ruleset.unit.BaseUnit
@@ -162,8 +163,7 @@ class CityConstructions {
     
     fun addFreeBuildings() {
         // "Provides a free [buildingName] [cityFilter]"
-        for (unique in cityInfo.getLocalMatchingUniques(UniqueType.ProvidesFreeBuildings)) {
+        for (unique in cityInfo.getLocalMatchingUniques(UniqueType.ProvidesFreeBuildings, StateForConditionals(cityInfo.civInfo, cityInfo))) {
-            if (!unique.conditionalsApply(cityInfo.civInfo, cityInfo)) continue
             val freeBuildingName = cityInfo.civInfo.getEquivalentBuilding(unique.params[0]).name
             val citiesThatApply = when (unique.params[1]) {
                 "in this city" -> listOf(cityInfo)

core/src/com/unciv/logic/city/CityInfo.kt

@@ -14,6 +14,7 @@ import com.unciv.models.ruleset.unique.Unique
 import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.ruleset.tile.ResourceSupplyList
 import com.unciv.models.ruleset.tile.ResourceType
+import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unit.BaseUnit
 import com.unciv.models.stats.Stat
 import java.util.*
@@ -160,8 +161,7 @@ class CityInfo {
 
         civInfo.civConstructions.tryAddFreeBuildings()
 
-        for (unique in getMatchingUniques(UniqueType.GainFreeBuildings)) {
+        for (unique in getMatchingUniques(UniqueType.GainFreeBuildings, stateForConditionals = StateForConditionals(civInfo, this))) {
-            if (!unique.conditionalsApply(civInfo, this)) continue
             val freeBuildingName = unique.params[0]
             if (matchesFilter(unique.params[1])) {
                 if (!cityConstructions.isBuilt(freeBuildingName))
@@ -723,14 +723,16 @@ class CityInfo {
         uniqueType: UniqueType,
         // We might have this cached to avoid concurrency problems. If we don't, just get it directly
         localUniques: Sequence<Unique> = getLocalMatchingUniques(uniqueType),
+        stateForConditionals: StateForConditionals? = null,
     ): Sequence<Unique> {
         // The localUniques might not be filtered when passed as a parameter, so we filter it anyway
         // The time loss shouldn't be that large I don't think
-        return civInfo.getMatchingUniques(uniqueType, this) +
+        return civInfo.getMatchingUniques(uniqueType, stateForConditionals, this) +
-                localUniques.filter {
+            localUniques.filter {
-                    it.isOfType(uniqueType)
+                it.isOfType(uniqueType)
-                    && it.params.none { param -> param == "in other cities" }
+                && it.conditionalsApply(stateForConditionals)
-                }
+                && it.params.none { param -> param == "in other cities" }
+            }
     }
 
     // Matching uniques provided by sources in the city itself
@@ -740,10 +742,14 @@ class CityInfo {
                 religion.getUniques().filter { it.placeholderText == placeholderText }
     }
 
-    fun getLocalMatchingUniques(uniqueType: UniqueType): Sequence<Unique> {
+    fun getLocalMatchingUniques(uniqueType: UniqueType, stateForConditionals: StateForConditionals? = null): Sequence<Unique> {
-        return cityConstructions.builtBuildingUniqueMap.getUniques(uniqueType)
+        return (
-            .filter { it.params.none { param -> param == "in other cities" } } +
+            cityConstructions.builtBuildingUniqueMap.getUniques(uniqueType)
-                religion.getUniques().filter { it.isOfType(uniqueType) }
+                .filter { it.params.none { param -> param == "in other cities" } } 
+            + religion.getUniques().filter { it.isOfType(uniqueType) }
+        ).filter {
+            it.conditionalsApply(stateForConditionals)
+        }
     }
 
     // Get all uniques that originate from this city
@@ -755,21 +761,21 @@ class CityInfo {
     fun getMatchingUniquesWithNonLocalEffects(placeholderText: String): Sequence<Unique> {
         return cityConstructions.builtBuildingUniqueMap.getUniques(placeholderText)
             .filter { it.params.none { param -> param == "in this city" } }
-        // Note that we don't query religion here, as those only have local effects (for now at least)
+        // Note that we don't query religion here, as those only have local effects
     }
 
 
-    fun getMatchingUniquesWithNonLocalEffects(uniqueType: UniqueType): Sequence<Unique> {
+    fun getMatchingUniquesWithNonLocalEffects(uniqueType: UniqueType, stateForConditionals: StateForConditionals? = null): Sequence<Unique> {
         return cityConstructions.builtBuildingUniqueMap.getUniques(uniqueType)
-            .filter { it.params.none { param -> param == "in this city" } }
+            .filter { it.params.none { param -> param == "in this city" } && it.conditionalsApply(stateForConditionals) }
-        // Note that we don't query religion here, as those only have local effects (for now at least)
+        // Note that we don't query religion here, as those only have local effects
     }
 
     // Get all uniques that don't apply to only this city
     fun getAllUniquesWithNonLocalEffects(): Sequence<Unique> {
         return cityConstructions.builtBuildingUniqueMap.getAllUniques()
             .filter { it.params.none { param -> param == "in this city" } }
-        // Note that we don't query religion here, as those only have local effects (for now at least)
+        // Note that we don't query religion here, as those only have local effects
     }
 
     fun isHolyCity(): Boolean = religion.religionThisIsTheHolyCityOf != null

core/src/com/unciv/logic/city/CityStats.kt

@@ -7,6 +7,7 @@ import com.unciv.logic.map.RoadStatus
 import com.unciv.models.Counter
 import com.unciv.models.ruleset.Building
 import com.unciv.models.ruleset.ModOptionsConstants
+import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unique.Unique
 import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.ruleset.unit.BaseUnit
@@ -222,8 +223,12 @@ class CityStats(val cityInfo: CityInfo) {
                 stats.add(unique.stats)
             }
             
-            if (unique.isOfType(UniqueType.StatsPerCity) && cityInfo.matchesFilter(unique.params[1]) && unique.conditionalsApply(cityInfo.civInfo))
+            if (unique.isOfType(UniqueType.StatsPerCity) 
+                && cityInfo.matchesFilter(unique.params[1]) 
+                && unique.conditionalsApply(cityInfo.civInfo)
+            ) {
                 stats.add(unique.stats)
+            }
 
             // "[stats] per [amount] population [cityFilter]"
             if (unique.placeholderText == "[] per [] population []" && cityInfo.matchesFilter(unique.params[2])) {
@@ -510,6 +515,12 @@ class CityStats(val cityInfo: CityInfo) {
         // We calculate this here for concurrency reasons
         // If something needs this, we pass this through as a parameter
         val localBuildingUniques = cityInfo.cityConstructions.builtBuildingUniqueMap.getAllUniques()
+        
+        // Is This line really necessary? There is only a single unique that actually uses this, 
+        // and it is passed to functions at least 3 times for that
+        // It's the only reason `cityInfo.getMatchingUniques` has a localUniques parameter,
+        // which clutters readability, and also the only reason `CityInfo.getAllLocalUniques()`
+        // exists in the first place, though that could be useful for the previous line too.
         val citySpecificUniques = cityInfo.getAllLocalUniques()
 
         // We need to compute Tile yields before happiness

core/src/com/unciv/logic/civilization/CivInfoStats.kt

@@ -7,6 +7,7 @@ import com.unciv.models.ruleset.BeliefType
 import com.unciv.models.ruleset.Policy
 import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.ruleset.tile.ResourceType
+import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.stats.Stat
 import com.unciv.models.stats.StatMap
 import com.unciv.models.stats.Stats
@@ -21,8 +22,7 @@ class CivInfoStats(val civInfo: CivilizationInfo) {
     private fun getUnitMaintenance(): Int {
         val baseUnitCost = 0.5f
         var freeUnits = 3
-        for (unique in civInfo.getMatchingUniques(UniqueType.FreeUnits)) {
+        for (unique in civInfo.getMatchingUniques(UniqueType.FreeUnits, StateForConditionals(civInfo))) {
-            if (!unique.conditionalsApply(civInfo)) continue
             freeUnits += unique.params[0].toInt()
         }
 
@@ -36,8 +36,7 @@ class CivInfoStats(val civInfo: CivilizationInfo) {
 
         var numberOfUnitsToPayFor = max(0f, unitsToPayFor.count().toFloat() - freeUnits)
 
-        for (unique in civInfo.getMatchingUniques(UniqueType.UnitMaintenanceDiscount)) {
+        for (unique in civInfo.getMatchingUniques(UniqueType.UnitMaintenanceDiscount, StateForConditionals(civInfo))) {
-            if (!unique.conditionalsApply(civInfo)) continue
             val numberOfUnitsWithDiscount = min(
                 numberOfUnitsToPayFor,
                 unitsToPayFor.count { it.matchesFilter(unique.params[1]) }.toFloat()

core/src/com/unciv/logic/civilization/CivilizationInfo.kt

@@ -18,6 +18,7 @@ import com.unciv.models.ruleset.*
 import com.unciv.models.ruleset.tile.ResourceSupplyList
 import com.unciv.models.ruleset.tile.ResourceType
 import com.unciv.models.ruleset.tile.TileResource
+import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unique.Unique
 import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.ruleset.unit.BaseUnit
@@ -337,48 +338,46 @@ class CivilizationInfo {
 
     fun hasUnique(uniqueType: UniqueType) = getMatchingUniques(uniqueType).any()
     fun hasUnique(unique: String) = getMatchingUniques(unique).any()
-
+        
-    /** Destined to replace getMatchingUniques, gradually, as we fill the enum */
-    fun getMatchingUniques(uniqueType: UniqueType, cityToIgnore: CityInfo?=null): Sequence<Unique> {
-        val ruleset = gameInfo.ruleSet
-        return nation.uniqueObjects.asSequence().filter { it.matches(uniqueType, ruleset) } +
-                cities.asSequence().filter { it != cityToIgnore }.flatMap { city ->
-                    city.getMatchingUniquesWithNonLocalEffects(uniqueType)
-                } +
-                policies.policyUniques.getUniques(uniqueType) +
-                tech.techUniques.getUniques(uniqueType) +
-                temporaryUniques
-                    .asSequence().map { it.first }
-                    .filter { it.matches(uniqueType, ruleset) } +
-                getEra().getMatchingUniques(uniqueType) +
-                (
-                        if (religionManager.religion != null)
-                            religionManager.religion!!.getFounderUniques()
-                                .filter { it.isOfType(uniqueType) }
-                        else sequenceOf()
-                        )
-    }
-
     // Does not return local uniques, only global ones.
-    fun getMatchingUniques(uniqueTemplate: String, cityToIgnore: CityInfo? = null): Sequence<Unique> {
+    /** Destined to replace getMatchingUniques, gradually, as we fill the enum */
-        return nation.uniqueObjects.asSequence().filter { it.placeholderText == uniqueTemplate } +
+    fun getMatchingUniques(uniqueType: UniqueType, stateForConditionals: StateForConditionals? = null, cityToIgnore: CityInfo? = null) = sequence {
-                cities.asSequence().filter { it != cityToIgnore}.flatMap {
+        val ruleset = gameInfo.ruleSet
-                    city -> city.getMatchingUniquesWithNonLocalEffects(uniqueTemplate)
+        yieldAll(nation.uniqueObjects.asSequence().filter {it.matches(uniqueType, ruleset) })
-                } +
+        yieldAll(cities.asSequence()
-                policies.policyUniques.getUniques(uniqueTemplate) +
+            .filter { it != cityToIgnore }
-                tech.techUniques.getUniques(uniqueTemplate) +
+            .flatMap { city -> city.getMatchingUniquesWithNonLocalEffects(uniqueType) }
-                temporaryUniques
+        )
-                    .asSequence()
+        yieldAll(policies.policyUniques.getUniques(uniqueType))
-                    .filter { it.first.placeholderText == uniqueTemplate }.map { it.first } +
+        yieldAll(tech.techUniques.getUniques(uniqueType))
-                getEra().getMatchingUniques(uniqueTemplate)
+        yieldAll(temporaryUniques.asSequence()
-                    .asSequence() +
+            .map { it.first }
-                (
+            .filter { it.matches(uniqueType, ruleset) }
-                    if (religionManager.religion != null) 
+        )
-                        religionManager.religion!!.getFounderUniques()
+        yieldAll(getEra().getMatchingUniques(uniqueType))
-                            .asSequence()
+        if (religionManager.religion != null)
-                            .filter { it.placeholderText == uniqueTemplate }
+            yieldAll(religionManager.religion!!.getFounderUniques().filter { it.isOfType(uniqueType) })
-                    else sequenceOf()
+    }.filter {
-                )
+        it.conditionalsApply(stateForConditionals)
+    }
+    
+    fun getMatchingUniques(uniqueTemplate: String, cityToIgnore: CityInfo? = null) = sequence {
+        yieldAll(nation.uniqueObjects.asSequence().filter { it.placeholderText == uniqueTemplate })
+        yieldAll(cities.asSequence()
+            .filter { it != cityToIgnore }
+            .flatMap { city -> city.getMatchingUniquesWithNonLocalEffects(uniqueTemplate) }
+        )
+        yieldAll(policies.policyUniques.getUniques(uniqueTemplate))
+        yieldAll(tech.techUniques.getUniques(uniqueTemplate))
+        yieldAll(temporaryUniques.asSequence()
+            .filter { it.first.placeholderText == uniqueTemplate }.map { it.first }
+        )
+        yieldAll(getEra().getMatchingUniques(uniqueTemplate).asSequence())
+        if (religionManager.religion != null)
+            yieldAll(religionManager.religion!!.getFounderUniques()
+                .asSequence()
+                .filter { it.placeholderText == uniqueTemplate }
+            )
     }
 
     //region Units

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

@@ -1,5 +1,6 @@
 package com.unciv.models.ruleset
 
+import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unique.Unique
 import com.unciv.models.ruleset.unique.UniqueTarget
 import com.unciv.models.ruleset.unique.UniqueType
@@ -10,15 +11,22 @@ import com.unciv.models.ruleset.unique.UniqueType
 interface IHasUniques {
     var uniques: ArrayList<String> // Can not be a hashset as that would remove doubles
     // I bet there's a way of initializing these without having to override it everywhere...
-    val uniqueObjects: List<Unique>
+    val uniqueObjects: List<Unique> 
+    
     /** Technically not currently needed, since the unique target can be retrieved from every unique in the uniqueObjects,
      * But making this a function is relevant for future "unify Unciv object" plans ;)
      * */
     fun getUniqueTarget(): UniqueTarget
-
+    
-    fun getMatchingUniques(uniqueTemplate: String) = uniqueObjects.asSequence().filter { it.placeholderText == uniqueTemplate }
+    fun getMatchingUniques(uniqueTemplate: String, stateForConditionals: StateForConditionals? = null) = 
-    fun getMatchingUniques(uniqueType: UniqueType) = uniqueObjects.asSequence().filter { it.isOfType(uniqueType) }
+        uniqueObjects.asSequence().filter { it.placeholderText == uniqueTemplate && it.conditionalsApply(stateForConditionals) }
-
+    
-    fun hasUnique(uniqueTemplate: String) = uniqueObjects.any { it.placeholderText == uniqueTemplate }
+    fun getMatchingUniques(uniqueType: UniqueType, stateForConditionals: StateForConditionals? = null) = 
-    fun hasUnique(uniqueType: UniqueType) = uniqueObjects.any { it.isOfType(uniqueType) }
+        uniqueObjects.asSequence().filter { it.isOfType(uniqueType) && it.conditionalsApply(stateForConditionals) }
+    
+    fun hasUnique(uniqueTemplate: String, stateForConditionals: StateForConditionals? = null) = 
+        uniqueObjects.any { it.placeholderText == uniqueTemplate && it.conditionalsApply(stateForConditionals) }
+    
+    fun hasUnique(uniqueType: UniqueType, stateForConditionals: StateForConditionals? = null) = 
+        uniqueObjects.any { it.isOfType(uniqueType) && it.conditionalsApply(stateForConditionals) }
 }

core/src/com/unciv/models/ruleset/unique/StateForConditionals.kt

@@ -0,0 +1,9 @@
+package com.unciv.models.ruleset.unique
+
+import com.unciv.logic.city.CityInfo
+import com.unciv.logic.civilization.CivilizationInfo
+
+data class StateForConditionals(
+    val civInfo: CivilizationInfo? = null,
+    val cityInfo: CityInfo? = null,
+)

core/src/com/unciv/models/ruleset/unique/Unique.kt

@@ -27,29 +27,29 @@ class Unique(val text: String, val sourceObjectType: UniqueTarget? = null, val s
     fun matches(uniqueType: UniqueType, ruleset: Ruleset) = isOfType(uniqueType)
         && uniqueType.getComplianceErrors(this, ruleset).isEmpty()
 
-    // This function will get LARGE, as it will basically check for all conditionals if they apply
-    // This will require a lot of parameters to be passed (attacking unit, tile, defending unit, civInfo, cityInfo, ...)
-    // I'm open for better ideas, but this was the first thing that I could think of that would
-    // work in all cases.
     fun conditionalsApply(civInfo: CivilizationInfo? = null, city: CityInfo? = null): Boolean {
+        return conditionalsApply(StateForConditionals(civInfo, city))
+    }
+    
+    fun conditionalsApply(state: StateForConditionals?): Boolean {
+        if (state == null) return conditionals.isEmpty() 
         for (condition in conditionals) {
-            if (!conditionalApplies(condition, civInfo, city)) return false
+            if (!conditionalApplies(condition, state)) return false
         }
         return true
     }
-
+    
     private fun conditionalApplies(
         condition: Unique,
-        civInfo: CivilizationInfo? = null,
+        state: StateForConditionals
-        city: CityInfo? = null
     ): Boolean {
         return when (condition.placeholderText) {
-            UniqueType.ConditionalNotWar.placeholderText -> civInfo?.isAtWar() == false
+            UniqueType.ConditionalNotWar.placeholderText -> state.civInfo?.isAtWar() == false
-            UniqueType.ConditionalWar.placeholderText -> civInfo?.isAtWar() == true
+            UniqueType.ConditionalWar.placeholderText -> state.civInfo?.isAtWar() == true
             UniqueType.ConditionalSpecialistCount.placeholderText -> 
-                city != null && city.population.getNumberOfSpecialists() >= condition.params[0].toInt()
+                state.cityInfo != null && state.cityInfo.population.getNumberOfSpecialists() >= condition.params[0].toInt()
             UniqueType.ConditionalHappy.placeholderText -> 
-                civInfo != null && civInfo.statsForNextTurn.happiness >= 0
+                state.civInfo != null && state.civInfo.statsForNextTurn.happiness >= 0
             else -> false
         }
     }

core/src/com/unciv/models/ruleset/unique/UniqueType.kt

@@ -109,6 +109,7 @@ enum class UniqueType(val text:String, vararg targets: UniqueTarget) {
     NaturalWonderLatitude("Occurs on latitudes from [amount] to [amount] percent of distance equator to pole", UniqueTarget.Terrain),
     NaturalWonderGroups("Occurs in groups of [amount] to [amount] tiles", UniqueTarget.Terrain),
     NaturalWonderConvertNeighbors("Neighboring tiles will convert to [baseTerrain]", UniqueTarget.Terrain),
+    // The "Except [terrainFilter]" could theoretically be implemented with a conditional
     NaturalWonderConvertNeighborsExcept("Neighboring tiles except [terrainFilter] will convert to [baseTerrain]", UniqueTarget.Terrain),
 
     TerrainGrantsPromotion("Grants [promotion] ([comment]) to adjacent [mapUnitFilter] units for the rest of the game", UniqueTarget.Terrain),