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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

There is another bug here.  I thought it was in my patch, but...

The citymap is accessed before the first turn initializes it.  When 
starting a new game this is probably okay, but if the same behavior 
happens when loading it could be a problem...and a cause of some 
reloading-causes-different-behavior issues.

This patch works around it.  If we knew that turn_init was called before 
the data was accessed, we could just do the realloc there and there'd be 
no need for init_map.  And the client (which doesn't use citymap - why 
is it in common anyway?) would never need to alloc this data.  Instead 
the init_map must have not only a realloc but also a memset.

That said, this patch saves over 200kb of memory in both server and client.

jason


? new
? orig
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        26 Sep 2004 00:28:41 -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     26 Sep 2004 00:28:42 -0000
@@ -47,26 +47,36 @@
  * 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.
 **************************************************************************/
 void citymap_turn_init(struct player *pplayer)
 {
-  memset(citymap, 0, sizeof(citymap));
+  memset(citymap, 0, MAX_MAP_INDEX * sizeof(*citymap));
   players_iterate(pplayer) {
     city_list_iterate(pplayer->cities, pcity) {
       map_city_radius_iterate(pcity->x, pcity->y, x1, y1) {
         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     26 Sep 2004 00:28:42 -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