Privateer capture, plunder, and raze Notifications (#4698)

* Privateer capture, plunder, and raze Notifications

* Privateer capture, plunder, and raze Notifications - patch1

* Privateer capture, plunder, and raze Notifications - patch2

core/src/com/unciv/logic/battle/Battle.kt

@@ -15,6 +15,7 @@ import com.unciv.models.stats.Stat
 import com.unciv.models.stats.Stats
 import java.util.*
 import kotlin.math.max
+import kotlin.math.min
 
 /**
  * Damage calculations according to civ v wiki and https://steamcommunity.com/sharedfiles/filedetails/?id=170194443
@@ -58,12 +59,20 @@ object Battle {
 
         takeDamage(attacker, defender)
 
-        postBattleNotifications(attacker, defender, attackedTile, attacker.getTile())
+        // check if unit is captured by the attacker (prize ships unique)
+        // As ravignir clarified in issue #4374, this only works for aggressor
+        val captureSuccess = defender is MapUnitCombatant && attacker is MapUnitCombatant
+                && defender.isDefeated() && !defender.unit.isCivilian()
+                && tryCaptureUnit(attacker, defender)
+
+        if (!captureSuccess)  // capture creates a new unit, but `defender` still is the original, so this function would still show a kill message
+            postBattleNotifications(attacker, defender, attackedTile, attacker.getTile())
 
         postBattleNationUniques(defender, attackedTile, attacker)
 
         // This needs to come BEFORE the move-to-tile, because if we haven't conquered it we can't move there =)
-        if (defender.isDefeated() && defender is CityCombatant && attacker is MapUnitCombatant && attacker.isMelee() && !attacker.unit.hasUnique("Unable to capture cities"))
+        if (defender.isDefeated() && defender is CityCombatant && attacker is MapUnitCombatant
+                && attacker.isMelee() && !attacker.unit.hasUnique("Unable to capture cities"))
             conquerCity(defender.city, attacker)
 
         // Exploring units surviving an attack should "wake up"
@@ -72,14 +81,11 @@ object Battle {
 
         // Add culture when defeating a barbarian when Honor policy is adopted, gold from enemy killed when honor is complete
         // or any enemy military unit with Sacrificial captives unique (can be either attacker or defender!)
-        // or check if unit is captured by the attacker (prize ships unique)
         if (defender.isDefeated() && defender is MapUnitCombatant && !defender.unit.isCivilian()) {
             tryEarnFromKilling(attacker, defender)
-            tryCaptureUnit(attacker, defender)
             tryHealAfterKilling(attacker)
         } else if (attacker.isDefeated() && attacker is MapUnitCombatant && !attacker.unit.isCivilian()) {
             tryEarnFromKilling(defender, attacker)
-            tryCaptureUnit(defender, attacker)
             tryHealAfterKilling(defender)
         }
 
@@ -92,7 +98,8 @@ object Battle {
 
         // we're a melee unit and we destroyed\captured an enemy unit
         // Should be called after tryCaptureUnit(), as that might spawn a unit on the tile we go to
-        postBattleMoveToAttackedTile(attacker, defender, attackedTile)
+        if (!captureSuccess)
+            postBattleMoveToAttackedTile(attacker, defender, attackedTile)
 
         reduceAttackerMovementPointsAndAttacks(attacker, defender)
 
@@ -138,31 +145,36 @@ object Battle {
         }
     }
 
-    private fun tryCaptureUnit(attacker: ICombatant, defender: MapUnitCombatant) {
+    private fun tryCaptureUnit(attacker: MapUnitCombatant, defender: MapUnitCombatant): Boolean {
         // https://forums.civfanatics.com/threads/prize-ships-for-land-units.650196/
         // https://civilization.fandom.com/wiki/Module:Data/Civ5/GK/Defines
-        if (!defender.isDefeated()) return
-        if (attacker !is MapUnitCombatant) return
-        if (attacker.unit.getMatchingUniques("May capture killed [] units").none { defender.matchesCategory(it.params[0]) }) return
 
-        var captureChance = 10 + attacker.getAttackingStrength().toFloat() / defender.getDefendingStrength().toFloat() * 40
+        if (attacker.unit.getMatchingUniques("May capture killed [] units").none { defender.matchesCategory(it.params[0]) }) return false
-        if (captureChance > 80) captureChance = 80f
-        if (100 * Random().nextFloat() > captureChance) return
 
-        val newUnit = attacker.getCivInfo().placeUnitNearTile(defender.getTile().position, defender.getName())
+        val captureChance = min(0.8f, 0.1f + attacker.getAttackingStrength().toFloat() / defender.getDefendingStrength().toFloat() * 0.4f)
-        if (newUnit == null) return // silently fail
+        if (Random().nextFloat() > captureChance) return false
-        attacker.getCivInfo().addNotification("Your [${attacker.getName()}] captured an enemy [${defender.getName()}]", newUnit.getTile().position, NotificationIcon.War)
+
+        // This is called after takeDamage and so the defeated defender is already destroyed and
+        // thus removed from the tile - but MapUnit.destroy() will not clear the unit's currentTile.
+        // Therefore placeUnitNearTile _will_ place the new unit exactly where the defender was
+        val defenderName = defender.getName()
+        val newUnit = attacker.getCivInfo().placeUnitNearTile(defender.getTile().position, defenderName)
+            ?: return false  // silently fail
+
+        attacker.getCivInfo().addNotification(
+            "Your [${attacker.getName()}] captured an enemy [$defenderName]",
+            newUnit.getTile().position, attacker.getName(), NotificationIcon.War, defenderName )
 
         newUnit.currentMovement = 0f
         newUnit.health = 50
+        return true
     }
 
     private fun takeDamage(attacker: ICombatant, defender: ICombatant) {
         var potentialDamageToDefender = BattleDamage.calculateDamageToDefender(attacker, attacker.getTile(), defender)
         var potentialDamageToAttacker = BattleDamage.calculateDamageToAttacker(attacker, attacker.getTile(), defender)
 
-        var damageToAttacker = attacker.getHealth() // These variables names don't make any sense as of yet ...
+        val defenderHealthBefore = defender.getHealth()
-        var damageToDefender = defender.getHealth()
 
         if (defender is MapUnitCombatant && defender.unit.isCivilian() && attacker.isMelee()) {
             captureCivilianUnit(attacker, defender)
@@ -185,38 +197,44 @@ object Battle {
             }
         }
 
-        damageToAttacker -= attacker.getHealth() // ... but from here on they are accurate
+        plunderFromDamage(attacker, defender, defenderHealthBefore - defender.getHealth())
-        damageToDefender -= defender.getHealth()
-
-        plunderFromDamage(attacker, defender, damageToDefender)
-        plunderFromDamage(defender, attacker, damageToAttacker)
     }
 
-    private fun plunderFromDamage(plunderingUnit: ICombatant, plunderedUnit: ICombatant, damageDealt: Int) {
+    private object PlunderableStats {
-        val plunderedGoods = Stats()
+        val stats = setOf (Stat.Gold, Stat.Science, Stat.Culture, Stat.Faith)
+    }
+    private fun plunderFromDamage(
+        plunderingUnit: ICombatant,
+        plunderedUnit: ICombatant,
+        damageDealt: Int
+    ) {
+        // implementation based on the description of the original civilopedia, see issue #4374
         if (plunderingUnit !is MapUnitCombatant) return
+        val plunderedGoods = Stats()
 
         for (unique in plunderingUnit.unit.getMatchingUniques("Earn []% of the damage done to [] units as []")) {
             if (plunderedUnit.matchesCategory(unique.params[1])) {
-                val resourcesPlundered =
+                // silently ignore bad mods here - or test in checkModLinks
-                    unique.params[0].toFloat() / 100f * damageDealt
+                val stat = Stat.values().firstOrNull { it.name == unique.params[2] }
-                plunderedGoods.add(Stat.valueOf(unique.params[2]), resourcesPlundered)
+                    ?: continue  // stat badly defined in unique
+                if (stat !in PlunderableStats.stats)
+                    continue     // stat known but not valid 
+                val percentage = unique.params[0].toFloatOrNull()
+                    ?: continue  // percentage parameter invalid
+                plunderedGoods.add(stat, percentage / 100f * damageDealt)
             }
         }
         
-        val plunderableStats = listOf("Gold", "Science", "Culture", "Faith").map { Stat.valueOf(it) }
+        val civ = plunderingUnit.getCivInfo()
-        for (stat in plunderableStats) {
+        plunderedGoods.toHashMap().filterNot { it.value == 0f }.forEach {
-            val resourcesPlundered = plunderedGoods.get(stat)
+            val plunderedAmount = it.value.toInt()
-            if (resourcesPlundered == 0f) continue
+            civ.addStat(it.key, plunderedAmount)
-            plunderingUnit.getCivInfo().addStat(stat, resourcesPlundered.toInt())
+            civ.addNotification(
-            plunderingUnit.getCivInfo()
+                "Your [${plunderingUnit.getName()}] plundered [${plunderedAmount}] [${it.key.name}] from [${plunderedUnit.getName()}]",
-                .addNotification(
+                plunderedUnit.getTile().position,
-                    "Your [${plunderingUnit.getName()}] plundered [${resourcesPlundered}] [${stat.name}] from [${plunderedUnit.getName()}]",
+                plunderingUnit.getName(), NotificationIcon.War, "StatIcons/${it.key.name}",
-                        plunderedUnit.getTile().position,
+                if (plunderedUnit is CityCombatant) NotificationIcon.City else plunderedUnit.getName()
-                        NotificationIcon.War
+            )
-                )
-            
-            
         }
     }
 
@@ -226,15 +244,18 @@ object Battle {
         attackedTile: TileInfo,
         attackerTile: TileInfo? = null
     ) {
-        if (attacker.getCivInfo() != defender.getCivInfo()) { // If what happened was that a civilian unit was captures, that's dealt with in the captureCivilianUnit function
+        if (attacker.getCivInfo() != defender.getCivInfo()) {
-            val whatHappenedString =
+            // If what happened was that a civilian unit was captured, that's dealt with in the captureCivilianUnit function
-                    if (attacker !is CityCombatant && attacker.isDefeated()) " was destroyed while attacking"
+            val (whatHappenedIcon, whatHappenedString) = when {
-                    else " has " + (
+                attacker !is CityCombatant && attacker.isDefeated() ->
-                            if (defender.isDefeated())
+                    NotificationIcon.War to " was destroyed while attacking"
-                                if (defender.getUnitType() == UnitType.City && attacker.isMelee())
+                !defender.isDefeated() ->
-                                    "captured"
+                    NotificationIcon.War to " has attacked"
-                                else "destroyed"
+                defender.getUnitType() == UnitType.City && attacker.isMelee() ->
-                            else "attacked")
+                    NotificationIcon.War to " has captured"
+                else ->
+                    NotificationIcon.Death to " has destroyed"
+            }
             val attackerString =
                     if (attacker.getUnitType() == UnitType.City) "Enemy city [" + attacker.getName() + "]"
                     else "An enemy [" + attacker.getName() + "]"
@@ -244,15 +265,14 @@ object Battle {
                         else " [" + defender.getName() + "]"
                     else " our [" + defender.getName() + "]"
             val notificationString = attackerString + whatHappenedString + defenderString
-            val cityIcon = "ImprovementIcons/Citadel"
+            val attackerIcon = if (attacker is CityCombatant) NotificationIcon.City else attacker.getName()
-            val attackerIcon = if (attacker is CityCombatant) cityIcon else attacker.getName()
+            val defenderIcon = if (defender is CityCombatant) NotificationIcon.City else defender.getName()
-            val defenderIcon = if (defender is CityCombatant) cityIcon else defender.getName()
             val locations = LocationAction (
                 if (attackerTile != null && attackerTile.position != attackedTile.position)
                         listOf(attackedTile.position, attackerTile.position)
                 else listOf(attackedTile.position)
             )
-            defender.getCivInfo().addNotification(notificationString, locations, attackerIcon, NotificationIcon.War, defenderIcon)
+            defender.getCivInfo().addNotification(notificationString, locations, attackerIcon, whatHappenedIcon, defenderIcon)
         }
     }
 
@@ -452,6 +472,7 @@ object Battle {
         }
     }
 
+    @Suppress("FunctionName")   // Yes we want this name to stand out
     fun NUKE(attacker: MapUnitCombatant, targetTile: TileInfo) {
         val attackingCiv = attacker.getCivInfo()
         fun tryDeclareWar(civSuffered: CivilizationInfo) {
@@ -554,7 +575,7 @@ object Battle {
         }
 
         // Damage and/or destroy units on the tile
-        for (unit in tile.getUnits().toList()) { // tolist so if it's destroyed there's no concurrent modification
+        for (unit in tile.getUnits().toList()) { // toList so if it's destroyed there's no concurrent modification
             val defender = MapUnitCombatant(unit)
             if (defender.unit.isCivilian()) {
                 unit.destroy() // destroy the unit
@@ -603,7 +624,7 @@ object Battle {
                 city.population.setPopulation(1) // For cities that cannot be destroyed, such as original capitals
                 city.destroyCity()
             } else {
-                var populationLoss = city.population.population * (0.6 + Random().nextFloat() * 0.2);
+                var populationLoss = city.population.population * (0.6 + Random().nextFloat() * 0.2)
                 var populationLossReduced = false
                 for (unique in city.civInfo.getMatchingUniques("Population loss from nuclear attacks -[]%")) {
                     populationLoss *= 1 - unique.params[0].toFloat() / 100f
@@ -621,7 +642,7 @@ object Battle {
         }
 
         // Destroy all hit units
-        for (defender in tile.getUnits().toList()) { // toList to avoid concurent modification exceptions
+        for (defender in tile.getUnits().toList()) { // toList to avoid concurrent modification exceptions
             defender.destroy()
             postBattleNotifications(attacker, MapUnitCombatant(defender), defender.currentTile)
             destroyIfDefeated(defender.civInfo, attacker.getCivInfo())

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

@@ -130,8 +130,9 @@ class CivInfoTransientUpdater(val civInfo: CivilizationInfo) {
                 if (city !in civInfo.citiesConnectedToCapitalToMediums && city.civInfo == civInfo && city != civInfo.getCapital())
                     civInfo.addNotification("[${city.name}] has been connected to your capital!", city.location, NotificationIcon.Gold)
 
+            // This may still contain cities that have just been destroyed by razing - thus the population test
             for (city in civInfo.citiesConnectedToCapitalToMediums.keys)
-                if (!citiesReachedToMediums.containsKey(city) && city.civInfo == civInfo)
+                if (!citiesReachedToMediums.containsKey(city) && city.civInfo == civInfo && city.population.population > 0)
                     civInfo.addNotification("[${city.name}] has been disconnected from your capital!", city.location, NotificationIcon.Gold)
         }
 

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

@@ -7,15 +7,17 @@ import com.unciv.ui.trade.DiplomacyScreen
 import com.unciv.ui.worldscreen.WorldScreen
 
 object NotificationIcon {
-    val Culture = "StatIcons/Culture"
+    const val Culture = "StatIcons/Culture"
-    val Construction = "StatIcons/Production"
+    const val Construction = "StatIcons/Production"
-    val Growth = "StatIcons/Population"
+    const val Growth = "StatIcons/Population"
-    val War = "OtherIcons/Pillage"
+    const val War = "OtherIcons/Pillage"
-    val Trade = "StatIcons/Acquire"
+    const val Trade = "StatIcons/Acquire"
-    val Science = "StatIcons/Science"
+    const val Science = "StatIcons/Science"
-    val Gold = "StatIcons/Gold"
+    const val Gold = "StatIcons/Gold"
-    val Death = "OtherIcons/DisbandUnit"
+    const val Death = "OtherIcons/DisbandUnit"
-    val Diplomacy = "OtherIcons/Diplomacy"
+    const val Diplomacy = "OtherIcons/Diplomacy"
+    const val City = "ImprovementIcons/City center"
+    const val Citadel = "ImprovementIcons/Citadel"
 }
 
 /**

core/src/com/unciv/logic/map/MapUnit.kt

@@ -961,21 +961,18 @@ class MapUnit {
                 civInfo.addNotification(
                     "An enemy [Citadel] has destroyed our [$name]",
                     locations,
-                    name,
+                    NotificationIcon.Citadel, NotificationIcon.Death, name
-                    NotificationIcon.Death
                 )
                 citadelTile.getOwner()?.addNotification(
                     "Your [Citadel] has destroyed an enemy [$name]",
                     locations,
-                    name,
+                    NotificationIcon.Citadel, NotificationIcon.Death, name
-                    NotificationIcon.Death
                 )
                 destroy()
             } else civInfo.addNotification(
                 "An enemy [Citadel] has attacked our [$name]",
                 locations,
-                name,
+                NotificationIcon.Citadel, NotificationIcon.War, name 
-                NotificationIcon.War
             )
         }
     }