(PR#13156) auto_settler_findwork infinite recursion

This is a multi-part message in MIME format...

------------=_1118539999-18827-3
Content-Type: text/plain; charset="utf-8"


<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


------------=_1118539999-18827-3
Content-Type: text/x-patch; charset="ascii"; name="autosettlers.diff"
Content-Disposition: inline; filename="autosettlers.diff"
Content-Transfer-Encoding: 7bit

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;

------------=_1118539999-18827-3--