[Freeciv-Dev] Re: new natural names patch (PR#1127)

[Jason Short <vze2zq63@xxxxxxxxxxx>]

Gregory Berkolaiko wrote:

>  --- jdorje@xxxxxxxxxxxxxxxxxxxxx wrote: 
>
> > Hmm, it looks like the nation.h file didn't get diffed.  After fixing 
> > it, and updating the data patch for current CVS, I get this.
> although i have verrrry little interest in city naming, i had abrief look
> at the patch.  two complaints:

Thanks, Gregory.



Here's a new patch.

Changes:

1.  Fixed up the comments to match the FreeCiv code guidelines.

2.  Removed extra cruft that I forgot to remove before :-(.  This 
includes the num_misc_city_names variable as well as some #undef lines.
3.  Cleaned up calculations in search_for_city_name.

4.  Changed the calculations in search_for_city_name.  We now give a 40% 
prority bonus to cities with matching terrain, and a 40% penalty for 
cities that don't match terrain that is requested.  I added an 
explanation for this (although the 40% is arbitrary).
5.  Added a boxed comment for search_for_city_name().


I've played a large part of a game with this patch, and everything seems 
quite good.
Note that the data patch (sent before) still contains changes only for 
the American nation; and these may or may not be correct.  If applied, 
I'd recommend applying the code patch and then reverting the data to 
what it was four weeks ago (that is, reversing the natural city names 
updates since then).  Then the nation writers can update the data again 
(this is unfortunate, but better to get over with ASAP since it'll only 
get harder...).
jason

? jason.gz
? old
? topology
? data/diff
Index: common/nation.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/nation.h,v
retrieving revision 1.7
diff -u -r1.7 nation.h
--- common/nation.h     2001/12/06 11:59:03     1.7
+++ common/nation.h     2001/12/22 00:01:51
@@ -27,6 +27,28 @@
 enum advisor_type {ADV_ISLAND, ADV_MILITARY, ADV_TRADE, ADV_SCIENCE, 
ADV_FOREIGN, 
                    ADV_ATTITUDE, ADV_DOMESTIC, ADV_LAST};
 
+/*
+ * The city_name structure holds information about a default choice for
+ * the city name.  The "name" field is, of course, just the name for
+ * the city.  The "value" is the priority rating of this name - lower
+ * priority cities will show up sooner.  The "river" and "terrain" fields
+ * are entries recording whether the terrain is present near the city -
+ * we give higher priority to cities which have matching terrain.  In the
+ * case of a river we only care if the city is _on_ the river, for other
+ * terrain features we give the bonus if the city is close to the
+ * terrain.  Both of these entries may hold a value of 0 (no preference),
+ * 1 (city likes the terrain), or -1 (city doesn't like the terrain).
+ *
+ * This is controlled through the nation's ruleset like this:
+ *   cities = "Washington (ocrean, river, swamp)", "New York (!mountains)"
+ */
+struct city_name {
+       char* name;
+       int value;
+       int river;
+       int terrain[T_COUNT];   
+};
+
 struct nation_type {
   char name[MAX_LEN_NAME];
   char name_plural[MAX_LEN_NAME];
@@ -36,11 +58,7 @@
   char *leader_name[MAX_NUM_LEADERS];
   int  leader_is_male[MAX_NUM_LEADERS];
   int city_style;
-  char **default_city_names;
-  char **default_rcity_names;          /* river city names */
-  char **default_crcity_names;         /* coastal-river city names */
-  char **default_ccity_names;          /* coastal city names */
-  char **default_tcity_names[T_COUNT]; /* terrain-specific city names */
+  struct city_name *city_names;                /* The default city names. */
   struct Sprite *flag_sprite;
 
   /* untranslated copies: */
Index: server/citytools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/citytools.c,v
retrieving revision 1.148
diff -u -r1.148 citytools.c
--- server/citytools.c  2001/12/11 16:48:48     1.148
+++ server/citytools.c  2001/12/22 00:01:54
@@ -52,12 +52,78 @@
 
 #include "citytools.h"
 
+static char *search_for_city_name(int x, int y, struct city_name *city_names);
 static void server_set_tile_city(struct city *pcity, int city_x, int city_y,
                                 enum city_tile_type type);
 
-char **misc_city_names; 
-int num_misc_city_names;
+struct city_name *misc_city_names;
 
+
+/****************************************************************
+Searches through a city name list (a struct city_name array)
+to pick the best available city name, and returns a pointer to
+it.  The function uses its own internal algorithm to prioritize
+the city names; this algorithm need not always return the same
+name when given the same list.  If the list has no valid
+entries in it, NULL will be returned.
+*****************************************************************/
+static char *search_for_city_name(int x, int y, struct city_name *city_names)
+{
+  struct city_name *choice;
+  int best_value = -1, terrain, goodness;
+  char* best_name = NULL;
+  for (choice = city_names; choice->name; choice++) {
+    if (!game_find_city_by_name(choice->name)) {
+      /* Lower values are better. */
+      int value = choice->value, type;
+
+      /* We do a little randomizing of our own. */
+      value *= 10 + myrand(5);
+
+      /*
+       * Use a special name if the tile has a river or there
+       * is an available name depending on the terrain.
+       */
+
+      /*
+       * "terrain" is -1 if we don't have the terrain, 1 if  we do.
+       * "goodness" therefore becomes positive if we like the terrain,
+       * negative if we don't.  The integer values are just
+       * approximations, so...
+       *
+       * The reason we multiply as well as divide the value is so
+       * that cities that don't care what terrain they are on (which
+       * is the default) will be left in the middle of the pack.  If
+       * we _only_ multiplied (or divided), then cities that had more
+       * terrain labels would have their priorities hurt (or helped).
+       */
+      terrain =  !!(map_get_special(x, y) & S_RIVER) * 2 - 1;
+      goodness = terrain * choice->river;
+      if (goodness > 0) {
+        value = (float)value / 1.4;
+      } else if (goodness < 0) {
+       value = (float)value * 1.4;
+      }
+
+      for (type = T_FIRST; type < T_COUNT; type++) {
+        terrain = !!is_terrain_near_tile(x, y, type) * 2 - 1;
+        goodness = terrain * choice->terrain[type];
+        if (goodness > 0) {
+          value = (float)value / 1.4;
+        } else if (goodness < 0) {
+         value = (float)value * 1.4;
+        }
+      }
+
+      if (best_value == -1 || value < best_value) {
+        best_value = value;
+        best_name = choice->name;
+      }
+    }
+  }
+  return best_name;
+}
+
 /****************************************************************
 Come up with a default name when a new city is about to be built.
 Handle running out of names etc. gracefully.  Maybe we should keep
@@ -69,9 +135,10 @@
 *****************************************************************/
 char *city_name_suggestion(struct player *pplayer, int x, int y)
 {
-  char **nptr;
-  int i, j;
+  char *name;
+  int i;
   struct nation_type *nation = get_nation_by_plr(pplayer);
+  /* tempname must be static because it's returned below. */
   static char tempname[MAX_LEN_NAME];
 
   static const int num_tiles = MAP_MAX_WIDTH * MAP_MAX_HEIGHT; 
@@ -81,62 +148,22 @@
   
   CHECK_MAP_POS(x,y);
 
-#define SEARCH_AND_RETURN_CITY_NAME(list)   \
-    for(nptr=list; *nptr; nptr++) {         \
-      if(!game_find_city_by_name(*nptr)) {  \
-        return *nptr;                       \
-      }                                     \
-    }
-
-  /* 
-   * Use a special name if the tile has a river, is coastal or there
-   * is an available name depending on the terrain.
-   */ 
-
-  /* deal with rivers */
-  if (map_get_special(x, y) & S_RIVER) {
-    if (is_terrain_near_tile(x, y, T_OCEAN)) {
-      /* coastal river */
-      SEARCH_AND_RETURN_CITY_NAME(nation->default_crcity_names);
-    } else {
-      /* non-coastal river */
-      SEARCH_AND_RETURN_CITY_NAME(nation->default_rcity_names);
-    }
-  }
-
   /* coastal */
-  if (is_terrain_near_tile(x, y, T_OCEAN)) {
-    SEARCH_AND_RETURN_CITY_NAME(nation->default_ccity_names);
-  }
-  
-  /* check terrain type */
-  SEARCH_AND_RETURN_CITY_NAME(nation->
-                             default_tcity_names[map_get_terrain(x, y)]);
-
-  /* we haven't found a name: it's a normal tile or they're all used */
-  SEARCH_AND_RETURN_CITY_NAME(nation->default_city_names);
-
-#undef SEARCH_AND_RETURN_CITY_NAME
+  name = search_for_city_name(x, y, nation->city_names);
+  if (name)
+    return name;
+
+  name = search_for_city_name(x, y, misc_city_names);
+  if (name)
+    return name;
 
-  if (num_misc_city_names > 0) {
-    j = myrand(num_misc_city_names);
-  
-    for (i = 0; i < num_misc_city_names; i++) {
-      if (j >= num_misc_city_names) {
-       j = 0;
-      }
-      if (!game_find_city_by_name(misc_city_names[j])) 
-       return misc_city_names[j];
-      j++;
-    }
-  }
-
   for (i = 1; i <= num_tiles; i++ ) {
     my_snprintf(tempname, MAX_LEN_NAME, _("City no. %d"), i);
-    if (!game_find_city_by_name(tempname)) 
-      return tempname;
+    if (!game_find_city_by_name(tempname))
+      return (tempname);
   }
-  
+
+  assert(0);
   return _("A poorly-named city");
 }
 
Index: server/citytools.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/citytools.h,v
retrieving revision 1.33
diff -u -r1.33 citytools.h
--- server/citytools.h  2001/12/06 11:59:07     1.33
+++ server/citytools.h  2001/12/22 00:01:54
@@ -15,6 +15,7 @@
 
 #include "packets.h"
 #include "city.h"
+#include "nation.h" /* for struct city_name */
 
 #define FOOD_WEIGHTING 19
 #define SHIELD_WEIGHTING 17
@@ -81,8 +82,7 @@
                         int target, int is_unit, int event);
 
 char *city_name_suggestion(struct player *pplayer, int x, int y);
-extern char **misc_city_names; 
-extern int num_misc_city_names;
+extern struct city_name *misc_city_names;
 
 
 int city_can_work_tile(struct city *pcity, int city_x, int city_y);
Index: server/ruleset.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/ruleset.c,v
retrieving revision 1.83
diff -u -r1.83 ruleset.c
--- server/ruleset.c    2001/12/06 11:59:07     1.83
+++ server/ruleset.c    2001/12/22 00:01:57
@@ -74,6 +74,7 @@
 static void load_terrain_names(struct section_file *file);
 static void load_citystyle_names(struct section_file *file);
 static void load_nation_names(struct section_file *file);
+static struct city_name* load_city_name_list(struct section_file *file, char 
*secfile_str1, char *secfile_str2);
 
 static void load_ruleset_techs(struct section_file *file);
 static void load_ruleset_units(struct section_file *file);
@@ -1791,6 +1792,175 @@
 }
 
 /**************************************************************************
+  This strips leading and trailing whitespace from a string.
+
+  Surely this capability is provided by some other source???
+**************************************************************************/
+static char* strip_space(char* str)
+{
+  char* start;
+  while(isspace(*str)) {
+    str++;
+  }
+  start = str;
+  while(*str) {
+    str++;
+  }
+  str--;
+  while(isspace(*str)) {
+    *str = 0;
+    str--;
+  }
+  return start;
+}
+
+/**************************************************************************
+  This function loads a city name list from a section file.  The file and
+  two section names (which will be concatenated) are passed in.  The
+  malloc'ed city name list (which is all filled out) will be returned.
+**************************************************************************/
+static struct city_name* load_city_name_list(struct section_file *file,
+                                            char *secfile_str1,
+                                            char *secfile_str2)
+{
+  char **cities;
+  int dim, j;
+  struct city_name *city_names;
+  int value;
+
+  /* First we read the strings from the section file. */
+  cities = secfile_lookup_str_vec(file, &dim, "%s%s",
+                                 secfile_str1, secfile_str2);
+
+  /*
+   * Now we allocate enough room in the city_names array to store
+   * all the name data.  The array is NULL-terminated by
+   * having a NULL name at the end.
+   */
+  city_names = fc_calloc(dim + 1, sizeof(struct city_name));
+  city_names[dim].name = NULL;
+
+  /*
+   * Each string will be of the form
+   * "<cityname> (<label>, <label>, ...)".  The cityname is just the
+   * name for this city, while each "label" matches a terrain type
+   * for the city (or "river"), with a preceeding ! to negate it.  The
+   * parentheses are optional (but necessary to have the settings, of
+   * course).  Our job is now to parse this into the city_name structure.
+   */
+  for (j = 0, value=10; j < dim; j++, value++) {
+    /* TODO: handle this more succinctly. */
+    char *name = cities[j], *next;
+
+    /*
+     * We assign a "base value" to each city based upon its
+     * position in the list.  We arbitrarily start at 10 and
+     * count up - higher values mean lower priority, meaning
+     * the name is less likely to be picked later.
+     */
+    city_names[j].value = value;
+
+    /*
+     * Now we wish to determine values for all of the city labels.
+     * A value of 0 means no preference (which is necessary so that
+     * the use of this is optional); -1 means the label is negated
+     * and 1 means it's labelled.  Mostly the parsing just involves
+     * a lot of ugly string handling...
+     */
+    memset(&city_names[j].terrain[0], 0,
+          T_COUNT * sizeof(city_names[j].terrain[0]));
+    city_names[j].river = 0;
+
+    name = strchr(name, '(');
+    if (name) {
+      /*
+       * 0-terminate the original string, then find the
+       * close-parenthesis so that we can make sure we stop there.
+       */
+      *name = 0;
+      name++;
+      next = strchr(name, ')');
+      assert(next);
+      *next = 0;
+
+      /* Handle the labels one at a time. */
+      do {
+        int setting = 1, i;
+       next = strchr(name, ',');
+       if (next)
+         *next = 0;
+       name = strip_space(name);
+       for (i=0; name[i]; i++)
+         name[i] = tolower(name[i]);
+       
+       /*
+        * The ! is used to mark a negative, which is recorded
+        * with a -1.  Otherwise we use a 1.
+        */
+       if (*name == '!') {
+         name++;
+         setting = -1;
+       }
+       
+       /*
+        * We used to have "coastal" as a special case along
+        * with river, but this is unnecessary since we
+        * also have the "ocean" terrain type.
+        */
+       if (!strcmp(name, "river")) {
+         city_names[j].river = setting;
+       } else {
+         /* "handled" tracks whether we find a match (for error handling) */
+         int handled = 0;
+         enum tile_terrain_type type;
+       
+         for (type = T_FIRST; type < T_COUNT && !handled; type++) {
+           int k;
+           char namebuf[MAX_LEN_NAME];
+            /*
+             * Note that at this time (before a call to
+             * translate_data_names) the terrain_name fields contains an
+             * untranslated string. Note that name of T_RIVER is "". However
+             * this is not a problem because we take care of rivers
+             * separately.
+             */
+            mystrlcpy(namebuf, tile_types[type].terrain_name,
+               sizeof(tile_types[type].terrain_name));
+               
+           /* transform to lower case */
+           for (k = 0; k < strlen(namebuf); k++) {
+             namebuf[k] = tolower(namebuf[k]);
+           }
+       
+           if (!strcmp(name, namebuf)) {
+             city_names[j].terrain[type] = setting;
+             handled = 1;
+           }
+         }
+         if (!handled) {
+           freelog(LOG_ERROR, "Unreadable terrain description %s "
+                   "in city name ruleset \"%s%s\" - skipping it.",
+                   name, secfile_str1, secfile_str2);
+         }
+       }
+       name = next ? next+1 : NULL;
+      } while (name && *name);
+    }
+    city_names[j].name = mystrdup(strip_space(cities[j]));
+    if (check_name(city_names[j].name)) {
+      freelog(LOG_ERROR, "City name %s in ruleset for %s%s is too long "
+             "- shortening it.",
+              city_names[j].name, secfile_str1, secfile_str2);
+      city_names[j].name[MAX_LEN_NAME - 1] = 0;
+    }
+  }
+  if (cities) {
+    free(cities);
+  }
+  return city_names;
+}
+
+/**************************************************************************
 Load nations.ruleset file
 **************************************************************************/
 static void load_ruleset_nations(struct section_file *file)
@@ -1800,9 +1970,8 @@
   struct government *gov;
   int *res, dim, val, i, j, nval;
   char temp_name[MAX_LEN_NAME];
-  char **cities, **techs, **leaders, **sec;
+  char **techs, **leaders, **sec;
   const char *filename = secfile_filename(file);
-  enum tile_terrain_type type;
 
   datafile_options = check_ruleset_capabilities(file, "+1.9", filename);
 
@@ -2010,72 +2179,13 @@
     }
     pl->goals.government = val;
 
