[Freeciv-Dev] Re: Map coordinate cleanups.

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 07:12 PM 01/08/15 -0700, Trent Piepho 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 you ever want to support topologies other than a cylinder, you probably
>don't want to make these a macro.  IS_NORM_TILE would become quite a mess of
>?: operators just to support the three most simple map types of plane,
>cylinder, and sphere, not even considering the crazy shit like Mobius strips
>some people were talking about.

These are already done as macros for the 4 rectangular based examples. Look
in map.h in corecleanup_05b-1-12.0. This is not a plug, just a comment that
it isn't that difficult - provided you do it systematically.

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

Not if you do things right. is_real_tile is a core element of 
normalize_map_pos and that is used everywhere.

>I also wonder if making city_map_iterate use normalized map coordinates, or
>creating another version that does, might be a good idea.  Is there much
>(any?) code that wants to iterate over non-normalized coordinates in the city
>map?

You mean like this one in city.h

/* Iterate a city map in checked real map coordinates */
#define city_map_iterate_checked(x0, y0, cx, cy, mx, my) {             \
  int _x0= (x0)-CITY_MAP_SIZE/2, _y0= (y0)-CITY_MAP_SIZE/2;            \
  city_map_iterate1(cx, cy, 0, (CITY_MAP_SIZE*CITY_MAP_SIZE-4)) {      \
    int my= cy + _y0, mx= cx + _x0;                                    \
    if(normalize_map_pos(&mx,&my)) {
   

There were a half dozen cases of explicit city coordiante to map
conversions with absolutely no checks. Luckily nobody puts cities on polar
squares and the x border is wrapped in the standard map. So current CVS
Freeciv is "bugfree" :-) :-).

If you want to see more, run the corecleanup patches in Flat-Earth mode and
watch it fault for you. 

>I also don't think making normal no longer a subset of real makes much sense.

There are problems with Gaute's ideas of REAL and NORM, and they have to 
do with whether the coordinate is wrapped or not, i.e. x is wrapped and
y is not in the standard map, but it is relatively straightforward to
make these both properly checked which gives you 

/* Map topology definitions and map dimension related macros             */
/* The default definition reflects a cylindrical map wrapping at map.xsize */

enum MAP_TYPE {
  MAP_TYPE_FLAT  = 0,  /* Note: not a WRAPTYPE */
  MAP_TYPE_WRAPX = 1,
  MAP_TYPE_WRAPY = 2,
  MAP_TYPE_TORUS = 3
};
   

i.e. is_real_tile needs to be something like this.

#define is_real_tile(X,Y)
  \
( (has_mapwrap(MAP_TYPE_WRAPX) || (unsigned)(X) < (unsigned)map.xsize) &&
  \
  (has_mapwrap(MAP_TYPE_WRAPY) || (unsigned)(Y) < (unsigned)map.ysize) )
   

You can hard define the constants in has_mapwrap so that the compiler will
reduce this to the current code. Or you can hard define to any of the 4 if
you don't like the dynamic capability.

Don't be too concerned about the rough edges on people's code just yet.
This is still very much in development by a number of people and by the
time all the
differences and bugs have been worked out it will probably be fine.

Many of the differences are cosmetic style, and the rest are because there
isn't a broad enough community that has worked through all the standard map
ideas let alone thought through the generalized cases. But it is coming ...

Cheers,
RossW