[Freeciv-Dev] Re: (PR#3450) square_iterate and unreal positions

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Raimar Falke wrote:
> On Mon, Feb 24, 2003 at 02:44:37AM -0800, Jason Short wrote:
> 
>>>>The problem is we can't 'fix' the caller, since the caller isn't broken 
>>>>in any way.  The broken thing here is square_iterate - it declares that 
>>>>the center position of the square _must_ be real, when there is no 
>>>>reason why this must be the case.  Since all other users do satisfy 
>>>>this, having this check in is slightly helpful in debugging - but it is 
>>>>still broken.
>>>
>>>
>>>The rule is that all functions/macros take/return normal map
>>>positions. The only exceptions are:
>>> - CHECK_MAP_POS
>>> - is_real_map_pos
>>> - is_normal_map_pos
>>> - regular_map_pos_is_normal
>>>
>>>This is an easy rule. Everybody can remember it.
>>>
>>>Why should this rule be broken for square_iterate? Why square_iterate
>>>and not is_water_adjacent_to_tile. How should the user know that he
>>>has to provide normal map positions to is_water_adjacent_to_tile but
>>>not to square_iterate? There is no reason why square_iterate is
>>>special.
>>
>>square_iterate is special because it operates over a range of map 
>>positions.  Why is it that it is the center position of the square that 
>>must be normal, and not, say, the bottom right position?
> 
> 
> Because if the bottom right position is returned it will be normal.

That's already true.  Right now the exception is the center position - 
it _has_ to be normal.

>>>>The alternative is to declare that square_dxy_iterate (and
>>>>square_iterate, circle iterate, and other wrappers) must have a
>>>>valid center, and introduce a new iterator macro for the new case.
>>>>I've already proposed rectangle_iterate, which is a more generally
>>>>useful macro than square_iterate anyway.  But I think this macro
>>>>needs to go into map.h - we don't want localized macros spread
>>>>around all over the code; they should be all in one place.  This is
>>>>a general macro, it's just that it only has two users right now
>>>>(both in mapview_common.c).
>>>
>>>
>>>I see the new for rectangle_iterate in the mapview. But if it is
>>>created it doesn't use map positions. It just sets an arbitrary 2D
>>>point to various positions. I would like to make this explicit like
>>>non_map_rectangle_iterate but I guess a comment has to be enough.
>>
>>You're wrong: the iterate loops in question are iterating over map 
>>positions, not GUI positions.  The code it uses is completely identical 
>>to what square_iterate uses, only without the normal-center requirement.
>>
>>There is a need for GUI position iterators, though - in 
>>show_city_descriptions and update_map_canvas, for instance.  But this is 
>>not an easy concept, and there aren't enough users (yet) to justify me 
>>trying to write them...
> 
> 
> This non_map_rectangle_iterate shouldn't do the normalization and test
> for unreal positions.

What users are there for non_map_rectangle_iterate?

> Looking through mapview_common.c there are some users for a
> map_rectangle_iterate (note there no "non_" prefix).
> 
> So code like this:
> 
>     for (x_itr = x - 1; x_itr <= x + width; x_itr++) {
>       for (y_itr = y - 1; y_itr <= y + height; y_itr++) {
>       int x1 = x_itr;
>       int y1 = y_itr;
>       if (normalize_map_pos(&x1, &y1)) {
>         adjc_dir_iterate(x1, y1, x2, y2, dir) {
>           if (get_drawn(x1, y1, dir)) {
>             draw_segment(x1, y1, dir);
>           }
>         } adjc_dir_iterate_end;
>       }
>       }
>     }
> 
> can be written as
> 
>     map_rectangle_iterate(-1, -1, x, y, width, height, x1, y1) {
>         adjc_dir_iterate(x1, y1, x2, y2, dir) {
>           if (get_drawn(x1, y1, dir)) {
>             draw_segment(x1, y1, dir);
>           }
>         } adjc_dir_iterate_end;
>       }
>     } end_map_rectangle_iterate;
> 
> The same can be applied to sq_iter:
> 
>   #define square_iterate(x, y, center_offset, center_offset, radius, x_itr, 
> y_itr)
> 
> where (x,y) is guaranteed to be normal. I guess I won't find my people
> which find this a good idea.

Using a macro here is a good idea, but I don't see why you have so many 
arguments.  In the above use of map_rectangle_iterate, (x,y) is _not_ 
guaranted to be normal.

And, why call it map_rectangle_iterate?  Do we have a 
map_adjacent_iterate or map_square_iterate?  Isn't the "map" implied?

Note, in the above cases we are on the boundary of confusing map and GUI 
coordinates.  The above is an iteration over a GUI rectangle in map 
coordinates - it just so happens that since the GUI and map coordinates 
line up correctly this is also an iteration over a map rectangle. 
Better, I think, would be to write it as something like:

        gui_rectangle_iterate(-1, -1, width, height, gui_x, gui_y) {
          if (gui_to_map_pos(&map_x, &map_y, gui_x, gui_y, x, y)) {
            ...
          }
        } gui_rectangle_iterate_end;

where the gui_rectangle_iterate loops over a rectangle in GUI 
coordinates, and the conversion converts it into map coordinates and 
normalizes (x,y is the GUI origin, in map coordinates; gui_to_map_pos 
returns the value of normalize_map_pos; map_x and map_y correspond to x1 
and y1 from before).  The advantage of doing it this way is that the 
conversion can be done differently for iso and non-iso cases, and so the 
same loop can potentially be used for both (but note that a rectangle in 
iso-gui cordinates is an iso-rectangle in map coordinates, which may not 
be what we want).

If the above sounded overly convluted, that's probably because it is. 
I'm not yet ready to write any such macros, and Ross hasn't done so 
either.  The problem is there are almost as many different cases 
(required macros) as there are users.

Even if all these macros existed, the current square_iterate caller 
would still use square_iterate...

jason