[Freeciv-Dev] Re: server/settlers.c cleanup 3

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Per I Mathisen <per@xxxxxxxxxxx> wrote:
<snip>
> > Index: server/unittools.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
> retrieving revision 1.174
> diff -u -r1.174 unittools.c
> --- server/unittools.c        2002/04/06 10:52:19     1.174
> +++ server/unittools.c        2002/04/13 21:51:23
> @@ -2386,8 +2386,7 @@
>  static void hut_get_city(struct unit *punit)
>  {
>    struct player *pplayer = unit_owner(punit);
> -  
> -  if (is_ok_city_spot(punit->x, punit->y)) {
> +  if (city_desirability(unit_owner(punit), punit->x, punit->y) > 0) {
>      notify_player_ex(pplayer, punit->x, punit->y, E_HUT_CITY,
>                    _("Game: You found a friendly city."));
>      create_city(pplayer, punit->x, punit->y,
> Index: server/settlers.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/server/settlers.c,v
> retrieving revision 1.138
> diff -u -r1.138 settlers.c
> --- server/settlers.c 2002/04/04 03:51:05     1.138
> +++ server/settlers.c 2002/04/13 21:51:24
> @@ -13,6 +13,8 @@
>  #include <stdio.h>
>  #include <string.h>
>  #include <assert.h>
> +#include <math.h>
> +#include <float.h>

I've had a long argument with Raimar about this. Good luck to you Per. I fully
agreed with the old patch, and I agree with your current approach as well. I
think the ridiculous extremes the Freeciv code goes to to avoid floating point
is pointless. 

>  #include "city.h"
>  #include "game.h"
> @@ -39,18 +41,12 @@
>  static unsigned int territory[MAP_MAX_WIDTH][MAP_MAX_HEIGHT];
>  /* negative: in_city_radius, 0: unassigned, positive: city_des */
>  
> -/*************************************************************************/
> -
> -static int city_desirability(struct player *pplayer, int x, int y);
> -

I don't understand this line. You call this function, and now your remove the
function prototype?

>  static void auto_settlers_player(struct player *pplayer); 
> -
>  static bool is_already_assigned(struct unit *myunit, struct player *pplayer,
>                               int x, int y);
>  

>  /**************************************************************************
> -amortize(benefit, delay) returns benefit * ((MORT - 1)/MORT)^delay
> -(^ = to the power of)
> +  Amortize means gradually paying off a cost or debt over time. In freeciv
> +  terms this means we calculate how much less worth something is to us
> +  depending on how long it will take to complete.
>  
> -Plus, it has tests to prevent the numbers getting too big.  It takes
> -advantage of the fact that (23/24)^12 approximately = 3/5 to chug through
> -delay in chunks of 12, and then does the remaining multiplications of
> (23/24).
> +  amortize(benefit, delay) returns benefit * ((MORT - 1)/MORT)^delay
> +  (^ = to the power of)
>  **************************************************************************/
>  int amortize(int benefit, int delay)
>  {

> +  float amortval;
>    assert(delay >= 0);

> +  amortval = pow(((float)MORT -1) / MORT, delay);
> +  return (benefit * amortval *(1+FLT_EPSILON));
>  }

I love it. Clear, simple and to the point.
  
>  /**************************************************************************
> -...
> +  The minimap marks areas that are occupied by other cities, so we
> +  don't settle in their area. This also include cities we plan on
> +  settling.
>  **************************************************************************/

Time for some comment removal from ai.

<snip>
> +  Remove a city from the minimap. We only do this when we become uncertain
> +  if our settler will really settle there. That happens a lot, 
> +  unfortunately. 
> +
> +  FIXME: It does not remove a planned city from the minimap if the
> +  settler planned to make this city dies! The result is that the AI
> +  never builds a city at this location. - Per
>  **************************************************************************/

What's the solution for this? Every time a unit dies, check if it is type
settler and pplayer is ai?


>  /**************************************************************************
> -this whole funct assumes G_REP^H^H^HDEMOCRACY -- Syela
> +  Calculates the desire for founding a new city at (x, y). This 
> +  information is cached in the minimap at (x, y) for the remainder of
> +  the game.
> +
> +  FIXME: We need to use a virtual unit here. Some defines added for
> +  now.- Per
> +
> +  this whole funct assumes G_REP^H^H^HDEMOCRACY -- Syela
>  **************************************************************************/
> -static int city_desirability(struct player *pplayer, int x, int y)
> +int city_desirability(struct player *pplayer, int x, int y)
>  {
> +#define SETTLER_COST 40
> +#define SETTLER_LOSS_CHANCE 12 /* totally arbitrary Syelaism */
> +#define TEMPLE_COST 40
>    int taken[5][5], food[5][5], shield[5][5], trade[5][5];
>    int irrig[5][5], mine[5][5], road[5][5];
>    int f, n;
> @@ -171,8 +168,7 @@
>    int d = 0;
>    int a, i0, j0; /* need some temp variables */
>    int temp=0, tmp=0;
> -  bool debug = FALSE;
> -  int g = 1;

Not sure removing g is correct. It's used elsewhere, this is probably an
attempt, shock horror, to be consistent.

> +  bool debug = FALSE; /* turn on extra LOG_DEBUG output */
>    struct tile *ptile;
>    int con, con2;
>    bool har;
> @@ -198,15 +194,6 @@
>  
>    con = ptile->continent;
>  
> -/* not worth the computations AFAICT -- Syela
> -   wrapped anyway in case it comes back -- dwp
> -  if (improvement_variant(B_PYRAMIDS)==0) {     
> -    city_list_iterate(pplayer->cities, acity) 
> -      if (city_got_building(acity, B_PYRAMIDS)) g++;
> -    city_list_iterate_end;
> -  }
> -*/
> -
>    memset(taken, 0, sizeof(taken));
>    memset(food, 0, sizeof(food));
>    memset(shield, 0, sizeof(shield));
> @@ -279,9 +266,9 @@
>      val = (shield[ii][jj] + mine[ii][jj]) +
>            (food[ii][jj] + irrig[ii][jj]) * FOOD_WEIGHTING + /* seems to be
> needed */
>            (trade[ii][jj] + road[ii][jj]);
> -    val -= amortize(40 * SHIELD_WEIGHTING + (50 - 20 * g) * FOOD_WEIGHTING,
> 12);
> -    /* 12 is arbitrary; need deterrent to represent loss
> -       of a settlers -- Syela */
> +    /* FIXME: Syela got to be kidding about this one - Per */
> +    val -= amortize(SETTLER_COST * SHIELD_WEIGHTING + (50 - 20) * 
> +                    FOOD_WEIGHTING, SETTLER_LOSS_CHANCE);
>      freelog(LOG_DEBUG, "Desire to immigrate to %s = %d -> %d",
>                 pcity->name, val, (val * 100) / MORT / 70);
>      return(val);
> @@ -299,7 +286,7 @@
>    db = get_tile_type(map_get_terrain(x, y))->defense_bonus;
>    if (map_has_special(x, y, S_RIVER))
>      db += (db * terrain_control.river_defense_bonus) / 100;
> -  val += (4 * db - 40) * SHIELD_WEIGHTING;
> +  val += (4 * db - SETTLER_COST) * SHIELD_WEIGHTING;
>    /* don't build cities in danger!! FIX! -- Syela */
>    val += 8 * MORT; /* one science per city */
>    
> @@ -367,10 +354,11 @@
>        }
>      }
>      if (best == 0) break;
> -    if (f > 0) d += (game.foodbox * MORT * n + (f*g) - 1) / (f*g);
> +    if (f > 0) d += (game.foodbox * MORT * n + f - 1) / f;
>      if (n == 4) {
> -      val -= amortize(40 * SHIELD_WEIGHTING + (50 - 20 * g) *
> FOOD_WEIGHTING, d); /* lers */
> -      temp = amortize(40 * SHIELD_WEIGHTING, d); /* temple */
> +      val -= amortize(SETTLER_COST * SHIELD_WEIGHTING + (50 - 20) * 
> +                      FOOD_WEIGHTING, d); /* settlers */
> +      temp = amortize(TEMPLE_COST * SHIELD_WEIGHTING, d); /* temple */
>        tmp = val;
>      }
>    }
> @@ -387,16 +375,20 @@
>    }
>    return(val);
>  }
> +#undef TEMPLE_COST
> +#undef SETTLER_COST
> +#undef SETTLER_LOSS_CHANCE
>  
>  /**************************************************************************
> -...
> +  Manages settlers.
>  **************************************************************************/
>  void ai_manage_settler(struct player *pplayer, struct unit *punit)
>  {
>    punit->ai.control = TRUE;
> -  if (punit->ai.ai_role == AIUNIT_NONE) /* if BUILD_CITY must remain
> BUILD_CITY */
> +  /* if BUILD_CITY must remain BUILD_CITY, otherwise turn into autosettler
> */
> +  if (punit->ai.ai_role == AIUNIT_NONE) {
>      punit->ai.ai_role = AIUNIT_AUTO_SETTLER;
> -/* gonna handle city-building in the auto-settler routine -- Syela */
> +  }
>    return;
>  }

