[Freeciv-Dev] (PR#12861) evaluate_improvements simplification

["Brian Dunstan" <bdunstan149@xxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=12861 >

For fixing the remaining problems with autosettlers, I
think it would be good to first simplify and
generalize some of the way settlers work.  Right now
in CVS consider_settler_action has a complicated
interface, and does not do much.  Also, settler
actions are often referred to by specific names, e.g.
ACTIVITY_IRRIGATE.  Here's a patch that does the
following:

1. makes an iterator for unit activities
2. changes evaluate_improvements from a laundry list
to an iteration over activity types
3. moves work by consider_settler_action to
evaluate_improvements
4. removes consider_settler_action
5. moves calculation of of climate extras
(pplayer->ai.warmth etc) to ai_calc_pollution,
ai_calc_fallout
6. Shortens the representation of activity values in
struct ai_city to a single array.  
7. Stores values in struct ai_city as the change in
tile value from taking an action, rather than the
absolute total after an action, to avoid an extra
function call and calculation in evaluate_improvements

The overall result cuts one function and about 100
lines from server/settlers.c.  Behavior and running
time should be unchanged.


-Brian

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com 

diff -Nur -Xfreeciv/diff_ignore freeciv/common/city.h 
freeciv_altered/common/city.h
--- freeciv/common/city.h       2005-04-21 09:13:28.641369296 -0400
+++ freeciv_altered/common/city.h       2005-04-21 11:06:57.688236832 -0400
@@ -180,16 +180,9 @@
                                    avoiding paradox */
   bool celebrate;               /* try to celebrate in this city */
 
-  /* Used for caching when settlers evalueate which tile to improve,
-     and when we place workers. */
-  signed short int detox[CITY_MAP_SIZE][CITY_MAP_SIZE];
-  signed short int derad[CITY_MAP_SIZE][CITY_MAP_SIZE];
-  signed short int mine[CITY_MAP_SIZE][CITY_MAP_SIZE];
-  signed short int irrigate[CITY_MAP_SIZE][CITY_MAP_SIZE];
-  signed short int road[CITY_MAP_SIZE][CITY_MAP_SIZE];
-  signed short int railroad[CITY_MAP_SIZE][CITY_MAP_SIZE];
-  signed short int transform[CITY_MAP_SIZE][CITY_MAP_SIZE];
-  signed short int tile_value[CITY_MAP_SIZE][CITY_MAP_SIZE];
+  /* Used for caching change in value from a worker performing
+     a particular activity on a particular tile */
+  int act_value[ACTIVITY_LAST][CITY_MAP_SIZE][CITY_MAP_SIZE];
 
   /* so we can contemplate with warmap fresh and decide later */
   /* These values are for builder (F_SETTLERS) and founder (F_CITIES) units.
diff -Nur -Xfreeciv/diff_ignore freeciv/common/fc_types.h 
freeciv_altered/common/fc_types.h
--- freeciv/common/fc_types.h   2005-04-21 09:13:28.633370512 -0400
+++ freeciv_altered/common/fc_types.h   2005-04-21 10:54:53.199375880 -0400
@@ -46,6 +46,7 @@
 typedef int Specialist_type_id;
 typedef int Impr_Type_id;
 typedef enum output_type Output_type_id;
+typedef enum unit_activity Activity_type_id;
 typedef int Nation_Type_id;
 typedef int Team_Type_id;
 
diff -Nur -Xfreeciv/diff_ignore freeciv/common/unit.h 
freeciv_altered/common/unit.h
--- freeciv/common/unit.h       2005-04-21 09:13:28.639369600 -0400
+++ freeciv_altered/common/unit.h       2005-04-21 13:24:20.451146400 -0400
@@ -209,6 +209,19 @@
   } \
 }
 
+
+/* Iterates over the types of unit activity. */
+#define activity_type_iterate(act)                                         \
+{                                                                          \
+  Activity_type_id act;                                                        
    \
+                                                                           \
+  for (act = 0; act < ACTIVITY_LAST; act++) {
+
+#define activity_type_iterate_end                                          \
+  }                                                                        \
+}
+
+
 struct unit *unit_list_find(const struct unit_list *This, int id);
 
 void unit_list_sort_ord_map(struct unit_list *This);
diff -Nur -Xfreeciv/diff_ignore freeciv/server/settlers.c 
freeciv_altered/server/settlers.c
--- freeciv/server/settlers.c   2005-04-21 09:13:28.610374008 -0400
+++ freeciv_altered/server/settlers.c   2005-04-21 12:17:09.120001888 -0400
@@ -214,6 +214,8 @@
   /* FIXME: need a better way to guarantee pollution is cleaned up. */
   goodness = (goodness + best + 50) * 2;
 
+  goodness += city_owner(pcity)->ai.warmth;
+
   return goodness;
 }
 
@@ -246,6 +248,8 @@
     goodness = (goodness + best + 50) * 2;
   }
 
