[Freeciv-Dev] Re: (PR#4004) A canvas_iterate macro for mapview

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

I sometimes despair when I see the same old anti-patterns recycled
again and again - old habits just never seem to die :-(.


Using citymap_checked_iterate as the template ...

#define canvas_checked_iterate(x0, y0, dx. dy, cx, cy, mx, my)  \
{  int cx, cy, mx, my;                                          \
    for (cy = y0; cy < y0+dy; cy += NORMAL_TILE_HEIGHT)          \
    for (cx = x0; cx < x0+dx; cx += NORMAL_TILE_WIDTH) {         \
      if (canvas_to_map_pos(&mx, &my, cx, cy)) {

#define canvas_checked_iterate_end                              \
      }                                                          \
    }                                                            \
}

And if you want to hide the canvas coordinates ...

#define canvas_iterate(x0, y0, dx. dy, mx, my)             \
  canvas_checked_iterate(x0, y0, dx. dy, _cx, _cy, mx, my)  \

#define canvas_iterate_end                                 \
   canvas_checked_iterate_end

This could be improved on by eliminating the double for loop and
replacing it with a single loop so break works correctly. It should
also check for (dx,dy) being positive and (x0,y0), (x0+dx,y0+dy)
lying within the canvas, or flip things around to guarantee this.
The above macro just has a very stringent interface that needs to
be appropriately documented.

Depending on exactly where in the tile you placed the original (x0,y0)
you will get equivalent tile offsets in every tile, i.e. NW bbox origin
or the tile center.

As desired it loops in array order over the canvas (for tiling purposes).

The inner loop outputs real map coordinates (mx, my).

Adding an " } else { " allows one to manage unreal tile positions for
filling in black tiles if desired.


Why one would ever choose to define the main loop in *map* coordinates
when you expressly require every condition for a canvas loop order is
baffling.

One should always choose the outer loop coordinates to manage the
desired loop coordinate conditions as simply as possible and do the
conversion to other coordinates within the loop as needed.

Sigh,
RossW
=====

Jason Short wrote:
> I would like to write a new macro, canvas_iterate, to the mapview code. 
>   The interface is
> 
>    canvas_iterate(canvas_x0, canvas_y0, width, height, x_itr, y_itr)
> 
> and it iterates over the mapview canvas, *in map coordinates*.  Thus to 
> iterate over every tile on the canvas we do
> 
>    canvas_iterate(0, 0, mapview_canvas.width, mapview_canvas.height,
>                   x, y) {
>      ...
>    } canvas_iterate_end;
> 
> and it is useful for just about every iteration we'd ever need.
> 
> 
> The problem, naturally, is with implementation.  I haven't thought about 
> this much, but the only iteration I can come up with is
> 
>    #define canvas_iterate(cx0, cy0, w, h, x, y)
>      whole_map_iterate(x, y) {
>        int cx, cy;
>        map_to_canvas_pos(&cx, &cy, x, y);
>        if (cx > cx0 - NORMAL_TILE_WIDTH
>            && cy > cy0 - NORMAL_TILE_HEIGHT
>            && cx <= cx + w
>            && cy <= cy + h) {
> 
> which is extremely inefficient.
> 
> To make the iterator more useful, it should guarantee the order of 
> iteration to be top->bottom, left->right (which the above does not do).
> 
> More on this to come.
> 
> jason