I have suspicions about WorkBoat construction AI

[SomeTroglodyte]

Questions about ConstructionAutomation.addWorkBoatChoice:

• alreadyHasWorkBoat doesn't test whether it's your WorkBoat...
• Testing the Unique in the alreadyHasWorkBoat tile search - I get it, the capability may not come from the BaseUnit. But a .baseunit in buildableWorkboatUnits.toSet could be much faster...
• alreadyHasWorkBoat searches only over getTiles - what if it's a new city close to an older one with tons of tiles owned that other city, inside the new city workable range but outside the old city workable range??? I'd say looking over tilesInRange would be more in line with the original intention? As in is there a unit close enough to improve our workable tiles - won't matter who or what city the tile belongs to, as it can move there?
• Mods with military units having CreateWaterImprovements??
• ~Isn't if (!alreadyHasWorkBoat) return the wrong way 'round????????~
• It counts owned, unimproved, reachable, and improveable tiles as worth to send a boat to - even it it's a Bonus resource outside any city workable range

[hackedpassword]

CreateWaterImprovements

But aren't civilians hardcored to only be able to construct? (observation not code analysis)

Being able to convert a military unit to civilian, or vice versa has mod value. If that is possible, I wouldn't "fix" it unless actual problems are resulting.

[SeventhM]

But aren't civilians hardcored to only be able to construct?

I'm not sure where this assumption comes from

[SomeTroglodyte]

• Isn't if (!alreadyHasWorkBoat) return the wrong way 'round????????

It's the right way 'round, but the flag's name is the wrong way 'round, both in and out. It's on if the ruleset has some workboat unit and none was found with a limited BFS. So it's correct to bail if it's off.

I still have to add another point to the list above...

[SomeTroglodyte]

Demo for that last point:

• 1= Capital had the Hubble Telescope as only queue entry and almost done. Next turn finished it - and logs two notifications.
• 2= This Work boat is not seen as "I already have one"
• 3= But this tile is seen as needing a Work boat Thus, you can see, the automation chose a Work boat over Bomb shelter, SS Booster, or Carrier - the other candidates that got a relativeCostEffectiveness weight.

[SeventhM]

Uh... Did you upload the wrong image?

[SomeTroglodyte]

Thanks. Fixed. Gimp and its virtual layers for text that you never know how to finalize - flatten does it but it's not always evident...

[SomeTroglodyte]

Anyway - any coder welcome to eval this patch:

Index: core/src/com/unciv/logic/city/CityConstructions.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/logic/city/CityConstructions.kt b/core/src/com/unciv/logic/city/CityConstructions.kt
--- a/core/src/com/unciv/logic/city/CityConstructions.kt	(revision 6d4603431bb2190d4df1ac6f156818d7670f6d8e)
+++ b/core/src/com/unciv/logic/city/CityConstructions.kt	(date 1696793673582)
@@ -31,7 +31,6 @@
 import com.unciv.models.stats.Stats
 import com.unciv.models.translations.tr
 import com.unciv.ui.components.Fonts
-import com.unciv.ui.components.extensions.addToMapOfSets
 import com.unciv.ui.components.extensions.withItem
 import com.unciv.ui.components.extensions.withoutItem
 import com.unciv.ui.screens.civilopediascreen.CivilopediaCategories
@@ -350,6 +349,7 @@
                     city.civ.addNotification("No space available to place [${construction.name}] near [${city.name}]",
                         city.location, NotificationCategory.Production, construction.name)
                 }
+                chooseNextConstruction()  // Has the checks for autoAssignCityProduction and isQueueEmptyOrIdle
             }
         }
     }
Index: core/src/com/unciv/logic/automation/city/ConstructionAutomation.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/logic/automation/city/ConstructionAutomation.kt b/core/src/com/unciv/logic/automation/city/ConstructionAutomation.kt
--- a/core/src/com/unciv/logic/automation/city/ConstructionAutomation.kt	(revision 6d4603431bb2190d4df1ac6f156818d7670f6d8e)
+++ b/core/src/com/unciv/logic/automation/city/ConstructionAutomation.kt	(date 1696796615511)
@@ -14,6 +14,7 @@
 import com.unciv.models.ruleset.MilestoneType
 import com.unciv.models.ruleset.PerpetualConstruction
 import com.unciv.models.ruleset.Victory
