[Freeciv-Dev] Re: (PR#6170) Alternative city square utilizations

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Remi Bonnet wrote:
> Ok first part of the work.
> 
> With this patch you can change the defines in city.h and the game will
> run. (that was not so easy i thought first so i send the patch)
> 
> Tested with CITY_MAP_RADIUS 3 and really quickly with CITY_MAP_RADIUS 1.
> Please review. 
> 
> Note: you need to set correctly both CITY_MAP_RADIUS && CITY_TILES.
> The attached patch make CITY_MAP_RADIUS 3 (but you can backchange it)

There's some good stuff in here, but it's too much for one patch.  Could 
you separate this?  The first patch should just have the routine 
substitutions; e.g., substituting CITY_MAP_SIZE or CITY_RADIUS in for 
fixed values.

Also some specific comments below:

> @@ -64,7 +64,7 @@
>       * subtract off half a tile in each direction.  For a more
>       * rigorous example, see map_pos_to_canvas_pos().
>       */
> -    int iso_x = (city_x - city_y) - (-4);
> +    int iso_x = (city_x - city_y) + (2 * CITY_MAP_RADIUS);
>      int iso_y = (city_x + city_y) - (0);

This changes the math.  Why?

> @@ -74,10 +74,8 @@
>      *canvas_y = city_y * NORMAL_TILE_HEIGHT;
>    }
>  
> -  if (!is_valid_city_coords(city_x, city_y)) {
> -    assert(FALSE);
> -    return FALSE;
> -  }
> +  assert(is_valid_city_coords(city_x, city_y));
> +
>    return TRUE;
>  }

This probably isn't so good.  If you're given invalid city coordinates 
and you've compiled without debugging, you don't want TRUE to be returned.

>  /* Iterate a city map, from the center (the city) outwards */
> +int** city_map_iterate_outwards_indices;

Maybe make this a struct map_position * instead?

> +/* Unused for the moment. Later maybe... */
> +int*  available_tiles;

Huh?

> +/* Chained list Structure and helper function for generate_city_map_indices 
> */
> +struct chained_dists {
> +  int dist;
> +  int x, y;
> +  struct chained_dists *prev;
> +  struct chained_dists *next;
>  };

I haven't looked closely at the iterate_indices generation yet.

>  /**************************************************************************
> @@ -78,15 +81,22 @@
>  **************************************************************************/
>  bool is_valid_city_coords(const int city_x, const int city_y)
>  {
> -  if ((city_x == 0 || city_x == CITY_MAP_SIZE-1)
> -      && (city_y == 0 || city_y == CITY_MAP_SIZE-1))
> -    return FALSE;
> -  if (city_x < 0 || city_y < 0
> -      || city_x >= CITY_MAP_SIZE
> -      || city_y >= CITY_MAP_SIZE)
> +  /* Why +1 here? Small schemas:
> +   *   Without   |     With
> +   *    3               333
> +   *  33233            32223  
> +   *  32123           3211123
> +   * 3210123          3210123 
> +   *  32123           3211123  
> +   *  33233            32223  
> +   *    3               333    */
> +  if (CITY_MAP_RADIUS * CITY_MAP_RADIUS + 1>= 
> +      (city_x - CITY_MAP_RADIUS) * (city_x - CITY_MAP_RADIUS) +
> +      (city_y - CITY_MAP_RADIUS) * (city_y - CITY_MAP_RADIUS)) {
> +    return TRUE;
> +  } else {
>      return FALSE;
> -
> -  return TRUE;
> +  }
>  }

But what if the player wants the first scenario?

More reasonably, what if you wanted

     1                  111
    101   instead of    101
     1                  111

?  I think there should be a CITY_MAP_RAIDUS_SQUARED defined 
independently of the CITY_MAP_RADIUS, to give greater flexibility.

