[Freeciv-Dev] Re: [PATCH] Corecleanup_07Part2 has been put in incoming

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Thu, 30 Aug 2001, rwetmore@xxxxxxxxxxxx wrote:
> At 02:06 PM 01/08/30 +0200, Gaute B Strokkenes wrote:
> I'll try to give this another go then ...
>
> The same stepping coordinates are used over and over again in
> different spots. It is better to define one array object that
> includes them all in a consistent way and keep all the related
> pieces in one spot for easy updating if needed.

I do not see how this follows.  I nearly all cases where you want to
iterate over a region of the map, an _iterate macro is the best way to
do it, mainly since it takes care of normalizing coordinates, skipping
unreal tiles etc. for you.  Inside these macros, of which there are
not many, it is just as straightforward to use a for loop or two.
This also has the advantage of being by far the most obvious way to do
it.  By contrast, overloading the concept of a direction to include
things other than simple steps between adjacent tiles is not obvious.

> The code consolidation for things like iterate macros is
> significant.

I do not see that.  The basic map iteration macros are adjc_iterate(),
square_iterate(), city_map_iterate(), and cardinal_adjc_iterate() (or
whatever it's called); not much consolidation there.

I see that you have defined some new _iterate macros using this, but
they seem to depend heavily on the way that you have ordered the
offsets, and moreover I do not see how most of them solve problems
that occur naturally in Freeciv.

Finally, I do not see how this is at all related to the ease of
implementing new topologies.

> This is not general in the sense of an arbitrary radius, but caching
> values for the first few rings can be a very useful optimization for
> most of the topologies that are liable to be implemented for awhile.

>>The revised map iteration macros are also much, much more
>>complicated than they need to be, and do not add anything relative
>>to the old ones.
>
> The complete set of adjacent_iterate macros is handled by two core
> loops. One for the first ring, and one for the second.
>
> Even doubling the size by putting each varible declaration on a
> separate line, they are still no larger than any of the "n"
> different macros (without such spacifying additions) that do various
> flavours of the same thing.
>
> Your comments before and now, just don't make sense. Here are two of
> the examples for general perusal so people can draw their own
> conclusion.

[snip a macro that is rather ugly, but easily readable in its current
form, and which looks a lot better in Ross' version.]

Now consider this:

 * block_iterate(x,y,x0,y0,dx,dy) loops the user variables x and y
 * block_xy_iterate(xy,x,y,x0,y0,dx,dy) loops user variables x, y, xy
 *   over the tiles in the rectangular block with corner x0, y0 and
 *   dimensions dx, dy. Reasonable argument checking is done.
 *
 * block_iterate2(x,y,x0,y0,x1,y1) loops the user variables x and y
 * block_xy_iterate2(xy,x,y,x0,y0,x1,y1) loops user variables x, y, xy
 *   over the tiles in the rectangular block defined by the two points
 *   x0, y0 and x1, y1. Reasonable argument checking is done.
 *
 * fastblock_iterate(xy,x,y,xy1,xy0,xs,xm,ys,ym,xs2,xm2) is the unchecked
 *   core loop from point xy1 to xy0 with skips of xs whenever x equals
 *   xm, and skip of ys plus region skip reset whenever xy < ym.
 *
 * *** WARNING: If you cannot figure out what block_iterate does,
 *              do not use fastblock_iterate directly.
 *
 * All allocate the user's chosen x, y, xy variables within the (core) loop.
 * All treat arguments other than x, y, xy as single use expressions.
 * All use a single loop on the linear offset xy = map-xsize*y + x.
 * All blocks will be correctly wrapped in the x or y dimension at the
 *   appropriate map size boundary as required by map_type, or off map
 *   values will be skipped.
 *
 * Depends on map.xsize, map.ysize, map_wrap(), map_trunc()
 */
#define fastblock_iterate(xy, x, y, xy1, xy0, xs, xm, ys, ym) {            \
  int x;                                                                   \
  int y;                                                                   \
  int xy = (xy1);                                                          \
  int _xy0 = (xy0);                                                        \
  int _xm = (xm);                                                          \
  int _xs = (xs);                                                          \
  int _ym = (ym);                                                          \
  int _ys = (ys);                                                          \
                                                                           \
  /* loop from upper xy1 to lower xy0 point, skip xs at xm and ys at ym */ \
  /* printf("fastblock: %d, %d, %d, %d. %d. %d \n",                        \
       xy, _xy0, _xs, _xm, _ys, _ym);  for debug */                        \
  while (xy-- > _xy0) {                                                    \
    if (xy < _ym) {                                                        \
      /* skip to region 2 */                                               \
      xy -= _ys;                                                           \
      _ym = -1;                                                            \
    }                                                                      \
    y = xy / map.xsize;                                                    \
    x = xy % map.xsize;                                                    \
    if (x == _xm) {                                                        \
      /* skip within a line */                                             \
      xy -= _xs;                                                           \
      continue;                                                            \
    }                                                                      \
    /* printf("fastblock: %d. %d. %d\n", xy, x, y); for debug */

#define fastblock_iterate_end                                              \
  }                                                                        \
}
/* 2001/08/11 -rww  Updated for Generic rectangular topologies, GS-ized */


/* This is 99% duplicate of the following - make fixes in both and diff check.
 * "sanitize" and "fix -ve delta" lines have most of the differences.
 * Special cases for multiple map topologoies has probably pushed this to
 * its conceptual limits, macros should never exceed 1 page in size :-).
 */
#define block_iterate(x, y, x0, y0, x1, y1)                                    \
  block_xy_iterate(_xy, x, y, x0, y0, x1, y1)

#define block_iterate_end                                                      \
  block_xy_iterate_end


#define block_xy_iterate(xy, x, y, x0, y0, dx, dy) {                           \
  int _x0= (x0), _x1= (dx), _y0= (y0), _y1= (dy);                              \
  int _um= -1, _us= -1, _vm= -1, _vs= -1;                                      \
  /* printf("block_iterate   %d, %d, %d, %d\n", _x0, _y0, _x1, _y1); */ \
  /* Y dimension                                                            */ \
  if( _y1 < 0 ) _y0+= _y1+1, _y1= -_y1;                  /* fix -ve delta Y */ \
  _y1+= _y0-1, _y1= map_trunc_y(_y1), _y0= map_trunc_y(_y0);  /* sanitize */ \
  /* X dimension                                                            */ \
  if( _x1 < 0 ) _x0+= _x1+1, _x1= -_x1;                   /* fix -ve deltaX */ \
  /* _x1+1 compensates for initial loop decrement + simplifies logic here */ \
  _x0= map_wrap_x(_x0), _x1+= _x0, _x1= map_wrap_x(_x1);      /* sanitize */ \
  if( _x0 == _x1 ) {                                                         \
    _x0= 0, _x1= map.xsize;                          /* full map, no skip */ \
  } else                                                                     \
  if( _x0 > _x1 ) {                            /* start top, skip midline */ \
    _us= _x0-_x1-1, _um= _x0-1, _x0= 0, _x1= map.xsize;       /* full map */ \
  } else {                            /* start midline, skip low and wrap */ \
    _us= _x0-_x1 + map.xsize-1;                                              \
    if( -1 < _us )                                   /* full map, no skip */ \
      _um= (_x0 <= 0 ? map.xsize-1 : x0-1);         /* mark skip, 1st OOB */ \
  }                                                                          \
  /* printf("block_iterate:  %d, %d, %d, %d, %d, %d, %d, %d\n", \
    _x0, _y0, _x1, _y1, _us, _um, _vs, _vm);                 */ \
  _y1= map_inx(_x1,_y1), _y0= map_inx(_x0,_y0);                                \
  /* Note: _args carefully do not overlap with fastblock_iterate tempvars   */ \
  fastblock_iterate(xy, x, y, _y1, _y0, _us, _um, _vs, _vm)

#define block_xy_iterate_end                                                   \
  fastblock_iterate_end                                                        \
}
/* 2001/08/27 -rww  Backdated for standard only topology, not GS-ized */

