[Freeciv-Dev] Re: [UPDATE] Corecleanup_08 patch to cvs-Sep28

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

At 11:00 AM 01/10/03 +0200, Raimar Falke wrote:
>On Tue, Oct 02, 2001 at 09:58:01PM -0400, Ross W. Wetmore wrote:
>> At 09:28 AM 01/10/02 +0200, Raimar Falke wrote:
>> >On Mon, Oct 01, 2001 at 08:04:17PM -0400, Ross W. Wetmore wrote:
>> >Can you make a proposal (list what has to be changed, how it is
>> >controlled, performance impact,...) of the generalization of the map
>> >topology? So that we can have the four types (flat, like now,
>> >donut,...). This is the next big step IMHO.
>> 
>> I suspect getting up to snuff with the corecleanups is still a prereq
>> since you have a lot of bugfixes and updates to work through yet.
>> 
>> However, when you are clean, these are the changes - pretty miniscule
>> right?.
>> 
>> Cheers,
>> RossW
>> =====
>> 
>> See map.h for more details:
>>  *
>>  * For toroidal maps (aka donut worlds), is_real_tile and normalize_map_pos
>>  *   always return TRUE. normalize_map_pos and _map_adjust_* always adjust
>>  *   both x and y.
>>  * For flat-earth, is_real_tile checks both x and y within bounds, and
>>  *   normalize_map_pos and map_adjust_* are just bounds checks.
>>  *
>>    
>> 
>> These two functions control most of the topology effects, both where used
>> explictly and in the iterate loops (map_adjust is effectively gone).
>> 
>> There are a small number of places where one still does explicit map 
>> wrap operations like the get_citymap_xy() example. You can grep for 
>> has_mapwrap() to locate them. They are in city.c, gotohand.c and mapgen.c
>> (and of course map.c and map.h where the macro and function definitions 
>> are).
>> 
>> 
>> #define is_real_tile(X,Y)
>>   \
>> ( (has_mapwrap(MAP_TYPE_WRAPX) || RANGE_CHECK_0(map.xsize, (X))) &&
>>   \
>>   (has_mapwrap(MAP_TYPE_WRAPY) || RANGE_CHECK_0(map.ysize, (Y))) )
>> 
>> This is the typical magnitude of the change. 
>
>> It does not show up on profiling even when using the functional form
>> of either of the two controlling operations.
>
>is_real_tile isn't used much anymore. normalize_map_pos is used most
>of the time and may became more expensive.

is_real_tile() should be the first element of normalize_map_pos(),
whether this is coded explicitly or implicitly.

But more importantly, it is used when you just want to do the first
part, i.e. the check, and don't want to update any values, which is 
where normalize_map_pos() should do most of its work.

This is why it is used in asserts. But there are other places where
the functionality will be needed, so it should always be considered.

If you are always going to proceed to use the values after a successful
check, then I agree that normalize_map_pos() is pretty much all that
will normally be required.

You should always code normalize_map_pos() to NOT become more expensive
unless you need to do the expensive things. That is why you check first
before doing the heavy work, and that is why you may want to have an
is_real_tile()-ish macro version to precheck and then make an expensive
function call.

It is also why Gaute's versions of normalize_map_pos() are broken.

>> stdinhand.c:
>>   { "maptype", (int *)&map_type, NULL, NULL,
>>     SSET_MAP_GEN, SSET_TO_CLIENT,
>>     MAP_TYPE_FLAT, MAP_TYPE_TORUS, MAP_TYPE_WRAPX,
>>     N_("Map type or topology"), "" },
>
>Can this be made a non-numeric argument?

You can do anything you want here. A numeric map_type is much easier
to code and validate, and the range is meaningful when guessing what
values are allowed. Strings would be perhaps more informative, but 
listing all the possibilities to make this worthwhile is a highly
non-extensible process. Personally I'd rather remember numbers, with
the clue when more numbers were available.

In a GUI version you would do a pick list and select. Putting a number
in front of each entry and doing a CLI list followed by an 
  Enter maptype[0-n]? 
is the usual way this is done. So numbers are probably more flexible
and common overall.

>What about map generation? What about isometric maps? What about
>savegames?

The cleanups remove off map access problems with all of these. There
should be no other issues with isometric and savegame since isometric
uses the same map access routines, and the map you save and restore is 
still rectangular.

Map generation will work. But you may want to change the polar 
restrictions it currently has, and the asserts in the server to make 
sure that stupid things like the polar passage exist.

There are lots of interesting things that can be done to make different
maps, but this issue is largely orthogonal to border wrapping. There 
are some changes (the few has_mapwrap()s in mapgen.c) to not propagate
or propagate land features across a wrapwall. But this is aesthetics,
and your current mapgen has far more noxious aesthetic problems than
this :-).

Cheers,
RossW