[Freeciv-Dev] Re: topology patches

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

"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);

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)) {

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.

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

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

jason