[Freeciv-Dev] Re: Safe paths for triremes etc. (PR#1007)

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 12:52 PM 01/10/12 -0700, Gregory Berkolaiko wrote:
>See warmap_split7.readme for the info on the patch.
>
>It is big but at least the changes are localised, more or less.
>
>Comments are most welcome.
>
>Best,
>G.
>
>
>____________________________________________________________
>Do You Yahoo!?
>Get your free @yahoo.co.uk address at http://mail.yahoo.co.uk
>or your free @yahoo.ie address at http://mail.yahoo.ie
>Attachment Converted: "c:\program files\eudora\attach\warmap_split7.readme"
>
>Attachment Converted: "c:\program files\eudora\attach\warmap_split7.diff.gz"


Sorry, for the delay, but I just got my machine back from Windows where it
was busy doing something else for the week.

I have code reviewed the patch. The comments are imbedded with liberal 
truncation of the diff as I went along.

Capsule summary: I found 2 possible bugs one of which isn't yours, but 
most of the issues raised are sufficiently murky that clarification or 
justification is more the concern than overt flaws.

The bugs (i.e. need for some real code updates) are in the do_unit_goto 
status checking, and the disembark cost into a city.

I then applied and ran it (without faulting in a server autogame) but 
haven't properly beat on it or looked too deeply into the results. In 
client autogame, explorer units seem to be skirting the edge as opposed
to driving right into the black when they circle back. Triremes head out
a few squares, then either park themselves or head home and park there.
They don't seem to have much courage or incentive to do much.

The code seems to be pretty stable and functional.

Cheers,
RossW
=====

> diff -ur --ignore-space-change -X freeciv/diff_ignore freeciv/ai/aiunit.c
freeciv_mod/ai/aiunit.c
> --- freeciv/ai/aiunit.c       Fri Sep 28 00:49:50 2001
> +++ freeciv_mod/ai/aiunit.c   Thu Oct 11 13:09:58 2001
> @@ -205,50 +202,44 @@
>    if (is_ground_unit(punit)) con = map_get_continent(x, y);
>    else con = 0; /* Thanks, Tony */
>  
> -  /* CPU-expensive but worth it -- Syela */
> -  generate_warmap(map_get_city(x, y), punit);
> -
>    /* BEGIN PART ONE: Look for huts.  Non-Barbarian Ground units ONLY. */
>    if (!is_barbarian(pplayer)
>        && is_ground_unit(punit)) { /* boats don't hunt huts */
> -    int maxcost = pplayer->ai.control ? 2 * THRESHOLD : 3;
> -    int bestcost = maxcost * SINGLE_MOVE + 1;
> +    /* maxcost is maximum distnace of search * SINLGE_MOVE */ 
> +    int maxcost = pplayer->ai.control ? 2 * THRESHOLD : 3 * SINGLE_MOVE; 
> +    enum goto_result gotores= GR_FAILED;
>  
>      /* Iterating outward so that with two tiles with the same movecost
>         the nearest is used */
> -    iterate_outward(x, y, maxcost, x1, y1) {

Two questions here ... besides the SINLGE typo.

AI maxcost is not quite the same as bestcost, missing the SINGLE_MOVE.
(The bestcost +1 went away, but I assume that was part of the previous
less than collection). Why the change in the THRESHOLD case?

Iterating outward tried to preserve physical as well as move_cost
proximity. Presumably, you can now send a unit 6 road moves away, as
opposed to 1 forest move. Why not try to preserve the original concept
i.e. continue collecting for equivalent move_costs and pick the one
with the smallest real_map_distance()?

> +    if (warmap_initialize(NULL, punit, maxcost) == 0) {
> +      freelog(LOG_FATAL, "Cannot initialize warmap.  Call your dealer.");
> +      abort();
> +    }
> +    while(warmap_next_location(&x1,&y1)){
>        if (map_get_special(x1, y1) & S_HUT
> -       && warmap.cost[x1][y1] < bestcost
>         && (!ai_handicap(pplayer, H_HUTS) || map_get_known(x1, y1, pplayer))
>         && tile_is_accessible(punit, x1, y1)
>         && ai_fuzzy(pplayer, 1)) {
> -     best_x = x1;
> -     best_y = y1;
> -     bestcost = warmap.cost[best_x][best_y];
> -      }
> -    } iterate_outward_end;
> -    if (bestcost <= maxcost * SINGLE_MOVE) {
> -      punit->goto_dest_x = best_x;
> -      punit->goto_dest_y = best_y;
> +     punit->goto_dest_x= x1;
> +     punit->goto_dest_y= y1;
>        set_unit_activity(punit, ACTIVITY_GOTO);
> -      do_unit_goto(punit, GOTO_MOVE_ANY, 0);
> -      if (!player_find_unit_by_id(pplayer, id))
> +     gotores= do_unit_goto(punit, GOTO_MOVE_ANY, 0);
> +     break;
> +      }      
> +      warmap_process_location(x1,y1);
> +    }
> +    if (gotores == GR_DIED)
>       return 0; /* died */
>  
> -      if (punit->moves_left) {
> -     if (punit->x == best_x && punit->y == best_y) {
> +    if (gotores == GR_ARRIVED)
>         return ai_manage_explorer(punit);
> -     } else {
> -       /* Something went wrong; fall through. This should almost never
happen. */
> -       if (punit->x != x || punit->y != y)
> -         generate_warmap(map_get_city(punit->x, punit->y), punit);

Note: The original flow initialized the warmap once, and reused it unless
      there was a GOTO move that changed starting position, like here.

Can you explain why 1) you need to do it this way, and 2) which way you
think is more cost effective plus why?

> -       x = punit->x; y = punit->y;

The style police will give you a hard time here. Use a comma operator to
give yourself a fighting chance. Do the same thing above with punit to
show that you are consistent in your aggravation of their sensibilities.

> -       /* Fallthough to next fase */
> -     }
> -      } else {
> +
> +    if (gotores == GR_OUT_OF_MOVEPOINTS)
>       return 1;
> -      }
> -    }
> +     
> +    /* Didn't find the hut or found but couldn't get to */
> +    x = punit->x; y = punit->y;
> +    /* Fallthough to next phase */
>    }
>  
>    /* BEGIN PART TWO: Move into unexplored territory */

