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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 02:06 PM 01/08/30 +0200, Gaute B Strokkenes wrote:
>On Tue, 28 Aug 2001, jshort@xxxxxxxxxxxxx wrote:
>> Raimar Falke wrote:
[...]
>>> I'm won't do anything before I heard Gautes position/comments on
>>> your patch.
>
>I think that while a lot of Ross' fixes are good, some are not.  
Thanks!  I hope we can do the one and root out the other.

>As
>previously mentioned, I think that overloading the DIR_D[XY] arrays to
>include non-neighbouring tiles does not add anything very useful.  

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.

The sub-blocks you typically think of and want to keep separate are just 
offsets into this total set. The separation can be done with MACROS, variable
pointers or other programming techniques if you want to keep the illusion
of separate objects.

The way to think of this "adjacent" array is for local coordinate lookups.
These are typically the first 3 rings outwards from a centre spot and
can be used for MAP_STEP, CITY_POS, UNIT_VIS as well as the *_iterate
macros. The fact that they form increasing squares as one moves outwards
allows one to use them for fast square_iterate loops over local coordinates.

The code consolidation for things like iterate macros is significant.

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.

So I think, you are just not looking very hard for useful things :-).

And besides you don't have to repeat things in separate spots or differnt
ways to prove your coding style is better,

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

/* Core adjacent iteration loop 1st square (+1)               */
/* Note: border guard avoids full map checks except           */
/*       where 1x increment could force out of bounds.        */
#define core_iterate(ni, n0, x0, y0, x, y, dir) {              \
  int x;                                                       \
  int y;                                                       \
  int dir= (n0);                                               \
  int _ni= (ni);                                               \
  int _x0= (x0);                                               \
  int _y0= (y0);                                               \
  int _border = is_border_tile(_x0, _y0);                      \
                                                               \
  while( (dir-= _ni) >= 0 ) {                                  \
    x= DIR_dx[dir+1]+_x0;                                      \
    y= DIR_dy[dir+1]+_y0;                                      \
    if( !_border || normalize_map_pos(&x,&y) ) {

#define core_iterate_end                                       \
    }                                                          \
  }                                                            \
   

#define cartesian_adjacent_iterate(x, y, IAC_x, IAC_y) \
{                                                      \
  int IAC_i;                                           \
  int IAC_x, IAC_y;                                    \
  for (IAC_i = 0; IAC_i < 4; IAC_i++) {                \
    switch (IAC_i) {                                   \
    case 0:                                            \
      IAC_x = x + 1;                                   \
      IAC_y = y;                                       \
      if (!normalize_map_pos(&IAC_x, &IAC_y))          \
    continue;                                      \
      break;                                           \
    case 1:                                            \
      IAC_x = x;                                       \
      IAC_y = y + 1;                                   \
      if (!normalize_map_pos(&IAC_x, &IAC_y))          \
    continue;                                      \
      break;                                           \
    case 2:                                            \
      IAC_x = x - 1;                                   \
      IAC_y = y;                                       \
      if (!normalize_map_pos(&IAC_x, &IAC_y))          \
    continue;                                      \
      break;                                           \
    case 3:                                            \
      IAC_x = x;                                       \
      IAC_y = y - 1;                                   \
      if (!normalize_map_pos(&IAC_x, &IAC_y))          \
    continue;                                      \
      break;                                           \
    default:                                           \
      abort();                                         \
    }

#define cartesian_adjacent_iterate_end                 \
  } 

                                                   \                       
>The biggest problem with Ross' patches is that they are mega-patches.

I agree. 

Freeciv doesn't seem to be able to handle much more than trivial bugfixes 
or new laregely independent additions. Is CVS holding the project back,
maybe? Having worked in a CVS environment professionally, it is known
to scale very poorly and cause a lot of problems when working on shared code.

The development branch idea and a number of others have been suggested to
try and overcome the limiting constraints that seem to stifle a lot of
things from moving forward.

Maybe it is really time to do something ...

>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 know, since I have sorted, scanned and rearranged these diffs more 
times than I want to remember, that this is not rocket science - 
drudgery maybe, but not rocket science.

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

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 :-)

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,

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

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

>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. But the fact that this got far more attention and
discussion space than real technical code analysis and review is a sad
testament to the current development climate.

>Below is a patch showing my private tree as off today.  When I get
>back from work I intend to change is_real_tile() IS_VALID_POS(), on
>the grounds that it tests a coordinate pair rather than a tile
>pointer, and because a position if valid iff it refers to a tile on
>the map.  I will change normalize_map_pos() to always adjust the x
>coordinate, and then I will add a IS_SANE_POS() to test whether a
>position refers to a tile on the map and is in canonical form.

So you are ruling out a flat-earth topology 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?

Come up with your own variant, but at least stop and understand the
problems and impact of such changes. The cosmetic renames aren't the
concern as this has little real impact other than for those merging 
updates, or the fact that it will probably be done piecemeal and add
yet another series of fracture lines to the codebase, but some of the 
changes as inferred from the above seem a bit iffy.

You really should spend some time understanding what has been done
even if this is difficult.

>I will not, however, add a function to test whether a position is
>normalized.  I hope that that will keep the contingent that insists on
>debating The True Meaning of normalize to death happy, since it does
>not mention "normalize" at all.  I hope that this will be something
>that everyone can live with.
>
>
>Attachment Converted: "c:\program files\eudora\attach\diff.diff"
>-- 
>Big Gaute                               http://www.srcf.ucam.org/~gs234/
>I've been WRITING to SOPHIA LOREN every 45 MINUTES since JANUARY 1ST!!

Cheers,
RossW