+  goodness += city_owner(pcity)->ai.frost;
+
   return goodness;
 }
 
@@ -728,64 +732,6 @@
   return NULL;
 }
 
-/****************************************************************************
-  Compares the best known tile improvement action with improving the tile
-  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.
-****************************************************************************/
-static void 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,
-                                   int *best_value,
-                                   int *best_old_tile_value,
-                                   enum unit_activity *best_act,
-                                   struct tile **best_tile,
-                                    struct tile *ptile)
-{
-  bool consider;
-  int total_value = 0, base_value = 0;
-  
-  if (extra >= 0) {
-    consider = TRUE;
-  } else {
-    consider = (new_tile_value > old_tile_value);
-    extra = 0;
-  }
-
-  /* find the present value of the future benefit of this action */
-  if (consider) {
-    const int FACTOR = 1024;
-
-    base_value = new_tile_value - old_tile_value;
-    total_value = base_value * FACTOR;
-    if (!in_use) {
-      total_value /= 2;
-    }
-    total_value += extra * FACTOR;
-
-    /* use factor to prevent rounding errors */
-    total_value = amortize(total_value, delay);
-  } else {
-    total_value = 0;
-  }
-
-  if (total_value > *best_value
-      || (total_value == *best_value
-         && old_tile_value > *best_old_tile_value)) {
-    freelog(LOG_DEBUG,
-           "Replacing (%d, %d) = %d with %s (%d, %d) = %d [d=%d b=%d]",
-           TILE_XY(*best_tile), *best_value, get_activity_text(act),
-           TILE_XY(ptile), total_value,
-            delay, base_value);
-    *best_value = total_value;
-    *best_old_tile_value = old_tile_value;
-    *best_act = act;
-    *best_tile = ptile;
-  }
-}
-
 /**************************************************************************
   Returns how much food a settler will consume out of the city's foodbox
   when created. If unit has id zero it is assumed to be a virtual unit
@@ -842,137 +788,62 @@
 {
   struct city *mycity = map_get_city(punit->tile);
   struct player *pplayer = unit_owner(punit);
-  bool in_use;                 /* true if the target square is being used
-                                  by one of our cities */
   Continent_id ucont     = map_get_continent(punit->tile);
   int mv_rate         = unit_type(punit)->move_rate;
   int mv_turns;                        /* estimated turns to move to target 
square */
   int oldv;                    /* current value of consideration tile */
-  int best_oldv = 9999;                /* oldv of best target so far; compared 
if
-                                  newv==best_newv; not initialized to zero,
-                                  so that newv=0 activities are not chosen */
   int food_upkeep        = unit_food_upkeep(punit);
   int food_cost          = unit_foodbox_cost(punit);