I have this sneaking feeling that PART TWO is now redundant if you are
actually using the warmap to properly search in closest to farthest 
order, and not generating any more of the (expensive) part than you
actually need. Comments?

> @@ -312,37 +307,38 @@
>         if (!map_get_known(x2, y2, pplayer))
>           unknown++;
>       } square_iterate_end;
> -     if (unknown) {
> -       if (is_sailing_unit(punit))
> -         unknown += 9 * (threshold - warmap.seacost[x1][y1]);
> -       else
> -         unknown += 9 * (threshold - warmap.cost[x1][y1]);

This has the effect of artificially boosting the "unknownness" of nearby
tiles, presumably so that you clean up odd patches before hiving off into
the bigger black. But it doesn't altogether stop you from hiving off.

Your algorithm will meticulously mop up by going for the first unknown, 
regardless of the "unknowness" weight or even closeness.

Moreover, because the move_cost is negative, the above will stop at
something shortly after the threshold but still dependent on how much
unknownness is there, while yours will stop dead at threshold or go on
to the otherside of the world depending on what your warmap_initialize
actually does? (A comment about how thresholds are implemented somewhere
near here wouldn't hurt).

How would you change yours to better follow the original? Why wouldn't
you? or maybe, why do you think this is better?

> -       if ((unknown > most_unknown || (unknown == most_unknown && myrand(2)))
> -           && !(is_barbarian(pplayer) && ptile->special & S_HUT)) {
> -         best_x = x1;
> -         best_y = y1;
> -         most_unknown = unknown;
> +     if (unknown && !(is_barbarian(pplayer) && ptile->special & S_HUT)) {
> +       punit->goto_dest_x= x1;
> +       punit->goto_dest_y= y1;
> +       handle_unit_activity_request(punit, ACTIVITY_GOTO);
> +       gotores= do_unit_goto(punit, GOTO_MOVE_ANY, 0);
> +       break;
>         }
>       }
> +      warmap_process_location(x1,y1);
>        }
> -    } whole_map_iterate_end;
>  
> -    if (most_unknown > 0) {
> -      punit->goto_dest_x = best_x;
> -      punit->goto_dest_y = best_y;
> -      handle_unit_activity_request(punit, ACTIVITY_GOTO);
> -      do_unit_goto(punit, GOTO_MOVE_ANY, 0);
> -      if (punit->moves_left) {
> -     if (punit->x != best_x || punit->y != best_y) {
> -       handle_unit_activity_request(punit, ACTIVITY_IDLE);
> -       return 1; /* Something wrong; what to do but return? */
> -     } else
> +    if (gotores == GR_DIED)
> +      return 0; /* died */
> +    
> +    if (gotores == GR_ARRIVED)
>         return ai_manage_explorer(punit);
> -      } else {
> +    
> +    if (gotores == GR_OUT_OF_MOVEPOINTS)
> +      return 1;
> +
> +    if (gotores == GR_FAILED 
> +     && unit_flag(punit, F_TRIREME) && unknown) {

Nit: do the unknown test before the unit_flag - it is less expensive.

> +      /* trireme has less than full movepoints and doesn't want to move
> +       * we could also check for open waters at (x1, y1) or for 
> +       * move_points < max_move_points but our route was safe 
> +       * and the trireme would follow it if could */
> +      freelog(LOG_DEBUG, "%s at (%d,%d) ending move early to keep to
shore",
> +           unit_type(punit)->name, punit->x, punit->y);
> +      /* pretend we have no movepoints left */
>       return 1;
>        }

The original code can return with move points left, not at the final
destination, and not just a trireme. 

Can you explain why this isn't handled explicitly in your code?

ACTIVITY_IDLE is also set in these cases. Why have you dropped this?

Why have you not put these successive if's into a switch with the
default case handled as an `oops, return 1`? It is really not clear here
if you have properly considered the GOTO return status. Also someone
may change or add a new enum, and your code will potentially do a very 
bad thing.

Finally, all these final "if"s really only need to be tested if you go
through the break clause. Why not move them all just before the break
i.e. after the do_unit_goto that they are actually checking and save a 
lot of unnecessary checks or potential mischecks on the miss flow?

This do_unit_goto code also seems to be common and could all perhaps
be moved off to a single location so there is just one place to fix
if anything needs to change in the future, right?

>      } /* no candidates; fall-through */
> -  }
>  
>    /* BEGIN PART FOUR: maybe go to another continent */
>    freelog(LOG_DEBUG, "%s's %s at (%d,%d) failed to explore.",
> diff -ur --ignore-space-change -X freeciv/diff_ignore
freeciv/server/gotohand.c freeciv_mod/server/gotohand.c
> --- freeciv/server/gotohand.c Tue Oct  9 18:20:32 2001
> +++ freeciv_mod/server/gotohand.c     Fri Oct 12 19:08:34 2001

Note: MAXCOST locally defined here, and MOVE_COST_FOR_VALID_SEA_STEP in map.h
      should be move to common/unit.h with the other MOVE_COST #defines.

      MOVE_COST_MAX would be a more compatible name when doing this.


This is a general comment for all the warmap functions below. It would be
best if they all took a "pointer to warmap" as their first argument,
rather than using the global "warmap". warmap_initialize() could return
the pointer to the static warmap if no user one were passed in.
This would permit one to have more than one warmap active at once, or to
save some of the cachable warmaps across intervening other uses.

The current statics are not thread safe, nor particularly good programming
style, and given these changes it would not be difficult to correct this
in passing.


> @@ -406,30 +410,377 @@
>    }
>  }
>  
> +/*-------------------- Possible cost functions -------------------*/
[...]
> +
> +/* LAND_MOVE cost function for a unit */
> +int normal_move_unit(int x, int y, int dir, int x1, int y1)
> +{
> +  struct tile *ptile = map_get_tile(x, y);
> +  int unitmoverate= unit_type(warmap.warunit)->move_rate;
> +  enum tile_terrain_type ter1;
> +
> +  if ((ter1 = map_get_terrain(x1, y1)) == T_OCEAN) {
> +    if (ground_unit_transporter_capacity(x1, y1,
unit_owner(warmap.warunit)) > 0)
> +      return SINGLE_MOVE;
> +    else
> +      return MAXCOST;
> +  } else if (ptile->terrain == T_OCEAN) {
> +    int tmp = get_tile_type(ter1)->movement_cost * SINGLE_MOVE;

This recomputes the cost for disembarking a ship based on the terrain cost
vs using ptile->move_cost[dir].  This should be done correctly in 
tile_move_cost_ai and the lookup used here like everywhere else.  Or more
importantly, these 3 lines go away.

I don't think there is ever a case where moving *from* T_OCEAN to land 
would not be valid. The issue is getting into the T_OCEAN square in the 
first place for a land unit :-). 

But there is an issue here for disembarking into a city. I believe it
should always be a SINGLE_MOVE_COST, no? Plus this is one of the land/sea
overlap situations.

> +    return MIN(tmp, unitmoverate);
> +  } else {
> +    return MIN(ptile->move_cost[dir], unitmoverate);
> +  }
> +}
> +
> +/* IGTER_MOVE cost function for a unit */
> +int igter_move_unit(int x, int y, int dir, int x1, int y1)
> +{
> +  if (map_get_terrain(x1, y1) == T_OCEAN) {
> +    if (ground_unit_transporter_capacity(x1, y1,
unit_owner(warmap.warunit)) > 0)
> +      return SINGLE_MOVE;
> +    else
> +      return MAXCOST;
> +  } else {
> +    return (map_get_tile(x, y)->move_cost[dir] ? MOVE_COST_ROAD : 0);
> +  } 
> +}
> +
> +/* LAND_MOVE cost function for a city ?? */
> +int normal_move_city(void *args, int x, int y, int dir, int x1, int y1)
> +{
> +  /* have no idea what code below is for */
> +  if (map_get_terrain(x1, y1) == T_OCEAN) 
> +    return MAXCOST;
> +  else {
> +    struct tile *ptile = map_get_tile(x, y);
> +    int tmp = map_get_tile(x1, y1)->move_cost[DIR_REVERSE(dir)];
> +    return (ptile->move_cost[dir] + tmp +
> +         (ptile->move_cost[dir] > tmp ? 1 : 0))/2;
> +  }

The weird averaging here needs a comment, at least to highlight the
confusion it causes and the need to come up with any rationale :-).