Is there anyone here who can tell at a glance what block_xy_iterate()
does?  I can't, though if I sat and studided it for a while I'm sure I
could.  I think two letter variable names ought to have died out along
with C64 basic, but apparently we're not that lucky.

Compare to this:

#define rectangle_iterate(x0, y0, x1, y1, x, y)       \
{                                                     \
  int _start_x = (x0);                                \
  int _start_y = (y0);                                \
  int _end_x   = (x1);                                \
  int _end_y   = (y1);                                \
                                                      \
  int x, y;                                           \
                                                      \
  normalize_map_pos(&_start_x, &_start_y);            \
  normalize_map_pos(&_end_x, &_end_y);                \
                                                      \
                                                      \
  /* We do not need to check map.topology & TOP_XWRAP \
     here since normalize_map_pos() will have taken   \
     care of that possibility.  */                    \
                                                      \
  if (_start_x < 0)                                   \
    _start_x = 0;                                     \
  if (_end_x >= map.xsize)                            \
    _end_x = map.xsize - 1;                           \
                                                      \
  if (_start_y < 0)                                   \
    _start_y = 0;                                     \
  if (_end_y >= map.ysize)                            \
    _end_y = map.ysize - 1;                           \
                                                      \
  for (y = _start_y; x != _end_y + 1; y++) {          \
    if (y == map.ysize)                               \
      y = 0;                                          \
                                                      \
    for (x = _start_x; x != _end_x + 1; x++) {        \
                                                      \
      if (x == map.xsize)                             \
        x = 0;                                        \
                                                      \
      {                                               \

#define rectangle_iterate_end                         \
      }                                               \
    }                                                 \
  }                                                   \
}                                                     \