-  bool can_rr = player_knows_techs_with_flag(pplayer, TF_RAILROAD);
-
   int best_newv = 0;
   enemy_mask my_enemies = enemies[pplayer->player_no]; /* optimalization */
 
   generate_warmap(mycity, punit);
 
   city_list_iterate(pplayer->cities, pcity) {
-#ifdef REALLY_DEBUG_THIS
-    freelog(LOG_DEBUG, "Evaluating improvements for %s...", pcity->name);
-#endif
-    /* try to work near the city */
     city_map_checked_iterate(pcity->tile, i, j, ptile) {
       if (get_worker_city(pcity, i, j) == C_TILE_UNAVAILABLE
          || terrain_has_flag(pcity->tile->terrain, TER_UNSAFE)) {
        /* Don't risk bothering with this tile. */
-       continue;
+       continue;   
       }
-      in_use = (get_worker_city(pcity, i, j) == C_TILE_WORKER);
       if (map_get_continent(ptile) == ucont
          && WARMAP_COST(ptile) <= THRESHOLD * mv_rate
          && !BV_CHECK_MASK(TERRITORY(ptile), my_enemies)
-         /* pretty good, hope it's enough! -- Syela */
          && !is_already_assigned(punit, pplayer, ptile)) {
-       /* calling is_already_assigned once instead of four times
-          for obvious reasons;  structure is much the same as it once
-          was but subroutines are not -- Syela */
-       int time;
+
+       int time = 0;
        mv_turns = (WARMAP_COST(ptile)) / mv_rate;
        oldv = city_tile_value(pcity, i, j, 0, 0);
 
        /* now, consider various activities... */
 
-       time = mv_turns
-         + get_turns_for_activity_at(punit, ACTIVITY_IRRIGATE, ptile);
-       consider_settler_action(pplayer, ACTIVITY_IRRIGATE, -1,
-                               pcity->ai.irrigate[i][j], oldv, in_use, time,
-                               &best_newv, &best_oldv, best_act, best_tile,
-                               ptile);
-
-       if (unit_flag(punit, F_TRANSFORM)) {
-         time = mv_turns
-           + get_turns_for_activity_at(punit, ACTIVITY_TRANSFORM, ptile);
-         consider_settler_action(pplayer, ACTIVITY_TRANSFORM, -1,
-                                 pcity->ai.transform[i][j], oldv, in_use, time,
-                                 &best_newv, &best_oldv, best_act, best_tile,
-                                 ptile);
-       }
-
-       time = mv_turns
-         + get_turns_for_activity_at(punit, ACTIVITY_MINE, ptile);
-       consider_settler_action(pplayer, ACTIVITY_MINE, -1,
-                               pcity->ai.mine[i][j], oldv, in_use, time,
-                               &best_newv, &best_oldv, best_act, best_tile,
-                               ptile);
-
-       if (!tile_has_special(ptile, S_ROAD)) {
-         time = mv_turns
-           + get_turns_for_activity_at(punit, ACTIVITY_ROAD, ptile);
-         consider_settler_action(pplayer, ACTIVITY_ROAD,
-                                 road_bonus(ptile, S_ROAD) * 5,
-                                 pcity->ai.road[i][j], oldv, in_use, time,
-                                 &best_newv, &best_oldv, best_act, best_tile,
-                                 ptile);
-
-         if (can_rr) {
-           /* Count road time plus rail time. */
-           time += get_turns_for_activity_at(punit, ACTIVITY_RAILROAD, ptile);
-           consider_settler_action(pplayer, ACTIVITY_ROAD,
-                                   road_bonus(ptile, S_RAILROAD) * 3,
-                                   pcity->ai.railroad[i][j], oldv,
-                                   in_use, time,
-                                   &best_newv, &best_oldv,
-                                   best_act, best_tile,
-                                   ptile);
+       activity_type_iterate(act) {
+
+         if(pcity->ai.act_value[act][i][j] > 0 &&
+            can_unit_do_activity_targeted_at(punit, act, 
+                                             S_NO_SPECIAL, ptile)) {
+           
+           /* use factor to prevent rounding errors */
+           const int FACTOR = 1024;
+           int total_value = pcity->ai.act_value[act][i][j] * FACTOR;
+           if(act == ACTIVITY_ROAD) {
+             total_value += road_bonus(ptile, S_ROAD) * FACTOR;
+           } else if (act == ACTIVITY_RAILROAD) {
+             total_value += road_bonus(ptile, S_RAILROAD) * FACTOR;
+           }
+
+           time = mv_turns + get_turns_for_activity_at(punit, act, ptile);
+           total_value = amortize(total_value, time);
+
+           if(total_value > best_newv) {
+             best_newv = total_value;
+             *best_act = act;
+             *best_tile = ptile;
+           }
          }
-       } else if (!tile_has_special(ptile, S_RAILROAD)
-                  && can_rr) {
-         time = mv_turns
-           + get_turns_for_activity_at(punit, ACTIVITY_RAILROAD, ptile);
-         consider_settler_action(pplayer, ACTIVITY_RAILROAD,
-                                 road_bonus(ptile, S_RAILROAD) * 3,
-                                 pcity->ai.railroad[i][j], oldv, in_use, time,
-                                 &best_newv, &best_oldv,
-                                 best_act, best_tile,
-                                 ptile);
-       } /* end S_ROAD else */
-
-       if (tile_has_special(ptile, S_POLLUTION)) {
-         time = mv_turns
-           + get_turns_for_activity_at(punit, ACTIVITY_POLLUTION, ptile);
-         consider_settler_action(pplayer, ACTIVITY_POLLUTION,
-                                 pplayer->ai.warmth,
-                                 pcity->ai.detox[i][j], oldv, in_use, time,
-                                 &best_newv, &best_oldv,
-                                 best_act, best_tile,
-                                 ptile);
-       }
-      
-       if (tile_has_special(ptile, S_FALLOUT)) {
-         time = mv_turns
-           + get_turns_for_activity_at(punit, ACTIVITY_FALLOUT, ptile);
-         consider_settler_action(pplayer, ACTIVITY_FALLOUT,
-                                 pplayer->ai.frost,
-                                 pcity->ai.derad[i][j], oldv, in_use, time,
-                                 &best_newv, &best_oldv,
-                                 best_act, best_tile,
-                                 ptile);
-       }
-
-#ifdef REALLY_DEBUG_THIS
-       freelog(LOG_DEBUG,
-               "(%d %d) I=%+-4d O=%+-4d M=%+-4d R=%+-4d RR=%+-4d P=%+-4d 
N=%+-4d",
-               i, j,
-               pcity->ai.irrigate[i][j], pcity->ai.transform[i][j],
-               pcity->ai.mine[i][j], pcity->ai.road[i][j],
-               pcity->ai.railroad[i][j], pcity->ai.detox[i][j],
-               pcity->ai.derad[i][j]);
-#endif
-      } /* end if we are a legal destination */
+       } activity_type_iterate_end;
+
+      } /* endif: are we a legal destination? */
     } city_map_checked_iterate_end;
   } city_list_iterate_end;
 
