[Freeciv-Dev] Re: Review of (PR#4659)

["Cameron Morland" <cameron@xxxxxxxxxx>]

On Tue, Sep 16, 2003 at 07:02:48PM +0100, Gregory Berkolaiko wrote:
> Cameron,
> 
> Here is the reveiw of your patch (not by me).

Thanks for the review (especially to the reviewer!).

> Yes.  I ran the game with the suggested parameters and waited long
> enough for the explorers to finish the islands and checked that land
> area was roughly the same (side note: nice work on the endgame report,
> saved me having to eyeball the land area size).  The land areas varied
> from 61000 to 58000 which is 5% difference.  It also looked roughly the
> same.  So the patch seems to work.

You can see the whole map if you disband all your units immediately,
but that works too. :) Also run with sufficient debug levels and it
will print the size of the islands created.

> It sets min_specific_island_size to 0 for generators 3 and 4.  Originally
> the code required that these be at least 10% of the size that was requested.
> The 10% comes from:
> -   while (!create_island(i--, pstate) && i * 10 > islemass) {
> The new 0% comes from:
> +               pstate, 0);
> and related.  This should be ... pstate, 10);... to preserve existing 
> gen 3 and gen 4 behavior.

Oops, I think I knew that at one point, but it leaked out of my brain.

> It might be worthwhile to consider extending this patch to gen 3 and gen 4.
> (Of course, those generators are used less than gen 2 so it doesn't matter
> as much).

Let's get this in now, and fix that later. I've made a note to do that
at some point.

> The patch should be changed so that it tries to make the primary 
> islands in gen 3 and gen 4 at least 10% as that was the previous 
> behavior, or a good justification changing the behavior needs to be made.

Done.

I used a #define instead of having a bunch of magic "10"s.

> There should be a comment for map_clear_all_specials.

Done.

-- 
+-----------------------------------------------------------------
| PGP http://www.eng.uwaterloo.ca/student/cjmorlan/public-key.pgp
| Cameron Morland             ----             Cameron@xxxxxxxxxx
|
| My political ideal is democracy. Let every man be respected as 
| an individual and no man idolized.
|     --Einstein
+-----------------------------------------------------------------

