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

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Tue, 28 Aug 2001, jshort@xxxxxxxxxxxxx wrote:
> Raimar Falke wrote:
>> 
>> On Sun, Aug 26, 2001 at 03:17:37PM -0400, Ross W. Wetmore wrote:
>> > Attached is the ReadMe for second Part of the corecleanup_07
>> > update to cvs-Aug-25.
> 
>> Let me state my position/plan:
>>  - make a is_normal(ized)_position
>>  - insert an "assert(is_normal(ized)_position);" into every access
>>  method of map.h
>>  - debug
>> 
>>  - remove map_adjust_x and map_adjust_y
>>  - debug
>> 
>>  - come to an agreement what the final direction system should be
>>  - migrate
>>  - debug
>> 
>> 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.  As
previously mentioned, I think that overloading the DIR_D[XY] arrays to
include non-neighbouring tiles does not add anything very useful.  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.
This is in addition to comments that Raimar and I have put forth
earlier.

The biggest problem with Ross' patches is that they are mega-patches.
This makes it a lot harder to read and review them because many
unrelated changes are mixed together.  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.  It also makes it
difficult to pick just the changes that you like.  This is why we do
not encourage megapatches.

> I will state again that non-real map positions should not be
> wrapped.  Imagine the situation of a hexagon that wraps along the
> sides only; for this simple topology many non-real tiles will have
> no canonical wrapping position:
> 
> <-     x     ->
> <-   x x x   ->
> <- x x x x x ->
> <- x x x x x ->
> <- x x x x x ->
> <-   x x x   ->
> <-     x     ->
> 
> (Note that this is different from Trent's proposed "sphere" topology.)

It is (almost) entirely equivalent.  It fails for the same reason:
given a pair of non-canoncial coordinates, it is not possible if you
created them by moving across an allowed border or not.  So the
current approach of looking at the coordinates after creation has to
be replaced by one where you look at the tile and what moves you are
allowed to make _before_ you make them.  If you go that far, you might
as well change all your function to never create non-canonical
coordinates.  This is in addition to the fact that you would have to
do something very clever thinking to enable the client to present such
a map in a sensible fashion.

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.

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

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.

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.

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.

diff.diff
Description: Text document

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
I've been WRITING to SOPHIA LOREN every 45 MINUTES since JANUARY 1ST!!