@@ -1167,41 +1038,53 @@
     int best = best_worker_tile_value(pcity);
 
     city_map_iterate(city_x, city_y) {
-      pcity->ai.detox[city_x][city_y] = -1;
-      pcity->ai.derad[city_x][city_y] = -1;
-      pcity->ai.mine[city_x][city_y] = -1;
-      pcity->ai.irrigate[city_x][city_y] = -1;
-      pcity->ai.transform[city_x][city_y] = -1;
-      pcity->ai.road[city_x][city_y] = -1;
-      pcity->ai.railroad[city_x][city_y] = -1;
+      activity_type_iterate(act) {
+       pcity->ai.act_value[act][city_x][city_y] = -1;
+      } activity_type_iterate_end;
     } city_map_iterate_end;
 
     city_map_checked_iterate(pcity->tile,
                             city_x, city_y, ptile) {
+      
+      int old_value = city_tile_value(pcity, city_x, city_y, 0, 0);
+
 #ifndef NDEBUG
       Terrain_type_id old_terrain = ptile->terrain;
       enum tile_special_type old_special = ptile->special;
 #endif
 
-      pcity->ai.detox[city_x][city_y]
+      pcity->ai.act_value[ACTIVITY_POLLUTION][city_x][city_y] 
        = ai_calc_pollution(pcity, city_x, city_y, best, ptile);
-      pcity->ai.derad[city_x][city_y] =
-       ai_calc_fallout(pcity, pplayer, city_x, city_y, best, ptile);
-      pcity->ai.mine[city_x][city_y]
+      pcity->ai.act_value[ACTIVITY_FALLOUT][city_x][city_y]
+       = ai_calc_fallout(pcity, pplayer, city_x, city_y, best, ptile);
+      pcity->ai.act_value[ACTIVITY_MINE][city_x][city_y]
        = ai_calc_mine(pcity, city_x, city_y, ptile);
-      pcity->ai.irrigate[city_x][city_y]
+      pcity->ai.act_value[ACTIVITY_IRRIGATE][city_x][city_y]
         = ai_calc_irrigate(pcity, pplayer, city_x, city_y, ptile);
-      pcity->ai.transform[city_x][city_y]
+      pcity->ai.act_value[ACTIVITY_TRANSFORM][city_x][city_y]
        = ai_calc_transform(pcity, city_x, city_y, ptile);
 
       /* road_bonus() is handled dynamically later; it takes into
        * account settlers that have already been assigned to building
        * roads this turn. */
-      pcity->ai.road[city_x][city_y]
+      pcity->ai.act_value[ACTIVITY_ROAD][city_x][city_y]
        = ai_calc_road(pcity, pplayer, city_x, city_y, ptile);
-      pcity->ai.railroad[city_x][city_y] =
+      pcity->ai.act_value[ACTIVITY_RAILROAD][city_x][city_y] =
        ai_calc_railroad(pcity, pplayer, city_x, city_y, ptile);
 
+      /* subtract old tile values, so that that ai.act_value stored the delta 
*/
+      activity_type_iterate(act) {
+       pcity->ai.act_value[act][city_x][city_y] -= old_value;
+      } activity_type_iterate_end;
+
+      /* if this tile is in use, activities on it assumed to be 
+        twice as valuable */
+      if(get_worker_city(pcity, city_x, city_y) == C_TILE_WORKER) {
+       activity_type_iterate(act) {
+         pcity->ai.act_value[act][city_x][city_y] *= 2;
+       } activity_type_iterate_end;
+      }
+
       /* Make sure nothing was accidentally changed by these calculations. */
       assert(old_terrain == ptile->terrain && old_special == ptile->special);
     } city_map_checked_iterate_end;