[Freeciv-Dev] Re: Map coordinate cleanups.

[Jason Dorje Short <jshort@xxxxxxxxxxxxx>]

Gaute B Strokkenes wrote:
> 
> On Wed, 15 Aug 2001, xyzzy@xxxxxxxxxxxxx wrote:
> > On Thu, 16 Aug 2001, Gaute B Strokkenes 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.

> If we ever have to change this back (which I don't think we have to,
> until and unless we implement `isometric view' shapes), it's only
> going to take two minutes to change it back, anyway.

I think isometric view shapes would be good.  They're not the only kind
of topology that doesn't wrap independently, but they're the most likely
kind.

> > Also, most of the places these functions are used is inside
> > debugging assert calls.  There is no point in making those faster
> > since they are just for debugging.  So there is no good reason to
> > make it a macro in the first place.
> 
> That's not true.  Compiling with assertions disabled is the exception
> rather than the norm, so most people would benefit from a speedup.
> Moreover, it is used in several places where we're looping over
> geographical regions and the like, and it would be good to locally
> expose the nature of this test to the compiler.

I think any benefit players may get from extra speed is more than
negated by the extra work developers have to do to use a macro for these
functions.  Thus I agree that there is no _good_ reason to make it a
macro.

In summary, I don't think these functions should be made macros. 
Efficiency must be balanced with code cleanliness.

> I haven't redefined anything; there is no evidence anywhere in the
> current sources that this has been given any thought at all.

As I see it, you've redefined "normal", never mind that the original
definition may have been by accident.

> In any case my point is that
> 
>   Stepping of the edge of the world.
> 
> and
> 
>   Accidentally forgetting to wrap an x coordinate.
> 
> are DIFFERENT kinds of error, and accordingly you want to be able to
> detect either kind or both according to your needs.

You're right on this point; I understand what you're saying entirely I
just disagree with your naming.  I would name things like this:

real tile: a coordinate position that is on the world
proper tile: a coordinate that is canonically wrapped
normal tile: one that is both real and proper

Which seems to be the opposite of what you've done.  I'll admit "normal"
and "proper" don't make much of a distinction between them.

Also, while you're right that coders should be able to use either or
both according to their needs, it will be very rare (possibly
nonexistent) to check for being "proper" under my naming system (that
is, to check the wrapping alone without worrying about the validity). 
Currently normalize_map_pos takes care of both, checking both validity
and wrapping - which is why I believe my naming system is most
consistent with what's currently in freeciv.

My solution would be to have three check functions: is_real_tile,
is_proper_tile, and is_normal_tile.  is_proper_tile probably won't be
needed, but should be included for completeness.  

jason