Where is city building handled now? I thought Syela's comment was right.
  
> @@ -421,16 +413,8 @@
>    return TEST_BIT(map_get_tile(x, y)->assigned, pplayer->player_no);
>  }
>  
> -/*************************************************************************/
> -
> -/* all of the benefit and ai_calc routines rewritten by Syela */
> -/* to conform with city_tile_value and related calculations elsewhere */
> -/* all of these functions are VERY CPU-inefficient and are being cached */
> -/* I don't really want to rewrite them and possibly screw them up. */
> -/* The cache should keep the CPU increase linear instead of quadratic. --
> Syela */
> -
>  /**************************************************************************
> -...
> +  Calculates the value of removing pollution.
>  **************************************************************************/
>  static int ai_calc_pollution(struct city *pcity, int cx, int cy, int best,
>                            int mx, int my)
> @@ -446,7 +430,7 @@
>  }
>  
>  /**************************************************************************
> -...
> +  Calculates the value of removing fallout.
>  **************************************************************************/
>  static int ai_calc_fallout(struct city *pcity, struct player *pplayer,
>                          int cx, int cy, int best, int mx, int my)
> @@ -463,7 +447,9 @@
>  }
>  
>  /**************************************************************************
> -...
> +  Returns TRUE if tile at x, y is useful as a source of irrigation. If
> +  the given player is an AI, it will ignore fog of war. (Do not "fix" this,
> +  since the AI does too little exploration yet to manage without this.)
>  **************************************************************************/
>  static bool is_wet(struct player *pplayer, int x, int y)
>  {
> @@ -480,7 +466,8 @@
>  }
>  
>  /**************************************************************************
> -...
> +  Returns TRUE if there is an irrigation source adjacent to the given x, y
> +  position.
>  **************************************************************************/
>  static bool is_wet_or_is_wet_cardinal_around(struct player *pplayer, int x,
>                                           int y)
> @@ -539,31 +526,14 @@
>  }
>  
>  /**************************************************************************
> -...
> +  Calculates the value of building a mine.
>  **************************************************************************/
>  static int ai_calc_mine(struct city *pcity, int cx, int cy, int mx, int my)
>  {
>    int m;
>    struct tile *ptile = map_get_tile(mx, my);
>  
> -#if 0
> -  enum tile_terrain_type t = ptile->terrain;
> -  struct tile_type *type = get_tile_type(t);
> -  int s = ptile->special;
> -
> -  if (ptile->terrain != type->mining_result &&
> -      type->mining_result != T_LAST) { /* EXPERIMENTAL 980905 -- Syela */
> -    ptile->terrain = type->mining_result;
> -    map_clear_special(x, y, S_FARMLAND);
> -    map_clear_special(x, y, S_IRRIGATION);
> -    m = city_tile_value(pcity, i, j, 0, 0);
> -    ptile->terrain = t;
> -    ptile->special = s;
> -    return(m);
> -  } else 
> -#endif
> -
> -  /* Note that this code means we will never try to mine a city into the
> ocean */
> +  /* Don't replace existing infrastructure */
>    if ((ptile->terrain == T_HILLS || ptile->terrain == T_MOUNTAINS) &&
>        !tile_has_special(ptile, S_IRRIGATION) && !tile_has_special(ptile,
> S_MINE)) {
>      map_set_special(mx, my, S_MINE);
> @@ -574,7 +544,7 @@
>  }
>

Why was that old junk kept for so damn long?
  
>  /**************************************************************************
> -...
> +  Calculates the value of doing a terrain transformation.
>  **************************************************************************/
>  static int ai_calc_transform(struct city *pcity, int cx, int cy, int mx,
>                            int my)
> @@ -626,6 +596,8 @@
>    struct tile *ptile;
>    bool is_border = IS_BORDER_MAP_POS(x, y, 2);
>    
> +  assert(spc == S_ROAD || spc == S_RAILROAD);
> +
>    if (!normalize_map_pos(&x, &y))
>      return 0;
>  
> @@ -663,7 +635,7 @@
>  }

