[Freeciv-Dev] (PR#10336) citymap cleanup

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=10336 >

This patch makes the following changes to the citymap code 
(common/aicore/citymap.[ch]):

- Make the "inline" functions into normal functions.  This has no 
measurable impact on speed and allows the global citymap variable to be 
made local and static.

- Make the "citymap" array allocated dynamically.  Instead of taking 4 * 
255 * 255 = 260k of memory, it usually only takes 4 * 4000 = 16k of 
memory.  However because index_to_map_pos is used it may be a little 
slower (though it could be faster depending on how your computer does 
memory caching).

- In passing, the following is fixed:

   -          citymap[x1][y1] = citymap[x1][y1]++;
   +          CITYMAP(x1, y1)++;

the former is clearly not what was intended since it does nothing (I 
think).  I assume the latter is what was meant.

- Another major bug is fixed: the citymap is never initialized.  It 
starts out as 0 but if you play multiple games it gets filled with 
garbage.  This is a severe bug and will probably never show up in any 
tests.  (To see it's a bug, remove the memset statement and see valgrind 
complain.)

(Side note: is this really the way it's supposed to be?  Shouldn't the 
citymap be reset at some point during the game?  Shouldn't there be one 
citymap per player?  Etc., etc.)

- A spurious assertion is removed.  No need to assert(is_normal_map_pos) 
when we're about to call map_pos_to_index().

jason

Index: common/map.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
retrieving revision 1.197
diff -u -r1.197 map.c
--- common/map.c        20 Sep 2004 16:42:30 -0000      1.197
+++ common/map.c        25 Sep 2004 21:46:54 -0000
@@ -18,6 +18,8 @@
 #include <assert.h>
 #include <string.h>            /* strlen */
 
+#include "citymap.h"
+
 #include "city.h"
 #include "fcintl.h"
 #include "log.h"
@@ -415,6 +417,7 @@
 
   generate_city_map_indices();
   generate_map_indices();
+  citymap_init_map();
 }
 
 /***************************************************************
Index: common/aicore/citymap.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/aicore/citymap.c,v
retrieving revision 1.3
diff -u -r1.3 citymap.c
--- common/aicore/citymap.c     5 Aug 2004 11:34:18 -0000       1.3
+++ common/aicore/citymap.c     25 Sep 2004 21:46:54 -0000
@@ -47,11 +47,21 @@
  * which has a negative value.
  */
 
-int citymap[MAP_MAX_WIDTH][MAP_MAX_HEIGHT];
+static int *citymap;
+#define CITYMAP(x, y) (citymap[map_pos_to_index((x), (y))])
 
 #define LOG_CITYMAP LOG_DEBUG
 
 /**************************************************************************
+  Initialize the citymap map.  Must be called when the map is generated.
+**************************************************************************/
+void citymap_init_map(void)
+{
+  citymap = fc_realloc(citymap, MAX_MAP_INDEX * sizeof(*citymap));
+  memset(citymap, 0, MAX_MAP_INDEX * sizeof(*citymap));
+}
+
+/**************************************************************************
   Initialize citymap by reserving worked tiles and establishing the
   crowdedness of (virtual) cities.
 **************************************************************************/
@@ -64,9 +74,9 @@
         struct tile *ptile = map_get_tile(x1, y1);
 
         if (ptile->worked) {
-          citymap[x1][y1] = -(ptile->worked->id);
+          CITYMAP(x1, y1) = -(ptile->worked->id);
         } else {
-          citymap[x1][y1] = citymap[x1][y1]++;
+          CITYMAP(x1, y1)++;
         }
       } map_city_radius_iterate_end;
     } city_list_iterate_end;
@@ -75,11 +85,11 @@
     if (unit_flag(punit, F_CITIES)
         && punit->ai.ai_role == AIUNIT_BUILD_CITY) {
       map_city_radius_iterate(goto_dest_x(punit), goto_dest_y(punit), x1, y1) {
-        if (citymap[x1][y1] >= 0) {
-          citymap[x1][y1]++;
+        if (CITYMAP(x1, y1) >= 0) {
+          CITYMAP(x1, y1)++;
         }
       } map_city_radius_iterate_end;
-      citymap[goto_dest_x(punit)][goto_dest_y(punit)] = -(punit->id);
+      CITYMAP(goto_dest_x(punit), goto_dest_y(punit)) = -(punit->id);
     }
   } unit_list_iterate_end;
 }
