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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Remi Bonnet wrote:
> 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.

Great.

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

My mistake.

>>>+/* 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.

I recommend developing a complete patch that implements the whole 
shebang.  But then to get it into CVS this is broken into smaller 
incremental patches.

>>>/**************************************************************************
>>>@@ -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.

Well, CITY_MAP_RADIUS isn't used anywhere now; it's all CITY_MAP_SIZE. 
But I think the latter is used in four situations:

- For the diameter of the city map.
- For the radius of the city map.
- For the city center X.
- For the city center Y.

With most users being the latter two.

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

Currently CITY_MAP_CENTER_X = CITY_MAP_CENTER_Y = CITY_MAP_RADIUS.  But 
if MAP_CITY_RADIUS were turned into a server variable then 
CITY_MAP_RADIUS would/should go away.  Or what if it were redefined as 
CITY_MAP_RADIUS_SQUARED?

In some alternate universe CITY_MAP_CENTER_X and CITY_MAP_CENTER_Y might 
not even be the same.  You could have an oval citymap, for instance.

The point is, CITY_MAP_CENTER_X and CITY_MAP_CENTER_Y have fixed 
definitions that aren't likely to change.  They should also make the 
code more readible.

I'm not set on using them, but I am against replacing CITY_MAP_SIZE / 2 
with CITY_RADIUS without a better reason.

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

I don't understand?


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

Run an autogame such as the following:

   set aifill 5
   set endyear 0
   set seed 12345
   set randseed 12345
   create foo
   start

and time this game with both versions ("time ./ser -r rc").  To get good 
statistical significance, you should do it multiple times and with 
multiple autogames, but usually just a snapshot is enough to convince 
people.

You can also compare the savegames generated between the two games. 
They should be identical.

> you mean #define city_map_to_index(x, y) CITY_MAP_SIZE * y + x

Yes.  Not sure about the name...should it be city_map_pos_to_index? 
Probably it's fine without the 'pos'.

jason