[Freeciv-Dev] Re: (PR#4004) A canvas_iterate macro for mapview
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
rwetmore@xxxxxxxxxxxx wrote:
> I sometimes despair when I see the same old anti-patterns recycled
> again and again - old habits just never seem to die :-(.
I said the whole_map_iterate implementation was the only one I could
come up with, not that I wanted to use it.
> 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 \
> } \
> } \
> }
This is a great example of a nice, clean implementation that just won't
work.
1. In iso-view there are additional map positions on the half-tile. So
a native-style loop (like is used in show_city_descriptions) would be
helpful.
2. We need to catch all tiles that overlap the area. Even in non-iso
view you need to consider the case where (x0,y0) does not mark a tile
origin. In iso-view it becomes quite a bit more complicated since the
tiles are aligned isometrically, and you need to account for the extra
UNIT_TILE_HEIGHT area (which is one reason why having a draw_type
parameter would be helpful - to allow the users to filter out unwanted
tiles).
3. The mapview often does need to know about unreal positions. Simply
filtering them out may not be the best choice.
4. The caller is most likely going to use map_to_canvas_pos to get a
location for drawing. Because of wrapping issues, this may not actually
be within the canvas window. The alternative is to reveal the (cx,cy)
canvas coordinates, which (if used) would (in principle) lead to
multiply-drawn tiles.
#4 is insoluble without drawing tiles more than once. The solution for
#3 will depend on usage. #1 and #2 do need to be addressed, and make
the loop much more complicated than what you have.
> 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.
Some of the users may want to use off-canvas positions. There is
nothing wrong with this. Consider for instance dragging the mouse to
select a part of the canvas - if you recenter the canvas your selection
rectangle is no longer on the canvas itself, but you still want it to
take effect.
> Adding an " } else { " allows one to manage unreal tile positions for
> filling in black tiles if desired.
This is a hack that assumes a particular interface to the macro. OTOH
indocucing a
#define canvas_iterate_else_unreal } else {
would have a similar effect without completely breaking everything. E.g.,
canvas_iterate(...) {
} canvas_iterate_else_unreal {
} canvas_iterate_end;
will work while
canvas_iterate(...) {
} else {
} canvas_iterate_end;
is not only unreadible, but won't compile.
> 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.
The existing loops (e.g., show_city_descriptions) are done in map
coordinates. However, they know the exact bounds of iteration which
makes it easy. This macro aims for something more general/harder.
> 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.
Iterating in canvas coordinates is helpful since they are inherently
aligned with the rectangle we want to iterate over. On the other hand,
the iso case requires special-casing of the boundaries of iteration and
this may be harder to do in canvas coordinates.
jason
|
|
