[Freeciv-Dev] Re: topology patches

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Oct 28, 2001 at 06:34:01PM -0500, Jason Dorje Short wrote:
> "Ross W. Wetmore" wrote:
> > 
> > At 04:46 AM 01/10/27 -0400, Jason Dorje Short wrote:
> > >I have the following topology patches prepared or planned.  All of them
> > >are relatively small.
> > >
> > >check_map_pos : this doesn't deal specifically with topologies but it's
> > >the same code that's being affected.  Since it's a pretty
> > >straightforward change and one that we should have in CVS for as much
> > >time as possible before the next release, this should get first billing.
> > >
> > >rand_pos: this creates a rand_pos function in mapgen.c.  It's a small
> > >issue, but it's been argued liberally: what form should rand_pos take?
> > 
> > Drop it ... it is far too restrictive or expensive an interface to use
> > for most cases and its presence will result in bad abuse. When you want
> > this particular limited functionality it is trivial to code.
> 
> This is one such change.  The random position code will fault if used in
> a different topology.
> 
> Really, I'm caring less and less about what form rand_pos takes.  If you
> want it to be topology-dependent, so be it.  But the fact remains that
> code like:
> 
>   x = myrand(map.xsize);
>   y = myrand(map.ysize);
> 
> must be replaced by something like
> 
>   rand_pos(&x, &y);
> 
> and code like
> 
>   x = myrand(map.xsize);
>   y = 1 + myrand(map.ysize-2);
> 
> must be replaced by some sort of similar code.  Options include
> 
>   do {
>     rand_pos(&x, &y);
>   } while(y == 0 || y == map.ysize-1);
> 
> or
> 
>   rand_pos_checked(x, y, y >= 1 && y < map.ysize-1);
> 
> or even the topology-dependent version
> 

>   rand_pos(x, y, 1, map.ysize-2);

I would vote for this. Make it local to mapgen if this is
possible. And provide a generic one.