>  /**************************************************************************
> @@ -108,8 +117,8 @@
>  {
>    map_distance_vector(city_map_x, city_map_y,
>                     city_center_x, city_center_y, map_x, map_y);
> -  *city_map_x += CITY_MAP_SIZE / 2;
> -  *city_map_y += CITY_MAP_SIZE / 2;
> +  *city_map_x += CITY_MAP_RADIUS;
> +  *city_map_y += CITY_MAP_RADIUS;
>    return is_valid_city_coords(*city_map_x, *city_map_y);
>  }

Changes like this seem mostly like noise.  But as long as we're doing 
it, what about adding a

   #define CITY_MAP_CENTER_X (CITY_MAP_SIZE / 2)

to city.h, and just use that value instead.  That should make things 
more readible and easier to change in the future (if it's ever necessary).

> diff -u -r -Xdiff_ignore ../freeciv/common/city.h ./common/city.h
> --- ../freeciv/common/city.h  2003-10-02 15:11:10.000000000 +0200
> +++ ./common/city.h   2003-10-15 19:11:39.000000000 +0200
> @@ -58,13 +58,17 @@
>  /* for new city: default auto-attack options all on, others off: */
>  #define CITYOPT_DEFAULT (CITYOPT_AUTOATTACK_BITS)
>  
> +
> +/* radius of workable area */
> +#define CITY_MAP_RADIUS 3
> +

This needs a comment: changing this requires adding a new network 
capability and updating CITY_TILES.

>  /* Diameter of the workable city area. Must be an odd number.
>     Some places in the code hardcodes this number, fx 
>     city_map_iterate_outwards_indices */
> -#define CITY_MAP_SIZE 5
> +#define CITY_MAP_SIZE (CITY_MAP_RADIUS * 2 + 1) 
>  
>  /* Number of tiles a city can use */
> -#define CITY_TILES (CITY_MAP_SIZE * CITY_MAP_SIZE - 4)
> +#define CITY_TILES 37

It would be nice to have CITY_TILES be generated by code; it's a pain to 
do it by hand.  But I haven't looked at how hard this is.

>  #define INCITE_IMPOSSIBLE_COST (1000 * 1000 * 1000)
>  
> @@ -82,38 +86,39 @@
>  
>  /* Iterate a city map */
>  
> -#define city_map_iterate(x, y)                     \
> -{                                                  \
> -  int x, y;                                        \
> -  for (y = 0; y < CITY_MAP_SIZE; y++)              \
> -    for (x = 0; x < CITY_MAP_SIZE; x++)            \
> -      if (! ((x == 0 || x == (CITY_MAP_SIZE-1)) && \
> -          (y == 0 || y == (CITY_MAP_SIZE-1))) )
> +#define city_map_iterate(x, y)                                \
> +{                                                             \
> +  int x, y;                                                   \
> +  for (y = 0; y < CITY_MAP_SIZE; y++)                         \
> +    for (x = 0; x < CITY_MAP_SIZE; x++)                       \
> +      if (is_valid_city_coords(x, y))
>  
>  #define city_map_iterate_end                       \
>  }

This makes things slower.  Can you give numbers on how much slower the 
game is even keeping CITY_MAP_RADIUS as 2?

> -     for (j = 0; j < 5; j++) {
> -       dest[city_map_index[i++]] = '0';
> +      for (y = 0; y < CITY_MAP_SIZE; y++) {
> +     for (x = 0; x < CITY_MAP_SIZE; x++) {
> +       if (is_city_center(x, y)) {
> +         dest[CITY_MAP_SIZE * y + x] = '1';
> +         continue;
> +       }
> +
> +       if (!is_valid_city_coords(x, y)) {
> +         dest[CITY_MAP_SIZE * y + x] = '2';
> +       } else {
> +         dest[CITY_MAP_SIZE * y + x] = '0';
> +       }

city_map_index should stay, but it should be renamed as 
city_map_to_index (or something like that).  And it should be a macro, 
obviously.

> diff -u -r -Xdiff_ignore ../freeciv/server/citytools.c ./server/citytools.c
> --- ../freeciv/server/citytools.c     2003-10-13 19:38:20.000000000 +0200
> +++ ./server/citytools.c      2003-10-13 19:39:46.000000000 +0200
> @@ -1580,78 +1580,81 @@
>  {
>    int i, x, y;
>    char *p;
> -  packet->id=pcity->id;
> -  packet->owner=pcity->owner;
> -  packet->x=pcity->x;
> -  packet->y=pcity->y;
> +  packet->id = pcity->id;
> +  packet->owner = pcity->owner;
> +  packet->x = pcity->x;
> +  packet->y = pcity->y;
>    sz_strlcpy(packet->name, pcity->name);

Avoid noise like this when you're making a large-ish patch.

-----

This looks like a good step in the right direction.  The only thing that 
would hold it out of CVS (aside from the indices implementation, which I 
haven't looked at) is speed issues (which some people are very concerned 
with...).  Hopefully the numbers will show that it is not a major impact.

jason