[Freeciv-Dev] Re: (PR#9593) another autosettlers patch

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=9593 >

Per I. Mathisen wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=9593 >
> 
> On Mon, 2 Aug 2004, Jason Short wrote:
> 
>>- Changes the structure of the functions slightly to be more similar. I
>>added an "r" variable to ai_calc_irrigate and ai_calc_mine.
> 
> This is a retrogression to Syelaism. IMHO, variables should either be
> descriptive, so that the code can be understood from a casual glance at
> it, or, if the benefits are great enough to warrant using something
> cryptic, explained in the function header.

Indeed.

This patch renames variables in the functions:

   m => goodness
   t => old_terrain
   s => old_special
   r => new_terrain

Note that the other ai_calc_*** functions generally use the old naming 
scheme.  I'd like to clean them up too but this patch is probably long 
enough as it is.  Should I make a single patch that renames all 
variables first?  Does such a patch even need to go through RT, or can I 
just commit it?

I also made two more changes to ai_calc_transform:

   - Check for cannot-transform-this-tile up at the top.
   - Make the check for ocean<->land conversion stricter.

(The latter allows conversion between ocean types without a check.  This 
corresponds to the checks in can_unit_do_activity_targeted.)

And I added the ocean<->land check to ai_calc_irrigate and ai_calc_mine. 
  This also corresponds to the checks in can_unit_do_activity_targeted.

Note that the logic - both for verification and for action - is 
duplicated.  This is less likely to cause major problems but more likely 
to cause minor ones.  We could do something like

   struct tile *ptile = map_get_tile(map_x, map_y);
   struct tile old_tile = *ptile;

   if (can_unit_do_activity_targeted_at(punit, ACTIVITY_MINE, -1,
                                        map_x, map_y)) {
     map_mine_tile(map_x, map_y);
     goodness = city_tile_value(...);
     *ptile = old_tile;
     return goodness;
   }

which would be more correct, much simpler, and avoid duplicated logic. 
However:

- There is potential for large bugs, because map_mine_tile has some side 
effects.

- It will be much slower (though I don't know if this is measurable).

- We don't have a punit at this point, so either a hack with a virtual 
unit is needed or a can_player_do_activity_targeted_at() function is needed.

-----

As a side note, I've seen some cool behavior by auto-settlers under this 
patch.  One worker mined my grassland/resources into a forest/silk. 
Then another one came along and irrigated that forest/silk into a 
plains/wheat.  Then they irrigated and farmed the tile.  The only 
drawback was that since the auto-settler code only lets one worker at a 
time operate on a tile, by the time these 4 operations were done all the 
other workers were idling.

jason

Index: server/settlers.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/settlers.c,v
retrieving revision 1.186
diff -u -r1.186 settlers.c
--- server/settlers.c   3 Aug 2004 03:08:54 -0000       1.186
+++ server/settlers.c   3 Aug 2004 17:09:50 -0000
@@ -554,8 +554,8 @@
 /**************************************************************************
   Calculate the benefit of irrigating the given tile.
 
-    (mx, my) is the map position of the tile.
-    (cx, cy) is the city position of the tile with respect to pcity.
+    (map_x, map_y) is the map position of the tile.
+    (city_x, city_y) is the city position of the tile with respect to pcity.
     pplayer is the player under consideration.
 
   The return value is the goodness of the tile after the irrigation.  This
@@ -564,54 +564,52 @@
   values).
 **************************************************************************/
 static int ai_calc_irrigate(struct city *pcity, struct player *pplayer,
-                           int cx, int cy, int mx, int my)
+                           int city_x, int city_y, int map_x, int map_y)
 {
-  int m;
-  struct tile *ptile = map_get_tile(mx, my);
-  enum tile_terrain_type t = ptile->terrain;
-  struct tile_type *type = get_tile_type(t);
-  enum tile_special_type s = ptile->special;
+  int goodness;
+  struct tile *ptile = map_get_tile(map_x, map_y);
+  enum tile_terrain_type old_terrain = ptile->terrain;
+  enum tile_special_type old_special = ptile->special;
+  struct tile_type *type = get_tile_type(old_terrain);
+  enum tile_terrain_type new_terrain = type->irrigation_result;
 
-  if (ptile->terrain != type->irrigation_result
-      && type->irrigation_result != T_LAST) {
+  if (old_terrain != new_terrain && new_terrain != T_LAST) {
     /* Irrigation would change the terrain type, clearing the mine
      * in the process.  Calculate the benefit of doing so. */
-    if (ptile->city && is_ocean(type->irrigation_result)) {
+    if (ptile->city && terrain_has_flag(new_terrain, TER_NO_CITIES)) {
       return -1;
     }
-    ptile->terrain = type->irrigation_result;
-    map_clear_special(mx, my, S_MINE);
-    m = city_tile_value(pcity, cx, cy, 0, 0);
-    ptile->terrain = t;
-    ptile->special = s;
-    return m;
-  } else if (ptile->terrain == type->irrigation_result
+    ptile->terrain = new_terrain;
+    map_clear_special(map_x, map_y, S_MINE);
+    goodness = city_tile_value(pcity, city_x, city_y, 0, 0);
+    ptile->terrain = new_terrain;
+    ptile->special = old_special;
+    return goodness;
+  } else if (old_terrain == new_terrain
             && !tile_has_special(ptile, S_IRRIGATION)
-            && !ptile->city
-            && is_wet_or_is_wet_cardinal_around(pplayer, mx, my)) {
+            && is_wet_or_is_wet_cardinal_around(pplayer, map_x, map_y)) {
     /* The tile is currently unirrigated; irrigating it would put an
      * S_IRRIGATE on it replacing any S_MINE already there.  Calculate
      * the benefit of doing so. */
-    map_clear_special(mx, my, S_MINE);
-    map_set_special(mx, my, S_IRRIGATION);
-    m = city_tile_value(pcity, cx, cy, 0, 0);
-    ptile->special = s;
-    assert(ptile->terrain == t);
-    return m;
-  } else if (ptile->terrain == type->irrigation_result
+    map_clear_special(map_x, map_y, S_MINE);
+    map_set_special(map_x, map_y, S_IRRIGATION);
+    goodness = city_tile_value(pcity, city_x, city_y, 0, 0);
+    ptile->special = old_special;
+    assert(ptile->terrain == old_terrain);
+    return goodness;
+  } else if (old_terrain == new_terrain
             && tile_has_special(ptile, S_IRRIGATION)
             && !tile_has_special(ptile, S_FARMLAND)
             && player_knows_techs_with_flag(pplayer, TF_FARMLAND)
-            && !ptile->city
-            && is_wet_or_is_wet_cardinal_around(pplayer, mx, my)) {
+            && is_wet_or_is_wet_cardinal_around(pplayer, map_x, map_y)) {
     /* The tile is currently irrigated; irrigating it more puts an
      * S_FARMLAND on it.  Calculate the benefit of doing so. */
     assert(!tile_has_special(ptile, S_MINE));
-    map_set_special(mx, my, S_FARMLAND);
-    m = city_tile_value(pcity, cx, cy, 0, 0);
-    map_clear_special(mx, my, S_FARMLAND);
-    assert(ptile->terrain == t && ptile->special == s);
-    return m;
+    map_set_special(map_x, map_y, S_FARMLAND);
+    goodness = city_tile_value(pcity, city_x, city_y, 0, 0);
+    map_clear_special(map_x, map_y, S_FARMLAND);
+    assert(ptile->terrain == old_terrain && ptile->special == old_special);
+    return goodness;
   } else {
     return -1;
   }
@@ -620,8 +618,8 @@
 /**************************************************************************
   Calculate the benefit of mining the given tile.
 
-    (mx, my) is the map position of the tile.
-    (cx, cy) is the city position of the tile with respect to pcity.
+    (map_x, map_y) is the map position of the tile.
+    (city_x, city_y) is the city position of the tile with respect to pcity.
     pplayer is the player under consideration.
 
   The return value is the goodness of the tile after the mining.  This
@@ -629,73 +627,104 @@
   city_tile_value(); note that this depends on the AI's weighting
   values).
 **************************************************************************/
-static int ai_calc_mine(struct city *pcity, int cx, int cy, int mx, int my)
+static int ai_calc_mine(struct city *pcity,
+                       int city_x, int city_y, int map_x, int map_y)
 {
-  int m;
-  struct tile *ptile = map_get_tile(mx, my);
-  enum tile_special_type s = ptile->special;
+  int goodness;
+  struct tile *ptile = map_get_tile(map_x, map_y);
+  enum tile_terrain_type old_terrain = ptile->terrain;
+  enum tile_special_type old_special = ptile->special;
+  struct tile_type *type = get_tile_type(old_terrain);
+  enum tile_terrain_type new_terrain = type->mining_result;
 
-  /* FIXME: currently this only handles building a mine on a hill or
-   * mountain terrain.  It should instead work the same way as
-   * ai_calc_irrigate does, by looking at mining_result. */
-
-  if ((ptile->terrain == T_HILLS || ptile->terrain == T_MOUNTAINS)
-      && !tile_has_special(ptile, S_MINE)) {
-    /* The tile is currently unmined; mining it would put an S_MINE
-     * on it replacing any S_IRRIGATION/S_FARMLAND already there.  Calculate
+  if (old_terrain != new_terrain && new_terrain != T_LAST) {
+    /* Mining would change the terrain type, clearing the irrigation
+     * in the process.  Calculate the benefit of doing so. */
+    if (ptile->city && terrain_has_flag(new_terrain, TER_NO_CITIES)) {
+      return -1;
+    }
+    ptile->terrain = new_terrain;
+    map_clear_special(map_x, map_y, S_IRRIGATION);
+    map_clear_special(map_x, map_y, S_FARMLAND);
+    goodness = city_tile_value(pcity, city_x, city_y, 0, 0);
+    ptile->terrain = old_terrain;
+    ptile->special = old_special;
+    return goodness;
+  } else if (old_terrain == new_terrain
+            && !tile_has_special(ptile, S_MINE)) {
+    /* The tile is currently unmined; mining it would put an S_MINE on it
+     * replacing any S_IRRIGATION/S_FARMLAND already there.  Calculate
      * the benefit of doing so. */
-    map_clear_special(mx, my, S_IRRIGATION);
-    map_clear_special(mx, my, S_FARMLAND);
-    map_set_special(mx, my, S_MINE);
-    m = city_tile_value(pcity, cx, cy, 0, 0);
-    ptile->special = s;
-    return m;
+    map_clear_special(map_x, map_y, S_IRRIGATION);
+    map_clear_special(map_x, map_y, S_FARMLAND);
+    map_set_special(map_x, map_y, S_MINE);
+    goodness = city_tile_value(pcity, city_x, city_y, 0, 0);
+    ptile->special = old_special;
+    assert(ptile->terrain == old_terrain);
+    return goodness;
   } else {
     return -1;
   }
+  return goodness;
 }
 
 /**************************************************************************
-  Calculates the value of doing a terrain transformation.
+  Calculate the benefit of transforming the given tile.
+
+    (map_x, map_y) is the map position of the tile.
+    (city_x, city_y) is the city position of the tile with respect to pcity.
+    pplayer is the player under consideration.
+
+  The return value is the goodness of the tile after the transform.  This
+  should be compared to the goodness of the tile currently (see
+  city_tile_value(); note that this depends on the AI's weighting
+  values).
 **************************************************************************/
-static int ai_calc_transform(struct city *pcity, int cx, int cy, int mx,
-                            int my)
+static int ai_calc_transform(struct city *pcity,
+                            int city_x, int city_y, int map_x, int map_y)
 {
-  int m;
-  struct tile *ptile = map_get_tile(mx, my);
-  enum tile_terrain_type t = ptile->terrain;
-  struct tile_type *type = get_tile_type(t);
-  enum tile_special_type s = ptile->special;
-  enum tile_terrain_type r = type->transform_result;
+  int goodness;
+  struct tile *ptile = map_get_tile(map_x, map_y);
+  enum tile_terrain_type old_terrain = ptile->terrain;
+  enum tile_special_type old_special = ptile->special;
+  struct tile_type *type = get_tile_type(old_terrain);
+  enum tile_terrain_type new_terrain = type->transform_result;
 
-  if (is_ocean(t) && !can_reclaim_ocean(mx, my)) {
+  if (old_terrain == new_terrain || new_terrain == T_LAST) {
     return -1;
   }
-  if (is_ocean(r) && !can_channel_land(mx, my)) {
+
+  if (is_ocean(old_terrain) && !is_ocean(new_terrain)
+      && !can_reclaim_ocean(map_x, map_y)) {
+    /* Can't change ocean into land here. */
+    return -1;
+  }
+  if (is_ocean(new_terrain) && !is_ocean(old_terrain)
+      && !can_channel_land(map_x, map_y)) {
+    /* Can't change land into ocean here. */
     return -1;
   }
 
-  if ((t == T_ARCTIC || t == T_DESERT || t == T_JUNGLE || t == T_SWAMP  || 
-       t == T_TUNDRA || t == T_MOUNTAINS) && r != T_LAST) {
-    if (is_ocean(r) && ptile->city) {
-      return -1;
-    }
-
-    ptile->terrain = r;
+  if (ptile->city && terrain_has_flag(new_terrain, TER_NO_CITIES)) {
+    return -1;
+  }
 
-    if (get_tile_type(r)->mining_result != r) 
-      map_clear_special(mx, my, S_MINE);
+  ptile->terrain = new_terrain;
 
-    if (get_tile_type(r)->irrigation_result != r) {
-      map_clear_special(mx, my, S_FARMLAND);
-      map_clear_special(mx, my, S_IRRIGATION);
-    }
+  if (get_tile_type(new_terrain)->mining_result != new_terrain) {
+    map_clear_special(map_x, map_y, S_MINE);
+  }
+  if (get_tile_type(new_terrain)->irrigation_result != new_terrain) {
+    map_clear_special(map_x, map_y, S_FARMLAND);
+    map_clear_special(map_x, map_y, S_IRRIGATION);
+  }
     
-    m = city_tile_value(pcity, cx, cy, 0, 0);
-    ptile->terrain = t;
-    ptile->special = s;
-    return(m);
-  } else return(-1);
+  goodness = city_tile_value(pcity, city_x, city_y, 0, 0);
+
+  ptile->terrain = old_terrain;
+  ptile->special = old_special;
+
+  return goodness;
 }
 
 /**************************************************************************