[Freeciv-Dev] Re: (PR#4718) topology cleanup for client autocenter code

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

I think you quit reading again ... I have reposted the fix supplied with
explanation to kickstart the process.

Note, your fix to the fix is wrong as you admit. Iterate_outwards can
take an arbitrarily large dimension value, and since it is a single value
(at least in the CVS code I am looking at) it has to correctly handle map
edge truncation.

It is the case that (xm + ym) will always cover the full map, while the
MAX of either one will fail in iso-native topologies. The sum is also
more efficient than the compare, and one should never write bad code when
good is trivially available.
 

/* This iterates outwards from the starting point (Duh?).
    Every tile within max_dist will show up exactly once. (even takes
    into account wrap). All positions given correspond to real tiles.
    The values given are adjusted.
    You should make sure that the arguments passed to the macro are adjusted,
    or you could have some very nasty intermediate errors.
 

    Internally it works by for a given distance
    1) assume y positive and iterate over x
    2) assume y negative and iterate over x
    3) assume x positive and iterate over y
    4) assume x negative and iterate over y
    Where in this we are is decided by the variables xcycle and positive.
    each of there distances give a box of tiles; to ensure each tile is only
    returned once we only return the corner when iterating over x.
    As a special case positive is initialized as 0 (ie we start in step 2) ),
    as the center tile would else be returned by both step 1) and 2).
*/
#define iterate_outward(ARG_start_x, ARG_start_y, ARG_max_dist, ARG_x_itr, 
ARG_y_itr) \
{

Cheers,
RossW
=====

Actually, the correct solution is probably even easier and certainly less
code change impact.

I can't actually fathom why you are so in love with this anti-pattern,
that you would do a whole_map_iterate with sqr test for something
that seldom requires more than a handful of iterations at most with an
appropriate iterator.

Cheers,
RossW
=====

      /* Just any known tile will do; search near the middle first. */
+   int x0, y0, xm = map.xsize/2, ym = map.ysize/2;
+   native_to_map_pos(&x0, &y0, xm, ym);
+   iterate_outward(x0, y0, xm + ym, x, y) {
                             ^^^^^^^
-   iterate_outward(map.xsize / 2, map.ysize / 2,
-                   MAX(map.xsize / 2, map.ysize / 2), x, y) {
        if (tile_get_known(x, y) != TILE_UNKNOWN) {
        center_tile_mapcanvas(x, y);
        goto OUT;
      } iterate_outward_end;
      /* If we get here we didn't find a known tile.
         Refresh a random place to clear the intro gfx. */
      center_tile_mapcanvas(map.xsize / 2, map.ysize / 2);
    OUT:
      ;                         /* do nothing */


Jason Short wrote:
> rwetmore@xxxxxxxxxxxx wrote:
> 
>>Actually, the correct solution is probably even easier and certainly less
>>code change impact.
> 
> 
> So what would that correct solution be?  I looked at the most recent 
> corecleanups (1yr ago) to see if you had any clever ideas, but the 
> solution you use there is wrong.
> 
> Admittedly this code is not called very much and isn't particularly 
> important, so it may be okay to have slightly wrong behavior in exchange 
> for shorter code.  But I'm not in favor of misusing native dimensions as 
> map ones; it sets a bad example.  And the logic also works the othe way: 
> since the code is not particularly important it's okay to have it be a 
> little bit inefficient.
> 
>       assert(punit != NULL);
>       center_tile_mapcanvas(punit->x, punit->y);
>     } else {
> +    int  x0 = map.xsize / 2, y0 = map.ysize / 2;
> +    native_to_map_pos(&x0, &y0, x0, y0);
> +
>       /* Just any known tile will do; search near the middle first. */
> -    iterate_outward(map.xsize / 2, map.ysize / 2,
> -                 MAX(map.xsize / 2, map.ysize / 2), x, y) {
> +    iterate_outward(x0, y0, MAX(map.xsize / 2, map.ysize / 2), x, y) {
>         if (tile_get_known(x, y) != TILE_UNKNOWN) {
>       center_tile_mapcanvas(x, y);
>       goto OUT;
> @@ -525,13 +526,14 @@
>       }
>       iterate_outward_end;
>       /* If we get here we didn't find a known tile.
> -       Refresh a random place to clear the intro gfx. */
> -    center_tile_mapcanvas(map.xsize / 2, map.ysize / 2);
> +     * Refresh a random place to clear the intro gfx.
> +     */
> +    center_tile_mapcanvas(x0, y0);
>     OUT:
>       ;                               /* do nothing */
>     }
>   }
> 
> jason
> 
> 
> 
>