[Freeciv-Dev] Re: the directional system

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

At 06:59 AM 01/09/07 -0400, Jason Dorje Short wrote:
>First, there's an experimental patch for testing attached.  This patch
>changes the DIR_D[XY] arrays to use the DIR_D[XY]2 directional system. 
>It replaces a last few magic numbers that I've found.  Most importantly,
>everything seems to work cleanly (although I have not done real tests
>like comparisons of profiles).
>
>Given this, I have to ask you Ross: what was the problem you were having
>with changing the DIR_D[XY] arrays?  Can you still reproduce it?

The client dumps core when you try to do goto lines. There were also
some problems with rivers not joining and other spritely things. The
server core is clean, i.e. autogames run fine. But if you start doing
manual things with the GUI interface, or look closely at what is being
drawn there are problems. Note, some of the sprite issues may be fixed
if the non-isometric changes in tilespec.c that Raimar didn't like go 
in, i.e. all the ttype_north changes. But this isn't all the problems.

In short you need to play a few games, and actually try to do things
to generate or notice some of the failures.

It wasn't a mapsize issue, so I don't think your goto change has fixed
any of this, besides I tried all this once.

Next time I grab a current CVS I will apply your patch and see if I can
still see the problems.

>I'd like to bring up the proposed plans for changing the directional
>system again:
>
>- Ross's plan is to switch over slowly.  All of the core server and
>common code will be changed to use the DIR_DX2 arrays directly.  The GUI
>code (that he had had problems with before) will continue to use the
>DIR_DX arrays until that problem is solved.

Actually, my plan was to switch last week :-).

>- My plan is to switch over all at once, but not until everything is
>ready.  First off drop the DIR_DX2 arrays and just use the DIR_DX ones
>for now.  Then find and replace all remaining magic numbers/code so that
>the directional system is all encapsulated within map.h.  Finally make
>the switchover all at once.

While your plan is to wait until all the GUI issues are resolved and
fixed. I don't think the two issues need to be linked. The GUI did its
own thing before and can continue to do it after.

The linkage is what is new, and I think it doubles the size and probably 
squares the risk of problems and extra quibble chances.

Note much of the current GUI direction system is in tilespec.h. If you
look at 07j, you can see where virtually everything currently in map.h
associated with DIR_DX is moved to tilespec.h to consolidate this and
make the cleanup easier. The CART_DIR stuff should go to mapgen which 
is the only place that uses this. It will then vanish if the mapgen 
replacement routine goes in :-).

This clearly leaves only DIR_DX2 in the core - it wouldn't compile
otherwise. And thus is a good way to check that you did everything right
rather than just claiming to not have seen any residual problems.

By the same token, when the GUI changes, the same trick can be applied.
And when everything is running DIR_DX2 happily, then you can rename any
or all of it with no danger of this missing things or causing problems
because there are uncaught older flavours still lurking in dark corners.

>Ross, I did find one extra place where "magic" code is used: get_drawn()
>in client/goto.c.  This was easily spotted because the assertions of
>July 28th that were added by Thue (before that I'm not sure if there
>would have been a failed assertion check).  Could this account for the
>problems you were having before?

No, as explained above. I tried fixing all these things before backing 
off the GUI changes.

>To move forward on making this changeover, one of the plans needs to be
>chosen.  The next step for each of them would be quite different, so
>until a decision is made no step can be taken.

You can't do anything until you resolve all potential GUI issues including
the places where it loads the graphics from datafiles into DIR_DX ordered
memory, and then builds all the composite tile elements based on directional
matches.

>Ross: if there is no structural problem with the GUI code preventing the
>use of the new system (as it appears to me there is not), would you
>still be opposed to my system?  If you can still reproduce the problem
>you were having before, I'd like to take a look at it.

Here is a quick list of things to fix up.

First, change the order to start at the origin which is the NW corner.
This is a trivial noop. It will at least keep the first few directions
overlapping with the current DIR_DX, reduce complaints from people that
like to see the memory accesses from adjacent locations wherever 
possible. It is also easier to deal with as 4 side walks vs 3 and 2 
half walks when you are trying to work with the adjacent loops, or do
other things with the array values.

