[Freeciv-Dev] Re: Map coordinate cleanups.

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Thu, 16 Aug 2001, gberkolaiko@xxxxxxxxxxx wrote:
>  --- Gaute B Strokkenes <gs234@xxxxxxxxx> wrote: 
>> 
>> Attached is patch that fixes several instances where Freeciv
>> inadvertently creates unnormalised map coordinates or makes use of
>> unreal map coordinates.  It also changes the is_real_tile()
>> function to a macro IS_REAL_TILE(), and adds macros IS_NORM_TILE()
>> and IS_PROP_TILE(), and some other coordinate related things.
> 
> I don't like this patch very much.  Apart from confusion with the
> names REAL, NORM and PROP,

Frankly, anyone who is unable to understand the difference between:

 * Real tile coordinates.
 * Tile coordinates that are not in canonical form.
 * Tile coordinates that are both.

should consider working on something other than Freeciv.  I agree that
there should be comments stating what these macros do, and in fact I
specifically stated that such comments would be added before
committing.  If people prefer, I could also change the names to NORMAL
and PROPER, if they feel it helps.

My point is that creating tile coordinates that do not refer to a real
tile and using tile coordinates with unwrapped x values are logically
distinct errors, and it is therefore good to be able to check for
either one or both as the situation demands.

> it seems to me that MAP duplicates map_get_tile

Not quite.  You may have noticed that MAP() has different error
handling, and that it is possible to change the way that MAP() deals
with improper coordinates without worrying about the masses of code
that calls map_get_tile().  I find this very helpful as I go through
all the code.

> and the checks are not getting less scattered around.

> I also think you should try to avoid using 
> 
> x=map_adjust_x(x)
> if (!IS_REAL_TILE(x,y)){ ... }
> 
> in favour of normalise_map_pos.  The reason is that it's much easier
> to adapt normalise_map_pos to different topologies, also those where
> adjusting x will depend on y (one of them being proper civ2-style
> isometric view).

Well, duh. Guess what?  That's just the sort of stuff that this patch
is full off.  I didn't claim to have covered all cases, though I'm
working on it.  If you've found a place in my patch that is not like
that for a good reason, then you've found a bug.

> I will try to try it on GTK ;)

Thanks.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
With YOU, I can be MYSELF..  We don't NEED Dan Rather..