Allow units to upgrade to more than one unit (#10947)

* Allow units to upgrade to more than one unit

* Actaully add changes to Base Unit

* And actually add the unique

* Kdocs fixes

* upgradeUnits to getUpgradeUnits and add missing state for condititonalss

* unique.params[0] shouldn't be empty..., right?

* Make old var first in the list, in case it matters

* wait... We're never yielding a null result

* Remove unnecessary function call

* Remove some indentation in RulesetValidation

* isEmpty instead none

* min cost instead of first

* Ruleset helper functions

* Imports

* helper functions part 2

* sanity check in case the unit actually isn't upgradable here

* I missed a spot

* This is NOT using a filter, itis looking for a unitName. Also, why do we use "unit" instead of "unitName"?

* Avoid crash in Scene2dExtentions, part 1

* Avoid crash in Scene2dExtentions, part 2

* inevitable request to move it to its own function

* Upgrade to the correct unit when upgrading all units

* Kdocs I overlooked

* Whoops

* Revert kdocs changes

* Should probably make sure action isn't null

* Fix loop in checkUnitRulesetInvariant

* Minor irrelevant fix

* No response, Removing getUpgradePath part 1 as it's redundant if we upgrade one at a time

* Move to UnitAutomation

* Imports

* Irrelevant fix: ignore consturctionRejections for upgrades

* Irrelevant fix: We should be able to upgrade to obsolete units, just not build new ones

* Simplify for clarity

* Irrelevant fix: unit is not destroyed is it doesn't upgrade

* Whoops

* Imports

* Remove getUpgradePath part 2: simplify UnitAutomation

* Remove now unnecessary circular reference check. We already check if it upgrades to itself

* Whoops, if we use a special upgrade, we should go back to a normal upgrade if we find no special upgrade unique

* Basic tests for last commit

* Forgot special to normal upgrade test

* whoops, forgot to change comment

* Not at computer: List.isEmpty to Sequence.none

* Not at computer: List.size to Sequence.count()

core/src/com/unciv/logic/automation/unit/UnitAutomation.kt

@@ -14,8 +14,10 @@ import com.unciv.logic.civilization.NotificationCategory
 import com.unciv.logic.civilization.diplomacy.DiplomaticStatus
 import com.unciv.logic.map.mapunit.MapUnit
 import com.unciv.logic.map.tile.Tile
+import com.unciv.models.UpgradeUnitAction
 import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unique.UniqueType
+import com.unciv.models.ruleset.unit.BaseUnit
 import com.unciv.ui.screens.worldscreen.unit.actions.UnitActionsPillage
 import com.unciv.ui.screens.worldscreen.unit.actions.UnitActionsUpgrade
 
@@ -127,22 +129,39 @@ object UnitAutomation {
     internal fun tryUpgradeUnit(unit: MapUnit): Boolean {
         if (unit.civ.isHuman() && (!UncivGame.Current.settings.automatedUnitsCanUpgrade
                 || UncivGame.Current.settings.autoPlay.isAutoPlayingAndFullAI())) return false
-        if (unit.baseUnit.upgradesTo == null) return false
-        val upgradedUnit = unit.upgrade.getUnitToUpgradeTo()
-        if (!upgradedUnit.isBuildable(unit.civ)) return false // for resource reasons, usually
+        
+        val upgradeUnits = getUnitsToUpgradeTo(unit)
+        if (upgradeUnits.none()) return false // for resource reasons, usually
+        val upgradedUnit = upgradeUnits.minBy { it.cost }
 
         if (upgradedUnit.getResourceRequirementsPerTurn(StateForConditionals(unit.civ, unit = unit)).keys.any { !unit.requiresResource(it) }) {
             // The upgrade requires new resource types, so check if we are willing to invest them
             if (!Automation.allowSpendingResource(unit.civ, upgradedUnit)) return false
         }
 
-        val upgradeAction = UnitActionsUpgrade.getUpgradeAction(unit)
-            ?: return false
+        val upgradeActions = UnitActionsUpgrade.getUpgradeActions(unit)
 
-        upgradeAction.action?.invoke()
+        upgradeActions.firstOrNull{ (it as UpgradeUnitAction).unitToUpgradeTo == upgradedUnit }?.action?.invoke() ?: return false
         return unit.isDestroyed // a successful upgrade action will destroy this unit
     }
 
+    /** Get the base unit this map unit could upgrade to, respecting researched tech and nation uniques only.
+     *  Note that if the unit can't upgrade, the current BaseUnit is returned.
+     */
+    private fun getUnitsToUpgradeTo(unit: MapUnit): Sequence<BaseUnit> {
+
+        fun isInvalidUpgradeDestination(baseUnit: BaseUnit): Boolean {
+            if (!unit.civ.tech.isResearched(baseUnit))
+                return true
+            return baseUnit.getMatchingUniques(UniqueType.OnlyAvailableWhen, StateForConditionals.IgnoreConditionals)
+                .any { !it.conditionalsApply(StateForConditionals(unit.civ, unit = unit)) }
+        }
+
+        return unit.baseUnit.getRulesetUpgradeUnits(StateForConditionals(unit.civ, unit = unit))
+            .map { unit.civ.getEquivalentUnit(it) }
+            .filter { !isInvalidUpgradeDestination(it) && unit.upgrade.canUpgrade(it) }
+    }
+
     fun automateUnitMoves(unit: MapUnit) {
         check(!unit.civ.isBarbarian()) { "Barbarians is not allowed here." }
 

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

@@ -413,9 +413,12 @@ class CityConstructions : IsPartOfGameInfoSerialization {
                     }
                 } else if (construction is BaseUnit) {
                     // Production put into upgradable units gets put into upgraded version
-                    if (rejectionReasons.all { it.type == RejectionReasonType.Obsoleted } && construction.upgradesTo != null) {
-                        val upgradedUnitName = city.civ.getEquivalentUnit(construction.upgradesTo!!).name
-                        inProgressConstructions[upgradedUnitName] = (inProgressConstructions[upgradedUnitName] ?: 0) + workDone
+                    val cheapestUpgradeUnit = construction.getRulesetUpgradeUnits(StateForConditionals(city.civ, city))
+                        .map { city.civ.getEquivalentUnit(it) }
+                        .filter { it.isBuildable(this) }
+                        .minByOrNull { it.cost }
+                    if (rejectionReasons.all { it.type == RejectionReasonType.Obsoleted } && cheapestUpgradeUnit != null) {
+                        inProgressConstructions[cheapestUpgradeUnit.name] = (inProgressConstructions[cheapestUpgradeUnit.name] ?: 0) + workDone
                     }
                 }
                 inProgressConstructions.remove(constructionName)

core/src/com/unciv/logic/map/mapunit/UnitUpgradeManager.kt

@@ -10,53 +10,15 @@ import kotlin.math.pow
 
 class UnitUpgradeManager(val unit:MapUnit) {
 
-    /** Returns FULL upgrade path, without checking what we can or cannot build currently.
-     * Does not contain current baseunit, so will be empty if no upgrades. */
-    private fun getUpgradePath(): Iterable<BaseUnit> {
-        var currentUnit = unit.baseUnit
-        val upgradeList = linkedSetOf<BaseUnit>()
-        while (currentUnit.upgradesTo != null) {
-            val nextUpgrade = unit.civ.getEquivalentUnit(currentUnit.upgradesTo!!)
-            if (nextUpgrade in upgradeList)
-                throw(UncivShowableException("Circular or self-referencing upgrade path for ${currentUnit.name}"))
-            currentUnit = nextUpgrade
-            upgradeList.add(currentUnit)
-        }
-        return upgradeList
-    }
-
-    /** Get the base unit this map unit could upgrade to, respecting researched tech and nation uniques only.
-     *  Note that if the unit can't upgrade, the current BaseUnit is returned.
-     */
-    // Used from UnitAutomation, UI action, canUpgrade
-    fun getUnitToUpgradeTo(): BaseUnit {
-        val upgradePath = getUpgradePath()
-
-        fun isInvalidUpgradeDestination(baseUnit: BaseUnit): Boolean{
-            if (!unit.civ.tech.isResearched(baseUnit))
-                return true
-            if (baseUnit.getMatchingUniques(UniqueType.OnlyAvailableWhen, StateForConditionals.IgnoreConditionals).any {
-                        !it.conditionalsApply(StateForConditionals(unit.civ, unit = unit ))
-                    }) return true
-            return false
-        }
-
-        for (baseUnit in upgradePath.reversed()) {
-            if (isInvalidUpgradeDestination(baseUnit)) continue
-            return baseUnit
-        }
-        return unit.baseUnit
-    }
-
     /** Check whether this unit can upgrade to [unitToUpgradeTo]. This does not check or follow the
-     *  normal upgrade chain defined by [BaseUnit.upgradesTo], unless [unitToUpgradeTo] is left at default.
+     *  normal upgrade chain defined by [BaseUnit.getUpgradeUnits]
      *  @param ignoreRequirements Ignore possible tech/policy/building requirements (e.g. resource requirements still count).
      *          Used for upgrading units via ancient ruins.
      *  @param ignoreResources Ignore resource requirements (tech still counts)
      *          Used to display disabled Upgrade button
      */
     fun canUpgrade(
-        unitToUpgradeTo: BaseUnit = getUnitToUpgradeTo(),
+        unitToUpgradeTo: BaseUnit,
         ignoreRequirements: Boolean = false,
         ignoreResources: Boolean = false
     ): Boolean {
@@ -64,7 +26,9 @@ class UnitUpgradeManager(val unit:MapUnit) {
 
         val rejectionReasons = unitToUpgradeTo.getRejectionReasons(unit.civ, additionalResources = unit.getResourceRequirementsPerTurn())
 
-        var relevantRejectionReasons = rejectionReasons.filterNot { it.type == RejectionReasonType.Unbuildable }
+        var relevantRejectionReasons = rejectionReasons.filterNot { 
+            it.isConstructionRejection() || it.type == RejectionReasonType.Obsoleted
+        }
         if (ignoreRequirements)
             relevantRejectionReasons = relevantRejectionReasons.filterNot { it.techPolicyEraWonderRequirements() }
         if (ignoreResources)
@@ -96,24 +60,15 @@ class UnitUpgradeManager(val unit:MapUnit) {
         for (unique in unit.civ.getMatchingUniques(UniqueType.UnitUpgradeCost, stateForConditionals))
             civModifier *= unique.params[0].toPercent()
 
-        val upgradePath = getUpgradePath()
-        var currentUnit = unit.baseUnit
-        for (baseUnit in upgradePath) {
-            // do clamping and rounding here so upgrading stepwise costs the same as upgrading far down the chain
-            var stepCost = constants.base
-            stepCost += (constants.perProduction * (baseUnit.cost - currentUnit.cost)).coerceAtLeast(0f)
-            val era = baseUnit.era(ruleset)
-            if (era != null)
-                stepCost *= (1f + era.eraNumber * constants.eraMultiplier)
-            stepCost = (stepCost * civModifier).pow(constants.exponent)
-            stepCost *= unit.civ.gameInfo.speed.modifier
-            goldCostOfUpgrade += (stepCost / constants.roundTo).toInt() * constants.roundTo
-            if (baseUnit == unitToUpgradeTo)
-                break  // stop at requested BaseUnit to upgrade to
-            currentUnit = baseUnit
-        }
-
-
+        var cost = constants.base
+        cost += (constants.perProduction * (unitToUpgradeTo.cost - unit.baseUnit.cost)).coerceAtLeast(0f)
+        val era = unitToUpgradeTo.era(ruleset)
+        if (era != null)
+            cost *= (1f + era.eraNumber * constants.eraMultiplier)
+        cost = (cost * civModifier).pow(constants.exponent)
+        cost *= unit.civ.gameInfo.speed.modifier
+        goldCostOfUpgrade += (cost / constants.roundTo).toInt() * constants.roundTo
+        
         return goldCostOfUpgrade
     }
 

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

@@ -18,6 +18,7 @@ import com.unciv.logic.civilization.TechAction
 import com.unciv.logic.civilization.managers.ReligionState
 import com.unciv.logic.map.mapunit.MapUnit
 import com.unciv.logic.map.tile.Tile
+import com.unciv.models.UpgradeUnitAction
 import com.unciv.models.ruleset.BeliefType
 import com.unciv.models.ruleset.Victory
 import com.unciv.models.stats.Stat
@@ -802,16 +803,16 @@ object UniqueTriggerActivation {
             }
             UniqueType.OneTimeUnitUpgrade -> {
                 val upgradeAction = UnitActionsUpgrade.getFreeUpgradeAction(unit)
-                    ?: return false
-                upgradeAction.action!!()
+                if (upgradeAction.none()) return false
+                (upgradeAction.minBy { (it as UpgradeUnitAction).unitToUpgradeTo.cost }).action!!()
                 if (notification != null)
                     unit.civ.addNotification(notification, unit.getTile().position, NotificationCategory.Units)
                 return true
             }
             UniqueType.OneTimeUnitSpecialUpgrade -> {
                 val upgradeAction = UnitActionsUpgrade.getAncientRuinsUpgradeAction(unit)
-                    ?: return false
-                upgradeAction.action!!()
+                if (upgradeAction.none()) return false
+                (upgradeAction.minBy { (it as UpgradeUnitAction).unitToUpgradeTo.cost }).action!!()
                 if (notification != null)
                     unit.civ.addNotification(notification, unit.getTile().position, NotificationCategory.Units)
                 return true

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

@@ -443,7 +443,8 @@ enum class UniqueType(
     InvisibleToNonAdjacent("Invisible to non-adjacent units", UniqueTarget.Unit),
     CanSeeInvisibleUnits("Can see invisible [mapUnitFilter] units", UniqueTarget.Unit),
 
-    RuinsUpgrade("May upgrade to [baseUnitFilter] through ruins-like effects", UniqueTarget.Unit),
+    RuinsUpgrade("May upgrade to [unit] through ruins-like effects", UniqueTarget.Unit),
+    CanUpgrade("Can upgrade to [unit]", UniqueTarget.Unit),
 
     DestroysImprovementUponAttack("Destroys tile improvements when attacking", UniqueTarget.Unit),
 

core/src/com/unciv/models/ruleset/unit/BaseUnit.kt

@@ -18,6 +18,7 @@ import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.stats.Stat
 import com.unciv.ui.components.extensions.getNeedMoreAmountString
 import com.unciv.ui.components.extensions.toPercent
+import com.unciv.ui.components.extensions.yieldIfNotNull
 import com.unciv.ui.objectdescriptions.BaseUnitDescriptions
 import com.unciv.ui.screens.civilopediascreen.FormattedLine
 import kotlin.math.pow
@@ -78,6 +79,21 @@ class BaseUnit : RulesetObject(), INonPerpetualConstruction {
     override fun getCivilopediaTextLines(ruleset: Ruleset): List<FormattedLine> =
             BaseUnitDescriptions.getCivilopediaTextLines(this, ruleset)
 
+    fun getUpgradeUnits(stateForConditionals: StateForConditionals? = null): Sequence<String> {
+        return sequence {
+            yieldIfNotNull(upgradesTo)
+            for (unique in getMatchingUniques(UniqueType.CanUpgrade, stateForConditionals))
+                yield(unique.params[0])
+        }
+    }
+
+    fun getRulesetUpgradeUnits(stateForConditionals: StateForConditionals? = null): Sequence<BaseUnit> {
+        return sequence { 
+            for (unit in getUpgradeUnits(stateForConditionals))
+                yieldIfNotNull(ruleset.units[unit])
+        }
+    }
+
     fun getMapUnit(civInfo: Civilization): MapUnit {
         val unit = MapUnit()
         unit.name = name

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

@@ -496,13 +496,6 @@ class RulesetValidator(val ruleset: Ruleset) {
             checkUnitRulesetSpecific(unit, lines)
             uniqueValidator.checkUniques(unit, lines, false, tryFixUnknownUniques)
         }
-
-        // We start with the units that are further along the tech tree, since they are likely to contain previous units.
-        // This allows us to minimize the double-checking.
-        val checkedUnits = HashSet<BaseUnit>()
-        for (unit in ruleset.units.values.sortedByDescending { it.techColumn(ruleset)?.columnNumber })
-            if (unit !in checkedUnits)
-                checkedUnits += checkUnitUpgradePath(unit, lines)
     }
 
     private fun addResourceErrorsRulesetInvariant(
@@ -674,8 +667,10 @@ class RulesetValidator(val ruleset: Ruleset) {
     }
 
     private fun checkUnitRulesetInvariant(unit: BaseUnit, lines: RulesetErrorList) {
-        if (unit.upgradesTo == unit.name || (unit.upgradesTo != null && unit.upgradesTo == unit.replaces))
-            lines += "${unit.name} upgrades to itself!"
+        for (upgradesTo in unit.getUpgradeUnits(StateForConditionals.IgnoreConditionals)) {
+            if (upgradesTo == unit.name || (upgradesTo == unit.replaces))
+                lines += "${unit.name} upgrades to itself!"
+        }
         if (unit.isMilitary() && unit.strength == 0)  // Should only match ranged units with 0 strength
             lines += "${unit.name} is a military unit but has no assigned strength!"
     }
@@ -687,22 +682,22 @@ class RulesetValidator(val ruleset: Ruleset) {
                 lines += "${unit.name} requires tech $requiredTech which does not exist!"
         for (obsoleteTech: String in unit.techsAtWhichNoLongerAvailable())
             if (!ruleset.technologies.containsKey(obsoleteTech))
-                lines += "${unit.name} obsoletes at tech ${obsoleteTech} which does not exist!"
-        if (unit.upgradesTo != null && !ruleset.units.containsKey(unit.upgradesTo!!))
-            lines += "${unit.name} upgrades to unit ${unit.upgradesTo} which does not exist!"
+                lines += "${unit.name} obsoletes at tech $obsoleteTech which does not exist!"
+        for (upgradesTo in unit.getUpgradeUnits(StateForConditionals.IgnoreConditionals))
+            if (!ruleset.units.containsKey(upgradesTo))
+                lines += "${unit.name} upgrades to unit $upgradesTo which does not exist!"
 
         // Check that we don't obsolete ourselves before we can upgrade
         for (obsoleteTech: String in unit.techsAtWhichAutoUpgradeInProduction())
-            if (unit.upgradesTo!=null && ruleset.units.containsKey(unit.upgradesTo!!)
-                && ruleset.technologies.containsKey(obsoleteTech)) {
-                val upgradedUnit = ruleset.units[unit.upgradesTo!!]!!
+            for (upgradesTo in unit.getUpgradeUnits(StateForConditionals.IgnoreConditionals)) {
+                if (!ruleset.units.containsKey(upgradesTo)) continue
+                if (!ruleset.technologies.containsKey(obsoleteTech)) continue
+                val upgradedUnit = ruleset.units[upgradesTo]!!
                 for (requiredTech: String in upgradedUnit.requiredTechs())
-                    if (requiredTech != obsoleteTech
-                        && !getPrereqTree(obsoleteTech).contains(requiredTech)
-                    )
+                    if (requiredTech != obsoleteTech && !getPrereqTree(obsoleteTech).contains(requiredTech))
                         lines.add(
                             "${unit.name} is supposed to automatically upgrade at tech ${obsoleteTech}," +
-                                " and therefore ${requiredTech} for its upgrade ${upgradedUnit.name} may not yet be researched!",
+                                " and therefore $requiredTech for its upgrade ${upgradedUnit.name} may not yet be researched!",
                             RulesetErrorSeverity.Warning
                         )
             }
@@ -745,47 +740,6 @@ class RulesetValidator(val ruleset: Ruleset) {
         reportError()
     }
 
-    /** Maps unit name to a set of all units naming it in its "replaces" property,
-     *  only for units having such a non-empty set, for use in [checkUnitUpgradePath] */
-    private val unitReplacesMap: Map<String, Set<BaseUnit>> by lazy {
-        ruleset.units.values.asSequence()
-            .mapNotNull { it.replaces }.distinct()
-            .associateWith { base ->
-                ruleset.units.values.filter { it.replaces == base }.toSet()
-            }
-    }
-
-    /** Checks all possible upgrade paths of [unit], reporting to [lines].
-     *  @param path used in recursion collecting the BaseUnits seen so far
-     *  @return units checked in this session - includes all units in this tree
-     *
-     *  Note: Since the units down the path will also be checked, this could log the same mistakes
-     *  repeatedly, but that is mostly prevented by RulesetErrorList.add(). Each unit involved in a
-     *  loop will still be flagged individually.
-     */
-    private fun checkUnitUpgradePath(
-        unit: BaseUnit,
-        lines: RulesetErrorList,
-        path: Set<BaseUnit> = emptySet()
-    ) : Set<BaseUnit> {
-        // This is similar to UnitUpgradeManager.getUpgradePath but without the dependency on a Civilization instance
-        // It also branches over all possible nation-unique replacements in one go, since we only look for loops.
-        if (unit in path) {
-            lines += "Circular or self-referencing upgrade path for ${unit.name}"
-            return setOf(unit)
-        }
-        val upgrade = ruleset.units[unit.upgradesTo] ?: return setOf(unit)
-        val newPath = path + unit // All Set additions are new Sets - we're recursing!
-        val newPathWithReplacements = unitReplacesMap[unit.name]?.let { newPath + it } ?: newPath
-        checkUnitUpgradePath(upgrade, lines, newPathWithReplacements)
-        val replacements = unitReplacesMap[upgrade.name] ?: return setOf(unit)
-        val checkedUnits = HashSet<BaseUnit>()
-        for (toCheck in replacements) {
-            checkedUnits += checkUnitUpgradePath(toCheck, lines, newPath)
-        }
-        return checkedUnits
-    }
-
     private fun checkTilesetSanity(lines: RulesetErrorList) {
         val tilesetConfigFolder = (ruleset.folderLocation ?: Gdx.files.internal("")).child("jsons\\TileSets")
         if (!tilesetConfigFolder.exists()) return

core/src/com/unciv/ui/popups/UnitUpgradeMenu.kt

@@ -1,6 +1,5 @@
 package com.unciv.ui.popups
 
-import com.badlogic.gdx.math.Vector2
 import com.badlogic.gdx.scenes.scene2d.Actor
 import com.badlogic.gdx.scenes.scene2d.Stage
 import com.badlogic.gdx.scenes.scene2d.ui.Table
@@ -38,6 +37,8 @@ class UnitUpgradeMenu(
     private val onButtonClicked: () -> Unit
 ) : AnimatedMenuPopup(stage, getActorTopRight(positionNextTo)) {
 
+    private val unitToUpgradeTo by lazy { unitAction.unitToUpgradeTo }
+
     private val allUpgradableUnits: Sequence<MapUnit> by lazy {
         unit.civ.units.getCivUnits()
             .filter {
@@ -45,7 +46,7 @@ class UnitUpgradeMenu(
                     && it.currentMovement > 0f
                     && it.currentTile.getOwner() == unit.civ
                     && !it.isEmbarked()
-                    && it.upgrade.canUpgrade(unitAction.unitToUpgradeTo, ignoreResources = true)
+                    && it.upgrade.canUpgrade(unitToUpgradeTo, ignoreResources = true)
             }
     }
 
@@ -59,7 +60,7 @@ class UnitUpgradeMenu(
 
     override fun createContentTable(): Table {
         val newInnerTable = BaseUnitDescriptions.getUpgradeInfoTable(
-            unitAction.title, unit.baseUnit, unitAction.unitToUpgradeTo
+            unitAction.title, unit.baseUnit, unitToUpgradeTo
         )
         newInnerTable.row()
         newInnerTable.add(getButton("Upgrade", KeyboardBinding.Upgrade, ::doUpgrade))
@@ -93,7 +94,9 @@ class UnitUpgradeMenu(
     private fun doAllUpgrade() {
         SoundPlayer.playRepeated(unitAction.uncivSound)
         for (unit in allUpgradableUnits) {
-            val otherAction = UnitActionsUpgrade.getUpgradeAction(unit)
+            val otherAction = UnitActionsUpgrade.getUpgradeActions(unit)
+                .firstOrNull{ (it as UpgradeUnitAction).unitToUpgradeTo == unitToUpgradeTo && 
+                    it.action != null }
             otherAction?.action?.invoke()
         }
     }

core/src/com/unciv/ui/screens/overviewscreen/UnitOverviewTab.kt

@@ -258,22 +258,9 @@ class UnitOverviewTab(
             add(promotionsTable)
 
             // Upgrade column
-            val unitAction = UnitActionsUpgrade.getUpgradeActionAnywhere(unit)
-            if (unitAction != null) {
-                val enable = unitAction.action != null && viewingPlayer.isCurrentPlayer() &&
-                    GUI.isAllowedChangeState()
-                val unitToUpgradeTo = (unitAction as UpgradeUnitAction).unitToUpgradeTo
-                val selectKey = getUnitIdentifier(unit, unitToUpgradeTo)
-                val upgradeIcon = ImageGetter.getUnitIcon(unitToUpgradeTo.name,
-                    if (enable) Color.GREEN else Color.GREEN.darken(0.5f))
-                if (enable) upgradeIcon.onClick {
-                    UnitUpgradeMenu(overviewScreen.stage, upgradeIcon, unit, unitAction) {
-                        unitListTable.updateUnitListTable()
-                        select(selectKey)
-                    }
-                }
-                add(upgradeIcon).size(28f)
-            } else add()
+            val upgradeTable = Table()
+            updateUpgradeTable(upgradeTable, unit)
+            add(upgradeTable)
 
             // Numeric health column - there's already a health bar on the button, but...?
             if (unit.health < 100) add(unit.health.toLabel()) else add()
@@ -282,6 +269,28 @@ class UnitOverviewTab(
         return this
     }
 
+    private fun updateUpgradeTable(table: Table, unit: MapUnit){
+        table.clearChildren()
+
+        val unitActions = UnitActionsUpgrade.getUpgradeActionAnywhere(unit)
+        if (unitActions.none()) table.add()
+        for (unitAction in unitActions){
+            val enable = unitAction.action != null && viewingPlayer.isCurrentPlayer() &&
+                GUI.isAllowedChangeState()
+            val unitToUpgradeTo = (unitAction as UpgradeUnitAction).unitToUpgradeTo
+            val selectKey = getUnitIdentifier(unit, unitToUpgradeTo)
+            val upgradeIcon = ImageGetter.getUnitIcon(unitToUpgradeTo.name,
+                if (enable) Color.GREEN else Color.GREEN.darken(0.5f))
+            if (enable) upgradeIcon.onClick {
+                UnitUpgradeMenu(overviewScreen.stage, upgradeIcon, unit, unitAction) {
+                    unitListTable.updateUnitListTable()
+                    select(selectKey)
+                }
+            }
+            table.add(upgradeIcon).size(28f)
+        }
+    }
+
     private fun updatePromotionsTable(table: Table, unit: MapUnit) {
         table.clearChildren()
 

core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActions.kt

@@ -100,7 +100,7 @@ object UnitActions {
             })
 
         addPromoteActions(unit)
-        yieldAll(UnitActionsUpgrade.getUnitUpgradeActions(unit, tile))
+        yieldAll(UnitActionsUpgrade.getUpgradeActions(unit))
         yieldAll(UnitActionsPillage.getPillageActions(unit, tile))
 
         addSleepActions(unit, tile)

core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActionsUpgrade.kt

@@ -1,7 +1,6 @@
 package com.unciv.ui.screens.worldscreen.unit.actions
 
 import com.unciv.logic.map.mapunit.MapUnit
-import com.unciv.logic.map.tile.Tile
 import com.unciv.models.Counter
 import com.unciv.models.UnitAction
 import com.unciv.models.UpgradeUnitAction
@@ -11,94 +10,95 @@ import com.unciv.models.translations.tr
 
 object UnitActionsUpgrade {
 
-    @Suppress("UNUSED_PARAMETER") // reference needs to have this signature
-    internal fun getUnitUpgradeActions(unit: MapUnit, tile: Tile) = sequenceOf(getUpgradeAction(unit)).filterNotNull()
-
     /**  Common implementation for [getUpgradeAction], [getFreeUpgradeAction] and [getAncientRuinsUpgradeAction] */
-    private fun getUpgradeAction(
+    private fun getUpgradeActions(
         unit: MapUnit,
         isFree: Boolean,
         isSpecial: Boolean,
         isAnywhere: Boolean
-    ): UnitAction? {
-        val specialUpgradesTo = unit.baseUnit().getMatchingUniques(UniqueType.RuinsUpgrade).map { it.params[0] }.firstOrNull()
-        if (unit.baseUnit().upgradesTo == null && specialUpgradesTo == null) return null // can't upgrade to anything
+    ): Sequence<UnitAction> {
         val unitTile = unit.getTile()
         val civInfo = unit.civ
-        if (!isAnywhere && unitTile.getOwner() != civInfo) return null
+        val specialUpgradesTo = if (isSpecial) 
+            unit.baseUnit().getMatchingUniques(UniqueType.RuinsUpgrade, StateForConditionals(civInfo, unit= unit))
+                .map { it.params[0] }.firstOrNull()
+        else null
+        val upgradeUnits = if (specialUpgradesTo != null) sequenceOf(specialUpgradesTo)
+            else unit.baseUnit.getUpgradeUnits(StateForConditionals(civInfo, unit = unit))
+        if (upgradeUnits.none()) return emptySequence() // can't upgrade to anything
+        if (!isAnywhere && unitTile.getOwner() != civInfo) return emptySequence()
 
-        val upgradesTo = unit.baseUnit().upgradesTo
-        val upgradedUnit = when {
-            isSpecial && specialUpgradesTo != null -> civInfo.getEquivalentUnit(specialUpgradesTo)
-            (isFree || isSpecial) && upgradesTo != null -> civInfo.getEquivalentUnit(upgradesTo) // Only get DIRECT upgrade
-            else -> unit.upgrade.getUnitToUpgradeTo() // Get EVENTUAL upgrade, all the way up the chain
-        }
+        var upgradeActions = emptySequence<UnitAction>()
+        for (upgradesTo in upgradeUnits){
+            val upgradedUnit = civInfo.getEquivalentUnit(upgradesTo)
 
-        if (!unit.upgrade.canUpgrade(unitToUpgradeTo = upgradedUnit, ignoreRequirements = isFree, ignoreResources = true))
-            return null
+            if (!unit.upgrade.canUpgrade(unitToUpgradeTo = upgradedUnit, ignoreRequirements = isFree, ignoreResources = true))
+                continue
 
-        // Check _new_ resource requirements (display only - yes even for free or special upgrades)
-        // Using Counter to aggregate is a bit exaggerated, but - respect the mad modder.
-        val resourceRequirementsDelta = Counter<String>()
-        for ((resource, amount) in unit.getResourceRequirementsPerTurn())
-            resourceRequirementsDelta.add(resource, -amount)
-        for ((resource, amount) in upgradedUnit.getResourceRequirementsPerTurn(StateForConditionals(unit.civ, unit = unit)))
-            resourceRequirementsDelta.add(resource, amount)
-        for ((resource, _) in resourceRequirementsDelta.filter { it.value < 0 })  // filter copies, so no CCM
-            resourceRequirementsDelta[resource] = 0
-        val newResourceRequirementsString = resourceRequirementsDelta.entries
-            .joinToString { "${it.value} {${it.key}}".tr() }
+            // Check _new_ resource requirements (display only - yes even for free or special upgrades)
+            // Using Counter to aggregate is a bit exaggerated, but - respect the mad modder.
+            val resourceRequirementsDelta = Counter<String>()
+            for ((resource, amount) in unit.getResourceRequirementsPerTurn())
+                resourceRequirementsDelta.add(resource, -amount)
+            for ((resource, amount) in upgradedUnit.getResourceRequirementsPerTurn(StateForConditionals(unit.civ, unit = unit)))
+                resourceRequirementsDelta.add(resource, amount)
+            for ((resource, _) in resourceRequirementsDelta.filter { it.value < 0 })  // filter copies, so no CCM
+                resourceRequirementsDelta[resource] = 0
+            val newResourceRequirementsString = resourceRequirementsDelta.entries
+                .joinToString { "${it.value} {${it.key}}".tr() }
 
-        val goldCostOfUpgrade = if (isFree) 0 else unit.upgrade.getCostOfUpgrade(upgradedUnit)
+            val goldCostOfUpgrade = if (isFree) 0 else unit.upgrade.getCostOfUpgrade(upgradedUnit)
 
-        // No string for "FREE" variants, these are never shown to the user.
-        // The free actions are only triggered via OneTimeUnitUpgrade or OneTimeUnitSpecialUpgrade in UniqueTriggerActivation.
-        val title = if (newResourceRequirementsString.isEmpty())
-            "Upgrade to [${upgradedUnit.name}] ([$goldCostOfUpgrade] gold)"
-        else "Upgrade to [${upgradedUnit.name}]\n([$goldCostOfUpgrade] gold, [$newResourceRequirementsString])"
+            // No string for "FREE" variants, these are never shown to the user.
+            // The free actions are only triggered via OneTimeUnitUpgrade or OneTimeUnitSpecialUpgrade in UniqueTriggerActivation.
+            val title = if (newResourceRequirementsString.isEmpty())
+                "Upgrade to [${upgradedUnit.name}] ([$goldCostOfUpgrade] gold)"
+            else "Upgrade to [${upgradedUnit.name}]\n([$goldCostOfUpgrade] gold, [$newResourceRequirementsString])"
 
-        return UpgradeUnitAction(
-            title = title,
-            unitToUpgradeTo = upgradedUnit,
-            goldCostOfUpgrade = goldCostOfUpgrade,
-            newResourceRequirements = resourceRequirementsDelta,
-            action = {
-                unit.destroy(destroyTransportedUnit = false)
-                val newUnit = civInfo.units.placeUnitNearTile(unitTile.position, upgradedUnit)
+            upgradeActions += UpgradeUnitAction(
+                title = title,
+                unitToUpgradeTo = upgradedUnit,
+                goldCostOfUpgrade = goldCostOfUpgrade,
+                newResourceRequirements = resourceRequirementsDelta,
+                action = {
+                    unit.destroy(destroyTransportedUnit = false)
+                    val newUnit = civInfo.units.placeUnitNearTile(unitTile.position, upgradedUnit)
 
-                /** We were UNABLE to place the new unit, which means that the unit failed to upgrade!
-                 * The only known cause of this currently is "land units upgrading to water units" which fail to be placed.
-                 */
+                    /** We were UNABLE to place the new unit, which means that the unit failed to upgrade!
+                     * The only known cause of this currently is "land units upgrading to water units" which fail to be placed.
+                     */
 
-                /** We were UNABLE to place the new unit, which means that the unit failed to upgrade!
-                 * The only known cause of this currently is "land units upgrading to water units" which fail to be placed.
-                 */
-                if (newUnit == null) {
-                    val resurrectedUnit = civInfo.units.placeUnitNearTile(unitTile.position, unit.baseUnit)!!
-                    unit.copyStatisticsTo(resurrectedUnit)
-                } else { // Managed to upgrade
-                    if (!isFree) civInfo.addGold(-goldCostOfUpgrade)
-                    unit.copyStatisticsTo(newUnit)
-                    newUnit.currentMovement = 0f
-                }
-            }.takeIf {
-                isFree || (
+                    /** We were UNABLE to place the new unit, which means that the unit failed to upgrade!
+                     * The only known cause of this currently is "land units upgrading to water units" which fail to be placed.
+                     */
+                    if (newUnit == null) {
+                        val resurrectedUnit = civInfo.units.placeUnitNearTile(unitTile.position, unit.baseUnit)!!
+                        unit.copyStatisticsTo(resurrectedUnit)
+                    } else { // Managed to upgrade
+                        if (!isFree) civInfo.addGold(-goldCostOfUpgrade)
+                        unit.copyStatisticsTo(newUnit)
+                        newUnit.currentMovement = 0f
+                    }
+                }.takeIf {
+                    isFree || (
                         unit.civ.gold >= goldCostOfUpgrade
-                                && unit.currentMovement > 0
-                                && unitTile.getOwner() == civInfo
-                                && !unit.isEmbarked()
-                                && unit.upgrade.canUpgrade(unitToUpgradeTo = upgradedUnit)
+                            && unit.currentMovement > 0
+                            && unitTile.getOwner() == civInfo
+                            && !unit.isEmbarked()
+                            && unit.upgrade.canUpgrade(unitToUpgradeTo = upgradedUnit)
                         )
-            }
-        )
+                }
+            )
+        }
+        return upgradeActions
     }
 
-    fun getUpgradeAction(unit: MapUnit) =
-            getUpgradeAction(unit, isFree = false, isSpecial = false, isAnywhere = false)
+    fun getUpgradeActions(unit: MapUnit) =
+        getUpgradeActions(unit, isSpecial = false, isFree = false, isAnywhere = false)
     fun getFreeUpgradeAction(unit: MapUnit) =
-            getUpgradeAction(unit, isFree = true, isSpecial = false, isAnywhere = true)
+        getUpgradeActions(unit, isSpecial = false, isFree = true, isAnywhere = true)
     fun getAncientRuinsUpgradeAction(unit: MapUnit) =
-            getUpgradeAction(unit, isFree = true, isSpecial = true, isAnywhere = true)
+        getUpgradeActions(unit, isSpecial = true, isFree = true, isAnywhere = true)
     fun getUpgradeActionAnywhere(unit: MapUnit) =
-            getUpgradeAction(unit, isFree = false, isSpecial = false, isAnywhere = true)
+        getUpgradeActions(unit, isSpecial = false, isFree = false, isAnywhere = true)
 }

tests/src/com/unciv/logic/map/UpgradeTests.kt

@@ -0,0 +1,118 @@
+package com.unciv.logic.map
+
+import com.badlogic.gdx.math.Vector2
+import com.unciv.models.UpgradeUnitAction
+import com.unciv.models.ruleset.unique.Unique
+import com.unciv.models.ruleset.unique.UniqueTriggerActivation
+import com.unciv.testing.GdxTestRunner
+import com.unciv.testing.TestGame
+import com.unciv.ui.screens.worldscreen.unit.actions.UnitActionsUpgrade
+import org.junit.Assert
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+
+@RunWith(GdxTestRunner::class)
+class UpgradeTests {
+
+    val testGame = TestGame()
+
+    @Before
+    fun initTest() {
+        testGame.makeHexagonalMap(5)
+    }
+
+    @Test
+    fun ruinsUpgradeToSpecialUnit() {
+        val unitToUpgradeTo = testGame.createBaseUnit()
+        val testUnit = testGame.createBaseUnit(uniques = arrayOf("May upgrade to [${unitToUpgradeTo.name}] through ruins-like effects"))
+        testUnit.upgradesTo = "Warrior"
+
+        val civ = testGame.addCiv()
+        var unit1 = testGame.addUnit(testUnit.name, civ, testGame.getTile(Vector2.Zero))
+        val triggerUnique = Unique("This Unit upgrades for free including special upgrades")
+        UniqueTriggerActivation.triggerUnitwideUnique(triggerUnique, unit1)
+        unit1 = testGame.getTile(Vector2.Zero).getFirstUnit()!!
+
+        Assert.assertTrue("Unit should upgrade to special unit, not warrior", unit1.baseUnit == unitToUpgradeTo)
+    }
+    
+    @Test
+    fun ruinsUpgradeToNormalUnitWithoutUnique() {
+        val unitToUpgradeTo = testGame.createBaseUnit()
+        val testUnit = testGame.createBaseUnit()
+        testUnit.upgradesTo = "Warrior"
+
+        val civ = testGame.addCiv()
+        var unit1 = testGame.addUnit(testUnit.name, civ, testGame.getTile(Vector2.Zero))
+        val triggerUnique = Unique("This Unit upgrades for free including special upgrades")
+        UniqueTriggerActivation.triggerUnitwideUnique(triggerUnique, unit1)
+        unit1 = testGame.getTile(Vector2.Zero).getFirstUnit()!!
+
+        Assert.assertTrue("Unit should upgrade to Warrior without unique", unit1.baseUnit.name == "Warrior")
+    }
+
+    @Test
+    fun regularUpgradeCannotUpgradeToSpecialUnit() {
+        val unitToUpgradeTo = testGame.createBaseUnit()
+        val testUnit = testGame.createBaseUnit(uniques = arrayOf("May upgrade to [${unitToUpgradeTo.name}] through ruins-like effects"))
+        testUnit.upgradesTo = "Warrior"
+
+        val civ = testGame.addCiv()
+        var unit1 = testGame.addUnit(testUnit.name, civ, testGame.getTile(Vector2.Zero))
+        val upgradeActions = UnitActionsUpgrade.getFreeUpgradeAction(unit1)
+
+        Assert.assertTrue(upgradeActions.count() == 1)
+        Assert.assertFalse("Unit should not be able to upgrade to special unit",
+            upgradeActions.any { (it as UpgradeUnitAction).unitToUpgradeTo == unitToUpgradeTo })
+
+        val triggerUnique = Unique("This Unit upgrades for free")
+        UniqueTriggerActivation.triggerUnitwideUnique(triggerUnique, unit1)
+        unit1 = testGame.getTile(Vector2.Zero).getFirstUnit()!!
+
+        Assert.assertTrue(unit1.baseUnit.name == "Warrior")
+    }
+
+    @Test
+    fun canUpgradeToMultipleWithUnique() {
+        val unitToUpgradeTo = testGame.createBaseUnit()
+        val testUnit = testGame.createBaseUnit(uniques = arrayOf(
+            "Can upgrade to [${unitToUpgradeTo.name}]",
+            "Can upgrade to [Warrior]",
+        ))
+
+        val civ = testGame.addCiv()
+        var unit1 = testGame.addUnit(testUnit.name, civ, testGame.getTile(Vector2.Zero))
+        val upgradeActions = UnitActionsUpgrade.getFreeUpgradeAction(unit1)
+
+        Assert.assertTrue(upgradeActions.count() == 2)
+
+        val triggerUnique = Unique("This Unit upgrades for free")
+        UniqueTriggerActivation.triggerUnitwideUnique(triggerUnique, unit1)
+        unit1 = testGame.getTile(Vector2.Zero).getFirstUnit()!!
+
+        Assert.assertFalse(unit1.baseUnit == testUnit)
+    }
+
+    @Test
+    fun cannotUpgradeWithoutGold() {
+        val unitToUpgradeTo = testGame.createBaseUnit()
+        val testUnit = testGame.createBaseUnit()
+        testUnit.upgradesTo = unitToUpgradeTo.name
+
+        val civ = testGame.addCiv()
+        testGame.addCity(civ, testGame.getTile(Vector2.Zero)) // We need to own the tile to be able to upgrade here
+        
+        val unit1 = testGame.addUnit(testUnit.name, civ, testGame.getTile(Vector2.Zero))
+        var upgradeActions = UnitActionsUpgrade.getUpgradeActionAnywhere(unit1)
+
+        Assert.assertTrue("We should need gold to upgrade here", upgradeActions.all { it.action == null })
+
+        civ.addGold(unit1.upgrade.getCostOfUpgrade(unitToUpgradeTo))
+
+        upgradeActions = UnitActionsUpgrade.getUpgradeActionAnywhere(unit1)
+
+        Assert.assertTrue(upgradeActions.count() == 1)
+        Assert.assertTrue(upgradeActions.none { it.action == null })
+    }
+}