[Freeciv-Dev] Re: Profiling Civserver again

[Trent Piepho <xyzzy@xxxxxxxxxxxxx>]

On Thu, 26 Jul 2001, Jason Dorje Short wrote:
> 
> I haven't tried your version.  I disagree that it is cleaner - a 9x8
> array of numbers is neither clean nor intuitive IMO.  Additionally, the
> overhead necessary to initialize DIR_DX/DIR_DY after the map is set up

I'm not sure what you mean by overhead.  If you're talking about CPU time,
only a few numbers need to be set once when the map is set up; you couldn't
even measure that with gprof.  If you talking about complexity overhead then I
do see your point.  It's yet one more thing to be initialized.

> is not worth it, IMO.  You'll have to spread the code around into 3
> places - one for the iterator macro, one for the declaration of
> DIR_DX/DIR_DY, and one to initialize the values of DIR_DX/DIR_DY.

Well, the iterator macro already exists, DIR_DX/DIR_XY already exist, so
you're talking about 3 parts instead of 2 parts.  The only thing more
complicated is that DIR_D[XY] need to be initialized when the map is made
instead of just being const globals.  

One thing I didn't point out before, is that with my method you can use
different types of map wrapping.  Currently, freeciv has a map that wraps
around on the east and west edges, but not on the north and south edges.  It's
sort of like a cylinder.  You could also have a flat map that doesn't wrap on
any edge, or a spherical map that wraps on all four edges.

With the way you have it, the map wrapping is hard coded into the iterator
macro.  In order to use a different type of map, you would either need to
recompile the server with a new macro, or stick some kind of gross pile of
nested if()s inside the iterator to test for every kind of wrapping style.

With my method one doesn't need to change the iterator macro at all.  The
only thing that is necessary is to initialize DIR_D[XY] to different values.


> Yes, DIR_DX2 uses a more intelligent numbering system.  However, it will
> be difficult to upgrade to use this data instead.  It's too bad DIR_DX
> wasn't written like this from the start.

I'm not sure it would really be that hard.  They are a lot of places that use
DIR_DX to iterate over tiles, but most of them never actually look at the dir
variable.  You could order the tiles any way you want and it wouldn't matter. 
There are a few places that do use the direction, but usually the value
doesn't matter, as long as it is consistent.  The only place the actual value
matters is where the opposite direction is found by using 7-dir instead of
(dir+4)%8.  And a few places which have a text array of "N", "S", etc. that
direction is looked up in.  Those should be changed to a function call anyway,
instead of identical arrays repeated in several functions.

There is even one place which looks that the direction name, ie. "N" or "NE",
to figure out if it's a cardinal direction or not.  If the second character of
the name is '\0', then it's a cardinal direction.  Actually I think that's
kind of clever...  But someone could change "N" to "North" and then
autosettlers would start screwing up!  With the DIR_DX2 way of ordering
things, even numbers are cardinal directions, odd numbers are diagonals.

> superior to the current setup).  It might be possible to do better by
> making the cardinality an option to the macro, since I know there are
> some places in the code where it is handled like this.  I'm not sure how
> many places there are that are like that, though.

Something like this:

/* x,y      = start point
   x1,y1    = coordinate of current tile
   dir      = direction of current tile, numbered clockwise with north at 0
   cardinal = false (0) means iterate over all eight adjacent tiles
              true (1) means iterate only over the four cardinal directions */

#define adjc_card_dir_iterate(x, y, x1, y1, dir, cardinal)      \
{ int type = map_tile_boundary_type(x,y);                       \
  for(dir=0;dir<8;dir+=1+cardinal) {                            \
    if(DIR_DX[type][dir]==MAX_INT) continue;                    \
    x1 = x+DIR_DX[type][dir];                                   \
    y1 = y+DIR_DY[type][dir];                                   \

/* convenience macro to iterate over eight adjacent tiles */
#define adjc_dir_iterate(x, y, x1, y1, dir) \
  adjc_card_dir_iterate(x, y, x1, y1, dir, 0)

There's a macro that can handle iterating over cardinal or non-cardinal
adjacent tiles, with cylindrical, spherical, or flat maps, in only five lines
of code.


> circulated for a while, and many circulate indefinitely.  I think patch
> writers therefore try to make their patches as unintrusive and simple as
> possible so that they have a better chance of being understood and
> applied.  I agree that this is a bad thing.  Hopefully after the release

I think it's more a matter of laziness.  People want to add their feature or
fix one bug while dealing with as little of the code as possible.  So instead
of trying to find consistent ways to name their functions and variables, they
just use their favorite names.  They block copy sections of other people's
code that do what they want without understanding how it works and get things
wrong.