[Freeciv-Dev] Re: Cleaning up server/savegame.c

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Mon, Oct 08, 2001 at 03:01:49PM -0400, Jason Dorje Short wrote:
> > Raimar Falke wrote:
> > >
> > > On Mon, Oct 08, 2001 at 01:50:41PM -0400, Jason Dorje Short wrote:
> >
> > > > Unfortunately this is not always the case, and the number of macros
> > > > would then grow.  Additionally, in some cases during load the code skips
> > > > over data if it's not present, while not in others - this would require
> > > > an addional macro or extra parameter to the existing one.  In summary,
> > > > it's not pretty.
> > >
> > > This encoding of the special and the known field is ugly, very ugly.
> >
> > Yeah.  I'm looking at another macro that would allow writing
> >
> >   DEC2HEX(map_get_tile(x, y)->special, 3)
> >
> > instead of
> >
> >   dec2hex[(map_get_tile(x, y)->special >> 12) & 0xf]
> >
> > or (what is now used)
> >
> >   dec2hex[(map_get_tile(x, y)->special & 0xf000) >> 12]
> >
> > But, DEC2HEX is a bit of a misnomer.
> >
> > > I don't see any code which skips over data (well savegame.c is
> > > huge). So I don't see the problem.
> >
> > One example:
> >
> > http://www.freeciv.org/lxr/source/server/savegame.c#L158
> >
> > If a line is not there, it is skipped.  This is done for each line.
> > Other usages should (but do not) assert that each line exists (rather
> > than being a null pointer) and possibly that it is long enough as well.
> 
> It may be possible that such lines exists or doesn't exists at
> all. Yes the check at every line is silly. And yes some asserts would
> be nice.
> 
> It turns out that map.n<number> is used a second time in
> <http://www.freeciv.org/lxr/source/server/savegame.c#L260>.

Yes, these save the exact same data.  It's a pretty ugly situation. 
I've simplified it a little bit by collapsing the overlay function call
(saving only, haven't looked at loading yet), which should still give
the same results.

> *looking more* It looks to me that the transformation
> map_rivers_overlay_load does can be done on already read data. But
> only if we have the "specials" capability. Which isn't part of the
> current capabilities. mhhhh Big problem is that we can't clean up this
> stuff because we don't know what kind of savegames are you there.

Indeed.

> > If it makes you more comfortable I'll choose a different character,
> > although this will make the files less human-readable.
> 
> I think a '#' would do nicely.

OK.

jason

? rc
Index: common/map.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
retrieving revision 1.94
diff -u -r1.94 map.c
--- common/map.c        2001/10/08 12:11:16     1.94
+++ common/map.c        2001/10/08 19:40:41
@@ -1314,6 +1314,19 @@
 }
 
 /**************************************************************************
+Returns TRUE iff the map position is normal.  "Normal" here means that it
+is both a real/valid coordinate set and that the coordinates are in their
+canonical/proper form.  In plain English: the coordinates must be on the
+map.
+**************************************************************************/
+int is_normal_map_pos(int x, int y)
+{
+  int x1 = x, y1 = y;
+
+  return normalize_map_pos(&x1, &y1) && x1 == x && y1 == y;
+}
+
+/**************************************************************************
 Normalizes the map position. Returns TRUE if it is real, FALSE otherwise.
 **************************************************************************/
 int normalize_map_pos(int *x, int *y)
Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.95
diff -u -r1.95 map.h
--- common/map.h        2001/10/08 12:11:16     1.95
+++ common/map.h        2001/10/08 19:40:42
@@ -231,6 +231,7 @@
 enum known_type tile_is_known(int x, int y);
 int check_coords(int *x, int *y);
 int is_real_tile(int x, int y);
+int is_normal_map_pos(int x, int y);
 int normalize_map_pos(int *x, int *y);
 void nearest_real_pos(int *x, int *y);
 
