Fix Right-Click attacks made no sound (#6906)

* Fix Right-Click attacks made no sound

* Fix Right-Click attacks made no sound - no UI in logic

* Fix Right-Click attacks made no sound - comments

* Fix Right-Click attacks made no sound - comments

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

@@ -27,8 +27,22 @@ import kotlin.math.min
  */
 object Battle {
 
+    /**
+     * Moves [attacker] to [attackableTile], handles siege setup then attacks if still possible
+     * (by calling [attack] or [NUKE]). Does _not_ play the attack sound!
+     */
     fun moveAndAttack(attacker: ICombatant, attackableTile: AttackableTile) {
-        if (attacker is MapUnitCombatant) {
+        if (!movePreparingAttack(attacker, attackableTile)) return
+        attackOrNuke(attacker, attackableTile)
+    }
+
+    /**
+     * Moves [attacker] to [attackableTile], handles siege setup and returns `true` if an attack is still possible.
+     *
+     * This is a logic function, not UI, so e.g. sound needs to be handled after calling this.
+     */
+    fun movePreparingAttack(attacker: ICombatant, attackableTile: AttackableTile): Boolean {
+        if (attacker !is MapUnitCombatant) return true
         attacker.unit.movement.moveToTile(attackableTile.tileToAttackFrom)
         /**
          * When calculating movement distance, we assume that a hidden tile is 1 movement point,
@@ -36,20 +50,27 @@ object Battle {
          * that you can attack a tile by passing through a HIDDEN TILE,
          * but the hidden tile is actually IMPASSIBLE so you stop halfway!
          */
-            if (attacker.getTile() != attackableTile.tileToAttackFrom) return
+        if (attacker.getTile() != attackableTile.tileToAttackFrom) return false
         /** Alternatively, maybe we DID reach that tile, but it turned out to be a hill or something,
          * so we expended all of our movement points!
          */
-            if (attacker.unit.currentMovement == 0f)
+        if (attacker.hasUnique(UniqueType.MustSetUp)
-                return
+                && !attacker.unit.isSetUpForSiege()
-            if (attacker.hasUnique(UniqueType.MustSetUp) && !attacker.unit.isSetUpForSiege()) {
+                && attacker.unit.currentMovement > 0f
+        ) {
             attacker.unit.action = UnitActionType.SetUp.value
             attacker.unit.useMovementPoints(1f)
         }
+        return (attacker.unit.currentMovement > 0f)
     }
 
+    /**
+     * This is meant to be called only after all prerequisite checks have been done.
+     */
+    fun attackOrNuke(attacker: ICombatant, attackableTile: AttackableTile) {
         if (attacker is MapUnitCombatant && attacker.unit.baseUnit.isNuclearWeapon())
-            return NUKE(attacker, attackableTile.tileToAttack)
+            NUKE(attacker, attackableTile.tileToAttack)
+        else
             attack(attacker, getMapCombatantOfTile(attackableTile.tileToAttack)!!)
     }
 

core/src/com/unciv/ui/worldscreen/WorldMapHolder.kt

@@ -116,29 +116,7 @@ class WorldMapHolder(internal val worldScreen: WorldScreen, internal val tileMap
                     val unit = worldScreen.bottomUnitTable.selectedUnit
                         ?: return
                     launchCrashHandling("WorldScreenClick") {
-                        val tile = tileGroup.tileInfo
+                        onTileRightClicked(unit, tileGroup.tileInfo)
-
-                        if (worldScreen.bottomUnitTable.selectedUnitIsSwapping) {
-                            if (unit.movement.canUnitSwapTo(tile)) {
-                                swapMoveUnitToTargetTile(unit, tile)
-                            }
-                            // If we are in unit-swapping mode, we don't want to move or attack
-                            return@launchCrashHandling
-                        }
-
-                        val attackableTile = BattleHelper.getAttackableEnemies(unit, unit.movement.getDistanceToTiles())
-                                .firstOrNull { it.tileToAttack == tileGroup.tileInfo }
-                        if (unit.canAttack() && attackableTile != null) {
-                            Battle.moveAndAttack(MapUnitCombatant(unit), attackableTile)
-                            worldScreen.shouldUpdate = true
-                            return@launchCrashHandling
-                        }
-
-                        val canUnitReachTile = unit.movement.canReach(tile)
-                        if (canUnitReachTile) {
-                            moveUnitToTargetTile(listOf(unit), tile)
-                            return@launchCrashHandling
-                        }
                     }
                 }
             })
@@ -202,6 +180,32 @@ class WorldMapHolder(internal val worldScreen: WorldScreen, internal val tileMap
         worldScreen.shouldUpdate = true
     }
 
+    private fun onTileRightClicked(unit: MapUnit, tile: TileInfo) {
+        if (worldScreen.bottomUnitTable.selectedUnitIsSwapping) {
+            if (unit.movement.canUnitSwapTo(tile)) {
+                swapMoveUnitToTargetTile(unit, tile)
+            }
+            // If we are in unit-swapping mode, we don't want to move or attack
+            return
+        }
+
+        val attackableTile = BattleHelper.getAttackableEnemies(unit, unit.movement.getDistanceToTiles())
+            .firstOrNull { it.tileToAttack == tile }
+        if (unit.canAttack() && attackableTile != null) {
+            worldScreen.shouldUpdate = true
+            val attacker = MapUnitCombatant(unit)
+            if (!Battle.movePreparingAttack(attacker, attackableTile)) return
+            Sounds.play(attacker.getAttackSound())
+            Battle.attackOrNuke(attacker, attackableTile)
+            return
+        }
+
+        val canUnitReachTile = unit.movement.canReach(tile)
+        if (canUnitReachTile) {
+            moveUnitToTargetTile(listOf(unit), tile)
+            return
+        }
+    }
 
     private fun moveUnitToTargetTile(selectedUnits: List<MapUnit>, targetTile: TileInfo) {
         // this can take a long time, because of the unit-to-tile calculation needed, so we put it in a different thread
@@ -550,7 +554,7 @@ class WorldMapHolder(internal val worldScreen: WorldScreen, internal val tileMap
             }
         }
 
-        // Same as below - randomly, tileGroups doesn't seem to contain the selected tile, and this doesn't seem duplicatable
+        // Same as below - randomly, tileGroups doesn't seem to contain the selected tile, and this doesn't seem reproducible
         val worldTileGroupsForSelectedTile = tileGroups[selectedTile]
         if (worldTileGroupsForSelectedTile != null)
             for (group in worldTileGroupsForSelectedTile)
@@ -681,7 +685,7 @@ class WorldMapHolder(internal val worldScreen: WorldScreen, internal val tileMap
         val originalScrollX = scrollX
         val originalScrollY = scrollY
 
-        // We want to center on the middle of the tilegroup (TG.getX()+TG.getWidth()/2)
+        // We want to center on the middle of the TileGroup (TG.getX()+TG.getWidth()/2)
         // and so the scroll position (== filter the screen starts) needs to be half the ScrollMap away
         val finalScrollX = tileGroup.x + tileGroup.width / 2 - width / 2
 

core/src/com/unciv/ui/worldscreen/WorldScreen.kt

@@ -407,7 +407,7 @@ class WorldScreen(val gameInfo: GameInfo, val viewingCiv:CivilizationInfo) : Bas
     // This is private so that we will set the shouldUpdate to true instead.
     // That way, not only do we save a lot of unnecessary updates, we also ensure that all updates are called from the main GL thread
     // and we don't get any silly concurrency problems!
-    internal fun update() {
+    private fun update() {
 
         displayTutorialsOnUpdate()
 

core/src/com/unciv/ui/worldscreen/bottombar/BattleTable.kt

@@ -16,8 +16,10 @@ import com.unciv.logic.automation.UnitAutomation
 import com.unciv.logic.battle.*
 import com.unciv.logic.map.TileInfo
 import com.unciv.models.AttackableTile
+import com.unciv.models.UncivSound
 import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.translations.tr
+import com.unciv.ui.audio.Sounds
 import com.unciv.ui.images.ImageGetter
 import com.unciv.ui.utils.*
 import com.unciv.ui.worldscreen.WorldScreen
@@ -251,10 +253,35 @@ class BattleTable(val worldScreen: WorldScreen): Table() {
         }
 
         else {
-            attackButton.onClick(attacker.getAttackSound()) {
+            attackButton.onClick(UncivSound.Silent) {  // onAttackButtonClicked will do the sound
-                Battle.moveAndAttack(attacker, attackableTile)
+                onAttackButtonClicked(attacker, defender, attackableTile, damageToAttacker, damageToDefender)
+            }
+        }
+
+        add(attackButton).colspan(2)
+
+        pack()
+
+        setPosition(worldScreen.stage.width/2-width/2, 5f)
+    }
+
+    private fun onAttackButtonClicked(
+        attacker: ICombatant,
+        defender: ICombatant,
+        attackableTile: AttackableTile,
+        damageToAttacker: Int,
+        damageToDefender: Int
+    ) {
+        val canStillAttack = Battle.movePreparingAttack(attacker, attackableTile)
         worldScreen.mapHolder.removeUnitActionOverlay() // the overlay was one of attacking
-                worldScreen.update()
+        // There was a direct worldScreen.update() call here, removing its 'private' but not the comment justifying the modifier.
+        // My tests (desktop only) show the red-flash animations look just fine without.
+        worldScreen.shouldUpdate = true
+        //Gdx.graphics.requestRendering()  // Use this if immediate rendering is required
+
+        if (!canStillAttack) return
+        Sounds.play(attacker.getAttackSound())
+        Battle.attackOrNuke(attacker, attackableTile)
 
         val actorsToFlashRed = arrayListOf<Actor>()
 
@@ -275,15 +302,6 @@ class BattleTable(val worldScreen: WorldScreen): Table() {
             }
         ))
     }
-        }
-
-        add(attackButton).colspan(2)
-
-        pack()
-
-        setPosition(worldScreen.stage.width/2-width/2, 5f)
-    }
-
 
     fun getMapActorsForCombatant(combatant: ICombatant):Sequence<Actor> =
         sequence {