[Freeciv-Dev] Re: PATCH: map iteration (PR#1018)

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Fri, Oct 19, 2001 at 09:15:46PM -0400, Ross W. Wetmore wrote:
> > The is_normal_map_pos() solution is really just a bad hack because you
> > aren't willing to think about the case and fix it right at the moment :-).
> >
> > But in most cases, the right fix it is far easier than what you are
> > proposing.
> >
> > The examples below should use whole_map_iterate(). When you get weird
> > maps, then whole_map_iterate() will become weird, but until then it will
> > remain as is, i.e. efficient iteration over the whole map. The change
> > when needed is to 1 line of code in a header.
> 
> Yes whole_map_iterate should be prefered. However whole_map_iterate
> doesn't allow an action if the map position is unreal or an extra
> action for every new line.

Indeed.  Adding such code to whole_map_iterate would be very unwieldy at
best.

My understanding is that whole_map_iterate also doesn't guarantee
anything about the order in which the coordinates are traversed.  The
stuff in server/savegame.c certainly needs such a guarantee.

> What about a new macro:
> 
> #define whole_map_iterate(x_itr, y_itr,new_line_code,real_code, unreal_code)
> {
>   int x_itr, y_itr;
>   for (y_itr = 0; y_itr < map.ysize; y_itr++){
>         new_line_code;
>     for (x_itr = 0; x_itr < map.xsize; x_itr++)
>     {
>         if(is_normal_map_pos(...))
>         {
>                 real_code;
>         }
>         else
>         {
>                 unreal_code;
>         }
>     }
> 
> ???

This is also unwieldy, partially since it doesn't lend itself to
particularly readable code and partially because it always calls
new_line_code before the X loop (in current usage, the code usually
comes after the loop but not always).

If you consider the looping the map iteration patch does too
inefficient, why not reconsider the y_map_iterate and yx_map_iterate
macros I suggested before?  Right now they would be the equivalent of
whole_map_iterate, but (like whole_map_iterate) could be extended to be
topology-specific:

#define y_map_iterate(y_itr) \
{  for(y_itr=0; y_itr<map.ysize; y_itr++) {

#define yx_map_iterate(y, x_itr) \
{
  for(x_itr=0; x_itr<map.xsize; x_itr++) {
    if (is_normal_map_pos2(x_itr, y)) {

Doing this would consolidate code that uses this kind of loop into one
place (map.h, with whole_map_iterate, y_map_iterate, and yx_map_iterate)
where it can later be made topology-specific if desired.  It would keep
the loops cleanly readable, and allow out-of-loop code to be placed
anywhere.  A trick like Ross proposes with the "} else {" could be used
for handling of non-normal positions (except that I'd like to make this
a macro as well, for readability).

Note, though, that this trick of Ross's would not work with a more
efficient iteration algorithm, for instance if we have something like

#define whole_map_iterate(x, y)
  for (y=0; y<map.ysize; y++)
    for (x=map_min_x(y); x<=map_map_x(y); x++) {

it will not work.

So, in fact, neither this solution nor any that Ross proposed will solve
the whole problem, unless I'm missing something.  And your (Raimar's)
whole_map_iterate idea is far too unwieldy (I won't use the word ugly
:-) for my taste (we already went through this with
[SAVE|LOAD]_MAP_POS).

However, it's probably posible to clean up all the looping code in
game.c into a single macro just as we did in savegame.c.  This would
significantly decrease the number of such loops.  I'll look into it.

> > You want to use square_iterate() or block_iterate() in most of the
> > other cases. These functions do the right thing by definition. They
> > will also not change until they need to.

Please explain.

jason