Index: server/savegame.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/savegame.c,v
retrieving revision 1.33
diff -u -r1.33 savegame.c
--- server/savegame.c   2001/10/03 09:16:56     1.33
+++ server/savegame.c   2001/10/08 19:40:44
@@ -51,9 +51,43 @@
 #include "savegame.h"
 
 
+#define DEC2HEX(int_value, halfbyte_wanted) \
+  dec2hex[ ((int_value) >> ((halfbyte_wanted) * 4)) & 0xf ]
 static const char dec2hex[] = "0123456789abcdef";
 static const char terrain_chars[] = "adfghjm prstu";
 
+/* This loops over the entire map to create save data, and stores it in the
+   file under the given secfile structure.  It works like this:
+     x_itr, y_itr -> x and y iterator variables
+     file -> the section_file we're saving to
+     get_xy_char -> a line of code that returns the character for each
+       (x, y) coordinate.  It should use the x/y iterator variables
+       appropriately.
+     secfile_index -> the indexor to the secfile, passed to secfile_insert.
+       It takes y_itr as an argument, so it should have a %03d in it. */
+#define save_map_data(secfile, get_xy_char, secfile_insert_line, x_itr, y_itr) 
      \
+{                                                                             \
+  char pbuf[map.xsize+1];                                                     \
+  int x_itr, y_itr;                                                           \
+  for (y_itr=0; y_itr<map.ysize; y_itr++) {                                   \
+    for (x_itr=0; x_itr<map.xsize; x_itr++) {                                 \
+      if (is_normal_map_pos(x, y)) {                                          \
+       pbuf[x_itr] = get_xy_char;                                            \
+      } else {                                                                \
+       pbuf[x_itr] = '#'; /* skipped over in loading */                      \
+      }                                                                       \
+    }                                                                         \
+    pbuf[map.xsize] = '\0';                                                   \
+    secfile_insert_line;                                                      \
+  }                                                                           \
+}
+
+#define save_reg_map_data(secfile, get_xy_char, secfile_name, x_itr, y_itr)   \
+       save_map_data(secfile, get_xy_char, secfile_insert_str(secfile, pbuf, 
secfile_name, y_itr), x_itr, y_itr)
+#define save_plr_map_data(secfile, get_xy_char, secfile_name, x_itr, y_itr, 
plrno) \
+       save_map_data(secfile, get_xy_char, secfile_insert_str(secfile, pbuf, 
secfile_name, plrno, y_itr), x_itr, y_itr)
+
+
 /* Following does not include "unirandom", used previously; add it if
  * appropriate.  (Code no longer looks at "unirandom", but should still
  * include it when appropriate for maximum savegame compatibility.)
@@ -230,27 +264,6 @@
 }
 
 /***************************************************************
-  Save the rivers overlay map; this is a special case to allow
-  re-saving scenarios which have rivers overlay data.  This
-  only applies if don't have rest of specials.
-***************************************************************/
-static void map_rivers_overlay_save(struct section_file *file)
-{
-  char *pbuf = fc_malloc(map.xsize+1);
-  int x, y;
-  
-  /* put "next" 4 bits of special flags */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++) {
-      pbuf[x]=dec2hex[(map_get_tile(x, y)->special&0xf00)>>8];
-    }
-    pbuf[x]='\0';
-    secfile_insert_str(file, pbuf, "map.n%03d", y);
-  }
-  free(pbuf);
-}
-
-/***************************************************************
 load a complete map from a savegame file
 ***************************************************************/
 static void map_load(struct section_file *file)
@@ -494,8 +507,7 @@
 ***************************************************************/
 static void map_save(struct section_file *file)
 {
-  int i, x, y;
-  char *pbuf=fc_malloc(map.xsize+1);
+  int i;
 
   /* map.xsize and map.ysize (saved as map.width and map.height)
    * are now always saved in game_save()
@@ -512,126 +524,53 @@
   }
     
   /* put the terrain type */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++)
-      pbuf[x]=terrain_chars[map_get_tile(x, y)->terrain];
-    pbuf[x]='\0';
+  save_reg_map_data(file, terrain_chars[map_get_tile(x, y)->terrain],
+                       "map.t%03d", x, y);
 