+import com.unciv.models.ruleset.tile.ResourceType
 import com.unciv.models.ruleset.unique.StateForConditionals
 import com.unciv.models.ruleset.unique.UniqueType
 import com.unciv.models.stats.Stat
@@ -158,11 +159,11 @@
                 it.hasUnique(UniqueType.CreateWaterImprovements)
                     && Automation.allowAutomatedConstruction(civInfo, city, it)
             }.filterBuildable()
-        val alreadyHasWorkBoat = buildableWorkboatUnits.any()
-            && !city.getTiles().any {
+        val alreadyHasWorkBoat = buildableWorkboatUnits.none()
+            || city.getTiles().any {
                 it.civilianUnit?.hasUnique(UniqueType.CreateWaterImprovements) == true
             }
-        if (!alreadyHasWorkBoat) return
+        if (alreadyHasWorkBoat) return
 
 
         val bfs = BFS(city.getCenterTile()) {
@@ -170,14 +171,22 @@
         }
         repeat(20) { bfs.nextStep() }
 
-        if (!bfs.getReachedTiles()
-            .any { tile ->
+        val goodTilesFilter = bfs.getReachedTiles().asSequence()
+            .filter {tile ->
+                // Look for owned tiles with unimproved resource
                 tile.hasViewableResource(civInfo) && tile.improvement == null && tile.getOwner() == civInfo
-                        && tile.tileResource.getImprovements().any {
+            }.filter { tile ->
+                // Limit to those we can improve
+                tile.tileResource.getImprovements().any {
                     tile.improvementFunctions.canBuildImprovement(tile.ruleset.tileImprovements[it]!!, civInfo)
                 }
+            }.filter { tile ->
+                tile.tileResource.resourceType != ResourceType.Bonus
+                || tile.getTilesInDistance(3).any {
+                    it.isCityCenter() && it.getOwner() == civInfo
+                }
             }
-        ) return
+        if (goodTilesFilter.none()) return
 
         addChoice(
             relativeCostEffectiveness, buildableWorkboatUnits.minByOrNull { it.cost }!!.name,

... started out as attempt to fix #10258, which it does, but with roughly 90% certainty the wrong way. Testing led to looking at ConstructionAutomation.addWorkBoatChoice _again_... Patch also only addresses the last points.

[SeventhM]

started out as attempt to fix #10258

Ugh... Wait, I'm knee deep checking something else rn, but why is this in constructIfEnough? Seems to me free buildings from policies will come along and trample over this anyways because I added the call to validate the construction queue (in case you get a free building that was building, which was a missed bug before to my knowledge). Shouldn't this be in addBuilding?

[SomeTroglodyte]

Yeah exactly - I grok'ed that the validate the construction queue call might drive around the automation call from remove, but haven't really got a clue how the code flow really is and how it should be...

[SomeTroglodyte]

Anyway, that one line is the kludge that allowed to test the AI, and it threw me that it insisted on useless workboats. That patch is still far from good, it probably needs to make that BFS supply scope for both the need test and the 'already covered' test - I just saw it automate a WB for a tile where the automation already had just finished one in the closer city. Locked closed seas, no chance to mistake, there was only the one unimproved tile in the entire ocean.

[SeventhM]

but haven't really got a clue how the code flow really is and how it should be...

Doing some quick searches, chooseNextConstruction is called in CityConstuctionTable.purchaseConstruction and CityConstuctions.removeFromQueue. It seems obvious that it's supposed to be called at the start of a turn to decide if it still makes sense (ideally before stuff is calculated to have the same information as a player) and after anything changes the calculation mid turn (through something being built). Seems to me that if you put it at the bottom of addBuilding, it'll cover these situations as intended with even less side effects (removeFromQueue is called before validating anyways in constructionComplete, so it never actually worked as intended and purchaseConstrcution checks for the improvement set for some reason when I swear chooseNextConstruction already dealt with...)

[SeventhM]

~~Actually, maybe also put it in BaseUnit.postBuildEvent to keep correct functionality~~

Edit: I'm dumb, that's still before validation. It would need to be in the validation code tbh

[SomeTroglodyte]

Well, if you think you have a clear "big picture".... Me, I'll pause after a good git gc --prune=now --aggressive

[rmingkil71]

Bugs and other bugs

[rmingkil71]

Applicatio@n

[github-actions[bot]]

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 15 days.