Units that can't fortify stay fortified after upgrading

[SpacedOutChicken]

Is there an existing issue for this?

• [X] I have searched the existing issues

Game Version

4.10.11

Describe the bug

When a unit that can fortify upgrades to a unit that can't fortify (such as upgrading from anti-tank gun to helicopter gunship) the unit will stay fortified, even though the new unit should now no longer be able to fortify.

Steps to Reproduce

1. Build anti-tank gun
2. Fortify unit
3. Upgrade unit to helicopter gunship
4. Unit will stay fortified

Screenshots

Link to save file

FortifiedHelicopter.txt

Operating System

Windows

Additional Information

No response

[SomeTroglodyte]

Same with sleep I noticed recently. Upgrade should "wake up" any unit, and likely even clear all orders except automation?? I remember - was that earlier Unciv of Civ4? - that even if the new unit can fortify you had to re-fortify. Even promoting IMHO should wake up and thus force you to have 2 turns with reduced fortification bonus?

[yairm210]

Not sure if units upgrading should cause wakeup in any case. Promoting - definitely no.

[SomeTroglodyte]

Not sure if units upgrading should cause wakeup in any case

Pretty sure it did in the original Civs I played - up to IV. Also, easiest solution for upgrade changing the ability to fortify (how tempting that word to introduce a typo is).

[SomeTroglodyte]

This ddg hit pretty much corroborates Civ5 does wake up too: https://forums.civfanatics.com/threads/civ-5-unit-upgrades.354720/

[SomeTroglodyte]

Since that link I cited pretty much disparaged the original's wakeup...

patch???

```patch Index: core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActionsUpgrade.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActionsUpgrade.kt b/core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActionsUpgrade.kt --- a/core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActionsUpgrade.kt (revision 52eae2a3f9f443a95c9fffbaa65ea5909bb6b266) +++ b/core/src/com/unciv/ui/screens/worldscreen/unit/actions/UnitActionsUpgrade.kt (date 1708694455534) @@ -60,20 +60,7 @@ 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. - */ - 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 - } + unit.upgrade.performUpgrade(upgradedUnit, isFree, goldCostOfUpgrade) }.takeIf { isFree || ( unit.civ.gold >= goldCostOfUpgrade Index: core/src/com/unciv/logic/map/mapunit/UnitUpgradeManager.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/logic/map/mapunit/UnitUpgradeManager.kt b/core/src/com/unciv/logic/map/mapunit/UnitUpgradeManager.kt --- a/core/src/com/unciv/logic/map/mapunit/UnitUpgradeManager.kt (revision 52eae2a3f9f443a95c9fffbaa65ea5909bb6b266) +++ b/core/src/com/unciv/logic/map/mapunit/UnitUpgradeManager.kt (date 1708694455552) @@ -1,6 +1,5 @@ package com.unciv.logic.map.mapunit -import com.unciv.logic.UncivShowableException import com.unciv.models.ruleset.RejectionReasonType import com.unciv.models.ruleset.unique.StateForConditionals import com.unciv.models.ruleset.unique.UniqueType @@ -26,7 +25,7 @@ val rejectionReasons = unitToUpgradeTo.getRejectionReasons(unit.civ, additionalResources = unit.getResourceRequirementsPerTurn()) - var relevantRejectionReasons = rejectionReasons.filterNot { + var relevantRejectionReasons = rejectionReasons.filterNot { it.isConstructionRejection() || it.type == RejectionReasonType.Obsoleted } if (ignoreRequirements) @@ -68,9 +67,31 @@ cost = (cost * civModifier).pow(constants.exponent) cost *= unit.civ.gameInfo.speed.modifier goldCostOfUpgrade += (cost / constants.roundTo).toInt() * constants.roundTo - + return goldCostOfUpgrade } + fun performUpgrade(upgradedUnit: BaseUnit, isFree: Boolean, goldCostOfUpgrade: Int? = null) { + unit.destroy(destroyTransportedUnit = false) + val civ = unit.civ + val position = unit.currentTile.position + val newUnit = civ.units.placeUnitNearTile(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. + */ + if (newUnit == null) { + val resurrectedUnit = civ.units.placeUnitNearTile(position, unit.baseUnit)!! + unit.copyStatisticsTo(resurrectedUnit) + return + } + + // Managed to upgrade + if (!isFree) civ.addGold(-(goldCostOfUpgrade ?: getCostOfUpgrade(upgradedUnit))) + unit.copyStatisticsTo(newUnit) + newUnit.currentMovement = 0f + // wake up if lost ability to fortify + if (newUnit.isFortified() && !newUnit.canFortify(ignoreAlreadyFortified = true)) + newUnit.action = null + } } Index: core/src/com/unciv/logic/map/mapunit/MapUnit.kt IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/core/src/com/unciv/logic/map/mapunit/MapUnit.kt b/core/src/com/unciv/logic/map/mapunit/MapUnit.kt --- a/core/src/com/unciv/logic/map/mapunit/MapUnit.kt (revision 52eae2a3f9f443a95c9fffbaa65ea5909bb6b266) +++ b/core/src/com/unciv/logic/map/mapunit/MapUnit.kt (date 1708694455543) @@ -377,14 +377,15 @@ } - fun canFortify(): Boolean { - if (baseUnit.isWaterUnit()) return false - if (isCivilian()) return false - if (baseUnit.movesLikeAirUnits()) return false - if (isEmbarked()) return false - if (hasUnique(UniqueType.NoDefensiveTerrainBonus)) return false - if (isFortified()) return false - return true + fun canFortify(ignoreAlreadyFortified: Boolean = false) = when { + baseUnit.isWaterUnit() -> false + isCivilian() -> false + baseUnit.movesLikeAirUnits() -> false + isEmbarked() -> false + hasUnique(UniqueType.NoDefensiveTerrainBonus) -> false + ignoreAlreadyFortified -> true + isFortified() -> false + else -> true } private fun adjacentHealingBonus(): Int { ```

Wrong save up there - we need the state before the upgrade to test!

That larger patch boils down to if (newUnit.isFortified() && !newUnit.canFortify(ignoreAlreadyFortified = true)) wake up! - meaning chooses the least intrusive of the above discussion. The rest is "WTF is there an UnitUpgradeManager that does not actually manage an upgrade?", and one "redundant if" warning workaround. There's four more such warnings in MapUnit, all could copy that approach...

[SomeTroglodyte]

OK, that patch tests out fine. Sleeping artillery stays asleep on upgrade, fortified anti-tank-thingys wake up when helicopterized, fortified musketmen re-fortify from zero "automatically".

But - where did that upgrade several steps all the way to the most modern one go? Must have missed or forgotten some aspects of those multi-upgrade-to unique PR's. Is a rule change unless we re-offer the one-step in the right-click menu... Anyone can provide a multi-upgrade-path mod and save to test that on?