> +}
> +
> +/*----------------- End of possible cost functions ---------------*/
> +
> +/*----------------- Corrected cost functions ---------------------*/
> +/* These functions are used in shortest path calculation and
> + * are designed to provide corrections to the basic terrain
> + * move cost basing on the ZOC, enemy and FOW considerations */
> +
> +/* LAND MOVE correction */
> +int cor_land_move(COSTFN *basic_cost, int x, int y, int dir, int x1, int
y1)
> +{
[...]
> +  /* basic_cost is pretty damn basic here */
> +  return SINGLE_MOVE;
> +}
> +
> +/*----------------- End of corrected cost functions --------------*/
> +

I know much of this is only copied, but any additional comments in the
cor_<functions> above would be to everyone's benefit.

A brief commentary about the key elements being initialized below and
how they are to be used is perhaps also in order - e.g. the meaning of
maxcost.

>  /**************************************************************************
> -Returns false if you are going the in opposite direction of the
destination.
> -The 3 directions most opposite the one to the target is considered wrong.
> +Initializes warmap for dynamic warmap generation
>  **************************************************************************/
> -static int dir_ok(int src_x, int src_y, int dest_x, int dest_y, int dir)
> +int warmap_initialize(struct city *pcity, struct unit *punit, 
> +                   unsigned char maxcost)
>  {
> -  int diff_x, diff_y, dx, dy, scalar_product;
> +  if (!maxcost) {
> +    maxcost= THRESHOLD * 6 + 2;
> +    if (punit && unit_flag(punit, F_SETTLERS)
> +     && unit_type(punit)->move_rate == SINGLE_MOVE)
> +      maxcost /= 2;
> +  }
> +  warmap.maxcost = maxcost;
>  
> -  if (dest_x > src_x) {
> -    diff_x = dest_x - src_x < map.xsize / 2 ? 1 : -1;
> -  } else if (dest_x < src_x) {
> -    diff_x = src_x - dest_x < map.xsize / 2 ? -1 : 1;
> -  } else {                   /* dest_x == src_x */
> -    diff_x = 0;
> +  if (punit)
> +    pcity= NULL;
> +  warmap.warcity = pcity;
> +  warmap.warunit = punit;
> +  warmap.caching = 0;  /* do not try to re-use this warmap */
> +
> +  if (punit) {
> +    int x= punit->x;
> +    int y= punit->y;
> +    if (is_sailing_unit(punit)) {
> +      int afraid_of_sinking= 
> +     (unit_flag(punit, F_TRIREME)
> +      && !player_owns_active_wonder(unit_owner(punit), B_LIGHTHOUSE));
> +      warmap.costmap = warmap.seacost;
> +      init_warmap(x, y, SEA_MOVING);
> +      if (afraid_of_sinking) {
> +     if (unit_type(punit)->move_rate >= 3*SINGLE_MOVE) {
> +       /* can stray a little bit */
> +       warmap.costfun= healthy_trireme_seamove_strict;
> +     } else {
> +       /* should not stray at all */
> +       warmap.costfun= sick_trireme_seamove_strict;
> +     }
> +      } else {
> +     warmap.costfun= single_seamove;
>    }
> -  if (dest_y != src_y)
> -    diff_y = dest_y > src_y ? 1 : -1;
> +    } else {
> +      warmap.costmap = warmap.cost;
> +      init_warmap(x, y, LAND_MOVING);
> +      if (unit_flag(punit, F_IGTER))
> +     warmap.costfun= igter_move_unit;        
>    else
> -    diff_y = 0;
> +     warmap.costfun= normal_move_unit;
> +    }
> +    warmap.orig_x = x;
> +    warmap.orig_y = y;
> +  } else {
> +    freelog(LOG_ERROR, "warmap_initialize does not work for non-units
yet");
> +    return 0;
> +  }
> +
> +  add_to_mapqueue(0, warmap.orig_x, warmap.orig_y);
> +  return 1;
> +}
> +
> +/**************************************************************************
> +Gives next nearest tile in a dynamic warmap generation

"Next nearest" can be misinterpreted. "Next tile in the warmap queue" is
precise, adding "ordered by move_cost distance from the starting location"
or something might be better than just "nearest".

> +**************************************************************************/
> +int warmap_next_location(int *x, int *y)
> +{
> +  while(get_from_mapqueue(x,y)) {
> +    if (warmap.costmap[*x][*y] >= lowest_cost)
> +      return 1;
> +    /* if warmap.costmap[*x][*y] < lowest_cost 
> +     * we've processed this tile already */

Can this ever legitimately happen? How about an assert?

> +  }
> +  return 0;
> +}
> +
> +/**************************************************************************
> +Processes the tile in a dynamic warmap generation:
> +-looks around
> +-updates distances to neighbouring tiles
> +-puts them into queue
> +NB: For units only so far!
> +NB: Does not respect unknown terrain!

Should be some cautionary comments about "what" tiles are valid to pass
to this function - i.e. tiles are not arbitrary, but need to have already
been processed and selected using warmap_next_location() or the warmap
will not make a lot of sense.

> +**************************************************************************/
> +void warmap_process_location(int x, int y)
> +{
> +  int cost;
> +  
> +  /* assuming everything is initialised */
>  
> -  DIRSTEP(dx, dy, dir);
> -  scalar_product = diff_x * dx + diff_y * dy;
> +  /* Step outwards from (x,y), updating the warmap with the move cost
> +   * for adjacent tiles and adding them to the bucket list. 
> +   */
> +  adjc_dir_iterate(x, y, x1, y1, dir) {
> +    /* Only update if this might shorten the paths */
> +    if (warmap.costmap[x1][y1] <= warmap.costmap[x][y]) 
> +      continue;
>  
> -  return scalar_product >= 0;
> +    /* Evaluate the cost of the move */
> +    cost= (*warmap.costfun)(x, y, dir, x1, y1);
> +    if( cost < 0 ) {
> +      /* Special case: only update costs, don't allow movement
> +       *   through the square, e.g. used by ships to attack shore,
> +       *   or drop off passengers. */
> +      cost= warmap.costmap[x][y] - cost;
> +      if (cost < warmap.costmap[x1][y1]) {
> +     warmap.costmap[x1][y1]= cost;
> +      }
> +    } else {
> +      /* Normal case: update costs, add_to_bucket for more moves. */
> +      cost+= warmap.costmap[x][y];
> +     if (cost < warmap.maxcost && cost < warmap.costmap[x1][y1]) {
> +       warmap.costmap[x1][y1]= cost;
> +       add_to_mapqueue(cost, x1, y1);
> +     }
> +    }
> +  } adjc_dir_iterate_end;
>  }
>  
>  /**************************************************************************
> @@ -481,12 +832,10 @@
>       This code makes sure that if there is a unit in the way that the GOTO
>       made a path over (attack), the unit's ZOC effect (which is now
>       irrelevant, it is dead) doesn't have any effect.
> +     That is, if there is a unit standing on a tile that we (possibly)
came 
> +     from, we will not take it into account.
[...]
> +        /* and there is an enemy there */
>         && is_enemy_unit_tile(map_get_tile(x, y), owner))
> +     /* then it counts in the zoc claculation */

                                  ^^ typo

>       return 0;
>      } adjc_dir_iterate_end;
> -    return 0;
> +    return 1;
>    }
>  }
>  
> @@ -566,64 +910,87 @@
>  
>  FIXME: this is a bit of a mess in not respecting FoW, and only sometimes
>  respecting if a square is known. (not introduced by me though) -Thue
> +Fixing in progress.  Moved now to the cost functions -GB
>  **************************************************************************/
>  static int find_the_shortest_path(struct unit *punit,
>                                 int dest_x, int dest_y,
>                                 enum goto_move_restriction restriction)
>  {
[...] 
> -  /* until we have found the shortest paths */
> +  /* the route calculation begins */
> +  /* loop until we have found the shortest paths */
>    while (get_from_mapqueue(&x, &y)) {

Why don't you use warmap_next_location() here?

> -    psrctile = map_get_tile(x, y);
>  
>      if (restriction == GOTO_MOVE_STRAIGHTEST)
>        straight_dir = straightest_direction(x, y, dest_x, dest_y);
> @@ -631,173 +998,32 @@
>      /* Try to move to all tiles adjacent to x,y. The coordinats of the
tile we
>         try to move to are x1,y1 */
>      adjc_dir_iterate(x, y, x1, y1, dir) {
> +
[...]
> +      total_cost = move_cost + warmap.costmap[x][y];
>  
>        /* Add the route to our warmap if it is worth keeping */
>        if (total_cost < maxcost) {
> -     if (warmap_cost[x1][y1] > total_cost) {
> -       warmap_cost[x1][y1] = total_cost;
> +     if (warmap.costmap[x1][y1] > total_cost) {
> +       warmap.costmap[x1][y1] = total_cost;
>         add_to_mapqueue(total_cost, x1, y1);
>         local_vector[x1][y1] = 1 << DIR_REVERSE(dir);
>         freelog(LOG_DEBUG,
>                 "Candidate: %s from (%d, %d) to (%d, %d), cost %d",
>                 dir_get_name(dir), x, y, x1, y1, total_cost);
> -     } else if (warmap_cost[x1][y1] == total_cost) {
> +     } else if (warmap.costmap[x1][y1] == total_cost) {
>         local_vector[x1][y1] |= 1 << DIR_REVERSE(dir);
>         freelog(LOG_DEBUG,
>                 "Co-Candidate: %s from (%d, %d) to (%d, %d), cost %d",

And what not use warmap_process_location to handle this?

If it is the GOTO_MOVE_CARDINAL_ONLY, then you merely need to create
a warmap_process_location_cardinal() that uses a different iterator 
and call the appropriate one, no?

If it is the local_vector updates, then perhaps a slightly more
complex version of process_location that takes a general post-process 
funtion or for now just updates local_vector is needed.

Any additional effort to make the similar parts look as similar as 
possible is in order.

Either way, a comment on why this is and isn't a clone of the dynamic
warmap code is also in order so that future fixes can easily hit both 
places if needed. And a corresponding comment in the warmap routines 
refering to this code should be added as well.

I apologize for trying to push this a litte further given all the work
to unscramble it to this stage, but it looks to my naive eye that just
a little more umph might remove the final bits. Could you at least look
and comment?

> diff -ur --ignore-space-change -X freeciv/diff_ignore
freeciv/server/gotohand.h freeciv_mod/server/gotohand.h
> --- freeciv/server/gotohand.h Thu Oct  4 22:09:29 2001
> +++ freeciv_mod/server/gotohand.h     Fri Oct 12 18:57:00 2001
> @@ -41,19 +41,41 @@
>  /* all other functions are internal */
>  
>  #define THRESHOLD 12
> -
> +#define MAXCOST 255

This really belongs in common/unit.h as suggested above and might better
be named MOVE_COST_MAX for consistency and easy association spotting.

> diff -ur --ignore-space-change -X freeciv/diff_ignore
freeciv/server/maphand.c freeciv_mod/server/maphand.c
> --- freeciv/server/maphand.c  Thu Oct 11 16:11:42 2001
> +++ freeciv_mod/server/maphand.c      Thu Oct 11 16:10:57 2001
> @@ -1269,3 +1269,51 @@
>      disable_fog_of_war_player(pplayer);
>    } players_iterate_end;
>  }
> +
> +/***************************************************************
> +Can pplayer conclude (at least by circumstantial evidence) that
> +(x,y) is on coastline?  Remember, coastline ocean tiles have a 
> +small stripe of land in them, even if the actual continent is 
> +not seen.
> +***************************************************************/
> +int is_coast_seen(int x, int y, struct player *pplayer)
> +{
> +  square_iterate(x, y, 1, x1, y1) {
> +    enum tile_terrain_type ter = map_get_terrain(x1, y1);
> +    if (ter != T_OCEAN) {
> +      /* AI cheats */
> +      if (pplayer->ai.control) return 1;
> +      square_iterate(x1, y1, 1, x2, y2) {
> +     if (map_get_known(x2, y2, pplayer)) {
> +       return 1;
> +     }
> +      } square_iterate_end; /* around x1,y1 */
> +    }
> +  } square_iterate_end; /*around x2,y2 */
> +  return 0;
> +}

You want to use adjacent_iterate, not square_iterate, right?

Even for the inner loop it would perhaps be better to test that the
the selected terrain is known, and only if not do the full loop logic.
        if (map_get_known(x1, y1, pplayer)) 
          return 1;
    adjacent_iterate(x2, y2, x1, y1) {

> +
> +/***************************************************************
> +Same as is_coast_seen but checks for land at distance 2 from 
> +the tile we are interested in
> +***************************************************************/
> +int is_coast_seen2(int x, int y, struct player *pplayer)
> +{
> +  /* this should really only interate over tiles distance 2 from (x,y)
> +   * but there is no such macro yet */  
> +  square_iterate(x, y, 2, x1, y1) {
> +    enum tile_terrain_type ter = map_get_terrain(x1, y1);
> +    if (ter != T_OCEAN) {
> +      /* AI cheats */
> +      if (pplayer->ai.control) return 1;
> +      square_iterate(x1, y1, 1, x2, y2) {
> +     if (map_get_known(x2, y2, pplayer)) {
> +       return 1;
> +     }
> +      } square_iterate_end; /* around x1,y1 */
> +    }
> +  } square_iterate_end; /*around x2,y2 */
> +  return 0;
> +}

I really would implement the correct adjacent_iterate2 or hard code the
outer loop here. The same comment applies to the inner as above.

> +