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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Aug 30, 2001 at 10:01:10PM -0400, Ross W. Wetmore wrote:
> 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 have missed this one.

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

I'm also thinking about this. Maybe there will be a separate
branch. I'm waiting for the opinions of the other maintainers. As a
side note: a part of the job of a maintainer is to say no (think
Linus).

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

So please resent them in a seperate email. And provide extra
explanation. See it from this view point: if a maintainer doesn't
understand your changes how should the average freeciv hacker?

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

As I said: the maintainers can accept all your changes and freeciv
maybe really nice afterwards but only you will understand it than.

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Tank: So what do you need? Besides a miracle.
  Neo: Guns. Lots of guns.
    -- From The Matrix