[Freeciv-Dev] Re: (PR#8959) remove CAR_DIR_D[XY]

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=8959 >

Gregory Berkolaiko wrote:

> My opinion is that the pressure on the brains of present and future coders
> might be too much for the benefit accrued.  As a compromise, I can suggest
> implementing cart_iterate not as
> adj_iterate() {
>  if (not_cartesian) skip;
> 
> but as an exact copy of adj_iterate code with "if (not_cartesian) skip"
> done right after "for()".  Instruction-wise this would be half-way
> between the above and the perfect solution.

If is_cardinal_dir is a macro or inline it is nearly a no-op and this 
method is great.  But if it's turned into a function then the overhead 
will become pretty significant.

> If the ordering of the directions is changed to rotational, the test for
> cardinal-ness is just a test for even-ness (or odd-ness).

Except on a hex map.

-----

We have a similar problem for adjc_dir_iterate in that in hex maps not 
all directions are "valid".  So either we need a call to is_valid_dir 
here or we need something else clever.  adjc_dir_iterate actually is 
used a lot (unlike cart_iterate which is almost never used) so there 
actually is a speed issue here.  However the alternatives have drawbacks 
as well:

1.  You might think we can just modify DIR_DX and DIR_DY and lower the 
number of directions.  But we can't!  enum direction8 hard-codes the 
list of directions, so the order of iteration is fixed.  We may skip 
directions but we can't reorder them.

DIR_DX and DIR_DY are therefore tied to the direction8 directions, and 
there is no need or possibility to change them.  It might be possible to 
remove direction8 entirely and just use an integer value, but this is a 
much longer-term goal.

But what we might be able to do is more efficient filtering.

2.  Make new arrays VALID_DIRS[] and CARINDAL_DIRS[].  These arrays 
contain the list of valid (for adjc_dir_iterate) and cardinal (for 
cart_iterate) directions.  Of course these arrays must be initialized, 
and that means extra code (in init_topology).

3.  Make is_valid_dir and is_cardinal_dir inline functions in map.h, and 
pass the filter function as a parameter to a third "backend" macro.  So 
we have something like

   #define adjc_dir_iterate(...) \
     adjc_dir_filter_iterate(..., is_valid_dir)

   #define cart_iterate(...) \
     adjc_dir_filter_iterate(..., is_cardinal_dir)

which gives the same performance as Greg's suggestion (that is, pretty 
good) but with much less code.  The drawback is that the filter function 
needs to be inline (it can't be a macro, and a function would be quite 
slow).

4.  Just make cart_iterate a wrapper for adjc_dir_iterate, with an extra 
filter function.  This is slower, but cart_iterate is almost never used 
anyway so it will be unmeasurable.  One of the other solutions is still 
needed for adjc_dir_iterate.

I really don't like the idea of duplicating the code in adjc_dir_iterate 
and cart_iterate...

jason