This is what I would consider an unobfuscated version of the above.
It is trivially extended to cover any and all topologies that Freeciv
is capable of handling now or in the immedaite future (if you're
wondering, just ask).  The only problem I can see is that you can't
use break from within the inner loop, but even adding support for that
is just a slight obfuscation.  Additionally the coordinate clipping is
not really necessary, but I had square_iter() in mind when I wrote it.
I will admitt to not having tried to compile the above, so it might
contain a bug or two, but it's basically correct.

But actually, I do not see a great need for lots of clever macros that
iterate over a rectangular area.  I think that adjc_iterate(),
square_iterate() and friends are natural fits to Freeciv's
requirement, but off hand I do not see a need for block_iterate(),
rectangle_iterate() or anything like it (except perhaps in one of the
map generators, or somewhere in the clients, though I think they have
more complicated things to worry about anyway).  Therefore, I do not
think that we should put such complicated stuff in the Freeciv source.

Another thing is the way you have various version of the map_get_
macros in a number of different ways, including macro and non-macro.
I think it is more complicated than what we need.

I'm not an engineer, but I've heard that engineering perfection is
achived when there is nothing more to remove, rather than nothing more
to add.  Alternatively, everything should be as complicated as it
needs to be, and no more complicated.

>>This makes it a lot harder to read and review them because many
>>unrelated changes are mixed together.
>
> Actually they are not really that unrelated. And when you change
> something like the map_adjust_* or normalize_map_pos mess you need
> to do it everywhere to gain the payback. The fact that this is done
> by *_iterate loops, replacement of map_adjust_* with
> normalize_map_pos etc. is just the local means.

I do not see why patches that

 * convert manual loops to use _iterate macros
 
 * change the implementation of _iterate_macros

 * change the order of the directions

 * change map_adjust_[xy] calls to use normalize_map_pos() or
   something else

could not profitably be reviewed and applied separately.  In fact this
is the preferred way, since it is much easier to sanity check such
patches than to check their sum.

>>This makes it hard to check whether a given change is
>>self-contained, or whether it depends on some other, subtle aspect
>>of the same patch.
>
> I assume in 90% of the cases your coding skills are up to the task.

Daming with faint praise, are we?

The issue is not that we (the maintainers) do not have the skill.  The
issue is lack of time and energy to do a rather boring job that we
would not have to if the changes had been properly presented.  This is
particularly important since one of the biggest problems is currently
lack of time and energy on part of maintainers to review patches.

> There are hints indicated by reducing diffs to a number of changes
> to core files like map.[ch] and then a patch with the fixes that use
> this.  Or the city.[ch] and follow bite-sized patch that uses this,
> or the DIR_DX stuff that no one has come close to looking at as it
> is in the Part3 chunk, while a good bit of the obvious local
> bugfixes in Part 1 have not been understood enough yet to be
> included.
>
>>It also makes it difficult to pick just the changes that you like.
>>This is why we do not encourage megapatches.
> 
> I suspect this is by far the most important reason :-)

