[freeciv-ai] Re: Possible bug in ai_military_gothere

["Per I. Mathisen" <per@xxxxxxxxxxx>]

On Thu, 4 Sep 2003, Jordi Negrevernis i Font wrote:
>   I just touched the ai_military_gothere() function to make sure that
> the unit get off the boat where it has arrived, and just touched the
> clients of find_beachhead and find_city_beach...
>
>   I played two autogames and the server did not crash, and i think it
> does what is suposed to do.

+++ freeciv-cvs-Aug-24-ferryboat/ai/aiunit.c    Thu Sep  4 15:25:08 2003
@@ -1484,7 +1485,16 @@
       ferryboat->ai.passenger = punit->id;

       /* Last ingredient: a beachhead. */
-      if (find_beachhead(punit, dest_x, dest_y, &boat_x, &boat_y)) {
+      if (!find_beachhead(punit, dest_x, dest_y, &boat_x, &boat_y)) {
+        /* we try the other function */
+        if (dcity) {
+          find_city_beach(dcity, punit, &boat_x, &boat_y);
+        } else {
+          /* the previous function failed, so... */
+          boat_x = punit->x; boat_y = punit->y;
+        }
+      }

Better:

if (!find_beachhead(punit, dest_x, dest_y, &boat_x, &boat_y)
    && dcity) {
  /* we try the other function */
  find_city_beach(dcity, punit, &boat_x, &boat_y);
}

+      if (!same_pos(punit->x, punit->y, boat_x, boat_y)) {

This is not good. I think the code below should be run in any case, since
we need to sentry and set variables.

+  if (ferryboat) {
+    /* we go on a ferry! did we arrive? */
+    boat_arrived = same_pos(ferryboat->x, ferryboat->y, 
ferryboat->goto_dest.x, ferryboat->goto_dest.y) ||
+                   is_tiles_adjacent(ferryboat->x, ferryboat->y, 
ferryboat->goto_dest.x, ferryboat->goto_dest.y);

Style nitpick: The || should be in front of the second conditional above.

+  } else {
+    boat_arrived = FALSE;
   }

-  if (goto_is_sane(punit, dest_x, dest_y, TRUE) && punit->moves_left > 0
-      && (!ferryboat
-          || (real_map_distance(punit->x, punit->y, dest_x, dest_y) < 3
-              && (punit->ai.bodyguard == BODYGUARD_NONE
+  if (goto_is_sane(punit, dest_x, dest_y, TRUE) && punit->moves_left > 0
+      && (!ferryboat
+          || ( boat_arrived
+              && (punit->ai.bodyguard == BODYGUARD_NONE

Can you explain this change?

     /* if we are on a boat, disembark only if we are within two tiles of
-     * our target, and either 1) we don't need a bodyguard, 2) we have a
-     * bodyguard, or 3) we are going to an empty city.  Previously, cannons
-     * would disembark before the cruisers arrived and die. -- Syela */
+     * our beach head near the target, and either 1) we don't need a bodyguard,
+     * 2) we have a bodyguard, or 3) we are going to an empty city. Previously,
+     * cannons would disembark before the cruisers arrived and die. -- Syela */

If you change comments attributed to an author (in this case syela), then
remove the attribution. We should never put words in anyone else's mouth.

-            /* the ferryboat needs to target the beachhead, but the unit
-             * needs to target the city itself.  This is a little weird,
+            /* the ferryboat needs to target the beachhead, but the unit
+             * needs to target the city itself.  This is a little weird,

Spurious diff? I see no differences.

              * but it's the best we can do. -- Syela */
-          } /* else do nothing, since we have no beachhead */
+          } else {
+            /* try to use find_city_beach */
+            find_city_beach(acity, punit, &xx, &yy);
+            if (!same_pos(punit->x, punit->y, xx, yy)) {
+              /* we find beach head, same comment as above! */
+              best = want;
+              *x = acity->x;
+              *y = acity->y;
+            }
+          }

Is the !same_pos() above necessary?

         if (!find_beachhead(punit, pc->x, pc->y, &fx, &fy)) {
           find_city_beach(pc, punit, &fx, &fy);
-          freelog(LOG_DEBUG, "Barbarian sailing to city");
-          ai_military_gothere(pplayer, punit, fx, fy);
-       }
+        }
+        freelog(LOG_DEBUG, "Barbarian sailing to city");
+        ai_military_gothere(pplayer, punit, fx, fy);
       }
     }

Ouch. This fixes a rather nasty bug with barbarians, I think. My fault, I
guess.

Lastly, can you please post some savegames to show that it used to do the
wrong thing but now does the right thing? Civworld is your friend.

  - Per