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

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

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.

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

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.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
    1) Customers cause problems.
    2) Marketing is trying to create more customers.
  Therefore:
    3) Marketing is evil.