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

["Remi Bonnet" <remi.bonnet@xxxxxxxxxxx>]

Jason Short wrote:

>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.
>  
>
ok, i will split the patch.

>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?
>  
>
Why does this change the math? - ( - X) = + X ? and CITY_MAP_RADIUS = 2 
=> 2 * CITY_MAP_RADIUS = 4.

>  
>
>>@@ -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.
>  
>
oh, ok.

>  
>
>> /* Iterate a city map, from the center (the city) outwards */
>>+int** city_map_iterate_outwards_indices;
>>    
>>
>
>Maybe make this a struct map_position * instead?
>  
>
yes.

>  
>
>>+/* Unused for the moment. Later maybe... */
>>+int*  available_tiles;
>>    
>>
>
>Huh?
>  
>
Sorry. When i write this patch, i thought that it will never be applied 
before it's complete and as generate_city_map_indices already fill this 
array, i add this, even if it's not used.

I will think to that when i'm spliting the patch.

>  
>
>>+/* 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?
>  
>
Because it's current behaviour for radius 2

>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.
>  
>
That could be a good idea, but i think it's better if i try first to 
make the game run without this define. Later why not...

>  
>
>> /**************************************************************************
>>@@ -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).
>  
>
A reason for why i change this is that i don't like the round down 
behaviour. Of course 5 / 2 = 2 but i prefer making it more explicit. And 
as radius is defined and CITY_MAP_SIZE linked to CITY_MAP_RADIUS, i 
don't think that changes will be harder. CITY_MAP_RADIUS is already used 
in many places.
And i don't understand why you want to add this define. Actually,  
CITY_MAP_CENTER_X = CITY_MAP_CENTER_Y = CITY_MAP_RADIUS.
If you want to add this line, it's even better to call #define 
CITY_MAP_CENTER_X CITY_MAP_RADIUS but i still don't see the point.

>  
>
>>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.
>  
>
Yes but looks below for 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.
>  
>
I know but it's a greater pain to generate it by code. I haven't found a 
simple way to generate it and calling an over-sized expression may slow 
the code.
And remember that this is only temporary, later it will be generated by 
available_tiles[CITY_MAP_RADIUS]

>> #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?
>  
>
How can i do that?

>  
>
>>-     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.
>  
>
you mean #define city_map_to_index(x, y) CITY_MAP_SIZE * y + x
looks good...

>  
>
>>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.
>  
>
ok, i will avoid that in the future.

>-----
>
>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
>
>
>
>  
>