Whether you like it or not, one of the jobs of the maintainers is to
perfrom final quality control and validate all changes.  It follows
that it is frequently desirable to apply some changes and hold off on
others.  In a patch as huge as yours, this is bound to happen.

> But the painstaking pick, choose and cosmetic reformatting, as
> opposed to technical validation and application as is, is really
> bogging down the development and making the application of serial
> patches a lot more work for all than is really needed,

I'm sorry, but the issues with your patch are more than just
"cosmetics and formatting".  The primary reason for trundling out such
things up front was so that you could fix them quickly, and then we
would all be able to to proceed to more interesting things.
Unfortunately, you have not fixed, for instance, the FC__MAP_C issue,
even though both Raimar and I have asked you to.

>>As earlier stated, the approach of declaring that x coordinates wrap
>>at a magical border is a sensible way to deal with a cylinder and
>>certain realted topologies.  It is not a sensible way to deal with
>>arbitrary topologies, and insisting on using it in such cases is
>>just trying to fit a square peg in a round hole.
>
> Good, so let's get the explicit map_adjust_* stuff into a couple
> places like is_real_tile and normaliz_map_pos, and code them to
> understand the map topologies they need to deal with. If you look at
> 07l, you will see that this is pretty much what has been done.

Looking at corecleanup_07l I see a lot of stuff like that.  I also see
lots and 

> I think isometric is the next interesting extension. And it will be
> fun to see how folding it in changes the thinking or model.
>
> But at the rate this is moving, that is a couple years off. Whereas
> we could have the basic 4 flavours in the time it takes to check
> them in, and play on them while this larger picture works its way
> through the process.
>
>>Now consider a map generator or somthing like that which attempts to
>>place islands (or island chains, or whatever) on the map.  In order
>>to introduce a bit of randomness, it does so by picking a starting
>>point and then adding a random displacement.  Off course, this means
>>that you can end up off the map.  But the generator can still do
>>something sensible by, say, clipping away the off-map bits.  The
>>generator can use the distance from the centre to work out the
>>chance of a special showing up somewhere etc. etc.  Off course, that
>>will silently stop working once someone blindly replaces a "x =
>>map_adjust_x" with a call to normalize_map_pos().
>
> Actually, it shouldn't because normalize_map_pos handles the
> map_adjust_x case correctly.

Currently it doesn't, though I agree that it should do.

> And if one was clipping at a non-wrapped boundary without regard to
> the fact that it could be wrapped in some cases, then the clipping
> code was not sufficiently generalized or was incorectly using the
> map_adjust_x function for something that a map_wrap or map_trunc do
> explicitly without regard for gaming coordinates.

A map generator is (obviously) not topology-neutral.  My point was
that having normalize_map_pos() wrap non-real positions is more
consistent with current behaviour (calling map_adjust_x()
unconditionally) and less likely to cause surprises.

But we seem to agree on this, so why argue?...

>>As for the lengthy discussion about the true meaning of "normal" and
>>"normalized", all I will say is that my dictionary defines
>>"normalize" as "to make standard, uniform".  Keeping in mind that
>>your is_normal_tile() operates on map positions rather than a tile,
>>in spite of the name, it should be blindingly obvious why I made the
>>choice that I did, and why it is correct.
>
> You lost on this one.

No.  But I sure as hell am tired of debating it.

> So you are ruling out a flat-earth topology

What do you mean by "flat-earth" ?  A torus?

> from Freeciv by locking in wrapping to the x coordinate, or 90
> degree rotated standard map because you haven't dealt with the
> map_adjust_y conditions properly?

That's not what I'm saying at all.

> Come up with your own variant, but at least stop and understand the
> problems and impact of such changes.

I understand the issues involved perfectly, thank you.  In fact, I
have thought long and hard about how to make Freeciv deal with a
reasonable set of topologies, including cylinders, rectangles, Möbius
strips, Klein bottles and isometric (view) variants.  I am convinced
that it is perfectly doable and have a very clear mental image of what
it takes.  I do not see how any of the criticism that has been raised
against your patch is a result of insufficient understanding of how
things are supposed to work.

--
Big Gaute                               http://www.srcf.ucam.org/~gs234/
The Osmonds!  You are all Osmonds!!  Throwing up on a freeway at dawn!!!