[Freeciv-Dev] (PR#3524) another look at the goto_dest problem

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

[glip - Wed Feb 26 11:55:30 2003]:

> Questions to Jason:
> 
> Do you get many cores with this patch?  If you do, you should file them, 
> as they are probably real bugs.

Just one: when removing a city from the minimap, sometimes the
assert(is_normal_map_pos) check will fail.  This is because the caller
has already reset the goto_dest, and is definitely a "real" bug.

Here is a possible fix for that bug, which incorporates the setting of
goto_dest into ai_unit_new_role.  This change has been in the works for
some time, AFAICT, and it was accomplished by Per's patch as well.  The
only possible problem is that some callers give ai_unit_new_role a
destination position, but don't actually set the goto_dest; now the
goto_dest will be set.  It seems likely that this is an improvement, but
it is hard to be sure that it won't break anything.

I suspect that any reasonable-sized patch we apply at this point will
have some chance of breaking something.  I suggest that the current code
is already broken, and that this is an acceptable risk.

jason

? _g
? diff
? ggz.diff
? lib
? nohup.out
? rc
? rc2
? ss
? test-game.gz
? test.pl
? common/fciconv.c
? common/fciconv.h
? data/civclient.dsc
? data/civclient.dsc.in
? data/civserver.dsc
? data/civserver.dsc.in
? data/civserver.room
? data/hills.spec
? data/mountains.spec
? data/theme
? data/nation/convert.pl
? data/trident/.new.tiles.xpm
? m4/ggz.m4
? m4/xaw-client.m4
Index: ai/aidiplomat.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aidiplomat.c,v
retrieving revision 1.13
diff -u -r1.13 aidiplomat.c
--- ai/aidiplomat.c     2003/02/17 22:49:27     1.13
+++ ai/aidiplomat.c     2003/02/26 23:33:19
@@ -630,9 +630,6 @@
       return;
     }
 
-    /* TODO: following lines to be absorbed in a goto wrapper */
-    punit->goto_dest_x = ctarget->x;
-    punit->goto_dest_y = ctarget->y;
     ai_unit_new_role(punit, task, ctarget->x, ctarget->y);
   }
 
Index: ai/aitools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aitools.c,v
retrieving revision 1.80
diff -u -r1.80 aitools.c
--- ai/aitools.c        2003/02/17 22:49:27     1.80
+++ ai/aitools.c        2003/02/26 23:33:20
@@ -206,9 +206,18 @@
   punit->ai.charge = BODYGUARD_NONE;
 
   punit->ai.ai_role = task;
-/* TODO:
-  punit->goto_dest_x = x;
-  punit->goto_dest_y = y; */
+
+  /* Verify and set the goto destination.  Eventually this can be a lot more
+   * stringent, but for now we don't want to break things too badly. */
+  if (x == -1 && y == -1) {
+    /* No goto_dest. */
+    punit->goto_dest_x = 0;
+    punit->goto_dest_y = 0;
+  } else {
+    assert(is_normal_map_pos(x, y));
+    punit->goto_dest_x = x;
+    punit->goto_dest_y = y;
+  }
 
   if (punit->ai.ai_role == AIUNIT_NONE && bodyguard) {
     ai_unit_new_role(bodyguard, AIUNIT_NONE, -1, -1);
Index: ai/aiunit.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
retrieving revision 1.265
diff -u -r1.265 aiunit.c
--- ai/aiunit.c 2003/02/23 12:47:35     1.265
+++ ai/aiunit.c 2003/02/26 23:33:27
@@ -2530,8 +2530,6 @@
   punit->ai.passenger = 0;
   UNIT_LOG(LOG_DEBUG, punit, "Ferryboat is lonely.");
   handle_unit_activity_request(punit, ACTIVITY_IDLE);
-  punit->goto_dest_x = 0; /* FIXME: -1 */
-  punit->goto_dest_y = 0; /* FIXME: -1 */
 
   /* Release bodyguard and let it roam */
   ai_unit_new_role(punit, AIUNIT_NONE, -1, -1);