-#define BASE_READ_CITY_NAME_LIST(target,format,arg1,arg2,arg3)  \
-  cities = secfile_lookup_str_vec(file, &dim, format,           \
-                                  arg1, arg2, arg3);            \
-  target = fc_calloc(dim + 1, sizeof(char *));                  \
-  target[dim] = NULL;                                           \
-  for (j = 0; j < dim; j++) {                                   \
-    target[j] = mystrdup(cities[j]);                            \
-    if (check_name(cities[j])) {                                \
-      target[j][MAX_LEN_NAME - 1] = 0;                          \
-    }                                                           \
-  }                                                             \
-  if (cities) {                                                 \
-    free(cities);                                               \
-  }
-
-#define READ_CITY_NAME_LIST(target_field,format,arg)            \
-  BASE_READ_CITY_NAME_LIST(pl->target_field, "%s." format "%s", \
-                           sec[i], arg, "cities")
-
     /* read "normal" city names */
-    READ_CITY_NAME_LIST(default_city_names, "%s", "");
-
-    /* read river city names */
-    READ_CITY_NAME_LIST(default_rcity_names, "%s", "river_");
 
-    /* read coastal-river city names */
-    READ_CITY_NAME_LIST(default_crcity_names, "%s","coastal_river_");
-
-    /* read coastal city names */
-    READ_CITY_NAME_LIST(default_ccity_names, "%s", "coastal_");
-
-    /* 
-     * Read terrain-specific city names. 
-     */    
-    for (type = T_FIRST; type < T_COUNT; type++) {
-      int k;
-      char namebuf[MAX_LEN_NAME];
-
-      /*
-       * Note that at this time (before a call to
-       * translate_data_names) the terrain_name fields contains an
-       * untranslated string. Note that name of T_RIVER is "". However
-       * this is not a problem because we take care of this using
-       * default_rcity_names.
-       */
-      mystrlcpy(namebuf, tile_types[type].terrain_name,
-               sizeof(tile_types[type].terrain_name));
-
-      /* transform to lower case */
-      for (k = 0; k < strlen(namebuf); k++) {
-       namebuf[k] = tolower(namebuf[k]);
-      }
-
-      READ_CITY_NAME_LIST(default_tcity_names[type], "%s_", namebuf);
-    }
+    pl->city_names = load_city_name_list(file, sec[i], ".cities");
   }
 
   /* read miscellaneous city names */
-  BASE_READ_CITY_NAME_LIST(misc_city_names, "misc.cities%s%s%s", "", "",
-                          "");
-
-  /* dim is set in BASE_READ_CITY_NAME_LIST */
-  num_misc_city_names = dim;
-
-#undef READ_CITY_NAME_LIST
-#undef BASE_READ_CITY_NAME_LIST
+  misc_city_names = load_city_name_list(file, "misc.cities", "");
 
   free(sec);
   section_file_check_unused(file, filename);