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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Oct 04, 2001 at 10:06:52PM -0400, Ross W. Wetmore wrote:
> 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.

IMHO there are two places where map positions had to be
checked/normalized:
 - map generation: (various iterates, MAPSTEP) a normalization is
 always required.
 - input checking at the server: we want to discard invalid inputs but
 we want also to normalize the good ones. normalize_map_pos can do
 this.

So in the long run there should be very few instances of is_real_tile
left.

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

Comments from anybody other? Gaute?

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "A common mistake that people make, when trying to design 
  something completely foolproof is to underestimate the 
  ingenuity of complete fools."
    -- Douglas Adams in Mostly Harmless