Good. 
  
>  /**************************************************************************
> -...
> +  Calculate the value of building a road.
>  **************************************************************************/
>  static int ai_calc_road(struct city *pcity, struct player *pplayer,
>                       int cx, int cy, int mx, int my)
> @@ -683,7 +655,7 @@
>  }
>  
>  /**************************************************************************
> -...
> +  Calculate the value of building a railroad.
>  **************************************************************************/
>  static int ai_calc_railroad(struct city *pcity, struct player *pplayer,
>                           int cx, int cy, int mx, int my)
> @@ -704,57 +676,6 @@
>    /* bonuses for adjacent railroad tiles */
>  }
>  
> -/*************************************************************************
> -  return how good this square is for a new city.
> -**************************************************************************/
> -bool is_ok_city_spot(int x, int y)
> -{
> -  int dx, dy;
> -
> -  switch (map_get_terrain(x,y)) {
> -  case T_OCEAN:
> -  case T_UNKNOWN:
> -  case T_MOUNTAINS:
> -  case T_FOREST:
> -  case T_HILLS:
> -  case T_ARCTIC:
> -  case T_JUNGLE:
> -  case T_SWAMP:
> -  case T_TUNDRA:
> -  case T_LAST:
> -    return FALSE;
> -  case T_DESERT:
> -    if (!map_has_special(x, y, S_SPECIAL_1)
> -     && !map_has_special(x, y, S_SPECIAL_2))
> -      return FALSE;
> -  case T_GRASSLAND:
> -  case T_PLAINS:
> -  case T_RIVER:
> -    break;
> -  default:
> -    break;
> -  }
> -
> -  players_iterate(pplayer) {
> -    city_list_iterate(pplayer->cities, pcity) {
> -      if (map_distance(x, y, pcity->x, pcity->y)<=8) {
> -     map_distance_vector(&dx, &dy, pcity->x, pcity->y, x, y);
> -     dx = abs(dx), dy = abs(dy);
> -     /* these are heuristics... */
> -        if (dx<=5 && dy<5)
> -          return FALSE;
> -        if (dx<5 && dy<=5)
> -          return FALSE;
> -     /* this is the law... */
> -     if (dx<game.rgame.min_dist_bw_cities && 
> dy<game.rgame.min_dist_bw_cities)

I'm not sure that city desire takes min_dis_bw_cities into account.


<snip>

> +  Tries to find a boat for our settler. Requires warmap to be initialized
> +  with respect to x, y. cap is the requested capacity on the transport.
> +  Note that it may return a transport with less than cap capacity if this
> +  transport has zero move cost to x, y.
> +
> +  The "virtual boats" code is not used. It is probably too unreliable, 
> +  since the AI switches its production back and forth continously.
>  **************************************************************************/
> -int find_boat(struct player *pplayer, int *x, int *y, int cap)
> -{ /* this function uses the current warmap, whatever it may hold */
> -/* unit is no longer an arg!  we just trust the map! -- Syela */
> -  int best = 22, id = 0; /* arbitrary maximum distance, I will admit! */
> +Unit_Type_id find_boat(struct player *pplayer, int *x, int *y, int cap)
> +{
> +  int best = 22; /* arbitrary maximum distance, I will admit! */
> +  Unit_Type_id id = 0;
>    unit_list_iterate(pplayer->units, aunit)
>      if (is_ground_units_transport(aunit)) {
>        if (warmap.cost[aunit->x][aunit->y] < best &&
> @@ -809,7 +736,8 @@
>  }
>  
>  /**************************************************************************
> -...
> +  Returns TRUE if there are (other) ground units than punit stacked on
> +  punit's tile.
>  **************************************************************************/
>  struct unit *other_passengers(struct unit *punit)
>  {
> @@ -860,17 +788,21 @@
>  }
>  
>  /**************************************************************************
> -...
> +  Returns how much food a settler will consume out of the city's foodbox
> +  when created.
> +
> +  FIXME: This function should be moved into common/unittype.c - Per
>  **************************************************************************/
> -static int unit_food_cost(struct unit *punit)
> +static int unit_foodbox_cost(struct unit *punit)

Much better name.

It looks ok so far. I'll have to run some savegames and come back to you on
any bizarre behaviour.

__________________________________________________
Do You Yahoo!?
Yahoo! Tax Center - online filing with TurboTax
http://taxes.yahoo.com/