BFS cleanup (#4859)

* BFS cleanup
- Kotlin ArrayDeque is no longer experimental
- Client code sidestepped class API even where API existed -> closed it down
- nextStep when already ended now no-op instead of crash, suits usecases
- predicate test very likely to be more expensive than tilesReached lookup
- getPathTo only used asSequence() - why not a Sequence in the first place
- Compleat documentation. This is very low level, so better help future coders to understand.
- Lint: One annoying hardcoded conditional breakpoint, one unhelpful exception

* BFS cleanup - patch1

core/src/com/unciv/logic/GameStarter.kt

@@ -347,7 +347,7 @@ object GameStarter {
         while (landTiles.any()) {
             val bfs = BFS(landTiles.random()) { it.isLand && !it.isImpassible() }
             bfs.stepToEnd()
-            val tilesInGroup = bfs.tilesReached.keys
+            val tilesInGroup = bfs.getReachedTiles()
             landTiles = landTiles.filter { it !in tilesInGroup }
             if (tilesInGroup.size > 20) // is this a good number? I dunno, but it's easy enough to change later on
                 landTilesInBigEnoughGroup.addAll(tilesInGroup)
@@ -463,4 +463,4 @@ object GameStarter {
         val distanceFromCenter = HexMath.getDistance(vector, Vector2.Zero)
         return hexagonalRadius - distanceFromCenter >= n
     }
-}
+}

core/src/com/unciv/logic/automation/Automation.kt

@@ -71,7 +71,7 @@ object Automation {
         val findWaterConnectedCitiesAndEnemies =
             BFS(city.getCenterTile()) { it.isWater || it.isCityCenter() }
         findWaterConnectedCitiesAndEnemies.stepToEnd()
-        if (findWaterConnectedCitiesAndEnemies.tilesReached.keys.none {
+        if (findWaterConnectedCitiesAndEnemies.getReachedTiles().none {
                 (it.isCityCenter() && it.getOwner() != city.civInfo)
                         || (it.militaryUnit != null && it.militaryUnit!!.civInfo != city.civInfo)
             }) // there is absolutely no reason for you to make water units on this body of water.
@@ -218,4 +218,4 @@ enum class ThreatLevel{
     Medium,
     High,
     VeryHigh
-}
+}

core/src/com/unciv/logic/automation/WorkerAutomation.kt

@@ -85,10 +85,10 @@ class WorkerAutomation(val unit: MapUnit) {
         // we order cities by their closeness to the worker first, and then check for each one whether there's a viable path
         // it can take to an existing connected city.
         for (bfs in citiesThatNeedConnectingBfs) {
-            while (bfs.tilesToCheck.isNotEmpty()) {
+            while (!bfs.hasEnded()) {
                 bfs.nextStep()
                 for (city in connectedCities)
-                    if (bfs.tilesToCheck.contains(city)) { // we have a winner!
+                    if (bfs.hasReachedTile(city)) { // we have a winner!
                         val pathToCity = bfs.getPathTo(city).asSequence()
                         val roadableTiles = pathToCity.filter { it.roadStatus < targetRoad }
                         val tileToConstructRoadOn: TileInfo

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

@@ -1,59 +1,80 @@
 package com.unciv.logic.map
 
-// Kotlin's ArrayDeque is experimental
+import kotlin.collections.ArrayDeque
-import java.util.ArrayDeque
 
 /**
- * Defines intermediate steps of a breadth-first search, for use in either get shortest path or get onnected tiles.
+ * Defines intermediate steps of a breadth-first search, for use in either get shortest path or get connected tiles.
  */
-class BFS(val startingPoint: TileInfo, val predicate : (TileInfo) -> Boolean) {
+class BFS(
-    var tilesToCheck = ArrayDeque<TileInfo>()
+    val startingPoint: TileInfo,
+    private val predicate : (TileInfo) -> Boolean
+) {
+    /** remaining tiles to check */
+    private val tilesToCheck = ArrayDeque<TileInfo>(37)  // needs resize at distance 4
 
     /** each tile reached points to its parent tile, where we got to it from */
-    val tilesReached = HashMap<TileInfo, TileInfo>()
+    private val tilesReached = HashMap<TileInfo, TileInfo>()
 
     init {
         tilesToCheck.add(startingPoint)
         tilesReached[startingPoint] = startingPoint
     }
 
+    /** Process fully until there's nowhere left to check */
     fun stepToEnd() {
         while (!hasEnded())
             nextStep()
     }
 
-
+    /**
+     * Process until either [destination] is reached or there's nowhere left to check
+     * @return `this` instance for chaining
+     */
     fun stepUntilDestination(destination: TileInfo): BFS {
         while (!tilesReached.containsKey(destination) && !hasEnded())
             nextStep()
         return this
     }
 
+    /**
+     * Process one tile-to-search, fetching all neighbors not yet touched
+     * and adding those that fulfill the [predicate] to the reached set
+     * and to the yet-to-be-processed set.
+     * 
+     * Will do nothing when [hasEnded] returns `true`
+     */
     fun nextStep() {
-        val current = tilesToCheck.remove()
+        val current = tilesToCheck.removeFirstOrNull() ?: return
         for (neighbor in current.neighbors) {
-            if (predicate(neighbor) && !tilesReached.containsKey(neighbor)) {
+            if (neighbor !in tilesReached && predicate(neighbor)) {
                 tilesReached[neighbor] = current
                 tilesToCheck.add(neighbor)
             }
         }
     }
 
-    fun getPathTo(destination: TileInfo): ArrayList<TileInfo> {
+    /**
-        val path = ArrayList<TileInfo>()
+     * @return a Sequence from the [destination] back to the [startingPoint], including both, or empty if [destination] has not been reached
-        path.add(destination)
+     */
+    fun getPathTo(destination: TileInfo): Sequence<TileInfo> = sequence {
         var currentNode = destination
-        while (currentNode != startingPoint) {
+        while (true) {
-            val parent = tilesReached[currentNode]
+            val parent = tilesReached[currentNode] ?: break  // destination is not in our path
-            if (parent == null) return ArrayList()// destination is not in our path
+            yield(currentNode)
+            if (currentNode == startingPoint) break
             currentNode = parent
-            path.add(currentNode)
         }
-        return path
     }
 
+    /** @return true if there are no more tiles to check */
     fun hasEnded() = tilesToCheck.isEmpty()
 
-    fun hasReachedTile(tile: TileInfo) =
+    /** @return true if the [tile] has been reached */
-            tilesReached.containsKey(tile)
+    fun hasReachedTile(tile: TileInfo) = tilesReached.containsKey(tile)
-}
+
+    /** @return all tiles reached so far */
+    fun getReachedTiles(): MutableSet<TileInfo> = tilesReached.keys
+
+    /** @return number of tiles reached so far */
+    fun size() = tilesReached.size
+}

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

@@ -254,10 +254,6 @@ class TileMap {
                 val containsViewableNeighborThatCanSeeOver = cTile.neighbors.any {
                         bNeighbor: TileInfo ->
                     val bNeighborHeight = bNeighbor.getHeight()
-                    if(cTile.resource=="Marble"
-                        && bNeighbor.terrainFeatures.contains("Forest")
-                    )
-                        println()
                     viewableTiles.contains(bNeighbor) && (
                             currentTileHeight > bNeighborHeight // a>b
                                     || cTileHeight > bNeighborHeight // c>b

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

@@ -183,12 +183,12 @@ class UnitMovementAlgorithms(val unit:MapUnit) {
 
         val distanceToTiles = getDistanceToTiles()
 
-        class UnreachableDestinationException : Exception()
+        class UnreachableDestinationException(msg: String) : Exception(msg)
 
         // If the tile is far away, we need to build a path how to get there, and then take the first step
         if (!distanceToTiles.containsKey(finalDestination))
             return getShortestPath(finalDestination).firstOrNull()
-                    ?: throw UnreachableDestinationException()
+                    ?: throw UnreachableDestinationException("$unit ${unit.currentTile.position} cannot reach $finalDestination")
 
         // we should be able to get there this turn
         if (canMoveTo(finalDestination))
@@ -565,4 +565,4 @@ class PathsToTilesWithinTurn : LinkedHashMap<TileInfo, UnitMovementAlgorithms.Pa
         }
         return reversePathList.reversed()
     }
-}
+}