[Freeciv-Dev] Re: (PR#6285) rand_map_pos using native coordinates

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

Try ...

   do {
     *x = myrand(map.xsize);
     *y = myrand(map.ysize);
     native_to_map_pos(x, y, *x, *y);
   } while (!normalize_map_pos(x, y));

This is actually a better solution, since it is possible under gen-topologies
to have unreal positions arise from filter constraints, rather than geometric
boundaries. The classic example is an elliptical map which fits inside the
native rectangle.

The assert() below is a bug in these cases, which are perfectly allowed, not
terminal conditions, and turning off assert() may result in unreal, aka bad,
coordinates being passed to code.

The while check is minimal noise to guarantee such cases are correctly handled
by looping but a second iteration is seldom required unless a significant 
fraction
of the native space is filtered.

The use of normalize_map_pos() is also required.

1)  because is_normal_map_pos() is a deprecated function and should never be
     used in new code.

2)  normalize_map_pos() is more efficient than is_normal_map_pos().

3)  native_to_map_pos() does not (and should never be constrained) to produce
     normalized map_postions - it is just the simple mathematical transformation
     function between two coordinate representations.

     Thus normalize_map_pos() insures that even if a set of native coordinates
     does not transform into some arbitrary definition of the normal set of map
     positions, the appropriate wrapping or whatever operations will be done to
     produce the associated normalized map coordinate pair, or if no such
     normalizing transformation exists, the unreal result is returned.

This code is much closer to the original, which apart from the previously
reported is_normal_map_pos() bug and need to add the missing native_to_map_pos()
really wasn't far off in covering the relevant concepts, unlike the two newer
suggestions.

Cheers,
RossW
=====

Jason Short wrote:
> Jason Short wrote:
> 
>>This simple patch rewrites rand_map_pos to use native coordinates.  This 
>>allows it to work with iso-maps.
> 
> 
>>+  native_to_map_pos(x, y, myrand(map.xsize), myrand(map.ysize));
> 
> 
> Except, with a macro form of native_to_map_pos, that will not work.  At 
> all.  This new patch is better.
> 
> 
> FYI, in gen-topologies (which is pretty well tested) the function looks like
> 
> +  int nat_x, nat_y;
> +
> +  nat_x = myrand(map.xsize);
> +  nat_y = myrand(map.ysize);
> +  native_to_map_pos(x, y, nat_x, nat_y);
> +  assert(is_normal_map_pos(*x, *y));
> 
> which is close to the attached version.  Just a matter of taste, I suppose.
> 
> jason
> 
> 
> 
> ------------------------------------------------------------------------
> 
> Index: common/map.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
> retrieving revision 1.145
> diff -u -r1.145 map.c
> --- common/map.c      2003/09/25 20:55:01     1.145
> +++ common/map.c      2003/09/25 21:10:09
> @@ -1489,10 +1489,11 @@
>  **************************************************************************/
>  void rand_map_pos(int *x, int *y)
>  {
> -  do {
> -    *x = myrand(map.xsize);
> -    *y = myrand(map.ysize);
> -  } while (!is_normal_map_pos(*x, *y));
> +  int nat_x = myrand(map.xsize), nat_y = myrand(map.ysize);
> +
> +  /* Don't pass non-deterministic expressions to native_to_map_pos! */
> +  native_to_map_pos(x, y, nat_x, nat_y);
> +  CHECK_MAP_POS(*x, *y);
>  }
>  
>  /**************************************************************************