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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Sun, Jan 06, 2002 at 06:35:15PM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
> Raimar Falke wrote:
> 
> > On Sat, Dec 22, 2001 at 12:31:31PM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx 
> > wrote:
> > 
> >>diff -u -r1.7 nation.h
> >>--- common/nation.h 2001/12/06 11:59:03     1.7
> >>+++ common/nation.h 2001/12/22 20:22:14
> >>@@ -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).
> >>
> > 
> > Either make the three values fix via enum or DEFINE or allow a wider
> > range (2 stands for really like it and 10 for really really like it).

MARK

> >>+/****************************************************************
> >>+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, 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);
> >>
> > 
> > Why 10? Why 5? E.g. what range does value have?
> 
> The whole formula thing is pretty fuzzy.
> 
> - City priorities are initialized to 10, 11, 12, ..., n+9
> - Value is multiplied by 10.  This is just to avoid rounding problems; 
> similar to what freeciv does in combat calculations.
> - myrand(5) is added.  This is just a very small random factor.
> - Any additional terrain factors are multiplied/divided out.  I though a 
> matching terrain should be about twice as good as a non-matching 
> terrain, which is where the 1.4 comes from.
> 
> Unless someone has a better idea for the formula, the only thing I think 
> needs changing is that I'll initialize the priorities to 1,2,3,...,n+10 
> and then add on 9 or 10 in the calculation.  This will move everything 
> into the calculation function.  I may also separate the calculation out 
> into it's own function, and I will in any case add a comment about the 
> fuzziness.  Later we can fine-tune the calculation, but it really 
> doesn't matter too much.

You don't have to explain it to me. Explain it to the future
programmers who read the code.

> >>+   * 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++) {
> >>
> > 
> > Why 10?
> 
> Again, it's fuzzy.  And it means each new city has a 10% increase 
> (arithmetic, not geometric), which seems to work okay.  As I said, I'll 
> move this out of the initialization and into the calculation step.

Same as above.

> >>+    memset(&city_names[j].terrain[0], 0,
> >>+      T_COUNT * sizeof(city_names[j].terrain[0]));
> >>
> > 
> > If somewhere is a
> >   #define CITY_NAME_DOESNT_CARE 0
> > is used you can't use the memset.
> 
> I don't quite understand.

See MARK

> >>+            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]);
> >>+       }
> >>
> > 
> > Doing this transformation for every city in every nation seems
> > sub-optimal. Although I don't suspect a performance difference the
> > reader wonders why this is done every time.
> 
> Indeed.  On the other hand, caching the value would require code that 
> was much less localized, so it'd make things less clear IMO.  

> One solution might be to drop the tolower() stuff altogether and say
> that the terrain description in the ruleset must match what the game
> has. This might be more "correct" since the game could theoretically
> have "Ocean" and "ocean" as two different terrain names (although
> this would be ludicrous).

Sounds sane.

> Also, I think we should make naturalcitynames a server variable, since 
> it'll be easy to disable the functionality.  I haven't done this yet, 
> though.
> 
> New patch attached.  I haven't tested it much since making the changes.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Living on earth may be expensive, but it includes an annual free trip
  around the sun.