@@ -94,22 +104,22 @@
 #ifdef DEBUG
   assert(is_normal_map_pos(x, y));
   freelog(LOG_CITYMAP, "id %d reserving (%d, %d), was %d", 
-          id, x, y, citymap[x][y]);
-  assert(citymap[x][y] >= 0);
+          id, x, y, CITYMAP(x, y));
+  assert(CITYMAP(x, y) >= 0);
 #endif
 
   /* Tiles will now be "reserved" by actual workers, so free excess
    * reservations. Also mark tiles for city overlapping, or 
    * 'crowding'. */
   map_city_radius_iterate(x, y, x1, y1) {
-    if (citymap[x1][y1] == -id) {
-      citymap[x1][y1] = 0;
+    if (CITYMAP(x1, y1) == -id) {
+      CITYMAP(x1, y1) = 0;
     }
-    if (citymap[x1][y1] >= 0) {
-      citymap[x1][y1]++;
+    if (CITYMAP(x1, y1) >= 0) {
+      CITYMAP(x1, y1)++;
     }
   } map_city_radius_iterate_end;
-  citymap[x][y] = -(id);
+  CITYMAP(x, y) = -(id);
 }
 
 /**************************************************************************
@@ -122,10 +132,10 @@
 #endif
 
   map_city_radius_iterate(x, y, x1, y1) {
-    if (citymap[x1][y1] == -(id)) {
-      citymap[x1][y1] = 0;
-    } else if (citymap[x1][y1] > 0) {
-      citymap[x1][y1]--;
+    if (CITYMAP(x1, y1) == -(id)) {
+      CITYMAP(x1, y1) = 0;
+    } else if (CITYMAP(x1, y1) > 0) {
+      CITYMAP(x1, y1)--;
     }
   } map_city_radius_iterate_end;
 }
@@ -137,9 +147,33 @@
 void citymap_reserve_tile(int x, int y, int id)
 {
 #ifdef DEBUG
-  assert(is_normal_map_pos(x, y));
   assert(!citymap_is_reserved(x,y));
 #endif
 
-  citymap[x][y] = -id;
+  CITYMAP(x, y) = -id;
+}
+
+/**************************************************************************
+  Returns a positive value if within a city radius, which is 1 x number of
+  cities you are within the radius of, or zero or less if not. A negative
+  value means this tile is reserved by a city and should not be taken.
+**************************************************************************/
+int citymap_read(int x, int y)
+{
+  return CITYMAP(x, y);
+}
+
+/**************************************************************************
+  A tile is reserved if it contains a city or unit id, or a worker is
+  assigned to it.
+**************************************************************************/
+bool citymap_is_reserved(int x, int y)
+{
+  struct tile *ptile;
+
+  ptile = map_get_tile(x, y);
+  if (ptile->worked || ptile->city) {
+    return TRUE;
+  }
+  return (CITYMAP(x, y) < 0);
 }
Index: common/aicore/citymap.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/aicore/citymap.h,v
retrieving revision 1.3
diff -u -r1.3 citymap.h
--- common/aicore/citymap.h     5 Aug 2004 11:34:18 -0000       1.3
+++ common/aicore/citymap.h     25 Sep 2004 21:46:54 -0000
@@ -13,36 +13,16 @@
 #ifndef FC__CITYMAP_H
 #define FC__CITYMAP_H
 
-extern int citymap[MAP_MAX_WIDTH][MAP_MAX_HEIGHT];
+#include "shared.h"
 
+#include "fc_types.h"
+
+void citymap_init_map(void);
 void citymap_turn_init(struct player *pplayer);
 void citymap_reserve_city_spot(int x, int y, int id);
 void citymap_free_city_spot(int x, int y, int id);
 void citymap_reserve_tile(int x, int y, int id);
-
-/**************************************************************************
-  Returns a positive value if within a city radius, which is 1 x number of
-  cities you are within the radius of, or zero or less if not. A negative
-  value means this tile is reserved by a city and should not be taken.
-**************************************************************************/
-static inline int citymap_read(int x, int y)
-{
-  return citymap[x][y];
-}
-
-/**************************************************************************
-  A tile is reserved if it contains a city or unit id, or a worker is
-  assigned to it.
-**************************************************************************/
-static inline bool citymap_is_reserved(int x, int y)
-{
-  struct tile *ptile;
-                                                                               
 
-  ptile = map_get_tile(x, y);
-  if (ptile->worked || ptile->city) {
-    return TRUE;
-  }
-  return (citymap[x][y] < 0);
-}
+int citymap_read(int x, int y);
+bool citymap_is_reserved(int x, int y);
 
 #endif