> At this point, take your pick.  But something like this must be done or
> we cannot move forward with map generation (we can still have alternate
> topologies, it just won't be possible to make random maps for them).
> 
> (Oh, and it's not just random generation that requires this.  There is
> core code that does it too - for instance barbarian generation, nuclear
> winter, and global warming - but they generally take the first form.)
> 
> If I provide that only makes changes of the first form and leave the
> rest of mapgen alone for now, would that be acceptable?
> 
> > >map_iterate: this changes the map iteration loops to use
> > >regular_map_pos_is_normal.  It is currently being held up (I believe) by
> > >several factors: (1) regular_map_pos_is_normal overlaps with rand_pos,
> > >(2) there was some thought of removing print_landarea_map in game.c, (3)
> > >Ross isn't happy with the form of many of the loops.  I'll propose an
> > >"updated" version of this shortly, though.
> > 
> > regular_map_pos_is_normal() is a very confused concept. I think you need
> > to review the topology RFC before adding such things to the code base.
> 
> I only see two options on this front:
> 

> #define whole_map_iterate(x, y)
>   for (y=0; y<=map.ysize; y++)
>     for (x=0; x<=map.xsize; x++)
>       if (regular_map_pos_is_normal(x, y)) {

I'm still for this one.

> or
> 
> #define whole_map_iterate(x, y)
>   for (y=get_map_y0(); y <= get_map_ymax(); y++)
>     for (x=get_map_x0(y); x <= get_map_xmax(y); x++) {
> 
> The second one might be more efficient (depending on how the topology
> functions get_map_[xy][0|max] are implemented), but doesn't allow easy
> handling of non-normal coordinates, will lead to exceptionally obtuse
> code (in the topology functions) for some topologies, and (most
> importantly) restricts usable topologies to only convex ones (I believe
> that is the correct terminology).
> 
> The current code will definitely not work here; it is not
> topology-dependent.  All regular coordinates are not normal (are not
> within N(0, 0)).
> 
> > >mapgen: (not yet proposed) this updates all of mapgen to be
> > >topology-independent.  It uses distance_from_pole,
> > >distance_from_[north|south]_pole, and (possibly) distance_from_edge to
> > >do the map computations.  rand_pos needs to be dealt with first, though.
> > 
> > No. Write a new map_generator if you want to do such things. Leave the
> > current ones alone for the forseeable future except for minor changes
> > to make sure they don't fault if used in various topologies they are
> > checked and registerd to support.
> 
> These are exactly the types of changes I'm talking about.  In addition
> to the rand_pos changes above (which assure that a generated random
> position is normal), the changes I'm talking about replace "y" with
> "distance_from_north_pole()", "map.ysize-1-y" with
> "distance_from_south_pole()", and "map.ysize" by
> "total_polar_distance()".  These changes are not intrusive, and every
> one of them makes mapgen (IMO) more clean and readable.  Without these
> changes, it will again be impossible to do any random map generation for
> general topologies.  

> We definitely do not want to have to write a new
> map generator for each topology when the existing ones will do a
> minimally adequate job of providing a basic one.

Ross was talking about a new generator which takes all the topologies
we are talking about into account. This one would general from the
start. I have no idea how much work it is to change the current ones
compared to the work for the new one.

> > >diff_map_pos: (not yet proposed) this introduces a function,
> > >diff_map_pos (or similar naming) that determines the difference between
> > >two map positions.  It will be necessary for straightest_direction,
> > >dir_ok, is_cardinal_move, [xy]dist, and other GOTO code to be
> > >topology-independent.
> > 
> > These functions currently exist ... use them. Don't proliferate more
> > noise.
> 
> diff_map_pos must replace the current xdist and ydist, and must be used
> as a backend for other similar functions like map_distance,
> real_map_distance, and sq_map_distance.  I see no other viable
> alternative.
> 
> xdist/ydist and these other functions all assume wrapping occurs along
> the X and Y axes.  This is not the case in general; diff_map_pos
> abstracts this problem away behind the topology interface.
> 
> Really, this change is pretty small and straightforward (although I
> thought some of the other changes were, too...:-).  If we bog down on
> other fronts, I'll go ahead and submit this patch (which I've been
> working on and testing), since it's independent of most other changes. 
> I'd rather avoid having too many patches in the queue at once, though
> (there are a fair number already).

I agree with you Jason, that a generic method for this is needed.

> > >misc_topology: (not yet proposed) miscellaneous cleanups may include
> > >moving map_adjust_[xy] into client-only code, removing any remaining
> > >topology-dependent code, etc.
> > 
> > It may not be necessary to remove things. Isolate them as (documented)
> > special cases if they are only used (safely) in special case code. Replace
> > only that which is being used for the general flow path or causes faults.
> 
> Sorry; by "remove" I mean "fix".  The problem here generally isn't
> fixing the code but tracking it down.  Once we're at that stage I'll do
> a serious review and search for remaining topology-dependent code.
> 
> > >At that point, we should have a topology-independent server (well, OK
> > >probably not...it's hard to verify such things...).  We can then move on
> > >to the client.  This will be a bigger job, depending on how much cleanup
> > >we want to do in the process, but at least we can do some testing as we
> > >go along.
> > 
> > Get the basic model right for the server part. Note it really only uses
> > game or map coordinates, and thus is not really that tricky. Accept a
> > limited set of current implementations to back the model interfaces, and
> > don't worry about over generalization below the model interface, i.e. a
> > switch function, or virtual function indirection based on map_type index
> > to handle case specific details should be fine.
> 
> I don't completely understand you here.  What is a "game" coordinate? 
> When you say "current implementations", what are you talking about
> implementing - topologies or core code?
> 
> In general, I'm not too concerned with this step as yet.  I'd like to
> get the code to a much more topology-independent state before working up
> any more patches to provide alternate topologies.  I've already made
> such changes, but the patch is so interwoven with core code changes that
> it is monolithic and not very useful.  (I've been thinking of making a
> local CVS repository to do such changes, and perhaps I will, but that
> doesn't really change things too much since the diffs won't be very
> useful against the real CVS tree.)

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  reality.sys corrupt. Reboot Universe? (y,n,q)