[Freeciv-Dev] Re: Corecleanup patch updates

[Gaute B Strokkenes <gs234@xxxxxxxxx>]

On Sat, 11 Aug 2001, rwetmore@xxxxxxxxxxxx wrote:

> At 08:48 AM 01/08/09 +0200, Gaute B Strokkenes wrote:
>
>>2) I'm still not sold on the DIR_D{X,Y}2 stuff.  What is the
>>   advantage of using them, as opposed to the old ones?
>
> First, there were two flavours in the original code, and the changes
> took over and have fully expanded on the second flavour. The first
> DIR_DX (in array sequence order) is now only really used in GUI
> code.  There it seems to have found its way into graphics data
> fragments. But this does seem to be completely self contained and
> isolated.
>
> A rotation order as used by DIR_DX2 allows one to do a lot more
> directional manipulations trivially. For example, moving forward,
> right or left one step is a wrapped linear index, and quite tricky
> in DIR_DX where 5 comes after 3 and 4 is between 1 and 6. For core
> code including all the AI heuristics, this is quite useful and the
> performance and robustness from a KISS implementation can be
> significant.

Well, I think I'm convinced that they're no worse than the old ones.
The only thing I can think about is that the old ones went through the
tiles in an order which maximizes locality.  I have no idea whether
this has any measurable effect on speed, though.  Incidentally, have
you tried to profile some AI-only games with and without your patches?

I see how the new system allows one to perform certain operations more
easily, but at the same time I do not see that much of a use for
them.  The most common operation is IIRC to find the opposite
direction, which is probably quicker with the old system.

As I see it, the iteration macros exist to allow user code to loop
over certain sets of tiles safely and efficiently, whitout having to
worry about the gory details.  There are not that many of them:

common/city.h:70:#define city_map_iterate(x, y) \
common/city.h:101:#define city_radius_iterate(dx, dy) \
common/city.h:109:#define map_city_radius_iterate(city_x, city_y, x_itr, y_itr) 
\
common/map.h:355:#define square_iterate(SI_center_x, SI_center_y, radius, 
SI_x_itr, SI_y_itr)  \
common/map.h:373:#define adjc_iterate(RI_center_x, RI_center_y, RI_x_itr, 
RI_y_itr)            \
common/map.h:395:#define adjc_dir_iterate(center_x, center_y, x_itr, y_itr, 
dir_itr)           \
common/map.h:423:#define whole_map_iterate(WMI_x_itr, WMI_y_itr)                
               \
common/map.h:453:#define cartesian_adjacent_iterate(x, y, IAC_x, IAC_y) \

In my opinion, it is not necessary to add several layers of
abstraction beneath them.  I am doubtful as to whether this improves
performance, and I certainly don't think that it makes the code any
more readable.  I find the succession of macros defined in terms of
core_iterate() and friends are rather tricky to follow (lots of magic
constants etc.)  Also, formatting:

This:

+#define fastblock_iterate(x, y, xy1, xy0, xs, xm) {                     \
+  int x, y, _xm= (xm), _xs= (xs), _xy0= (xy0), _xy= (xy1);              \
+  /* loop from upper xy1 to lower xy0 point skipping xs at xm */        \
+  while( _xy-- > _xy0 ) {                                               \
+    y= _xy/map.xsize, x= _xy%map.xsize;                                 \
+    if( x == _xm ) { _xy-= _xs; continue; }

should be:

+#define fastblock_iterate(x, y, xy1, xy0, xs, xm)                \
+{                                                                \
+  int x, y;                                                      \
+  int _xm = (xm);                                                \
+  int _xs = (xs);                                                \
+  int _xy0 = (xy0);                                              \
+  int _xy = (xy1);                                               \
+                                                                 \
+  /* Loop from upper xy1 to lower xy0 point skipping xs at xm */ \
+                                                                 \
+  while (_xy-- > _xy0) {                                         \
+    y = _xy / map.xsize;                                         \
+    y = _xy % map.xsize;                                         \
+    if (x == _xm) {                                              \
+      _xy -= _xs                                                 \
+      continue;                                                  \
+    }

In general, try running your code through indent -kr -i2.  That should
take care of the greater part, we can argue about the rest afterwards.

I honestly can't bear reading through and evaluating the rest of the
iteration macro stuff unless you do this, so beware.

I do not like your change to the map coordinate stuff.  The basic idea
is sound, but the implementation is far too complicated.  First of
all, it's too cumbersome to have one set of macro and a set of real
functions that do the same thing.  We should just stick to one of the
two.  Also, in general it does not make sense to macroise something
just because it is possible to do so.  It only really makes sense if
there is any time and / or readability to save.  Also, I don't like
macros with lower-case names that do not evaluate all their arguments
exactly once.  (I know we have a number of those in Freeciv already,
but I'd like to change that.)

Also, I don't like the way we have lots of map_{g,s}_fubaz macros /
functions.  You may recall that I announced my intent to do some work
in this area some time ago and I'll be posting patches soon, but
briefly I have one macro, MAP(), which takes coordinates x and y as
arguments and returns a pointer to the relevant tile.  Optionally,
this macro will bounds-check its arguments if you give the right
option at compile time.  The plan is to re-route all tile access
through this macro.  That way, a lot of map_set_this() and
map_set_that() can become MAP()->whatever = something; etc.

You're also assuming that you can normalize x and y coordinates
separately.  That's fine for rectangles, cylinders and tori but fails
for Möbius strips and Klein bottles.  The last two are perfectly
possibly, if a bit odd, choices of topology and it would be kind of
fun to support them.

>>4) Be aware that when you replace for loops with adjc_iterate you
>>   are no longer looping over the centre tile.  This causes bugs in
>>   e.g. ai_manage_barbarian_leader().
>
> When this was done, I usually also removed a loop check that skipped
> the centre square. If I used adjacent in a spot where square was
> still required, I agree I added a bug. I'll double check them all.
>
> In this case, can_unit_move_to_tile() always returns false for the
> current tile (it only wants to move to adjacent tiles). I think this
> means that the centre tile code is effectively optimized out.

Oops, you're right.  (While you're at it, please move the
initialization of safest_{x,y} to just before the loop.)

> This may not be desired of course ...

Dunno.

>>5) Do not peek at the variables that are used to implement any of
>>   the _iterate macros in the code that uses them since doing so
>>   defeats the purpose of the excercise.  In other words, if you
>>   need to know what direction you're going in you should use
>>   adjc_dir_iterate() rather than adjc_iterate() and peek(_nn).
>
> I disagree that this is the purpose of the exercise :-).
>
> I am also quite open to having the loop variable be considered as
> part of the macro interface, i.e. fair game. The primary reason I
> see for defining it internally is that it is always needed, and
> there may be times when the user doesn't use it and would be
> confused by having to provide a required element with no obvious
> reason.

