[Freeciv-Dev] Re: (PR#6189) 1.14: stacked with ai at start

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

Gregory Berkolaiko wrote:
> On Fri, 19 Sep 2003, Jason Short wrote:
> 
> 
>>[eneg - Thu Sep 18 15:46:51 2003]:
>>
>>
>>>One of my units began a game stacked with an ai's one.
>>
>>The attached patch should fix this for 1.14.  A similar fix is probably
>>needed for HEAD.
> 
> 
> I have 3 problems with the patch.
> 
> 1. It comes in octet-stream MIME-type, so I have to save it to read it.

Blame RT :-(.

> 2. dx, dy might be uninitialized when you use them

My mistake.  That should be (x,y) not (dx,dy) :-(.

> 3. It again doesn't fix the problem, just introduces another check.  We 
> need to know what the problem _is_, not what are the symptoms.  Maybe it 
> is present in the CVS too, just not for the given seeds.

Sorry; I thought the patch made this obvious.  The first unit gets put 
at the starting position, the second and thereafter get put nearby based 
on game.dispersion.  For the second and subsequent units there is an 
is_non_allied_unit_tile check, but not for the first.  So the second 
unit of nation 1 could be put over the starting position of civilization 2.

So it is a full fix, not just a check.  But it has some drawbacks - 
namely that the nation isn't guaranteed to be able to use its starting 
point.  If the nations are dense enough it might even be possible that 
there is no usable tile for the later nations.

An alternative would first place all the first units of all nations, 
then go back and put down all the others.  This method would fix the 
problems from above.

I like the original patch (minus the typo) for 1.14.  The attached patch 
makes the suggested change for cvs HEAD.  But in working with this code, 
I just couldn't stop until most of the function was restructured.  The 
big cleanup is that there are no longer all the overlapping variables 
used independently (i, j, x, y, dx, dy).

jason

? core.10813
Index: server/gamehand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/gamehand.c,v
retrieving revision 1.127
diff -u -r1.127 gamehand.c
--- server/gamehand.c   2003/09/09 15:49:07     1.127
+++ server/gamehand.c   2003/09/19 17:02:56
@@ -32,39 +32,80 @@
 #include "gamehand.h"
 
 
-/**************************************************************************
-...
-**************************************************************************/
-void init_new_game(void)
+/****************************************************************************
+  Initialize the game.id variable to a random string of characters.
+****************************************************************************/
+static void init_game_id(void)
 {
   static const char chars[] =
       "0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
-  int i, j, x, y;
-  int dx, dy;
-  Unit_Type_id utype;
-  int start_pos[MAX_NUM_PLAYERS]; /* indices into map.start_positions[] */
+  int i;
 
   for (i = 0; i < sizeof(game.id) - 1; i++) {
     game.id[i] = chars[myrand(sizeof(chars) - 1)];
   }
   game.id[i] = '\0';
+}
+
+/****************************************************************************
+  Place a starting unit for the player.
+****************************************************************************/
+static void place_starting_unit(int x, int y, struct player *pplayer,
+                               enum unit_flag_id role)
+{
+  Unit_Type_id utype;
+
+  assert(!is_non_allied_unit_tile(map_get_tile(x, y), pplayer));
+
+  /* For scenarios or dispersion, huts may coincide with player starts (in 
+   * other cases, huts are avoided as start positions).  Remove any such hut,
+   * and make sure to tell the client, since we may have already sent this
+   * tile (with the hut) earlier: */
+  if (map_has_special(x, y, S_HUT)) {
+    map_clear_special(x, y, S_HUT);
+    send_tile_info(NULL, x, y);
+    freelog(LOG_VERBOSE, "Removed hut on start position for %s",
+           pplayer->name);
+  }
+
+  /* Expose visible area. */
+  circle_iterate(x, y, game.rgame.init_vis_radius_sq, cx, cy) {
+    show_area(pplayer, cx, cy, 0);
+  } circle_iterate_end;
+
+  /* Create the unit of an appropriate type. */
+  utype = get_role_unit(role, 0);
+  (void) create_unit(pplayer, x, y, utype, FALSE, 0, -1);
+}
+
+/****************************************************************************
+  Initialize a new game: place the players' units onto the map, etc.
+****************************************************************************/
+void init_new_game(void)
+{
+  int start_pos[MAX_NUM_PLAYERS]; /* indices into map.start_positions[] */
+
+  init_game_id();
 
   if (!map.fixed_start_positions) {
     /* except in a scenario which provides them,
        shuffle the start positions around... */
-    assert(game.nplayers==map.num_start_positions);
-    for (i=0; i<game.nplayers;i++) { /* no advantage to the romans!! */
-      j=myrand(game.nplayers);
-      x=map.start_positions[j].x;
-      y=map.start_positions[j].y;
-      map.start_positions[j].x=map.start_positions[i].x;
-      map.start_positions[j].y=map.start_positions[i].y;
-      map.start_positions[i].x=x;
-      map.start_positions[i].y=y;
-    }
-    for(i=0; i<game.nplayers; i++) {
-      start_pos[i] = i;
-    } 
+    assert(game.nplayers == map.num_start_positions);
+    players_iterate(pplayer) {
+      /* no advantage to the romans!! */
+      int i = pplayer->player_no;
+      int j = myrand(game.nplayers);
+      int x = map.start_positions[j].x;
+      int y = map.start_positions[j].y;
+
+      map.start_positions[j].x = map.start_positions[i].x;
+      map.start_positions[j].y = map.start_positions[i].y;
+      map.start_positions[i].x = x;
+      map.start_positions[i].y = y;
+    } players_iterate_end;
+    players_iterate(pplayer) {
+      start_pos[pplayer->player_no] = pplayer->player_no;
+    } players_iterate_end;
   } else {
   /* In a scenario, choose starting positions by nation.
      If there are too few starts for number of nations, assign
@@ -75,82 +116,72 @@
     const int npos = map.num_start_positions;
     bool *pos_used = fc_calloc(npos, sizeof(bool));
     int nrem = npos;           /* remaining unused starts */
-    
-    for(i=0; i<game.nplayers; i++) {
-      int nation = game.players[i].nation;
+
+    players_iterate(pplayer) {
+      int nation = pplayer->nation;
+
       if (nation < npos) {
-       start_pos[i] = nation;
+       start_pos[pplayer->player_no] = nation;
        pos_used[nation] = TRUE;
        nrem--;
       } else {
-       start_pos[i] = npos;
+       start_pos[pplayer->player_no] = npos;
       }
-    }
-    for(i=0; i<game.nplayers; i++) {
-      if (start_pos[i] == npos) {
-       int k;
-       assert(nrem>0);
+    } players_iterate_end;
+
+    players_iterate(pplayer) {
+      if (start_pos[pplayer->player_no] == npos) {
+       int j, k;
+
+       assert(nrem > 0);
        k = myrand(nrem);
-       for(j=0; j<npos; j++) {
-         if (!pos_used[j] && (0==k--)) {
-           start_pos[i] = j;
+       for(j = 0; j < npos; j++) {
+         if (!pos_used[j] && (0 == k--)) {
+           start_pos[pplayer->player_no] = j;
            pos_used[j] = TRUE;
            nrem--;
            break;
          }
        }
-       assert(start_pos[i] != npos);
+       assert(start_pos[pplayer->player_no] != npos);
       }
-    }
+    } players_iterate_end;
     free(pos_used);
     pos_used = NULL;
   }
 
   /* Loop over all players, creating their initial units... */
-  for (i = 0; i < game.nplayers; i++) {
-    /* Start positions are warranted to be land. */
-    x = map.start_positions[start_pos[i]].x;
-    y = map.start_positions[start_pos[i]].y;
-    /* Loop over all initial units... */
-    for (j = 0; j < (game.settlers + game.explorer); j++) {
-      /* Determine a place to put the unit within the dispersion area.
-         (Always put first unit on start position.) */
-      if ((game.dispersion <= 0) || (j == 0)) {
-       dx = x;
-       dy = y;
-      } else {
-       bool is_real;
+  players_iterate(pplayer) {
+    int x = map.start_positions[start_pos[pplayer->player_no]].x;
+    int y = map.start_positions[start_pos[pplayer->player_no]].y;
 
-       do {
-         dx = x + myrand(2 * game.dispersion + 1) - game.dispersion;
-         dy = y + myrand(2 * game.dispersion + 1) - game.dispersion;
-         is_real = normalize_map_pos(&dx, &dy);
-       } while (!(is_real
-                  && map_get_continent(x, y) == map_get_continent(dx, dy)
-                  && !is_ocean(map_get_terrain(dx, dy))
-                  && !is_non_allied_unit_tile(map_get_tile(dx, dy),
-                                              get_player(i))));
-      }
-      /* For scenarios or dispersion, huts may coincide with player
-        starts (in other cases, huts are avoided as start positions).
-        Remove any such hut, and make sure to tell the client, since
-        we may have already sent this tile (with the hut) earlier:
-      */
-      if (map_has_special(dx, dy, S_HUT)) {
-        map_clear_special(dx, dy, S_HUT);
-       send_tile_info(NULL, dx, dy);
-        freelog(LOG_VERBOSE, "Removed hut on start position for %s",
-               game.players[i].name);
-      }
-      /* Expose visible area. */
-      circle_iterate(dx, dy, game.rgame.init_vis_radius_sq, cx, cy) {
-       show_area(&game.players[i], cx, cy, 0);
-      } circle_iterate_end;
+    /* Place the first unit. */
+    assert(game.settlers > 0);
+    place_starting_unit(x, y, pplayer, F_CITIES);
+  } players_iterate_end;
+
+  /* Place all other units. */
+  players_iterate(pplayer) {
+    int x = map.start_positions[start_pos[pplayer->player_no]].x;
+    int y = map.start_positions[start_pos[pplayer->player_no]].y;
+    int i, dx, dy;
+
+    for (i = 1; i < (game.settlers + game.explorer); i++) {
+      do {
+       dx = x + myrand(2 * game.dispersion + 1) - game.dispersion;
+       dy = y + myrand(2 * game.dispersion + 1) - game.dispersion;
+      } while (!(normalize_map_pos(&dx, &dy)
+                && map_get_continent(x, y) == map_get_continent(dx, dy)
+                && !is_ocean(map_get_terrain(dx, dy))
+                && !is_non_allied_unit_tile(map_get_tile(dx, dy),
+                                            pplayer)));
+
+
       /* Create the unit of an appropriate type. */
-      utype = get_role_unit((j < game.settlers) ? F_CITIES : L_EXPLORER, 0);
-      (void) create_unit(&game.players[i], dx, dy, utype, FALSE, 0, -1);
+      place_starting_unit(dx, dy, pplayer,
+                         (i < game.settlers) ? F_CITIES : L_EXPLORER);
     }
-  }
+  } players_iterate_end;
 
   /* Initialise list of improvements with world-wide equiv_range */
   improvement_status_init(game.improvements, ARRAY_SIZE(game.improvements));
Index: server/sanitycheck.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/sanitycheck.c,v
retrieving revision 1.32
diff -u -r1.32 sanitycheck.c
--- server/sanitycheck.c        2003/09/19 14:14:45     1.32
+++ server/sanitycheck.c        2003/09/19 17:02:56
@@ -119,6 +119,14 @@
 
     unit_list_iterate(ptile->units, punit) {
       assert(same_pos(punit->x, punit->y, x, y));
+
+      /* Check diplomatic status of stacked units. */
+      unit_list_iterate(ptile->units, punit2) {
+       assert(pplayers_allied(unit_owner(punit), unit_owner(punit2)));
+      } unit_list_iterate_end;
+      if (pcity) {
+       assert(pplayers_allied(unit_owner(punit), city_owner(pcity)));
+      }
     } unit_list_iterate_end;
   } whole_map_iterate_end;
 }