[Freeciv-Dev] Re: [PATCH] more small directional cleanups

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 09:03 AM 01/08/20 -0400, Jason Dorje Short wrote:
>Unfortunately, I appear to have missed at least one instance of "magic"
>code to handle the directional system in my previous patch.  A new patch
>fixes this, as well as cleaning up new magic code that I introduced:
>ftp://ftp.freeciv.org/freeciv/incoming/dir-patch2-1.diff.
>
>A follow-up to this patch replaces and unifies the current directional
>system by using the DIR_DX2 system instead of the current (DIR_D[XY])
>(and removing DIR_D[XY]2 in the process):
>ftp://ftp.freeciv.org/freeciv/incoming/dir-patch3-1.diff.
>
>Yet another tiny patch replaces a few simple instances of
>map_adjust_x/map_adjust_y with normalize_map_pos.  I've had this one
>sitting around for a while, but nobody else's changes seem to have been
>committed yet so here it is:
>ftp://ftp.freeciv.org/freeciv/incoming/dir-patch4-1.diff.  (It's a bit
>of a misnomer.)
>
>Each patch has a longer explanation included.  All have been tested
>under an all-AI game (although this should be unnecessary).
>
>jason

We are close. This is the version in corecleanup_06c, slightly more compact. 
Note normalize_map_pos, does the is_real_tile check, and only normalizes
on the way out when it returns true. Local vars are probably not that much
of an improvement, since there is just one assignment in typical case, so
it might as well be the right one.

/**************************************************************************
Random neighbouring square.
**************************************************************************/
void rand_neighbour(int x0, int y0, int *x, int *y)
{
  int choice;

  /* don't check twice for non-border cases (the norm), instead redo only
   * failed border ones, which are only 3 in 8 chances on average anyway.
   * This should also work for any map topography, not just cylindrical.
   * Initial point *must* be valid or this could loop forever.
   */
  assert(is_real_tile(x0, y0));
  do {
    choice = myrand(8);
    *x = x0 + DIR_DX2[choice];
    *y = y0 + DIR_DY2[choice];
  } while(!normalize_map_pos(x, y));
}

   
In the second one, you break tilespec by using DIR_DX directions without
also fixing up the hardcoded ttype_north = ttype_near[0] lines just after.
If you load up a client and find the coastlines have odd sandbars, you will
know you mussed up tilespec.c.

The correct patches for this are in corecleanup_06b, where the switch is
from the original DIR_DX2 to DIR_DX2(NW origin). Note. this is the only
place in the GUI where DIR_DX2 directions were used and it was (locally)
coded correctly.

The GUI must use DIR_DX, as there are datafiles for graphics components
that have been laid down in this order. Up until your previous patch the
GUI was self contained and completely separate from the core server DIR_DX
stuff. I actually reversed the changes in corecleanup_06b in the GUI when
all the DIR_DX2 directions were propagated into the main server. I left the
name as DIR_DX2, and didn't delete the DIR_DX pieces until all the GUI
implications were resolved.

Don't use % 8, use & 7 to mask off the dirbits. Divide operations are 
expensive compared to logical bit ones.

The aiunit code should be a square_iterate, as I think it is in 
corecleanup_06a.

Likewise most of the GUI normalize_map_pos fixes and a few more are in 
corecleanup_06a.

This is a better way to do cardinal_move ...
    cardinal_move =
       ( normalize_map_pos(&x1,&y1)
      && normalize_map_pos(&x2,&y2)
      && (x1 == x2 || y1 == y2) );
   
though your assert is probably better for development code.

We basically did the same things to the map_g/setters, except I have an
assert in the error return condition to trap all bad accesses. This is 
not triggered in my current code, which probably means that we are close
to getting rid of the normalize, i.e. it really should be done once in 
iteration loops or checked individual cases, and not on every low level
call - that is at least the goal. See corecleanup_06c for these as I
pretty much stuck all the controversial changes to such dearly beloved
functions into the later patch :-).

But, when both of us do pretty much the same thing, it is clear that it
probably needs to be done!

Cheers,
RossW