Reimplement and fix #10142 (#10213)

* Test Free Buildings

* Flip the order of logging free buildings to avoid loops with stat buildings

* unprivate constructionComplete for the notification and add validating the queue to addBuilding

* reimplement #10142

* Switch free buildings in cityFilter to also use constructionComplete for consistency

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

@@ -631,6 +631,13 @@ class City : IsPartOfGameInfoSerialization {
         }
     }
 
+    // Uniques coming from only this city
+    fun getMatchingLocalOnlyUniques(uniqueType: UniqueType, stateForConditionals: StateForConditionals): Sequence<Unique> {
+        val uniques = cityConstructions.builtBuildingUniqueMap.getUniques(uniqueType).filter { it.isLocalEffect } +
+            religion.getUniques().filter { it.isOfType(uniqueType) }
+        return if (uniques.any()) uniques.filter { it.conditionalsApply(stateForConditionals) }
+        else uniques
+    }
 
     // Uniques coming from this city, but that should be provided globally
     fun getMatchingUniquesWithNonLocalEffects(uniqueType: UniqueType, stateForConditionals: StateForConditionals): Sequence<Unique> {

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

@@ -245,7 +245,7 @@ class CityConstructions : IsPartOfGameInfoSerialization {
         throw NotBuildingOrUnitException("$constructionName is not a building or a unit!")
     }
 
-    internal fun getBuiltBuildings(): Sequence<Building> = builtBuildingObjects.asSequence()
+    fun getBuiltBuildings(): Sequence<Building> = builtBuildingObjects.asSequence()
 
     fun containsBuildingOrEquivalent(buildingNameOrUnique: String): Boolean =
             isBuilt(buildingNameOrUnique) || getBuiltBuildings().any { it.replaces == buildingNameOrUnique || it.hasUnique(buildingNameOrUnique) }
@@ -298,11 +298,11 @@ class CityConstructions : IsPartOfGameInfoSerialization {
         return ceil((workLeft-productionOverflow) / production.toDouble()).toInt()
     }
 
-    fun hasBuildableStatBuildings(stat: Stat): Boolean {
+    fun cheapestStatBuilding(stat: Stat): Building? {
         return getBasicStatBuildings(stat)
-            .map { city.civ.getEquivalentBuilding(it.name) }
+            .map { city.civ.getEquivalentBuilding(it) }
             .filter { it.isBuildable(this) || isBeingConstructedOrEnqueued(it.name) }
-            .any()
+            .minByOrNull { it.cost }
     }
 
     //endregion
@@ -455,7 +455,7 @@ class CityConstructions : IsPartOfGameInfoSerialization {
     }
 
     /** Returns false if we tried to construct a unit but it has nowhere to go */
-    private fun constructionComplete(construction: INonPerpetualConstruction): Boolean {
+    fun constructionComplete(construction: INonPerpetualConstruction): Boolean {
         val managedToConstruct = construction.postBuildEvent(this)
         if (!managedToConstruct) return false
 
@@ -524,6 +524,8 @@ class CityConstructions : IsPartOfGameInfoSerialization {
 
         updateUniques()
 
+        validateConstructionQueue()
+
         /** Support for [UniqueType.CreatesOneImprovement] */
         applyCreateOneImprovement(building)
 
@@ -544,7 +546,7 @@ class CityConstructions : IsPartOfGameInfoSerialization {
         }
         else city.reassignPopulationDeferred()
 
-        addFreeBuildings()
+        city.civ.civConstructions.tryAddFreeBuildings()
     }
 
     fun triggerNewBuildingUniques(building: Building) {
@@ -591,42 +593,6 @@ class CityConstructions : IsPartOfGameInfoSerialization {
         }
     }
 
-    fun addFreeBuildings() {
-        // "Gain a free [buildingName] [cityFilter]"
-        val freeBuildingUniques = city.getMatchingUniques(UniqueType.GainFreeBuildings, StateForConditionals(city.civ, city))
-
-        for (unique in freeBuildingUniques) {
-            val freeBuilding = city.civ.getEquivalentBuilding(unique.params[0])
-            val citiesThatApply =
-                if (unique.isLocalEffect) listOf(city)
-                else city.civ.cities.filter { it.matchesFilter(unique.params[1]) }
-
-            for (city in citiesThatApply) {
-                if (city.cityConstructions.containsBuildingOrEquivalent(freeBuilding.name)) continue
-                city.cityConstructions.addBuilding(freeBuilding)
-                freeBuildingsProvidedFromThisCity.addToMapOfSets(city.id, freeBuilding.name)
-            }
-        }
-
-        // Civ-level uniques - for these only add free buildings from each city to itself to avoid weirdness on city conquest
-        for (unique in city.civ.getMatchingUniques(UniqueType.GainFreeBuildings, stateForConditionals = StateForConditionals(city.civ, city))) {
-            val freeBuilding = city.civ.getEquivalentBuilding(unique.params[0])
-            if (city.matchesFilter(unique.params[1])) {
-                freeBuildingsProvidedFromThisCity.addToMapOfSets(city.id, freeBuilding.name)
-                if (!isBuilt(freeBuilding.name))
-                    addBuilding(freeBuilding)
-            }
-        }
-
-
-        val autoGrantedBuildings = city.getRuleset().buildings.values
-            .filter { it.hasUnique(UniqueType.GainBuildingWhereBuildable) }
-
-        for (building in autoGrantedBuildings)
-            if (building.isBuildable(city.cityConstructions))
-                addBuilding(building)
-    }
-
     /**
      *  Purchase a construction for gold (or another stat)
      *  called from NextTurnAutomation and the City UI
@@ -719,18 +685,6 @@ class CityConstructions : IsPartOfGameInfoSerialization {
         return true
     }
 
-    fun addCheapestBuildableStatBuilding(stat: Stat): String? {
-        val cheapestBuildableStatBuilding = getBasicStatBuildings(stat)
-            .map { city.civ.getEquivalentBuilding(it) }
-            .filter { it.isBuildable(this) || isBeingConstructedOrEnqueued(it.name) }
-            .minByOrNull { it.cost }
-            ?: return null
-
-        constructionComplete(cheapestBuildableStatBuilding)
-
-        return cheapestBuildableStatBuilding.name
-    }
-
     private fun removeCurrentConstruction() = removeFromQueue(0, true)
 
     fun chooseNextConstruction() {

core/src/com/unciv/logic/city/managers/CityConquestFunctions.kt

@@ -53,15 +53,6 @@ class CityConquestFunctions(val city: City){
         for (building in city.civ.civConstructions.getFreeBuildingNames(city)) {
             city.cityConstructions.removeBuilding(building)
         }
-
-        // Remove all buildings provided for free from here to other cities (e.g. CN Tower)
-        for ((cityId, buildings) in city.cityConstructions.freeBuildingsProvidedFromThisCity) {
-            val city = oldCiv.cities.firstOrNull { it.id == cityId } ?: continue
-            debug("Removing buildings %s from city %s", buildings, city.name)
-            for (building in buildings) {
-                city.cityConstructions.removeBuilding(building)
-            }
-        }
         city.cityConstructions.freeBuildingsProvidedFromThisCity.clear()
 
         for (building in city.cityConstructions.getBuiltBuildings()) {

core/src/com/unciv/logic/city/managers/CityTurnManager.kt

@@ -21,7 +21,6 @@ class CityTurnManager(val city: City) {
         // Construct units at the beginning of the turn,
         // so they won't be generated out in the open and vulnerable to enemy attacks before you can control them
         city.cityConstructions.constructIfEnough()
-        city.cityConstructions.addFreeBuildings()
 
         city.tryUpdateRoadStatus()
         city.attackedThisTurn = false

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

@@ -5,6 +5,7 @@ import com.unciv.logic.city.City
 import com.unciv.models.Counter
 import com.unciv.models.ruleset.Building
 import com.unciv.models.ruleset.INonPerpetualConstruction
+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.stats.Stat
@@ -64,6 +65,7 @@ class CivConstructions : IsPartOfGameInfoSerialization {
     fun tryAddFreeBuildings() {
         addFreeStatsBuildings()
         addFreeSpecificBuildings()
+        addFreeBuildings()
     }
 
     /** Common to [hasFreeBuildingByName] and [getFreeBuildingNames] - 'has' doesn't need the whole set, one enumeration is enough.
@@ -107,13 +109,13 @@ class CivConstructions : IsPartOfGameInfoSerialization {
     private fun addFreeStatBuildings(stat: Stat, amount: Int) {
         for (city in civInfo.cities.take(amount)) {
             if (freeStatBuildingsProvided.contains(stat.name, city.id)) continue
-            if (!city.cityConstructions.hasBuildableStatBuildings(stat)) continue
+            val building = city.cityConstructions.cheapestStatBuilding(stat)
+                ?: continue
 
-            val builtBuilding = city.cityConstructions.addCheapestBuildableStatBuilding(stat)
-            if (builtBuilding != null) {
-                freeStatBuildingsProvided.addToMapOfSets(stat.name, city.id)
-                addFreeBuilding(city.id, builtBuilding)
-            }
+            freeStatBuildingsProvided.addToMapOfSets(stat.name, city.id)
+            addFreeBuilding(city.id, building.name)
+            city.cityConstructions.constructionComplete(building)
+            building.postBuildEvent(city.cityConstructions)
         }
     }
 
@@ -129,15 +131,37 @@ class CivConstructions : IsPartOfGameInfoSerialization {
     }
 
     private fun addFreeBuildings(building: Building, amount: Int) {
-
         for (city in civInfo.cities.take(amount)) {
             if (freeSpecificBuildingsProvided.contains(building.name, city.id)
                 || city.cityConstructions.containsBuildingOrEquivalent(building.name)) continue
 
-            building.postBuildEvent(city.cityConstructions)
-
             freeSpecificBuildingsProvided.addToMapOfSets(building.name, city.id)
             addFreeBuilding(city.id, building.name)
+            city.cityConstructions.constructionComplete(building)
+        }
+    }
+
+    fun addFreeBuildings() {
+        val autoGrantedBuildings = civInfo.gameInfo.ruleset.buildings.values
+            .filter { it.hasUnique(UniqueType.GainBuildingWhereBuildable) }
+
+        // "Gain a free [buildingName] [cityFilter]"
+        val freeBuildingsFromCiv = civInfo.getMatchingUniques(UniqueType.GainFreeBuildings, StateForConditionals.IgnoreConditionals)
+        for (city in civInfo.cities) {
+            val freeBuildingsFromCity = city.getMatchingLocalOnlyUniques(UniqueType.GainFreeBuildings, StateForConditionals.IgnoreConditionals)
+            val freeBuildingUniques = (freeBuildingsFromCiv + freeBuildingsFromCity)
+                .filter { city.matchesFilter(it.params[1]) && it.conditionalsApply(StateForConditionals(city.civ, city)) }
+            for (unique in freeBuildingUniques){
+                val freeBuilding = city.civ.getEquivalentBuilding(unique.params[0])
+                city.cityConstructions.freeBuildingsProvidedFromThisCity.addToMapOfSets(city.id, freeBuilding.name)
+
+                if (city.cityConstructions.containsBuildingOrEquivalent(freeBuilding.name)) continue
+                city.cityConstructions.constructionComplete(freeBuilding)
+            }
+
+            for (building in autoGrantedBuildings)
+                if (building.isBuildable(city.cityConstructions))
+                    city.cityConstructions.constructionComplete(building)
         }
     }
 

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

@@ -641,7 +641,8 @@ object UniqueTriggerActivation {
                 return true
             }
 
-            UniqueType.FreeStatBuildings, UniqueType.FreeSpecificBuildings -> {
+            UniqueType.FreeStatBuildings, UniqueType.FreeSpecificBuildings,
+            UniqueType.GainFreeBuildings -> {
                 civInfo.civConstructions.tryAddFreeBuildings()
                 return true // not fully correct
             }

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

@@ -129,7 +129,7 @@ enum class UniqueType(val text: String, vararg targets: UniqueTarget, val flags:
     TileImprovementTime("[relativeAmount]% tile improvement construction time", UniqueTarget.Global, UniqueTarget.Unit),
 
     /// Building Maintenance
-    GainFreeBuildings("Gain a free [buildingName] [cityFilter]", UniqueTarget.Global),
+    GainFreeBuildings("Gain a free [buildingName] [cityFilter]", UniqueTarget.Global, UniqueTarget.Triggerable),
     BuildingMaintenance("[relativeAmount]% maintenance cost for buildings [cityFilter]", UniqueTarget.Global, UniqueTarget.FollowerBelief),
 
     /// Border growth

tests/src/com/unciv/logic/civilization/FreeBuildingTests.kt

@@ -0,0 +1,92 @@
+package com.unciv.logic.civilization
+
+import com.badlogic.gdx.math.Vector2
+import com.unciv.testing.GdxTestRunner
+import com.unciv.testing.TestGame
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+@RunWith(GdxTestRunner::class)
+class FreeBuildingTests {
+
+    private val testGame = TestGame()
+
+    @Before
+    fun setup(){
+        testGame.makeHexagonalMap(5)
+    }
+
+    @Test
+    fun `should only give cheapest stat building in set amount of cities`(){
+        val civ = testGame.addCiv("Provides the cheapest [Culture] building in your first [4] cities for free")
+        for (tech in testGame.ruleset.technologies.keys)
+            civ.tech.addTechnology(tech)
+        val capitalCity = testGame.addCity(civ, testGame.getTile(Vector2(1f,1f)))
+        val city2 = testGame.addCity(civ, testGame.getTile(Vector2(1f,2f)))
+        val city3 = testGame.addCity(civ, testGame.getTile(Vector2(2f,2f)))
+        val city4 = testGame.addCity(civ, testGame.getTile(Vector2(2f,1f)))
+        val city5 = testGame.addCity(civ, testGame.getTile(Vector2(0f,1f)))
+
+        val numberOfMonuments = civ.cities.count { it.cityConstructions.isBuilt("Monument") }
+
+        Assert.assertTrue(numberOfMonuments == 4)
+    }
+
+    @Test
+    fun `should only give 1 stat building`(){
+        val civ = testGame.addCiv("Provides the cheapest [Culture] building in your first [4] cities for free")
+        for (tech in testGame.ruleset.technologies.keys)
+            civ.tech.addTechnology(tech)
+        val capitalCity = testGame.addCity(civ, testGame.getTile(Vector2(1f,1f)))
+
+        Assert.assertTrue(capitalCity.cityConstructions.isBuilt("Monument"))
+        Assert.assertFalse(capitalCity.cityConstructions.getBuiltBuildings().any { it.name != "Monument" && it.name != "Palace" })
+    }
+
+    @Test
+    fun `should only give the specific building in set amount of cities`(){
+        val civ = testGame.addCiv("Provides a [Monument] in your first [4] cities for free")
+        for (tech in testGame.ruleset.technologies.keys)
+            civ.tech.addTechnology(tech)
+        val capitalCity = testGame.addCity(civ, testGame.getTile(Vector2(1f,1f)))
+        val city2 = testGame.addCity(civ, testGame.getTile(Vector2(1f,2f)))
+        val city3 = testGame.addCity(civ, testGame.getTile(Vector2(2f,2f)))
+        val city4 = testGame.addCity(civ, testGame.getTile(Vector2(2f,1f)))
+        val city5 = testGame.addCity(civ, testGame.getTile(Vector2(0f,1f)))
+
+        val numberOfMonuments = civ.cities.count { it.cityConstructions.isBuilt("Monument") }
+
+        Assert.assertTrue(numberOfMonuments == 4)
+    }
+
+    @Test
+    fun `free specific buildings should ONLY give the specific building`(){
+        val civ = testGame.addCiv("Provides a [Monument] in your first [4] cities for free")
+        for (tech in testGame.ruleset.technologies.keys)
+            civ.tech.addTechnology(tech)
+        val capitalCity = testGame.addCity(civ, testGame.getTile(Vector2(1f,1f)))
+
+        val numberOfMonuments = civ.cities.count { it.cityConstructions.isBuilt("Monument") }
+
+        Assert.assertTrue(capitalCity.cityConstructions.isBuilt("Monument"))
+        Assert.assertFalse(capitalCity.cityConstructions.getBuiltBuildings().any { it.name != "Monument" && it.name != "Palace" })
+    }
+
+    @Test
+    fun `can give specific buildings in all cities`(){
+        val civ = testGame.addCiv("Gain a free [Monument] [in all cities]")
+        for (tech in testGame.ruleset.technologies.keys)
+            civ.tech.addTechnology(tech)
+        val capitalCity = testGame.addCity(civ, testGame.getTile(Vector2(1f,1f)))
+        val city2 = testGame.addCity(civ, testGame.getTile(Vector2(1f,2f)))
+        val city3 = testGame.addCity(civ, testGame.getTile(Vector2(2f,2f)))
+        val city4 = testGame.addCity(civ, testGame.getTile(Vector2(2f,1f)))
+        val city5 = testGame.addCity(civ, testGame.getTile(Vector2(0f,1f)))
+
+        val numberOfMonuments = civ.cities.count { it.cityConstructions.isBuilt("Monument") }
+
+        Assert.assertTrue(numberOfMonuments == 5)
+    }
+}