-    secfile_insert_str(file, pbuf, "map.t%03d", y);
-  }
-
-  if (!map.have_specials) {
-    if (map.have_rivers_overlay) {
-      map_rivers_overlay_save(file);
-    }
-    free(pbuf);
+  if (!map.have_specials && map.have_rivers_overlay) {
+    /* Save the rivers overlay map; this is a special case to allow
+       re-saving scenarios which have rivers overlay data.  This
+       only applies if don't have rest of specials. */
+
+    /* bits 8-11 of special flags field */
+    save_reg_map_data(file,
+                       DEC2HEX(map_get_tile(x, y)->special, 2),
+                       "map.n%03d", x, y);
     return;
   }
-
-  /* put lower 4 bits of special flags */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++)
-      pbuf[x]=dec2hex[map_get_tile(x, y)->special&0xf];
-    pbuf[x]='\0';
-
-    secfile_insert_str(file, pbuf, "map.l%03d", y);
-  }
-
-  /* put upper 4 bits of special flags */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++)
-      pbuf[x]=dec2hex[(map_get_tile(x, y)->special&0xf0)>>4];
-    pbuf[x]='\0';
-
-    secfile_insert_str(file, pbuf, "map.u%03d", y);
-  }
-
-  /* put "next" 4 bits of special flags */
-  for(y=0; y<map.ysize; y++) {
-    for(x=0; x<map.xsize; x++)
-      pbuf[x]=dec2hex[(map_get_tile(x, y)->special&0xf00)>>8];
-    pbuf[x]='\0';
 
-    secfile_insert_str(file, pbuf, "map.n%03d", y);
-  }
+  /* put 4-bit segments of 12-bit "special flags" field */
+  save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->special, 0),
+                   "map.l%03d", x, y);
+  save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->special, 1),
+                   "map.u%03d", x, y);
+  save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->special, 2),
+                   "map.n%03d", x, y);
 
   secfile_insert_int(file, game.save_options.save_known, "game.save_known");
   if (game.save_options.save_known) {
     /* put "final" 4 bits of special flags */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_tile(x, y)->special&0xf000)>>12];
-      pbuf[x]='\0';
-
-      secfile_insert_str(file, pbuf, "map.f%03d", y);
-    }
-
-    /* put bit 0-3 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_tile(x, y)->known&0xf)];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.a%03d", y);
-    }
-
-    /* put bit 4-7 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf0))>>4];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.b%03d", y);
-    }
-
-    /* put bit 8-11 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf00))>>8];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.c%03d", y);
-    }
-
-    /* put bit 12-15 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf000))>>12];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.d%03d", y);
-    }
+    save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->special, 3),
+                       "map.f%03d", x, y);
 
-    /* put bit 16-19 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf0000))>>16];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.e%03d", y);
-    }
-
-    /* put bit 20-23 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf00000))>>20];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.g%03d", y); /* f already taken! */
-    }
-
-    /* put bit 24-27 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf000000))>>24];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.h%03d", y);
-    }
-
-    /* put bit 28-31 of known bits */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[((map_get_tile(x, y)->known&0xf0000000))>>28];
-      pbuf[x]='\0';
-      secfile_insert_str(file, pbuf, "map.i%03d", y);
-    }
+    /* Put 4-bit segments of the 32-bit "known" field */
+    save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->known, 0),
+                     "map.a%03d", x, y);
+    save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->known, 1),
+                     "map.b%03d", x, y);
+    save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->known, 2),
+                     "map.c%03d", x, y);
+    save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->known, 3),
+                     "map.d%03d", x, y);
+    save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->known, 4),
+                     "map.e%03d", x, y);
+    save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->known, 5),
+                     "map.g%03d", x, y);
+    save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->known, 6),
+                     "map.h%03d", x, y);
+    save_reg_map_data(file, DEC2HEX(map_get_tile(x, y)->known, 7),
+                     "map.i%03d", x, y);
   }