In current CVS there is adjc_dir_iterate() and adjc_iterate().  Apart
from some minor details that we should eradicate, the difference is
that adjc_dir_iterate() allows you to name the direction variable that
you're looping over.  If a piece of code

You can always define one in terms of the other, e.g.

#define adjc_iterate (x, y, x1, y1)                \
  {                                                \
    int AI_MACRO_dir;                              \
    adjc_dir_iterate(x, y, x1, y1, AI_MACRO_dir) {

I may have gotten the order of arguments wrong, but you get the idea.

I think having the user code specify the name of the variable is much
cleaner than reading a magic variable called _nn.

>>7) FC__MAP_C is not necessary.  AFAIK there is no harm in declaring
>>   them as extern in map.h, and then defining them in map.c.
>
> Again I disagree. This time quite strongly. Splitting the definition
> and allocation/default values, which in themselves are very useful
> documentation just makes finding things in the code more difficult.
> In this case all the parts are in one spot, no confusion results and
> the fact that you need to allocation the storage in map.c or
> params.c or wherever one chooses tommorrow is totally irrelevant and
> meaningless.

From my perspective, headers that do one thing when included from one
file and something else from others are bad.  If you see extern int
foo; in bar.h it's a no brainer to look in bar.c for the actual
definition.

>>8) You dropped an assert(is_real_tile(x,y)) in find_route().
>
> There is only one assert(0) at the end in 1.11.10, and it is still
> there. Did I miss something?

Yes.  This is one place where I think it's better no to use
adc_iterate(), since doing it the current way ensures that the goto
route is checked.

>>9) I'm not optimistic about reading values from void_tile.  IIRC we
>>   have had void_tile clobberation bugs in the past, and in the long
>>   term we ought to get rid of it entirely.
>
> I am beginning to recognize the nerves some of these things hit. But
> the problems are not in reading from void_tile, but the fact that it
> is (still) returned by get_map_tile as a (bad) fix for people that
> haven't checked their coordinates. And this is caused by not really
> having a well defined set of coordinate checking tools available, or
> those that were available being perverted for sleazy reasons.
>
> BTW: most of the underlying problems with this are dimishing rapidly
> and it should be the case that you can remove it from here without
> problems now. In fact I set the return value here to "NULL" to trap
> a lot of the access points, and don't have any problems currently.
>
> Doing iterate loops means most accesses are checked implicitly
> before the map_get_tile() anyway.
>
> Getting rid of adjust_map_* in most of its invocations is also
> solving a lot of this. Using this, one usually doesn't do proper
> error checking and there were no end of fixups to "adjust"
> coordinates that were not legitimately adjustable. The "y"
> truncation fix is a prime example.

This is right on all counts, but it doesn't change the fact that
void_tile is a bad hack which ought to go away.  Cleanup patches
should move us closer to that goal, and not further away.

>>10) I don't think the use of adjc_iterate() in reset_move_costs() is
>>    appropriate, since memcpy() is rather ugly.  I think it would be
>>    better to leave as-is here.
>
> If you look at Jason's latest flavour for these which I have
> adopted, the main body of the loop is a "if" clause. One can do an
> "} else {" and run an invalid clause within the loop if needed.

Hm.  I don't think we want to make that a part of the documented
interface.

> But doing a structure copy which is what the memcpy does, is
> perfectly reasonable to initialize, and there is no need to manually
> set each value to a possibly hardcoded magic number that is the
> current buggy style.  If one wants to change the initialized
> default, one should be able to do this once where it is defined and
> have all the code that uses it copy from there. void_tile seemed to
> be the place to do this, i.e. where it was already done.

I would prefer to make MAX_COST a define somewhere.  Reasonable people
can disagree on how beautiful memcpy() is.  However, if we're not
going to use void_tile we would have to use another array somewhere,
at which point it would definitely look silly.

> On the flip side, are you suggesting that all the alternate map
> border conditions should be hardcoded into this loop, even though it
> would be just about the only place in the code that does this?

I don't see why that would be necessary.  We should have a macro
called XYSTEP() or something like that which takes in a (normalized)
coordinate and a direction and spits out the normalized coordinates of
the neighbouring tile in that direction.  Combined with
is_real_tile(), that's all we need.

> Note: if you do a "set maptype 3" in my current sandbox, this loop
> now will correctly handle Donut-World wrap conditions, whereas the
> original code would quite possibly fault on map_get_tile, or do
> something bad to the void_tile address that it returned (if you were
> running lenient).

It doesn't.  normalize_map_pos() is returns FALSE when the coordinates
do not correspond to a real tile, remember?

--
Big Gaute                               http://www.srcf.ucam.org/~gs234/
..  bleakness....  desolation....  plastic forks...