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

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Mon, Feb 24, 2003 at 04:01:33AM -0800, Jason Short wrote:
> 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?

I don't know.

> > 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.

I want to catch errors (here non-normal map positions passed to
square_iterate). I want to catch the case where in

bool is_friendly_city_near(struct player *owner, int x, int y)
{
  square_iterate (x, y, 3, x1, y1) {

the caller didn't provide a normal map position.

  If a non-normal map position is passed this will go unnoticed.

This is bad. So the CHECK_MAP_POS was added to square_iterate. This
isn't a problem since most of the users of square_iterate just pass
some variable in. The only other user I caught with
'square_itera.*-[^>]' was

./client/mapview_common.c:67: square_iterate(x - 1, y - 1, 1, city_x, city_y)

This user should be changed.

To answer the question: the extra parameters are needed to test of the
"seed/base" map position is normal.

> In the above use of map_rectangle_iterate, (x,y) is _not_ guaranted
> to be normal.

Uhhh. I didn't know.

> 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...

This is another topic.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  "The primary purpose of the DATA statement is to give names to
   constants; instead of referring to pi as 3.141592653589793 at every
   appearance, the variable PI can be given that value with a DATA
   statement and used instead of the longer form of the constant. This
   also simplifies modifying the program, should the value of pi
   change."
    -- FORTRAN manual for Xerox Computers