-  
-  free(pbuf);
 }
 
 /***************************************************************
@@ -1458,10 +1397,9 @@
 static void player_save(struct player *plr, int plrno,
                        struct section_file *file)
 {
-  int i, x, y;
+  int i;
   char invs[A_LAST+1];
   struct player_spaceship *ship = &plr->spaceship;
-  char *pbuf=fc_malloc(map.xsize+1);
 
   secfile_insert_str(file, plr->name, "player%d.name", plrno);
   secfile_insert_str(file, plr->username, "player%d.username", plrno);
@@ -1724,78 +1662,34 @@
   if (game.fogofwar
       && game.save_options.save_private_map) {
     /* put the terrain type */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=terrain_chars[map_get_player_tile(x, y, plr)->terrain];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_t%03d", plrno, y);
-    }
-    
-    /* put lower 4 bits of special flags */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-      pbuf[x]=dec2hex[map_get_player_tile(x, y, plr)->special&0xf];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_l%03d",plrno,  y);
-    }
-    
-    /* put upper 4 bits of special flags */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, plr)->special&0xf0)>>4];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_u%03d", plrno,  y);
-    }
-    
-    /* put "next" 4 bits of special flags */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, plr)->special&0xf00)>>8];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_n%03d", plrno, y);
-    }
-    
-    /* put lower 4 bits of updated */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[map_get_player_tile(x, y, plr)->last_updated&0xf];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_ua%03d",plrno,  y);
-    }
-    
-    /* put upper 4 bits of updated */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, plr)->last_updated&0xf0)>>4];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_ub%03d", plrno,  y);
-    }
-    
-    /* put "next" 4 bits of updated */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, 
plr)->last_updated&0xf00)>>8];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_uc%03d", plrno, y);
-    }
-    
-    /* put "yet next" 4 bits of updated */
-    for(y=0; y<map.ysize; y++) {
-      for(x=0; x<map.xsize; x++)
-       pbuf[x]=dec2hex[(map_get_player_tile(x, y, 
plr)->last_updated&0xf000)>>12];
-      pbuf[x]='\0';
-      
-      secfile_insert_str(file, pbuf, "player%d.map_ud%03d", plrno, y);
-    }
-    
-    free(pbuf);
+    save_plr_map_data(file,
+                 terrain_chars[map_get_player_tile(x, y, plr)->terrain],
+                  "player%d.map_t%03d", x, y, plrno);
+
+    /* Put 4-bit segments of 12-bit "special flags" field */
+    save_plr_map_data(file,
+                  DEC2HEX(map_get_player_tile(x, y, plr)->special, 0),
+                 "player%d.map_l%03d", x, y, plrno);
+    save_plr_map_data(file,
+                  DEC2HEX(map_get_player_tile(x, y, plr)->special, 1),
+                  "player%d.map_u%03d", x, y, plrno);
+    save_plr_map_data(file,
+                  DEC2HEX(map_get_player_tile(x, y, plr)->special, 2),
+                 "player%d.map_n%03d", x, y, plrno);
+
+    /* put 4-bit segments of 16-bit "updated" field */
+    save_plr_map_data(file,
+                 DEC2HEX(map_get_player_tile(x, y, plr)->last_updated, 0),
+                 "player%d.map_ua%03d", x, y, plrno);
+    save_plr_map_data(file,
+                 DEC2HEX(map_get_player_tile(x, y, plr)->last_updated, 1),
+                 "player%d.map_ub%03d", x, y, plrno);  
+    save_plr_map_data(file,
+                 DEC2HEX(map_get_player_tile(x, y, plr)->last_updated, 2),
+                 "player%d.map_uc%03d", x, y, plrno);
+    save_plr_map_data(file,
+                 DEC2HEX(map_get_player_tile(x, y, plr)->last_updated, 3),
+                 "player%d.map_ud%03d", x, y, plrno);
     
     if (1) {
       struct dumb_city *pdcity;