[Freeciv-Dev] Re: [PATCH] aiunit.c ai_manage_explorer cleanup (PR#1210)

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

Few comments on the patch.

1. The vision range should NOT take into account watchtower benefit.  Now
it is done in a wrong way: the watchtower bonus is computed locally, at
the point where the unit is, and then it is applied at a faraway
location, as if there is a watchtower there too.  To make it completely
correct, you should make a separate function 
unknown_tiles_around(int x, int y, unit *punit)
which 
 a. determines unit's vision range at (x,y) -- if the player has no info
    on (x,y), assume there is no watchtower
 b. count the number of unknown location within the vision range.
This function would be used at least in 2 places, so it's worth it.

2. Comments like
> +  /* The unit's owner */
>    struct player *pplayer = unit_owner(punit);
are completely useles IMO, as the function name says the same thing.

3. Splitting of long if (...) condition into many if() continue;
is nice.  There is another place that you can do the same.

<ASIDE>
I really prefer statements like
        if (!points_left) continue;
to 
        if (points_left) {
          /* very long block */
        }
as it explicitly says that there will be no "else" 234 lines down the
code, it also reduces indentation which is sometimes hard to trace.
Shall we include it in the style guide?
</ASIDE>

4. do_unit_goto now returns a meaningful value, you can check for it
instead of using unit.id

5. you can move the first generate_warmap call inside the 
if (looks_for_huts) block for a slight improve in efficiency.


Sorry I didn't try the patch in practice -- no computer available :(

Best,
G.

 --- Petr Baudis <pasky@xxxxxxxxxxx> wrote: 
> > Index: ai/aiunit.c
> ===================================================================
> RCS file: /home/cvs/aiciv/freeciv-a2/ai/aiunit.c,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 aiunit.c
> --- ai/aiunit.c       19 Dec 2001 20:43:22 -0000      1.1.1.1
> +++ ai/aiunit.c       7 Jan 2002 21:50:43 -0000
> @@ -177,191 +177,313 @@
>  }
>   
> 
>
/**************************************************************************
> -Explores unknown territory, finds huts.
> +Handle eXplore mode of a unit - explores unknown territory, finds
> huts.
>  Returns whether there is any more territory to be explored.
> 
>
**************************************************************************/
> -int ai_manage_explorer(struct unit *punit)
> +int ai_manage_exploring(struct unit *punit)
>  {
> +  /* The unit's owner */
>    struct player *pplayer = unit_owner(punit);
> -  int x, y; /* is the position of the unit; updated inside the
> function */
> -  int con; /* continent the unit is on */
> -  struct city *pcity;
> -  int id = punit->id; /* we can now die because easy AI may accidently
> -                      stumble on huts it fuzzily ignored */
> -  int best_x = -1, best_y = -1;
> +  /* The position of the unit; updated inside the function */
> +  int x, y;
> +  /* Continent the unit is on */
> +  int continent;
> +  /* Unit's speed */
>    int move_rate = unit_move_rate(punit);
> +  /* Range of unit's vision */
>    int range;
>  
> +  /* Get the range */
> +
>    if (unit_profits_of_watchtower(punit)
>        && map_get_tile(punit->x, punit->y)->special & S_FORTRESS)
> -    range =get_watchtower_vision(punit);
> +    range = get_watchtower_vision(punit);
>    else
>      range = unit_type(punit)->vision_range;
>  
> +  /* Idle unit */
> +
>    if (punit->activity != ACTIVITY_IDLE)
>      handle_unit_activity_request(punit, ACTIVITY_IDLE);
>  
> -  x = punit->x; y = punit->y;
> -  if (is_ground_unit(punit)) con = map_get_continent(x, y);
> -  else con = 0; /* Thanks, Tony */
> +  /* Localize the unit */
> +
> +  x = punit->x;
> +  y = punit->y;
> +  
> +  if (is_ground_unit(punit)) {
> +    continent = map_get_continent(x, y);
> +  } else {
> +    continent = 0;
> +  }
>  
>    /* 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.
> */
> +  /*
> +   * PART 1: Look for huts
> +   * Non-Barbarian Ground units ONLY.
> +   */
> +  
>    if (!is_barbarian(pplayer)
> -      && is_ground_unit(punit)) { /* boats don't hunt huts */
> +      && is_ground_unit(punit)) {
> +    /* Maximal acceptable move cost to the target */
>      int maxcost = pplayer->ai.control ? 2 * THRESHOLD : 3;
> +    /* Move cost to the best target (=> lower is better) */
>      int bestcost = maxcost * SINGLE_MOVE + 1;
> +    /* Desired destination */
> +    int best_x = -1, best_y = -1;
>  
> -    /* Iterating outward so that with two tiles with the same movecost
> -       the nearest is used */
> +    /* We're iterating outward so that with two tiles with the same
> movecost
> +     * the nearest is used. */
>      iterate_outward(x, y, maxcost, 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];
> +          && 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) {
> +      /* We can die because easy AI may accidently stumble on huts it
> fuzzily
> +       * ignored - unit_id is used for the check of this */
> +      int unit_id = punit->id;
> +
> +      /* Go there! */
>        punit->goto_dest_x = best_x;
>        punit->goto_dest_y = best_y;
>        set_unit_activity(punit, ACTIVITY_GOTO);
>        do_unit_goto(punit, GOTO_MOVE_ANY, 0);
> -      if (!player_find_unit_by_id(pplayer, id))
> -     return 0; /* died */
> +      
> +      if (!player_find_unit_by_id(pplayer, unit_id)) {
> +        /* We're dead. */
> +        return 0;
> +      }
>  
>        if (punit->moves_left) {
> -     if (punit->x == best_x && punit->y == best_y) {
> -       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);
> -       x = punit->x; y = punit->y;
> -       /* Fallthough to next fase */
> -     }
> +        /* We can still move on... */
> +
> +        if (punit->x == best_x && punit->y == best_y) {
> +          /* ...and got into desired place. */
> +          return ai_manage_exploring(punit);
> +  
> +        } else {
> +          /* Something went wrong. This should almost never happen. */
> +          if (punit->x != x || punit->y != y)
> +            generate_warmap(map_get_city(punit->x, punit->y), punit);
> +          
> +          x = punit->x;
> +          y = punit->y;
> +          /* Fallthrough to next part. */
> +        }
> +
>        } else {
> -     return 1;
> +        return 1;
>        }
>      }
>    }
>  
> -  /* BEGIN PART TWO: Move into unexplored territory */
> -  /* move the unit as long as moving will unveil unknown territory */
> +  /* 
> +   * PART 2: Move into unexplored territory
> +   * Move the unit as long as moving will unveil unknown territory
> +   */
> +  
>    while (punit->moves_left) {
> +    /* Best (highest) number of unknown tiles adjectent (in vision
> range) */
>      int most_unknown = 0;
> -    int unknown;
> +    /* Desired destination */
> +    int best_x = -1, best_y = -1;
>  
> -    /* evaluate all adjacent tiles */
> +    /* Evaluate all adjacent tiles. */
> +    
>      square_iterate(x, y, 1, x1, y1) {
> -      unknown = 0;
> +      /* Number of unknown tiles in vision range around this tile */
> +      int unknown = 0;
> +      
>        square_iterate(x1, y1, range, x2, y2) {
> -     if (!map_get_known(x2, y2, pplayer))
> -       unknown++;
> +        if (!map_get_known(x2, y2, pplayer))
> +          unknown++;
>        } square_iterate_end;
>  
> -      if (unknown > most_unknown && (!unit_flag(punit, F_TRIREME)
> -                                  || trireme_loss_pct(pplayer, x1,
> -                                                      y1) == 0)
> -       && map_get_continent(x1, y1) == con
> -       && can_unit_move_to_tile_with_notify(punit, x1, y1, 0)
> -       && !((pcity = map_get_city(x1,y1))
> -            && (unit_flag(punit, F_DIPLOMAT)
> -                || unit_flag(punit, F_CARAVAN)))
> -       && !(is_barbarian(pplayer) && map_get_special(x1, y1) & S_HUT)) {
> -     most_unknown = unknown;
> -     best_x = x1;
> -     best_y = y1;
> +      if (unknown > most_unknown) {
> +        if (unit_flag(punit, F_TRIREME)
> +            && trireme_loss_pct(pplayer, x1, y1) != 0)
> +          continue;
> +        
> +        if (map_get_continent(x1, y1) != continent)
> +          continue;
> +        
> +        if (!can_unit_move_to_tile_with_notify(punit, x1, y1, 0))
> +          continue;
> +
> +        /* We won't travel into cities, unless we are able to do so -
> diplomats
> +         * and caravans can. */
> +        /* FIXME/TODO: special flag for this */
> +        if (map_get_city(x1, y1) && !(unit_flag(punit, F_DIPLOMAT) ||
> +                                      unit_flag(punit, F_CARAVAN)))
> +          continue;
> +
> +        if (is_barbarian(pplayer) && map_get_special(x1, y1) & S_HUT)
> +          continue;
> +          
> +        most_unknown = unknown;
> +        best_x = x1;
> +        best_y = y1;
>        }
>      } square_iterate_end;
>  
> -    if (most_unknown > 0) { /* a tile have unexplored territory
> adjacent */
> -      int res = handle_unit_move_request(punit, best_x, best_y, FALSE,
> FALSE);
> -      if (!res) /* This shouldn't happen */
> -     break;
> -      if (!player_find_unit_by_id(pplayer, id))
> -     return 0; /* died */
> -      x = punit->x; y = punit->y;
> +    if (most_unknown > 0) {
> +      /* We can die because easy AI may accidently stumble on huts it
> fuzzily
> +       * ignored - unit_id is used for the check of this */
> +      int unit_id = punit->id;
> +      
> +      /* Some tile have unexplored territory adjacent, let's move
> there. */
> +      
> +      if (!handle_unit_move_request(punit, best_x, best_y, FALSE,
> FALSE)) {
> +        /* This shouldn't happen */
> +        break;
> +      }
> +      
> +      if (!player_find_unit_by_id(pplayer, unit_id)) {
> +        /* We're dead. */
> +        return 0;
> +      }
> +      
> +      x = punit->x;
> +      y = punit->y;
> +      
>      } else {
> +      /* Everything is already explored. */
>        break;
>      }
>    }
>  
> -  if (!punit->moves_left) return 1;
> +  if (!punit->moves_left) {
> +    /* We can't move on anymore. */
> +    return 1;
> +  }
>  
> -  /* BEGIN PART THREE: Go towards unexplored territory */
> -  /* no adjacent squares help us to explore - really slow part follows
> */
> +  /* 
> +   * PART 3: Go towards unexplored territory
> +   * No adjacent squares help us to explore - really slow part
> follows.
> +   */
> +  
>    generate_warmap(map_get_city(x, y), punit);
>  
>    {
> -    int unknown, most_unknown = 0;
> -    int threshold = THRESHOLD * move_rate;
> +    /* Best (highest) number of unknown tiles adjectent (in vision
> range) */
> +    int most_unknown = 0;
> +    /* Desired destination */
> +    int best_x = -1, best_y = -1;
> +
> +    /* XXX: There's some duplicate code here, but it's several tiny
> pieces,
> +     * impossible to group together and not worth their own function
> +     * separately. --pasky */
> +    
>      whole_map_iterate(x1, y1) {
> +      /* The actual map tile */
>        struct tile *ptile = map_get_tile(x1, y1);
> -      unknown = 0;
> -      if (ptile->continent == con
> -       && !is_non_allied_unit_tile(ptile, pplayer)
> -       && !is_non_allied_city_tile(ptile, pplayer)
> -       && tile_is_accessible(punit, x1, y1)) {
> -     square_iterate(x1, y1, range, x2, y2) {
> -       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]);
> -       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 (ptile->continent == continent
> +          && !is_non_allied_unit_tile(ptile, pplayer)
> +          && !is_non_allied_city_tile(ptile, pplayer)
> +          && tile_is_accessible(punit, x1, y1)) {
> +        /* Number of unknown tiles in vision range around this tile */
> +        int unknown = 0;
> +        
> +        square_iterate(x1, y1, range, x2, y2) {
> +          if (!map_get_known(x2, y2, pplayer))
> +            unknown++;
> +        } square_iterate_end;
> +        
> +        if (unknown) {
> +          /* How far it's worth moving away */
> +          int threshold = THRESHOLD * move_rate;
> +          
> +          if (is_sailing_unit(punit))
> +            unknown += 9 * (threshold - warmap.seacost[x1][y1]);
> +          else
> +            unknown += 9 * (threshold - warmap.cost[x1][y1]);
> +          
> +          /* XXX: Why we don't do same tests like in part 2? --pasky
> */
> +          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;
> +          }
> +        }
>        }
>      } whole_map_iterate_end;
>  
>      if (most_unknown > 0) {
> +      /* Go there! */
> +      
>        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);
> +
> +      /* XXX: Why we don't test if the unit is still alive? */
> +      
>        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
> -       return ai_manage_explorer(punit);
> +        /* We can still move on... */
> +
> +        if (punit->x == best_x && punit->y == best_y) {
> +          /* ...and got into desired place. */
> +          return ai_manage_exploring(punit);
> +          
> +        } else {
> +          /* Something went wrong. What to do but return? */
> +          handle_unit_activity_request(punit, ACTIVITY_IDLE);
> +          return 1;
> +        }
> +        
>        } else {
> -     return 1;
> +        return 1;
>        }
> -    } /* no candidates; fall-through */
> +    }
> +    
> +    /* No candidates; fall-through. */
>    }
>  
> -  /* BEGIN PART FOUR: maybe go to another continent */
> +  /* We have nothing to explore, so we can go idle. */
> +  
>    freelog(LOG_DEBUG, "%s's %s at (%d,%d) failed to explore.",
> -       pplayer->name, unit_type(punit)->name, punit->x, punit->y);
> +          pplayer->name, unit_type(punit)->name, punit->x, punit->y);
>    handle_unit_activity_request(punit, ACTIVITY_IDLE);
> +  
> +  /* 
> +   * PART 4: Go home
> +   * If we are AI controlled _military_ unit (so Explorers don't
> count, why?
> +   * --pasky), we will return to our homecity, maybe even to another
> continent.
> +   */
> +  
>    if (pplayer->ai.control && is_military_unit(punit)) {
> -    pcity = find_city_by_id(punit->homecity);
> -    if (pcity && map_get_continent(pcity->x, pcity->y) == con)
> -      ai_military_gohome(pplayer, punit);
> -    else if (pcity) {
> -      if (!find_boat(pplayer, &x, &y, 0)) /* Gilligan's Island */
> -        punit->ai.ferryboat = -1;
> -      else {
> -        punit->goto_dest_x = x;
> -        punit->goto_dest_y = y;
> -        do_unit_goto(punit, GOTO_MOVE_ANY, 0);
> +    /* Unit's homecity */
> +    struct city *pcity = find_city_by_id(punit->homecity);
> +
> +    if (pcity) {
> +      if (map_get_continent(pcity->x, pcity->y) == continent) {
> +        ai_military_gohome(pplayer, punit);
> +      } else {
> +        /* Sea travel */
> +        if (!find_boat(pplayer, &x, &y, 0)) {
> +          punit->ai.ferryboat = -1;
> +        } else {
> +          punit->goto_dest_x = x;
> +          punit->goto_dest_y = y;
> +          do_unit_goto(punit, GOTO_MOVE_ANY, 0);
> +        }
>        }
>      }
>    }
> +
>    return 0;
>  }
>  
> @@ -1498,7 +1620,7 @@
>              if (find_nearest_friendly_port(punit))
>             do_unit_goto(punit, GOTO_MOVE_ANY, 0);
>            } else {
> -            ai_manage_explorer(punit); /* nothing else to do */
> +            ai_manage_exploring(punit); /* nothing else to do */
>              /* you can still have some moves left here, but barbarians
> should
>                 not sit helplessly, but advance towards nearest known
> enemy city */
>           punit = find_unit_by_id(id);   /* unit might die while exploring
> */
> @@ -1658,7 +1780,7 @@
>        freelog(LOG_DEBUG, "Ferryboat %d@(%d,%d) stalling.",
>                   punit->id, punit->x, punit->y);
>        if(is_barbarian(pplayer)) /* just in case */
> -        ai_manage_explorer(punit);
> +        ai_manage_exploring(punit);
>      }
>      return;
>    }
> @@ -1724,7 +1846,7 @@
>      }
>    }
>    if (map_get_terrain(punit->x, punit->y) == T_OCEAN) /* thanks, Tony
> */
> -    ai_manage_explorer(punit);
> +    ai_manage_exploring(punit);
>    return;
>  }
>  
> @@ -1782,7 +1904,7 @@
>      return; /* when you pillage, you have moves left, avoid later
> fortify */
>      break;
>    case AIUNIT_EXPLORE:
> -    ai_manage_explorer(punit);
> +    ai_manage_exploring(punit);
>      break;
>    default:
>      abort();
> @@ -1879,7 +2001,7 @@
>      return;
>    } else {
>      if (!punit->moves_left) return; /* can't do anything with no moves
> */
> -    ai_manage_explorer(punit); /* what else could this be? -- Syela */
> +    ai_manage_exploring(punit); /* what else could this be? -- Syela
> */
>      return;
>    }
>    /* should never get here */
> Index: ai/aiunit.h
> ===================================================================
> RCS file: /home/cvs/aiciv/freeciv-a2/ai/aiunit.h,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 aiunit.h
> --- ai/aiunit.h       19 Dec 2001 20:43:22 -0000      1.1.1.1
> +++ ai/aiunit.h       6 Jan 2002 21:18:49 -0000
> @@ -23,7 +23,7 @@
>  int look_for_charge(struct player *pplayer, struct unit *punit,
>                      struct unit **aunit, struct city **acity);
>  
> -int ai_manage_explorer(struct unit *punit);
> +int ai_manage_exploring(struct unit *punit);
>  
>  int find_something_to_kill(struct player *pplayer, struct unit *punit,
> 
>                              int *x, int *y);
> Index: server/unithand.c
> ===================================================================
> RCS file: /home/cvs/aiciv/freeciv-a2/server/unithand.c,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 unithand.c
> --- server/unithand.c 19 Dec 2001 20:43:29 -0000      1.1.1.1
> +++ server/unithand.c 6 Jan 2002 21:19:16 -0000
> @@ -1256,7 +1256,7 @@
>    case ACTIVITY_EXPLORE:
>      if (punit->moves_left > 0) {
>        int id = punit->id;
> -      int more_to_explore = ai_manage_explorer(punit);
> +      int more_to_explore = ai_manage_exploring(punit);
>        /* ai_manage_explorer sets the activity to idle, so we reset it
> */
>        if (more_to_explore && (punit = find_unit_by_id(id))) {
>       set_unit_activity(punit, ACTIVITY_EXPLORE);
> Index: server/unittools.c
> ===================================================================
> RCS file: /home/cvs/aiciv/freeciv-a2/server/unittools.c,v
> retrieving revision 1.1.1.2
> diff -u -r1.1.1.2 unittools.c
> --- server/unittools.c        21 Dec 2001 18:15:44 -0000      1.1.1.2
> +++ server/unittools.c        6 Jan 2002 21:19:42 -0000
> @@ -803,7 +803,7 @@
>    }
>  
>    if (activity == ACTIVITY_EXPLORE) {
> -    int more_to_explore = ai_manage_explorer(punit);
> +    int more_to_explore = ai_manage_exploring(punit);
>      if (more_to_explore && player_find_unit_by_id(pplayer, id))
>        handle_unit_activity_request(punit, ACTIVITY_EXPLORE);
>      else
>  

__________________________________________________
Do You Yahoo!?
Everything you'll ever need on one web page
from News and Sport to Email and Music Charts
http://uk.my.yahoo.com