http://bugs.freeciv.org/Ticket/Display.html?id=39792 >
Unfortunately, it appears that the activity and explore code has so many
problems that it's not practical to completely fix at this time.
One of the most egregious problems is that folks didn't always use access
functions for setting activities, and/or didn't uniformly use the same
functions. In this case, one code path set ACTIVITY_IDLE directly with
set_unit_activity(), while another used an intermediate function that in
turn calls unit_activity_dependencies(). They achieve different results
There are far too many such paths used for all activities. This will need
considerable cleanup to find them all, such as PR#39870.
With 24 set_unit_activity() call variants, 44 unit_activity_handling()
call variants, and 31 ai.done references, not to mention 198 ai.control
references, taming this beast will take some doing!
===
I've checked in this pair of patches (for 2.1 and 2.2) to at least allow
the current savegame(s) to go forward without crashing.
First and foremost, combine the similar code paths into do_explore(), which
will indirectly call unit_activity_dependencies().
Secondly, but perhaps more importantly, use enum unit_move_result returns
for ai_manage_explorer(), and pass along the new MR_DEATH result. This
eliminates several crashing bugs where ai.done was set for dead units.
More FIXME added for future work!
Committed S2_1 revision 14246.
Committed S2_2 revision 14247.
Committed trunk revision 14248.
Index: server/unittools.c
===
--- server/unittools.c (revision 14245)
+++ server/unittools.c (working copy)
@@ -652,24 +652,7 @@
unit_restore_movepoints(pplayer, punit);
if (activity == ACTIVITY_EXPLORE) {
-bool more_to_explore = ai_manage_explorer(punit);
-
-if (!player_find_unit_by_id(pplayer, id)) {
- /* Died */
- return;
-}
-
-/* ai_manage_explorer isn't supposed to change the activity but we
- * don't count on this. */
-if (punit->activity != ACTIVITY_EXPLORE || !more_to_explore) {
- handle_unit_activity_request(punit, ACTIVITY_IDLE);
-
- /* FIXME: When the ai_manage_explorer call changes the activity from
- * EXPLORE to IDLE, then for some reason the ai.control value gets left
- * set. We reset it here. See PR#12931. */
- punit->ai.control = FALSE;
-}
-send_unit_info(NULL, punit);
+do_explore(punit);
return;
}
@@ -2041,6 +2024,35 @@
}
/**
+ ...
+**/
+void do_explore(struct unit *punit)
+{
+ switch (ai_manage_explorer(punit)) {
+ case MR_DEATH:
+/* don't use punit! */
+return;
+ case MR_OK:
+/* FIXME: ai_manage_explorer() isn't supposed to change the activity,
+ * but don't count on this. See PR#39792.
+ */
+if (punit->activity == ACTIVITY_EXPLORE) {
+ break;
+}
+/* fallthru */
+ default:
+handle_unit_activity_request(punit, ACTIVITY_IDLE);
+
+/* FIXME: When the ai_manage_explorer() call changes the activity from
+ * EXPLORE to IDLE, in unit_activity_handling() ai.control is left
+ * alone. We reset it here. See PR#12931. */
+punit->ai.control = FALSE;
+break;
+ };
+ send_unit_info(NULL, punit); /* probably duplicate */
+}
+
+/**
Returns whether the drop was made or not. Note that it also returns 1
in the case where the drop was succesful, but the unit was killed by
barbarians in a hut.
Index: server/unittools.h
===
--- server/unittools.h (revision 14245)
+++ server/unittools.h (working copy)
@@ -81,6 +81,7 @@
/* doing a unit activity */
void do_nuclear_explosion(struct player *pplayer, struct tile *ptile);
bool do_airline(struct unit *punit, struct city *city2);
+void do_explore(struct unit *punit);
bool do_paradrop(struct unit *punit, struct tile *ptile);
void load_unit_onto_transporter(struct unit *punit, struct unit *ptrans);
void unload_unit_from_transporter(struct unit *punit);
Index: server/unithand.c
===
--- server/unithand.c (revision 14245)
+++ server/unithand.c (working copy)
@@ -591,34 +591,35 @@
return;
}
- if (punit->activity != activity ||
- punit->activity_target != activity_target ||
- punit->ai.control) {
+ if (punit->activity == activity
+ && punit->activity_target == activity_target
+ && !punit->ai.control) {
/* Treat change in ai.control as change in activity, so
- * idle autosettlers behave correctly when selected --dwp
+ * idle autosettlers behave correctly when selected --dwp
*/
-punit->ai.control = FALSE;
-punit->goto_tile = NULL;
+return;
+ }
+
+ punit-
