[Freeciv-Dev] (PR#12776) Bug? MOVE_COST_AIR is 1.

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=12776 >

> [jdorje - Tue Apr 12 16:43:38 2005]:
> 
> in unit.h MOVE_COST_AIR is defined as 1.  But looking at 
> server/gotohand.c (the only user) it looks like it should be SINGLE_MOVE.
> 
> In the actual server (non-AI) code I guess air moves are already 
> special-cased as SINGLE_MOVE.
> 
> If this is a bug it seems like a very major one that would cause AI 
> fighters to crash and burn.

Well, there's a problem here but it's not exactly a bug.

MOVE_COST_AIR is a special case for one function in gotohand.c
(air_can_move_between).  Users of this function should *already* have
divided by SINGLE_MOVE so this function just operates in steps (not in
unit moves).

The problem is that MOVE_COST_AIR is supposed to be SINGLE_MOVE.  It
takes SINGLE_MOVE moves to take an air step.  So I suggest that:

1.  We remove MAX_COST_AIR and fix air_can_move_between (no constant is
needed here, it's just unity).

2.  Add back MAX_COST_AIR meaning the correct thing (SINGLE_MOVE) and
actually use it in the core code.

This patch implements part 1.  I also added some simple sanity checks.

-jason

? diff
Index: common/unit.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/unit.h,v
retrieving revision 1.139
diff -u -r1.139 unit.h
--- common/unit.h       21 Mar 2005 12:28:00 -0000      1.139
+++ common/unit.h       18 Apr 2005 06:06:23 -0000
@@ -189,7 +189,6 @@
 #define MOVE_COST_RIVER 1
 #define MOVE_COST_RAIL 0
 #define MOVE_COST_ROAD 1
-#define MOVE_COST_AIR 1
 
 #define unit_list_iterate_safe(unitlist, punit) \
 { \
Index: server/gotohand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/gotohand.c,v
retrieving revision 1.190
diff -u -r1.190 gotohand.c
--- server/gotohand.c   13 Apr 2005 18:41:11 -0000      1.190
+++ server/gotohand.c   18 Apr 2005 06:06:24 -0000
@@ -1472,6 +1472,10 @@
  possible without running out of moves. It returns -1 if it is
  impossible.
 
+  Note that the 'moves' passed to this function should be the number of
+  steps the air unit has left.  The caller should *already* have
+  divided by SINGLE_MOVE.
+
 The function has 3 stages:
 Try to rule out the possibility in O(1) time              else
 Try to quickly verify in O(moves) time                    else
@@ -1493,6 +1497,7 @@
     return -1;
   }
   if (total_distance == 0) {
+    assert(moves >= 0);
     return moves;
   }
 
@@ -1524,7 +1529,8 @@
   if (dist == 1) {
     /* Looks like the O(n) quicksearch worked. */
     assert(real_map_distance(ptile, dest_tile) == 1);
-    return moves - total_distance * MOVE_COST_AIR;
+    assert(moves - total_distance >= 0);
+    return moves - total_distance;
   }
 
   /* 
@@ -1559,15 +1565,16 @@
       if (same_pos(tile1, dest_tile)) {
        /* We're there! */
        freelog(LOG_DEBUG, "air_can_move_between: movecost: %i",
-               WARMAP_COST(ptile) + MOVE_COST_AIR);
-       /* The -MOVE_COST_AIR is because we haven't taken the final
+               WARMAP_COST(ptile) + 1);
+       /* The -1 is because we haven't taken the final
           step yet. */
-       return moves - WARMAP_COST(ptile) - MOVE_COST_AIR;
+       assert(moves - WARMAP_COST(ptile) - 1 >= 0);
+       return moves - WARMAP_COST(ptile) - 1;
       }
 
       /* We refuse to goto through unsafe airspace. */
       if (airspace_looks_safe(tile1, pplayer)) {
-       int cost = WARMAP_COST(ptile) + MOVE_COST_AIR;
+       int cost = WARMAP_COST(ptile) + 1;
 
        WARMAP_COST(tile1) = cost;