[Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
Raimar Falke wrote:
>
> On Wed, Aug 15, 2001 at 03:28:26AM -0400, Jason Dorje Short wrote:
> > Trent Piepho wrote:
> > >
> > > On Wed, 15 Aug 2001, Jason Dorje Short wrote:
> > > > This patch removes the magic numbers and expressions associated with the
> > > > DIR_D[XY] macros, collecting all such implementation-dependent code into
> > > > map.[ch].
> > > > Additions to map.[ch]:
> > > > - macros dir_reverse(dir) and dir_is_cardinal(dir) are created
> > >
> > > Since these are macros, convention is to give them names in all caps.
> >
> > They are macros, but act just like functions (and could be made so in
> > the future).
>
> I would like to second Trent here. There are macros and this property
> should be visible by the name. You may also define both: a macro and a
> function.
How come this convention isn't held by, say, the iteration macros?
Sorry to be argumentative here, but this seems counter-intuitive to me -
and also makes the code look uglier IMO, which you cite as a reason for
another change.
As for the names being too long, DIR_REVERSE seems to be much less
confusing than DIR_REV.
> > > > - the directional names used for logging ("N") are defined globally
> > >
> > > Wouldn't a function be better than a global variable?
>
> I also thought about this when I read the patch.
> > These strings are only used in debugging output, and should not be
> > translated. I don't see any other advantage of a function - you
> > might as well use a function in place of DIR_DX and DIR_DY. Am I
> > missing something?
>
> Compare
> freelog(LOG_NORMAL, "direction of next step is %s",DIR_NAMES[dir]);
> with
> freelog(LOG_NORMAL, "direction of next step is %s",get_dir_name(dir));
>
> At least for me the second one looks nicer. And you can also put extra
> checks in the function. So I would propose something like this:
Okay - although I don't think it really looks nicer, having extra checks
in the function is nice.
I've currently put the function prototype in map.h next to the
definition of DIR_DX/DIR_DY, even though all other function prototypes
are collected up above. This makes sense to me, but is it the correct
thing to do?
The attached patch capitalizes the macro names and makes the directional
names a function, as well as removing the unnecessary comment that I
forgot to take out before (pointed out by someone else).
> > Well, there's another mess here. The client defines its own set of
> > cardinal directions, using an enumeration and the names DIR_NORTH, etc.
> > There will inevitably be confusion. I made a comment about this in my
> > own enumeration.
>
> After this one gets committed I hope I will see a second patch from
> you.
Indeed...
jasonIndex: client/goto.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/goto.c,v
retrieving revision 1.16
diff -u -r1.16 goto.c
--- client/goto.c 2001/08/14 11:04:19 1.16
+++ client/goto.c 2001/08/15 15:52:34
@@ -306,7 +306,6 @@
static void create_goto_map(struct unit *punit, int src_x, int src_y,
enum goto_move_restriction restriction)
{
- static const char *d[] = { "NW", "N", "NE", "W", "E", "SW", "S", "SE" };
int x, y, x1, y1, dir;
struct tile *psrctile, *pdesttile;
enum unit_move_type move_type = unit_types[punit->type].move_type;
@@ -325,8 +324,8 @@
/* Try to move to all tiles adjacent to x,y. The coordinats of the tile we
try to move to are x1,y1 */
for (dir = 0; dir < 8; dir++) {
- /* if the direction is N, S, E or W d[dir][1] is the /0 character... */
- if ((restriction == GOTO_MOVE_CARDINAL_ONLY) && d[dir][1]) continue;
+ if ((restriction == GOTO_MOVE_CARDINAL_ONLY) && !DIR_IS_CARDINAL(dir))
+ continue;
x1 = x + DIR_DX[dir];
y1 = y + DIR_DY[dir];
@@ -385,10 +384,10 @@
goto_map.move_cost[x1][y1] = total_cost;
if (add_to_queue)
add_to_mapqueue(total_cost, x1, y1);
- goto_map.vector[x1][y1] = 128>>dir;
+ goto_map.vector[x1][y1] = 1 << DIR_REVERSE(dir);
freelog(LOG_DEBUG,
"Candidate: %s from (%d, %d) to (%d, %d), cost %d",
- d[dir], x, y, x1, y1, total_cost);
+ dir_get_name(dir), x, y, x1, y1, total_cost);
}
break;
@@ -417,10 +416,10 @@
goto_map.move_cost[x1][y1] = total_cost;
if (add_to_queue)
add_to_mapqueue(total_cost, x1, y1);
- goto_map.vector[x1][y1] = 128>>dir;
+ goto_map.vector[x1][y1] = 1 << DIR_REVERSE(dir);
freelog(LOG_DEBUG,
"Candidate: %s from (%d, %d) to (%d, %d), cost %d",
- d[dir], x, y, x1, y1, total_cost);
+ dir_get_name(dir), x, y, x1, y1, total_cost);
}
break;
@@ -443,10 +442,10 @@
goto_map.move_cost[x1][y1] = total_cost;
if (add_to_queue)
add_to_mapqueue(total_cost, x1, y1);
- goto_map.vector[x1][y1] = 128>>dir;
+ goto_map.vector[x1][y1] = 1 << DIR_REVERSE(dir);
freelog(LOG_DEBUG,
"Candidate: %s from (%d, %d) to (%d, %d), cost %d",
- d[dir], x, y, x1, y1, total_cost);
+ dir_get_name(dir), x, y, x1, y1, total_cost);
}
break;
@@ -644,7 +643,7 @@
if (dir >= 4) {
x = x1;
y = y1;
- dir = 7 - dir;
+ dir = DIR_REVERSE(dir);
}
return goto_map.drawn[x] + y*4 + dir;
Index: client/gui-gtk/mapview.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk/mapview.c,v
retrieving revision 1.87
diff -u -r1.87 mapview.c
--- client/gui-gtk/mapview.c 2001/08/13 21:17:53 1.87
+++ client/gui-gtk/mapview.c 2001/08/15 15:52:37
@@ -2012,8 +2012,8 @@
put_line(map_canvas->window, src_x, src_y, dir);
}
if (tile_visible_mapcanvas(dest_x, dest_y)) {
- put_line(map_canvas_store, dest_x, dest_y, 7-dir);
- put_line(map_canvas->window, dest_x, dest_y, 7-dir);
+ put_line(map_canvas_store, dest_x, dest_y, DIR_REVERSE(dir));
+ put_line(map_canvas->window, dest_x, dest_y, DIR_REVERSE(dir));
}
increment_drawn(src_x, src_y, dir);
@@ -2062,14 +2062,15 @@
normalize_map_pos(&dest_x, &dest_y);
refresh_tile_mapcanvas(dest_x, dest_y, 1);
if (NORMAL_TILE_WIDTH%2 == 0 || NORMAL_TILE_HEIGHT%2 == 0) {
- if (dir == 2) { /* Since the tle doesn't have a middle we draw an extra
pixel
- on the adjacent tile when drawing in this direction. */
+ if (dir == DIR8_NORTHEAST) {
+ /* Since the tile doesn't have a middle we draw an extra pixel
+ on the adjacent tile when drawing in this direction. */
dest_x = src_x + 1;
dest_y = src_y;
assert(is_real_tile(dest_x, dest_y));
normalize_map_pos(&dest_x, &dest_y);
refresh_tile_mapcanvas(dest_x, dest_y, 1);
- } else if (dir == 5) { /* the same */
+ } else if (dir == DIR8_SOUTHWEST) { /* the same */
dest_x = src_x;
dest_y = src_y + 1;
assert(is_real_tile(dest_x, dest_y));
Index: client/gui-mui/mapclass.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-mui/mapclass.c,v
retrieving revision 1.56
diff -u -r1.56 mapclass.c
--- client/gui-mui/mapclass.c 2001/08/05 14:44:58 1.56
+++ client/gui-mui/mapclass.c 2001/08/15 15:52:40
@@ -1283,8 +1283,10 @@
put_line(_rp(o), _mleft(o),_mtop(o),src_x, src_y, dir);
}
if (tile_visible_mapcanvas(dest_x, dest_y)) {
- put_line(data->map_layer->rp, 0,0,dest_x, dest_y, 7-dir);
- put_line(_rp(o), _mleft(o),_mtop(o),dest_x, dest_y, 7-dir);
+ put_line(data->map_layer->rp, 0, 0,
+ dest_x, dest_y, DIR_REVERSE(dir));
+ put_line(_rp(o), _mleft(o),_mtop(o),
+ dest_x, dest_y, DIR_REVERSE(dir));
}
increment_drawn(src_x, src_y, dir);
}
@@ -1336,14 +1338,15 @@
normalize_map_pos(&dest_x, &dest_y);
refresh_tile_mapcanvas(dest_x, dest_y, 1); /* !! */
if (NORMAL_TILE_WIDTH%2 == 0 || NORMAL_TILE_HEIGHT%2 == 0) {
- if (dir == 2) { /* Since the tile doesn't have a middle we draw
an extra pixel
- on the adjacent tile when drawing in this
direction. */
+ if (dir == DIR8_NORTHEAST) {
+ /* Since the tile doesn't have a middle we draw an extra pixel
+ on the adjacent tile when drawing in this direction. */
dest_x = src_x + 1;
dest_y = src_y;
assert(is_real_tile(dest_x, dest_y));
normalize_map_pos(&dest_x, &dest_y);
refresh_tile_mapcanvas(dest_x, dest_y, 1); /* !! */
- } else if (dir == 5) { /* the same */
+ } else if (dir == DIR8_SOUTHWEST) { /* the same */
dest_x = src_x;
dest_y = src_y + 1;
assert(is_real_tile(dest_x, dest_y));
Index: client/gui-xaw/mapview.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-xaw/mapview.c,v
retrieving revision 1.73
diff -u -r1.73 mapview.c
--- client/gui-xaw/mapview.c 2001/08/14 07:21:57 1.73
+++ client/gui-xaw/mapview.c 2001/08/15 15:52:41
@@ -1329,8 +1329,8 @@
}
if (tile_visible_mapcanvas(dest_x, dest_y)) {
- put_line(map_canvas_store, dest_x, dest_y, 7-dir);
- put_line(XtWindow(map_canvas), dest_x, dest_y, 7-dir);
+ put_line(map_canvas_store, dest_x, dest_y, DIR_REVERSE(dir));
+ put_line(XtWindow(map_canvas), dest_x, dest_y, DIR_REVERSE(dir));
}
increment_drawn(src_x, src_y, dir);
@@ -1359,14 +1359,15 @@
normalize_map_pos(&dest_x, &dest_y);
refresh_tile_mapcanvas(dest_x, dest_y, 1);
if (NORMAL_TILE_WIDTH%2 == 0 || NORMAL_TILE_HEIGHT%2 == 0) {
- if (dir == 2) { /* Since the tle doesn't have a middle we draw an extra
pixel
- on the adjacent tile when drawing in this direction. */
+ if (dir == DIR8_NORTHEAST) {
+ /* Since the tile doesn't have a middle we draw an extra pixel
+ on the adjacent tile when drawing in this direction. */
dest_x = src_x + 1;
dest_y = src_y;
assert(is_real_tile(dest_x, dest_y));
normalize_map_pos(&dest_x, &dest_y);
refresh_tile_mapcanvas(dest_x, dest_y, 1);
- } else if (dir == 5) { /* the same */
+ } else if (dir == DIR8_SOUTHWEST) { /* the same */
dest_x = src_x;
dest_y = src_y + 1;
assert(is_real_tile(dest_x, dest_y));
Index: common/map.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
retrieving revision 1.79
diff -u -r1.79 map.c
--- common/map.c 2001/08/14 14:31:19 1.79
+++ common/map.c 2001/08/15 15:52:42
@@ -1058,8 +1058,8 @@
tile0->move_cost[dir] = tile_move_cost_ai(tile0, tile1, x, y,
x1, y1, maxcost);
/* reverse: not at all obfuscated now --dwp */
- tile1->move_cost[7 - dir] = tile_move_cost_ai(tile1, tile0, x1, y1,
- x, y, maxcost);
+ tile1->move_cost[DIR_REVERSE(dir)] =
+ tile_move_cost_ai(tile1, tile0, x1, y1, x, y, maxcost);
} else {
/* trying to move off the screen. */
tile0->move_cost[dir] = maxcost;
@@ -1333,4 +1333,15 @@
assert(is_real_tile(*x, *y));
normalize_map_pos(x, y);
+}
+
+/**************************************************************************
+Return the debugging name of the direction.
+**************************************************************************/
+char* dir_get_name(int dir)
+{
+ static const char *names[8] = { "NW", "N", "NE", "W",
+ "E", "SW", "S", "SE" };
+ assert(dir >= 0 && dir < 8);
+ return (char*)names[dir]; /* discard "const" qualifier */
}
Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.85
diff -u -r1.85 map.h
--- common/map.h 2001/08/14 14:31:19 1.85
+++ common/map.h 2001/08/15 15:52:43
@@ -429,6 +429,25 @@
|5|6|7|
-------
*/
+enum {
+ /* FIXME: DIR8 is used to avoid conflict with another enum */
+ DIR8_NORTHWEST = 0,
+ DIR8_NORTH = 1,
+ DIR8_NORTHEAST = 2,
+ DIR8_WEST = 3,
+ DIR8_EAST = 4,
+ DIR8_SOUTHWEST = 5,
+ DIR8_SOUTH = 6,
+ DIR8_SOUTHEAST = 7
+};
+
+#define DIR_REVERSE(dir) (7 - (dir))
+#define DIR_IS_CARDINAL(dir) \
+ ((dir) == DIR8_NORTH || (dir) == DIR8_EAST || \
+ (dir) == DIR8_WEST || (dir) == DIR8_SOUTH)
+
+char* dir_get_name(int dir);
+
extern const int DIR_DX[8];
extern const int DIR_DY[8];
Index: server/gotohand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/gotohand.c,v
retrieving revision 1.102
diff -u -r1.102 gotohand.c
--- server/gotohand.c 2001/08/14 11:04:22 1.102
+++ server/gotohand.c 2001/08/15 15:52:45
@@ -329,7 +329,7 @@
/* else c = ptile->move_cost[k];
This led to a bad bug where a unit in a swamp was considered too far
away */
else { /* we have a city */
- int tmp = map_get_tile(x1, y1)->move_cost[7-dir]; /* The move in the
opposite direction*/
+ int tmp = map_get_tile(x1, y1)->move_cost[DIR_REVERSE(dir)];
move_cost = (ptile->move_cost[dir] + tmp +
(ptile->move_cost[dir] > tmp ? 1 : 0))/2;
}
@@ -624,7 +624,6 @@
enum goto_move_restriction restriction)
{
struct player *pplayer = unit_owner(punit);
- static const char *d[] = { "NW", "N", "NE", "W", "E", "SW", "S", "SE" };
int igter, x, y, x1, y1, dir;
int orig_x, orig_y;
struct tile *psrctile, *pdesttile;
@@ -678,8 +677,8 @@
/* Try to move to all tiles adjacent to x,y. The coordinats of the tile we
try to move to are x1,y1 */
for (dir = 0; dir < 8; dir++) {
- /* if the direction is N, S, E or W d[dir][1] is the /0 character... */
- if ((restriction == GOTO_MOVE_CARDINAL_ONLY) && d[dir][1]) continue;
+ if ((restriction == GOTO_MOVE_CARDINAL_ONLY) && !DIR_IS_CARDINAL(dir))
+ continue;
x1 = x + DIR_DX[dir];
y1 = y + DIR_DY[dir];
@@ -755,15 +754,15 @@
if (warmap.cost[x1][y1] > total_cost) {
warmap.cost[x1][y1] = total_cost;
add_to_mapqueue(total_cost, x1, y1);
- local_vector[x1][y1] = 128>>dir;
+ local_vector[x1][y1] = 1 << DIR_REVERSE(dir);
freelog(LOG_DEBUG,
"Candidate: %s from (%d, %d) to (%d, %d), cost %d",
- d[dir], x, y, x1, y1, total_cost);
+ dir_get_name(dir), x, y, x1, y1, total_cost);
} else if (warmap.cost[x1][y1] == total_cost) {
- local_vector[x1][y1] |= 128>>dir;
+ local_vector[x1][y1] |= 1 << DIR_REVERSE(dir);
freelog(LOG_DEBUG,
"Co-Candidate: %s from (%d, %d) to (%d, %d), cost %d",
- d[dir], x, y, x1, y1, total_cost);
+ dir_get_name(dir), x, y, x1, y1, total_cost);
}
}
break;
@@ -822,15 +821,15 @@
if (warmap.seacost[x1][y1] > total_cost) {
warmap.seacost[x1][y1] = total_cost;
add_to_mapqueue(total_cost, x1, y1);
- local_vector[x1][y1] = 128>>dir;
+ local_vector[x1][y1] = 1 << DIR_REVERSE(dir);
freelog(LOG_DEBUG,
"Candidate: %s from (%d, %d) to (%d, %d), cost %d",
- d[dir], x, y, x1, y1, total_cost);
+ dir_get_name(dir), x, y, x1, y1, total_cost);
} else if (warmap.seacost[x1][y1] == total_cost) {
- local_vector[x1][y1] |= 128>>dir;
+ local_vector[x1][y1] |= 1 << DIR_REVERSE(dir);
freelog(LOG_DEBUG,
"Co-Candidate: %s from (%d, %d) to (%d, %d), cost %d",
- d[dir], x, y, x1, y1, total_cost);
+ dir_get_name(dir), x, y, x1, y1, total_cost);
}
}
break;
@@ -865,15 +864,15 @@
if (warmap.cost[x1][y1] > total_cost) {
warmap.cost[x1][y1] = total_cost;
add_to_mapqueue(total_cost, x1, y1);
- local_vector[x1][y1] = 128>>dir;
+ local_vector[x1][y1] = 1 << DIR_REVERSE(dir);
freelog(LOG_DEBUG,
"Candidate: %s from (%d, %d) to (%d, %d), cost %d",
- d[dir], x, y, x1, y1, total_cost);
+ dir_get_name(dir), x, y, x1, y1, total_cost);
} else if (warmap.cost[x1][y1] == total_cost) {
- local_vector[x1][y1] |= 128>>dir;
+ local_vector[x1][y1] |= 1 << DIR_REVERSE(dir);
freelog(LOG_DEBUG,
"Co-Candidate: %s from (%d, %d) to (%d, %d), cost %d",
- d[dir], x, y, x1, y1, total_cost);
+ dir_get_name(dir), x, y, x1, y1, total_cost);
}
}
break;
@@ -912,7 +911,7 @@
break;
for (dir = 0; dir < 8; dir++) {
- if ((restriction == GOTO_MOVE_CARDINAL_ONLY) && d[dir][1])
+ if ((restriction == GOTO_MOVE_CARDINAL_ONLY) && !DIR_IS_CARDINAL(dir))
continue;
if (local_vector[x][y] & (1<<dir)) {
@@ -923,10 +922,10 @@
move_cost = (move_type == SEA_MOVING) ? warmap.seacost[x1][y1] :
warmap.cost[x1][y1];
add_to_mapqueue(MAXCOST-1 - move_cost, x1, y1);
- warmap.vector[x1][y1] |= 128>>dir; /* Mark it on the warmap */
+ warmap.vector[x1][y1] |= 1 << DIR_REVERSE(dir); /* Mark it on the
warmap */
local_vector[x][y] -= 1<<dir; /* avoid repetition */
freelog(LOG_DEBUG, "PATH-SEGMENT: %s from (%d, %d) to (%d, %d)",
- d[7-dir], x1, y1, x, y);
+ dir_get_name(DIR_REVERSE(dir)), x1, y1, x, y);
}
}
}
@@ -950,7 +949,6 @@
const int dest_x, const int dest_y)
{
int k, d[8], x, y, n, a, best = 0, d0, d1, h0, h1, u, c;
- const char *dir[] = { "NW", "N", "NE", "W", "E", "SW", "S", "SE" };
struct tile *ptile, *adjtile;
int nearland;
struct city *pcity;
@@ -965,7 +963,7 @@
from going round in little circles to move across desirable squares */
for (k = 0; k < 8; k++) {
if (warmap.vector[punit->x][punit->y]&(1<<k)
- && !(restriction == GOTO_MOVE_CARDINAL_ONLY && dir[k][1])) {
+ && !(restriction == GOTO_MOVE_CARDINAL_ONLY && !DIR_IS_CARDINAL(k))) {
x = map_adjust_x(punit->x + DIR_DX[k]);
y = map_adjust_y(punit->y + DIR_DY[k]);
if (x == dest_x && y == dest_y)
@@ -974,7 +972,9 @@
}
for (k = 0; k < 8; k++) {
- if ((restriction == GOTO_MOVE_CARDINAL_ONLY) && dir[k][1]) continue;
+ if ((restriction == GOTO_MOVE_CARDINAL_ONLY) && !DIR_IS_CARDINAL(k))
+ continue;
+
if (!(warmap.vector[punit->x][punit->y]&(1<<k))) d[k] = 0;
else {
if (is_ground_unit(punit))
@@ -1066,7 +1066,7 @@
do {
do {
k = myrand(8);
- } while ((restriction == GOTO_MOVE_CARDINAL_ONLY) && dir[k][1]);
+ } while ((restriction == GOTO_MOVE_CARDINAL_ONLY) && !DIR_IS_CARDINAL(k));
} while (d[k] < best);
return(k);
}
@@ -1128,7 +1128,6 @@
{
struct player *pplayer = unit_owner(punit);
int x, y, dir;
- static const char *d[] = { "NW", "N", "NE", "W", "E", "SW", "S", "SE" };
int unit_id, dest_x, dest_y, waypoint_x, waypoint_y;
struct unit *penemy = NULL;
@@ -1188,7 +1187,7 @@
return;
}
- freelog(LOG_DEBUG, "Going %s", d[dir]);
+ freelog(LOG_DEBUG, "Going %s", dir_get_name(dir));
x = map_adjust_x(punit->x + DIR_DX[dir]);
y = punit->y + DIR_DY[dir]; /* no need to adjust this */
penemy = is_enemy_unit_tile(map_get_tile(x, y), punit->owner);
- [Freeciv-Dev] [PATCH] directional code cleanup: patch #1, Jason Dorje Short, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Trent Piepho, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Jason Dorje Short, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Raimar Falke, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1,
Jason Dorje Short <=
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Raimar Falke, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Jason Dorje Short, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Trent Piepho, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Raimar Falke, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Jason Dorje Short, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Raimar Falke, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Gaute B Strokkenes, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Gregory Berkolaiko, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Raimar Falke, 2001/08/15
- [Freeciv-Dev] Re: [PATCH] directional code cleanup: patch #1, Trent Piepho, 2001/08/15
|
|
