[freeciv-ai] (PR#13156) auto_settler_findwork infinite recursion

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=13156 >

And, here's a fix.

The problem was in the lines:

              if (best_newv > old_best_value) {
                *travel_time = mv_turns;
              }

the conditional here is wrong/incomplete.  Thus the travel_time was
sometimes not being set, and since it's the travel time that's used to
determine displacement this could cause an infinitely recursing
displacement.

This patch fixes it.  consider_settler_action returns TRUE if the action
is accepted, and it's this return value that is checked.  I also added
lots of assertions, logs, calculation of recursiveness (based on Marko's
patch), and it will recover from an infinite recursion by dismissing the
autosettler (plus an assertion).

One thing I haven't thought about is the other consider_settler_action
callers.  They probably need fixing as well; I'll look at this tomorrow.

-jason

Index: server/settlers.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/settlers.c,v
retrieving revision 1.239
diff -u -r1.239 settlers.c
--- server/settlers.c   7 Jun 2005 16:55:00 -0000       1.239
+++ server/settlers.c   12 Jun 2005 01:29:17 -0000
@@ -707,8 +707,10 @@
   at (x,y) with activity act.  Calculates the value of improving the tile
   by discounting the total value by the time it would take to do the work
   and multiplying by some factor.
+
+  Returns TRUE iff the action is accepted.
 ****************************************************************************/
-static void consider_settler_action(struct player *pplayer, 
+static bool consider_settler_action(struct player *pplayer, 
                                     enum unit_activity act, int extra, 
                                     int new_tile_value, int old_tile_value,
                                    bool in_use, int delay,
@@ -756,7 +758,9 @@
     *best_old_tile_value = old_tile_value;
     *best_act = act;
     *best_tile = ptile;
+    return TRUE;
   }
+  return FALSE;
 }
 
 /**************************************************************************
@@ -890,6 +894,15 @@
                && (real_map_distance(ptile, punit->tile)
                    < inbound_distance))) {
 
+         if (enroute) {
+           UNIT_LOG(LOG_DEBUG, punit,
+                    "Considering %d,%d because we're closer "
+                    "(%d,%d) than %d (%d,%d)",
+                    TILE_XY(ptile), mv_turns,
+                    real_map_distance(ptile, punit->tile),
+                    enroute->id, eta, inbound_distance);
+         }
+
          /* now, consider various activities... */
          activity_type_iterate(act) {
            if (pcity->ai.act_value[act][cx][cy] >= 0
@@ -897,7 +910,6 @@
                                                    S_LAST, ptile)) {
              int extra = 0;
              int base_value = pcity->ai.act_value[act][cx][cy];
-             int old_best_value = best_newv;
 
              time = mv_turns + get_turns_for_activity_at(punit, act, ptile);
              
@@ -930,15 +942,15 @@
                extra = pplayer->ai.warmth;
              }    
              
-             consider_settler_action(pplayer, act,
-                                     extra, 
-                                     base_value, oldv, 
-                                     in_use, time,
-                                     &best_newv, &best_oldv,
-                                     best_act, best_tile,
-                                     ptile);
-             
-             if (best_newv > old_best_value) {
+             if (consider_settler_action(pplayer, act,
+                                         extra, 
+                                         base_value, oldv, 
+                                         in_use, time,
+                                         &best_newv, &best_oldv,
+                                         best_act, best_tile,
+                                         ptile)) {
+               UNIT_LOG(LOG_DEBUG, punit, "time to get to %d,%d: %d",
+                        TILE_XY(*best_tile), mv_turns);
                *travel_time = mv_turns;
              }
 
@@ -977,7 +989,8 @@
 #define LOG_SETTLER LOG_DEBUG
 static void auto_settler_findwork(struct player *pplayer, 
                                  struct unit *punit,
-                                 struct settlermap *state)
+                                 struct settlermap *state,
+                                 int recursion)
 {
   struct cityresult result;
   int best_impr = 0;            /* best terrain improvement we can do */
@@ -988,6 +1001,14 @@
   /* time it will take worker to complete its given task */
   int completion_time = 0;
 
+  if (recursion > unit_list_size(pplayer->units)) {
+    assert(recursion <= unit_list_size(pplayer->units));
+    ai_unit_new_role(punit, AIUNIT_NONE, NULL);
+    set_unit_activity(punit, ACTIVITY_IDLE);
+    send_unit_info(NULL, punit);
+    return; /* avoid further recursion. */
+  }
+
   CHECK_UNIT(punit);
 
   result.total = 0;
@@ -1093,12 +1114,27 @@
       struct unit *displaced
        = player_find_unit_by_id(pplayer, state[best_tile->index].enroute);
 
+      if (displaced) {
+       assert(state[best_tile->index].enroute == displaced->id);
+       assert(state[best_tile->index].eta > completion_time
+              || (state[best_tile->index].eta == completion_time
+                  && (real_map_distance(best_tile, punit->tile)
+                      < real_map_distance(best_tile, displaced->tile))));
+       UNIT_LOG(LOG_DEBUG, punit,
+                "%d (%d,%d) has displaced %d (%d,%d) on %d,%d",
+               punit->id, completion_time,
+               real_map_distance(best_tile, punit->tile),
+               displaced->id, state[best_tile->index].eta,
+               real_map_distance(best_tile, displaced->tile),
+               TILE_XY(best_tile));
+      }
+
       state[best_tile->index].enroute = punit->id;
       state[best_tile->index].eta = completion_time;
       
       if (displaced) {
        displaced->goto_tile = NULL;
-       auto_settler_findwork(pplayer, displaced, state);
+       auto_settler_findwork(pplayer, displaced, state, recursion + 1);
       }
     } else {
       UNIT_LOG(LOG_DEBUG, punit, "giving up trying to improve terrain");
@@ -1120,7 +1156,7 @@
 
   if (punit->ai.ai_role == AIUNIT_BUILD_CITY
       && punit->moves_left > 0) {
-    auto_settler_findwork(pplayer, punit, state);
+    auto_settler_findwork(pplayer, punit, state, recursion + 1);
   }
 }
 #undef LOG_SETTLER
@@ -1257,7 +1293,7 @@
         handle_unit_activity_request(punit, ACTIVITY_IDLE);
       }
       if (punit->activity == ACTIVITY_IDLE) {
-        auto_settler_findwork(pplayer, punit, state);
+        auto_settler_findwork(pplayer, punit, state, 0);
       }
     }
   } unit_list_iterate_end;