Get rid of the ^*&# "%" in DIR_REVERSE. If you want to stop the complaint
about doubling the cost of reversing directions (after you use efficient
operations), then you can do an unchecked version as follows comparable to 
the current unchecked one.

#define DIR_REVERSE(dir) (((dir) ^ 4))
  vs
#define DIR_REVERSE_CHECKED(dir) (((dir) ^ 4)&7)

It doesn't look like you fixed dir_ok in server/gotohand.c. The best way
to do this is to compute straightest_direction, then reverse it and see
if you are in the range -1/0/1. But the current straightest direction
is slightly buggy in that it always moves "diagonally" first, then runs
straight down the cartesian coordinate, rather than "straightest" which
would have it move in the "largest" direction first.

Until I can do a diff, I can't be sure if you have caught all the rest
of the fixes in the previous submissions. There are sufficient omissions 
here to make me think this would be really worthwhile before getting into
serious testing.

But if you haven't they will come up as "rediscovered" bugs under testing, 
so at this point in the development it won't matter that much. One really 
needs to bang on things a bit though to one's raise confidence levels.

In the end, though this needs to be done 100%. You can't really afford to
miss something, or have things dropped out for stupid reasons, or get them
fouled up by amateur massaging along the way when it comes to final release.

That's one of the reasons why I don't favour the kitchen sink approach.

Cheers,
RossW
=====

