[Freeciv-Dev] Re: (PR#9505) assert in mapgen.c

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

James Canete wrote:
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=9505 >
> 
>>set seed 2837624
>>set randseed 23456124
>>create idiot
>>start
> 
> Assertion failed: best_val != -1, file mapgen.c, line 850
> 
> This application has requested the Runtime to terminate it in an unusual
> way.
> Please contact the application's support team for more information.

Same as PR#9502.

The last two commits to mapgen.c have touched this code:

revision 1.140
date: 2004/07/24 04:02:36;  author: jdorje;  state: Exp;  lines: +42 -42
Fix several bugs (new and old) in the creation of rivers.

Reported by Martin Schroder <martin@xxxxxxxxxx> and A. Gorshenev
<nikodimka@xxxxxxxxx> in PR#9439.  Patch by Frank Richter <resqu@xxxxxx>
and myself.
----------------------------
revision 1.139
date: 2004/07/09 19:30:58;  author: jdorje;  state: Exp;  lines: +12 -24
Add a new macro cardinal_adjc_dir_iterate, and use it in several places.
Remove the CAR_DIR_D[XY] arrays.

Patch by me in PR#9162.

which must surely be responsible for these errors, in some form or 
other.  (But don't expect to be able to reproduce it from earlier 
versions anyway, since the bugfixes in PR#9439 will change the random 
order.)

Attached are the differences from these two changes.  The first loop 
(that finds best_val) is practically unchanged.  I don't know what could 
be happening here.

jason

Index: mapgen.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/mapgen.c,v
retrieving revision 1.138
retrieving revision 1.140
diff -u -r1.138 -r1.140
--- mapgen.c    9 Jul 2004 18:52:36 -0000       1.138
+++ mapgen.c    24 Jul 2004 04:02:36 -0000      1.140
@@ -793,13 +793,14 @@
 *********************************************************************/
 static bool make_river(int x, int y)
 {
-  /* The comparison values of the 4 tiles surrounding the current
-     tile. It is the suitability to continue a river to that tile that
-     is being compared. Lower is better.                  -Erik Sigra */
-  static int rd_comparison_val[4];
+  /* Comparison value for each tile surrounding the current tile.  It is
+   * the suitability to continue a river to the tile in that direction;
+   * lower is better.  However rivers may only run in cardinal directions;
+   * the other directions are ignored entirely. */
+  int rd_comparison_val[8];
 
-  bool rd_direction_is_valid[4];
-  int num_valid_directions, dir, func_num, direction;
+  bool rd_direction_is_valid[8];
+  int num_valid_directions, func_num, direction;
 
   while (TRUE) {
     /* Mark the current tile as river. */
@@ -822,21 +823,22 @@
 
     /* Else choose a direction to continue the river. */
     freelog(LOG_DEBUG,
-           "The river did not end at (%d, %d). Evaluating directions...\n", x, 
y);
+           "The river did not end at (%d, %d). Evaluating directions...\n",
+           x, y);
 
-    /* Mark all directions as available. */
-    for (dir = 0; dir < 4; dir++)
+    /* Mark all available cardinal directions as available. */
+    memset(rd_direction_is_valid, 0, sizeof(rd_direction_is_valid));
+    cardinal_adjc_dir_iterate(x, y, x1, y1, dir) {
       rd_direction_is_valid[dir] = TRUE;
+    } cardinal_adjc_dir_iterate_end;
 
     /* Test series that selects a direction for the river. */
     for (func_num = 0; func_num < NUM_TEST_FUNCTIONS; func_num++) {
       int best_val = -1;
+
       /* first get the tile values for the function */
-      for (dir = 0; dir < 4; dir++) {
-       int x1 = x + CAR_DIR_DX[dir];
-       int y1 = y + CAR_DIR_DY[dir];
-       if (normalize_map_pos(&x1, &y1)
-           && rd_direction_is_valid[dir]) {
+      cardinal_adjc_dir_iterate(x, y, x1, y1, dir) {
+       if (rd_direction_is_valid[dir]) {
          rd_comparison_val[dir] = (test_funcs[func_num].func) (x1, y1);
          if (best_val == -1) {
            best_val = rd_comparison_val[dir];
@@ -844,71 +846,57 @@
            best_val = MIN(rd_comparison_val[dir], best_val);
          }
        }
-      }
+      } cardinal_adjc_dir_iterate_end;
       assert(best_val != -1);
 
       /* should we abort? */
-      if (best_val > 0 && test_funcs[func_num].fatal) return FALSE;
+      if (best_val > 0 && test_funcs[func_num].fatal) {
+       return FALSE;
+      }
 
       /* mark the less attractive directions as invalid */
-      for (dir = 0; dir < 4; dir++) {
-       int x1 = x + CAR_DIR_DX[dir];
-       int y1 = y + CAR_DIR_DY[dir];
-       if (normalize_map_pos(&x1, &y1)
-           && rd_direction_is_valid[dir]) {
-         if (rd_comparison_val[dir] != best_val)
+      cardinal_adjc_dir_iterate(x, y, x1, y1, dir) {
+       if (rd_direction_is_valid[dir]) {
+         if (rd_comparison_val[dir] != best_val) {
            rd_direction_is_valid[dir] = FALSE;
+         }
        }
-      }
+      } cardinal_adjc_dir_iterate_end;
     }
 
     /* Directions evaluated with all functions. Now choose the best
        direction before going to the next iteration of the while loop */
     num_valid_directions = 0;
-    for (dir = 0; dir < 4; dir++)
-      if (rd_direction_is_valid[dir])
+    cardinal_adjc_dir_iterate(x, y, x1, y1, dir) {
+      if (rd_direction_is_valid[dir]) {
        num_valid_directions++;
+      }
+    } cardinal_adjc_dir_iterate_end;
 
-    switch (num_valid_directions) {
-    case 0:
+    if (num_valid_directions == 0) {
       return FALSE; /* river aborted */
-    case 1:
-      for (dir = 0; dir < 4; dir++) {
-       int x1 = x + CAR_DIR_DX[dir];
-       int y1 = y + CAR_DIR_DY[dir];
-       if (normalize_map_pos(&x1, &y1)
-           && rd_direction_is_valid[dir]) {
+    }
+
+    /* One or more valid directions: choose randomly. */
+    freelog(LOG_DEBUG, "mapgen.c: Had to let the random number"
+           " generator select a direction for a river.");
+    direction = myrand(num_valid_directions);
+    freelog(LOG_DEBUG, "mapgen.c: direction: %d", direction);
+
+    /* Find the direction that the random number generator selected. */
+    cardinal_adjc_dir_iterate(x, y, x1, y1, dir) {
+      if (rd_direction_is_valid[dir]) {
+       if (direction > 0) {
+         direction--;
+       } else {
          river_blockmark(x, y);
          x = x1;
          y = y1;
+         break;
        }
       }
-      break;
-    default:
-      /* More than one possible direction; Let the random number
-        generator select the direction. */
-      freelog(LOG_DEBUG, "mapgen.c: Had to let the random number"
-             " generator select a direction for a river.");
-      direction = myrand(num_valid_directions - 1);
-      freelog(LOG_DEBUG, "mapgen.c: direction: %d", direction);
-
-      /* Find the direction that the random number generator selected. */
-      for (dir = 0; dir < 4; dir++) {
-       int x1 = x + CAR_DIR_DX[dir];
-       int y1 = y + CAR_DIR_DY[dir];
-       if (normalize_map_pos(&x1, &y1)
-           && rd_direction_is_valid[dir]) {
-         if (direction > 0) direction--;
-         else {
-           river_blockmark(x, y);
-           x = x1;
-           y = y1;
-           break;
-         }
-       }
-      }
-      break;
-    } /* end switch (rd_number_of_directions()) */
+    } cardinal_adjc_dir_iterate_end;
+    assert(direction == 0);
 
   } /* end while; (Make a river.) */
 }