[Freeciv-Dev] Re: (PR#6257) recodify map_pos<->index conversion to use native positions

["rwetmore@xxxxxxxxxxxx" <rwetmore@xxxxxxxxxxxx>]

Jason Short wrote:
> Index positions are ordered based off of native ones.  So the current 
> conversion operations (e.g., map_pos_to_index and index_to_map_pos) are 
> no good when we start using native positions.

This can be considered to be the case at the current level of gen_topologies
but it is not in fact a general invariant.

One counter example is a filtered ellipse, where native coordinates are
a rectangular grid. It would not be unreasonable to define indexed as an
outward spiral of points for use in minimizing memory storage, where the
indexed and native spaces were not in fact tightly coupled. Native would
still be most useful for any wrapping or 2-D debug/display purposes in
typical rectangular views.

> Unfortunately going through native coordinates requires an intermediate 
> pair of variables, which is a problem with macros.  There are three 
> possible solutions:
> 
> 1.  Change them both into standard functions.
> 
> This is the prettiest solution.  The only drawback is that it's slower - 
> about a 5-10% slowdown in autogame performance in a few tests I did.
> 
> 2.  Change them both into inline functions.
> 
> This is almost as pretty, and just as fast as the current code (for 
> reasonable compilers).  The only real drawback is that we're not 
> "supposed" to use inline functions in freeciv.  Also compilers that 
> don't recognize inline will give warnings in files that don't use the 
> functions (unavoidable when using inline).
> 
> 3.  Keep them as macros with temporary (global) variables in the header 
> file.
> 
> The obvious drawback is that this is incredibly ugly.  The less obvious 
> drawback is that the line

Technical merit which can be generally universally agreed upon, rather than
obscure religious or aesthetic preferences, should always be the primary
consideration. Any use of the latter without a strong majority view is almost
always a mistake, and even in this case should not hold much weight.

>    array[map_pos_to_index(x, y)] = array[map_pos_to_index(x1, y1)]
> 
> is not legal C (just like a ^= b ^= a ^= b). 

Please explain what is illegal in your map_to_index_pos() macro?

Of course a better way to code the above line to avoid issues surrounding
execution order and overwriting of globals in a global form of the macro
is to cache the index computations in local storage to guarantee sequential
execution order. Option 4 should also help by decoupling the instances.

> So without some other 
> cleverness either all lines like this will have to be changed (see 
> settlers.c and mapgen.c) or this is not an option.  Gcc 3.2.1 just gives 
> a warning, and *hopefully* compiles it into the proper code:
> 
>    settlers.c:167: warning: operation on `_fc_nat_y' may be undefined
>    settlers.c:167: warning: operation on `_fc_nat_x' may be undefined
>    settlers.c:169: warning: operation on `_fc_nat_y' may be undefined
>    settlers.c:169: warning: operation on `_fc_nat_x' may be undefined
> 
> If only C had tuples...

   You might try initializing the global storage elements explicitly in map.c
to get rid of noise warnings, though I suspect that you actually have a poorly
defined macro somewhere that makes its assignment in a conditional and then
(probably mistakenly) uses it, i.e. the warning is a valid hint. The code in
question is not executable in my CVS tree, so a more complete listing of code
here might help in debugging your particular problems.


   4. Pass in local references for working storage variables vs using well
      documented global locations.

> Patches for all three variants are attached.  The macro one won't 
> compile under strictly conformant C compilers.
> 
> jason
[...]
> Index: common/map.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
> retrieving revision 1.144
> diff -u -r1.144 map.c
> --- common/map.c      2003/09/23 16:10:23     1.144
> +++ common/map.c      2003/09/23 17:39:51
> @@ -31,6 +31,9 @@
>  
>  #include "map.h"
>  
> +/* Intermediate variables used in map.h macros. */
> +int _fc_nat_x, _fc_nat_y;
> +
>  /* the very map */
>  struct civ_map map;
>  
> Index: common/map.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
> retrieving revision 1.153
> diff -u -r1.153 map.h
> --- common/map.h      2003/09/19 13:17:12     1.153
> +++ common/map.h      2003/09/23 17:39:51
> @@ -235,21 +235,25 @@
>  #define map_adjust_y(Y) \
>    (((Y)<0) ? 0 : (((Y)>=map.ysize) ? map.ysize-1 : (Y)))
>  
> +extern int _fc_nat_x, _fc_nat_y;
> +
>  #define native_to_map_pos(pmap_x, pmap_y, nat_x, nat_y) \
>    (*(pmap_x) = (nat_x), *(pmap_y) = (nat_y))
>  
>  #define map_to_native_pos(pnat_x, pnat_y, map_x, map_y) \
>    (*(pnat_x) = (map_x), *(pnat_y) = (map_y))
>  
> -#define map_pos_to_index(map_x, map_y)        \
> -  (CHECK_MAP_POS((map_x), (map_y)),           \
> -   (map_x) + (map_y) * map.xsize)
> +#define map_pos_to_index(map_x, map_y)                                      \
> +  (CHECK_MAP_POS((map_x), (map_y)),                                         \
> +   map_to_native_pos(&_fc_nat_x, &_fc_nat_y, map_x, map_y),                 \
> +   (_fc_nat_x) + (_fc_nat_y) * map.xsize)
>  
>  /* index_to_map_pos(int *, int *, int) inverts map_pos_to_index */
> -#define index_to_map_pos(pmap_x, pmap_y, index) \
> -  (CHECK_INDEX(index),                          \
> -   *(pmap_x) = (index) % map.xsize,             \
> -   *(pmap_y) = (index) / map.xsize)
> +#define index_to_map_pos(pmap_x, pmap_y, index)                             \
> +  (CHECK_INDEX(index),                                                      \
> +   *(pmap_x) = (index) % map.xsize,                                         \
> +   *(pmap_y) = (index) / map.xsize,                                         \
> +   native_to_map_pos(pmap_x, pmap_y, *(pmap_x), *(pmap_y)))
>  
>  #define DIRSTEP(dest_x, dest_y, dir) \
>  (    (dest_x) = DIR_DX[(dir)],       \