>jason? rc
>Index: client/goto.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/goto.c,v
>retrieving revision 1.20
>diff -u -r1.20 goto.c
>--- client/goto.c      2001/08/30 10:44:16     1.20
>+++ client/goto.c      2001/09/07 10:42:23
>@@ -671,8 +671,8 @@
> ***********************************************************************/
> int get_drawn(int x, int y, int dir)
> {
>-  if ((y == 0 && dir <= 2)
>-      || (y == map.ysize-1 && dir >= 5))
>+  if ((y == 0 && DIR_DY[dir] < 0)
>+      || (y == map.ysize-1 && DIR_DY[dir] > 0))
>     return 0;
> 
>   return *get_drawn_char(x, y, dir);
>Index: client/tilespec.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/client/tilespec.c,v
>retrieving revision 1.49
>diff -u -r1.49 tilespec.c
>--- client/tilespec.c  2001/08/24 08:22:01     1.49
>+++ client/tilespec.c  2001/09/07 10:42:25
>@@ -1097,8 +1097,8 @@
>   }
> 
>   for (dir=0; dir<8; dir++) {
>-    int x1 = x + DIR_DX2[dir];
>-    int y1 = y + DIR_DY2[dir];
>+    int x1 = x + DIR_DX[dir];
>+    int y1 = y + DIR_DY[dir];
>     if (normalize_map_pos(&x1, &y1)) {
>       ttype_near[dir] = map_get_terrain(x1, y1);
>       tspecial_near[dir] = map_get_special(x1, y1);
>Index: common/map.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
>retrieving revision 1.84
>diff -u -r1.84 map.c
>--- common/map.c       2001/09/06 21:23:07     1.84
>+++ common/map.c       2001/09/07 10:42:27
>@@ -42,16 +42,12 @@
> struct tile_type tile_types[T_LAST];
> 
> /* used to compute neighboring tiles */
>-const int DIR_DX[8] = { -1, 0, 1, -1, 1, -1, 0, 1 };
>-const int DIR_DY[8] = { -1, -1, -1, 0, 0, 1, 1, 1 };
>+const int DIR_DX[8] = { 0, 1, 1, 1, 0, -1, -1, -1 };
>+const int DIR_DY[8] = { -1, -1, 0, 1, 1, 1, 0, -1 };
> 
> /* like DIR_DX[] and DIR_DY[], only cartesian */
> const int CAR_DIR_DX[4] = {1, 0, -1, 0};
> const int CAR_DIR_DY[4] = {0, 1, 0, -1};
>-
>-/* used to compute neighboring tiles */
>-const int DIR_DX2[8] = { 0, 1, 1, 1, 0, -1, -1, -1 };
>-const int DIR_DY2[8] = { -1, -1, 0, 1, 1, 1, 0, -1 };
> 
> /* Names of specials.
>  * (These must correspond to enum tile_special_type in terrain.h.)
>Index: common/map.h
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
>retrieving revision 1.88
>diff -u -r1.88 map.h
>--- common/map.h       2001/09/06 18:19:09     1.88
>+++ common/map.h       2001/09/07 10:42:27
>@@ -417,29 +417,29 @@
> y1 = y + DIR_DX[dir];
> will give you the tile as shown below.
> -------
>-|0|1|2|
>+|7|0|1|
> |-+-+-|
>-|3| |4|
>+|6| |2|
> |-+-+-|
>-|5|6|7|
>+|5|4|3|
> -------
>  */
> 
> enum direction8 {
>   /* FIXME: DIR8 is used to avoid conflict with
>    * enum Directions in client/tilespec.h */
>-  DIR8_NORTHWEST = 0,
>-  DIR8_NORTH = 1,
>-  DIR8_NORTHEAST = 2,
>-  DIR8_WEST = 3,
>-  DIR8_EAST = 4,
>+  DIR8_NORTHWEST = 7,
>+  DIR8_NORTH = 0,
>+  DIR8_NORTHEAST = 1,
>+  DIR8_WEST = 6,
>+  DIR8_EAST = 2,
>   DIR8_SOUTHWEST = 5,
>-  DIR8_SOUTH = 6,
>-  DIR8_SOUTHEAST = 7
>+  DIR8_SOUTH = 4,
>+  DIR8_SOUTHEAST = 3
> };
> 
> /* return the reverse of the direction */
>-#define DIR_REVERSE(dir) (7 - (dir))
>+#define DIR_REVERSE(dir) (((dir) + 4) % 8)
> 
> /* is the direction "cardinal"?  Cardinal directions
>  * (also called cartesian) are the four main ones */
>@@ -494,23 +494,6 @@
>   }                                                    \
> }
> 
>-
>-/*
>-used to compute neighboring tiles:
>-using
>-x1 = x + DIR_DX2[dir];
>-y1 = y + DIR_DY2[dir];
>-will give you the tile as shown below.
>--------
>-|7|0|1|
>-|-+-+-|
>-|6| |2|
>-|-+-+-|
>-|5|4|3|
>--------
>- */
>-extern const int DIR_DX2[8];
>-extern const int DIR_DY2[8];
> 
> #define MAP_DEFAULT_HUTS         50
> #define MAP_MIN_HUTS             0
>Index: server/gotohand.c
>===================================================================
>RCS file: /home/freeciv/CVS/freeciv/server/gotohand.c,v
>retrieving revision 1.111
>diff -u -r1.111 gotohand.c
>--- server/gotohand.c  2001/08/30 13:00:16     1.111
>+++ server/gotohand.c  2001/09/07 10:42:30
>@@ -504,13 +504,13 @@
>   go_y = dest_y > src_y ? 1 : -1;
> 
>   if (src_x == dest_x)
>-    best_dir = (go_y > 0) ? 6 : 1;
>+    best_dir = (go_y > 0) ? DIR8_SOUTH : DIR8_NORTH;
>   else if (src_y == dest_y)
>-    best_dir = (go_x > 0) ? 4 : 3;
>+    best_dir = (go_x > 0) ? DIR8_EAST : DIR8_WEST;
>   else if (go_x > 0)
>-    best_dir = (go_y > 0) ? 7 : 2;
>+    best_dir = (go_y > 0) ? DIR8_SOUTHEAST : DIR8_NORTHEAST;
>   else /* go_x < 0 */
>-    best_dir = (go_y > 0) ? 5 : 0;
>+    best_dir = (go_y > 0) ? DIR8_SOUTHWEST : DIR8_NORTHWEST;
> 
>   return (best_dir);
> }
>