? core.11259
? core.11261
? core.12061
? core.12065
? core.12156
? core.12417
? core.12421
? map.serv
Index: common/map.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
retrieving revision 1.142
diff -u -3 -p -r1.142 map.c
--- common/map.c        2003/09/11 11:30:42     1.142
+++ common/map.c        2003/09/16 19:41:27
@@ -1285,6 +1285,14 @@ void map_clear_special(int x, int y, enu
 }
 
 /***************************************************************
+ * Remove any specials which may exist at these map co-ordinates.
+ **************************************************************/
+void map_clear_all_specials(int x, int y)
+{
+  MAP_TILE(x, y)->special = S_NO_SPECIAL;
+}
+
+/***************************************************************
 ...
 ***************************************************************/
 struct city *map_get_city(int x, int y)
Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.152
diff -u -3 -p -r1.152 map.h
--- common/map.h        2003/09/11 11:30:42     1.152
+++ common/map.h        2003/09/16 19:41:27
@@ -278,6 +278,7 @@ enum tile_special_type map_get_special(i
 void map_set_terrain(int x, int y, enum tile_terrain_type ter);
 void map_set_special(int x, int y, enum tile_special_type spe);
 void map_clear_special(int x, int y, enum tile_special_type spe);
+void map_clear_all_specials(int x, int y);
 bool is_real_map_pos(int x, int y);
 bool is_normal_map_pos(int x, int y);
 
Index: server/mapgen.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/mapgen.c,v
retrieving revision 1.116
diff -u -3 -p -r1.116 mapgen.c
--- server/mapgen.c     2003/09/11 12:09:46     1.116
+++ server/mapgen.c     2003/09/16 19:41:29
@@ -1630,11 +1630,14 @@ static bool create_island(int islemass, 
 /*************************************************************************/
 
 /**************************************************************************
-  make an island, fill every tile type except plains
-  note: you have to create big islands first.
-**************************************************************************/
-static void make_island(int islemass, int starters,
-                       struct gen234_state *pstate)
+ * make an island, fill every tile type except plains
+ * note: you have to create big islands first.
+ * Return zero if successful.
+ * min_specific_island_size is a percent value.
+ *************************************************************************/
+static int make_island(int islemass, int starters,
+                      struct gen234_state *pstate,
+                      int min_specific_island_size)
 {
   static long int tilefactor, balance, lastplaced;/* int may be only 2 byte ! 
*/
   static long int riverbuck, mountbuck, desertbuck, forestbuck, swampbuck;
@@ -1642,6 +1645,8 @@ static void make_island(int islemass, in
   int i;
 
   if (islemass == 0) {
+    /* this only runs to initialise static things, not to actually
+     * create an island. */
     balance = 0;
     pstate->isleindex = 3;     /* 0= none, 1= arctic, 2= antarctic */
 
@@ -1663,31 +1668,44 @@ static void make_island(int islemass, in
     lastplaced = pstate->totalmass;
   } else {
 
-   /* makes the islands here */
+   /* makes the islands this big */
     islemass = islemass - balance;
 
     /* don't create continents without a number */
-    if (pstate->isleindex >= MAP_NCONT)
-      return;
+    if (pstate->isleindex >= MAP_NCONT) {
+      return -1;
+    }
 
-    if(islemass>lastplaced+1+lastplaced/50)/*don't create big isles we can't 
place*/
-      islemass= lastplaced+1+lastplaced/50;
+    if(islemass>lastplaced+1+lastplaced/50) {
+      /* don't create big isles we can't place */
+      islemass = lastplaced + 1 + lastplaced / 50;
+    }
 
     /* isle creation does not perform well for nonsquare islands */
-    if(islemass>(map.ysize-6)*(map.ysize-6))
+    if(islemass > (map.ysize - 6) * (map.ysize - 6)) {
       islemass= (map.ysize-6)*(map.ysize-6);
+    }
 
-    if(islemass>(map.xsize-2)*(map.xsize-2))
+    if(islemass > (map.xsize - 2) * (map.xsize - 2)) {
       islemass= (map.xsize-2)*(map.xsize-2);
+    }
 
     i = islemass;
-    if (i <= 0) return;
+    if (i <= 0) {
+      return -2;
+    }
     islands[pstate->isleindex].starters = starters;
 
     freelog(LOG_VERBOSE, "island %i", pstate->isleindex);
 
-    while (!create_island(i--, pstate) && i * 10 > islemass) {
-      /* nothing */
+    /* keep trying to place an island, and decrease the size of
+     * the island we're trying to create until we succeed.
+     * If we get too small, return an error. */
+    while (!create_island(i, pstate)) {
+      if(i < islemass * min_specific_island_size / 100) {
+       return -3;
+      }
+      i--;
     }
     i++;
     lastplaced= i;
@@ -1736,6 +1754,7 @@ static void make_island(int islemass, in
     pstate->isleindex++;
     map.num_continents++;
   }
+  return 0;
 }
 
 /**************************************************************************
@@ -1752,6 +1771,7 @@ static void initworld(struct gen234_stat
     for (x = 0 ; x < map.xsize ; x++) {
       map_set_terrain(x, y, T_OCEAN);
       map_set_continent(x, y, 0);
+      map_clear_all_specials(x, y);
       map_set_owner(x, y, NULL);
     }
   for (x = 0 ; x < map.xsize; x++) {
@@ -1769,7 +1789,7 @@ static void initworld(struct gen234_stat
     }
   }
   map.num_continents = 2;
-  make_island(0, 0, pstate);
+  make_island(0, 0, pstate, 0);
   islands[2].starters = 0;
   islands[1].starters = 0;
   islands[0].starters = 0;
@@ -1784,9 +1804,15 @@ static void mapgenerator2(void)
   struct gen234_state state;
   struct gen234_state *pstate = &state;
   int i;
+  int done = 0;
   int spares= 1; 
   /* constant that makes up that an island actually needs additional space */
 
+  /* put 70% of land in big continents, 
+   *     20% in medium, and 
+   *     10% in small. */ 
+  int bigfrac = 70, midfrac = 20, smallfrac = 10;
+
   if (map.landpercent > 85) {
     map.generator = 1;
     return;
@@ -1796,21 +1822,43 @@ static void mapgenerator2(void)
       ((map.ysize - 6 - spares) * map.landpercent * (map.xsize - spares)) /
       100;
 
-  /*!PS: The weights NEED to sum up to totalweight (dammit) */
-  /* copying the flow of the make_island loops is the safest way */
   totalweight = 100 * game.nplayers;
 
-  initworld(pstate);
-
-  for (i = game.nplayers; i > 0; i--) {
-    make_island(70 * pstate->totalmass / totalweight, 1, pstate);
+  while (!done) {
+    done = 1;
+    initworld(pstate);
+    
+    /* Create one big island for each player. */
+    for(i = game.nplayers; i > 0; i--) {
+      if(make_island(bigfrac * pstate->totalmass / totalweight,
+                    1, pstate, 95) == -3) {
+       /* we couldn't make an island at least 95% as big as we wanted,
+        * and since we're trying hard to be fair, we need to start again,
+        * with all big islands reduced slightly in size.
+        * Take the size reduction from the big islands and add it to the 
+        * small islands to keep overall landmass unchanged.
+        * Note that the big islands can get very small if necessary, and
+        * the smaller islands will not exist if we can't place them easily. */
+       freelog(LOG_VERBOSE,
+               "Island too small, trying again with all smaller islands.\n");
+       midfrac += bigfrac * 0.01;
+       smallfrac += bigfrac * 0.04;
+       bigfrac *= 0.95;
+       done = 0;       
+       break;
+      }
+    }
   }
-  for (i = game.nplayers; i > 0; i--) {
-    make_island(20 * pstate->totalmass / totalweight, 0, pstate);
+
+  /* Now place smaller islands, but don't worry if they're small,
+   * or even non-existent. One medium and one small per player. */
+  for( i = game.nplayers; i > 0; i--) {
+    make_island(midfrac * pstate->totalmass / totalweight, 0, pstate, 0);
   }
-  for (i = game.nplayers; i > 0; i--) {
-    make_island(10 * pstate->totalmass / totalweight, 0, pstate);
+  for( i = game.nplayers; i > 0; i--) {
+    make_island(smallfrac * pstate->totalmass / totalweight, 0, pstate, 0);
   }
+
   make_plains();  
   free(height_map);
   height_map = NULL;
@@ -1820,6 +1868,13 @@ static void mapgenerator2(void)
   }
 }
 
+/* This variable is the Default Minimum Specific Island Size, 
+ * ie the smallest size we'll typically permit our island, as a % of
+ * the size we wanted. So if we ask for an island of size x, the island 
+ * creation will return if it would create an island smaller than
+ *  x * DMSIS / 100 */
+#define DMSIS 10
+
 /**************************************************************************
 On popular demand, this tries to mimick the generator 3 as best as possible.
 **************************************************************************/
@@ -1870,7 +1925,7 @@ static void mapgenerator3(void)
 
   while (pstate->isleindex - 2 <= bigislands && checkmass > islandmass
         && ++j < 500) {
-    make_island(islandmass, 1, pstate);
+    make_island(islandmass, 1, pstate, DMSIS);
   }
 
   if(j==500){
@@ -1892,7 +1947,7 @@ static void mapgenerator3(void)
       if(size<2) size=2;
 
       make_island(size, (pstate->isleindex - 2 <= game.nplayers) ? 1 : 0,
-                 pstate);
+                 pstate, DMSIS);
   }
 
   make_plains();  
@@ -1947,18 +2002,20 @@ static void mapgenerator4(void)
 
   i = game.nplayers / 2;
   if ((game.nplayers % 2) == 1) {
-    make_island(bigweight * 3 * pstate->totalmass / totalweight, 3, pstate);
+    make_island(bigweight * 3 * pstate->totalmass / totalweight, 3, 
+               pstate, DMSIS);
   } else {
     i++;
   }
   while ((--i) > 0) {
     make_island(bigweight * 2 * pstate->totalmass / totalweight, 2,
-               pstate);}
+               pstate, DMSIS);
+  }
   for (i = game.nplayers; i > 0; i--) {
-    make_island(20 * pstate->totalmass / totalweight, 0, pstate);
+    make_island(20 * pstate->totalmass / totalweight, 0, pstate, DMSIS);
   }
   for (i = game.nplayers; i > 0; i--) {
-    make_island(10 * pstate->totalmass / totalweight, 0, pstate);
+    make_island(10 * pstate->totalmass / totalweight, 0, pstate, DMSIS);
   }
   make_plains();  
   free(height_map);
@@ -1968,6 +2025,8 @@ static void mapgenerator4(void)
     freelog(LOG_VERBOSE, "%ld mass left unplaced", checkmass);
   }
 }
+
+#undef DMSIS
 
 /**************************************************************************
 Recursive function which does the work for generator 5

pgpsVqRjeN9VX.pgp
Description: PGP signature