[Freeciv-Dev] Re: Map coordinate cleanups.

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Fri, 17 Aug 2001, gberkolaiko@xxxxxxxxxxx wrote:
>  --- Gaute B Strokkenes <gs234@xxxxxxxxx> wrote: 
>> On Thu, 16 Aug 2001, gberkolaiko@xxxxxxxxxxx wrote:
>> 
>> 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
> 
> Thanks for your friendly advice.

I'm sorry if you took that personally, but it wasn't meant that way.
Honest.  Sorry.

>> > 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.
> 
> ok, if you find it useful.  but look at your proposed form of
> map_get_tile: first you adjust_x and then check if the tile is REAL
> (why not just write if(!normalize_map_pos(&x,&y)) ?)  then you
> return MAP and in MAP you check if the tile is REAL (and NORM)
> again.  I don't see much sense in this double check.  I guess you
> plan to remove it later, but why introduce it at all?

My original intent of MAP was to have a tile accessor function which
would dump core if you gave it a non-real tile, without having to add
assert() calls all over the code.  Changing map_get_tile() won't do;
not until you fix all the code that calls it, which is a lot.  I would
have made it dump core on unwrapped x as well, except that I decided
that I wanted to concentrate on unreal tiles at first.

As for the double check, let the optimiser worry about that at the
moment, that's what it's good at (as long as all the information is
visible).

I hope that makes things more clear.

>> > 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
> 
> I don't understand you here.  Are you claiming that your patch will
> make transition to, say, isometric view topology smoother?

No.  That is not the purpose of this patch.  Currently, Freeciv
frequently creates coordinates that are not sensible--the y values are
out of bounds, or the x coordinates are not wrapped.  We rely on very
late checks to make sure that this does not cause any difficulties.
This patch is intended to rectify the situation by making sure that
Freeciv wraps x coordinates as it creates them, and by avoiding
calling map_get_tile() on tile coordinates that do not refer to
actually existing tiles.  But it does not cover all the places, not
yet.

I claim that the patch does not make it harder to implement isometric
maps.  In a lot of cases, it makes it easier.  This is mainly by
accident, since the fix (handilng tile coordinates properly) is
frequently the same.  For instance, consider the AI loops which were
hardcoded to avoid going of the edge of the world and were changed to
use generic iterator macros.

I was slightly peeved since your criticism seemed to say that I didn't
understand or care about map coordinate issues, which seemed a bit
off-key when the patch is full of map coordinate fixes, and I have
frequently commented on these issues here before.  It seemed to me
that you were complaining about my failing to fix broken things in one
fell swoop, when I explicitely stated that I wanted to do it step by
step.

> Well, map_adjust_x is definitely better than what was there before
> your patch, but it's not isometric-compatible and cannot be made so
> (unless you change the number of arguments it takes).

I stuck some "x = map_adjust_x(x)"-like statements in a number of the
low-level map_get_fubar() functions because 1) the old code contained
the equivalent and 2) we're not ready to remove them yet and 3) my
first priority was to catch invalid y values, rather than unwrapped x
values.

-- 
Big Gaute                               http://www.srcf.ucam.org/~gs234/
I have no actual hairline...