Dev RFC: MapUnit and automated - doc and roadmap?

[SomeTroglodyte]

Problem Description

The action and automated fields of MapUnit lack documentation and clarity. I tried to encapsulate - make just the set private, replace with accessors, and build a Kdoc from there - and gave up - I don't know all ramifications, and deducting them from code is effort.

• Is an automated unit supposed to have action == null and automated == true in the near future?
• If the boolean is derived from action at load time - why not for ConnectRoad, while the code that sets ConnectRoad also sets the boolean true?
• What about the action clear in ImprovementPicker - that would leave the boolean on, but likely disturb ConnectRoad?
• Who would advocate making all four of these private and how should the accessor signature look like? My opinion - at least the writing part, the knowledge about which combinations are good / which are not should be encapsulated. But to do that I'd need help with what the intentions actually are.

Code places to look into

Index: core/src/com/unciv/ui/screens/pickerscreens/ImprovementPickerScreen.kt
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/com/unciv/ui/screens/pickerscreens/ImprovementPickerScreen.kt b/core/src/com/unciv/ui/screens/pickerscreens/ImprovementPickerScreen.kt
--- a/core/src/com/unciv/ui/screens/pickerscreens/ImprovementPickerScreen.kt	(revision 3cd45dea458eda16afb54b6f219cf3a3ded11ca4)
+++ b/core/src/com/unciv/ui/screens/pickerscreens/ImprovementPickerScreen.kt	(date 1719217196700)
@@ -65,7 +65,7 @@
                 if (secondImprovement != null)
                     tile.queueImprovement(secondImprovement, currentPlayerCiv, unit)
             }
-            unit.action = null // this is to "wake up" the worker if it's sleeping
+            if (unit.isSleeping()) unit.action = null // "wake up" the worker
             onAccept()
         }
         game.popScreen()
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 3cd45dea458eda16afb54b6f219cf3a3ded11ca4)
+++ b/core/src/com/unciv/logic/map/mapunit/MapUnit.kt	(date 1719222664988)
@@ -58,7 +58,12 @@
     var health: Int = 100
     var id: Int = Constants.NO_ID
 
-    // work, automation, fortifying, ...
+    /** work, exploring, fortifying, ... This should be one of the [UnitActionType.value] entries.
+     *  - automation is a special case (TODO explain)
+     *  @see automated
+     *  @see automatedRoadConnectionPath
+     *  @see automatedRoadConnectionDestination
+     */
     // Connect roads implies automated is true. It is specified by the action type.
     var action: String? = null
     var automated: Boolean = false
@@ -621,7 +626,8 @@
                 ?: throw java.lang.Exception("Unit $name is not found!")
 
         updateUniques()
-        if (action == UnitActionType.Automate.value) automated = true
+        // Temporary, for compatibility - we want games serialized *moving through old versions* to come out the other end with units still automated
+        if (action == UnitActionType.Automate.value || action == UnitActionType.ConnectRoad.value) automated = true
     }
 
     fun updateUniques() {

[yairm210]

Automated units are supposed to have only "automated true" set, not the action - this is so that automated units can have a state, like "set up", "fortified", etc, without taking them out of automation

[SomeTroglodyte]

So UnitActionType.ConnectRoad isn't supposed to be treated as automated?

[yairm210]

ConnectRoad is a specific action we ask the unit to do. You could consider that automation or not, that's semantics. What the unit is not, is Automated, with a capital A. Capital A Automated units are set into that mode by the user, and from then on do whatever they think is best. In fact an automated unit could decide that it needs to connect a road and then it's be doubly automated

[SomeTroglodyte]

What the unit is not, is Automated, with a capital A

Then the connect road code is bugged in a minor way - perfect, all perceived inconsistencies explained.

[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.

[github-actions[bot]]

This issue was closed because it has been stalled for 5 days with no activity.