<URL: http://bugs.freeciv.org/Ticket/Display.html?id=40095 >

In PR#40072, the first reported error has Tenochtitlan shown with the wrong
border.  This patch does not fix the underlying issue, but is a step toward
making the savegame playable.

This patch also fixes other problems found along the way....

===

We've seen these problems in both 2.1 and 2.2, because the the city itself
has an owner field, and the tiles have another owner field.

The 2.2 code studiously attempted to yield the same borders as 2.1, and
also yields the same 2.1 bug!

After considerable testing, found a proximal cause of the problem, in
server/maphand.c map_claim_border():

   circle_dxyr_iterate(ptile, range, dtile, dx, dy, dr) {

     if (!map_is_known(dtile, powner)) {
       continue;
     }

       int r = sq_map_distance(dsite->location, dtile);

       } else if (r < dr) {
         /* nearest shall prevail */
         continue;

When the dtile location is a city, dsite->location == dtile, so r should
be 0.  And dr = map_vector_to_sq_distance(dx, dy) should be the distance
from the ptile city center to the dtile city center.  In PR#40072, they
have 1 tile between them, so to the undiscriminating eye, it would seem
that dr should be 4.

When r < dr (that is, 0 < 4), then no change should occur!

That's not working correctly.  That may explain some of the odd borders,
too.  In this case, the city is a formerly owned city that has been
captured.  It should be known.  Apparently, there's something wrong with
the city transfer code relating to borders.

The fix is two-fold.  Changing map_is_known() to map_is_known_and_seen()
fixes some of the odd boundaries around Middelburg and Tenochtitlan.
Adding an omniscient test for a city, the problem goes away:

     struct city *dcity = tile_city(dtile);

     if (NULL != dcity) {
       /* cannot affect existing cities (including self) */
       continue;
     }

     if (!map_is_known_and_seen(dtile, powner, V_MAIN)) {
       /* TODO: border should expand vision */
       continue;
     }

The test isn't entirely time wasting, as the first time through the loop is
the border source city center, and there's no reason to reclaim itself.

But these tests are just a fig leaf covering underlying problems.

These tests also change the behavior from 2.1, so loading older games will
be affected.  That's not entirely a bad thing, as tests against games in
2.1.x tickets show cities with wrong tile owners, while some other cities
have workers in the same tile!

All this is now fixed automatically on game load.

===

In ai/aisettler.c, a temporary city is created for testing placement.

Unfortunately, create_virtual_city() setup a default production that
expects the owner to be the tester, not the current border owner.
Temporarily swap the owners.

Similar problems found in server/citytools.c create_city() and
server/savegame.c player_load_cities().

===

In common/city.c, it makes no sense for create_virtual_city() to set a
default production.  Certainly not loading a saved game, as it will
immediately be replaced with loaded values.  Split the code into a new
city_choose_build_default().

Re-ordered the city variables to better match the .h and .c files, and the
load and save order in savegame.c (PR#40093).  It's decidedly difficult to
visually compare random order code!

Moved synced into server.synced with the other server-only variables.

Added placeholder comments for (formerly) uninitialized variables.

===

Renamed tile_set_city() to tile_set_worked(), to reflect its current use.
Use a define for city_owner(), matching other such definitions.  Probably
could be inline functions....

===

Fixed a potential bug in common/game.c: always unlink cities before
clearing the data fields....  Revised and extended logging.

===

Other updates to logging in server/sanitycheck.c and client/packhand.c,
checking the validity of players and tiles.  Never trust network data!

Index: server/citytools.c
===================================================================
--- server/citytools.c  (revision 14397)
+++ server/citytools.c  (working copy)
@@ -845,9 +845,11 @@
   /* Has to follow the unfog call above. */
   city_list_unlink(pgiver->cities, pcity);
   pcity->owner = ptaker;
+  city_list_prepend(ptaker->cities, pcity);
+
+  /* Update the national borders. */
   map_claim_ownership(pcity->tile, ptaker, pcity->tile);
   map_claim_border(pcity->tile, ptaker);
-  city_list_prepend(ptaker->cities, pcity);
 
   transfer_city_units(ptaker, pgiver, old_city_units,
                      pcity, NULL,
@@ -957,11 +959,14 @@
 {
   int x_itr, y_itr;
   struct nation_type *nation = nation_of_player(pplayer);
+  struct player *saved_owner = tile_owner(ptile);
   struct base_type *pbase = tile_get_base(ptile);
   struct city *pcity = create_city_virtual(pplayer, ptile, name);
 
-  freelog(LOG_DEBUG, "Creating city %s", name);
+  freelog(LOG_DEBUG, "create_city() %s", name);
 
+  tile_set_owner(ptile, pplayer); /* temporarily */
+  city_choose_build_default(pcity);
   pcity->ai.trade_want = TRADE_WEIGHTING;
   pcity->id = identity_number();
   idex_register_city(pcity);
@@ -997,10 +1002,11 @@
   vision_reveal_tiles(pcity->server.vision, game.info.city_reveal_tiles);
   city_refresh_vision(pcity);
 
-  tile_set_city(ptile, pcity); /* partly redundant to server_set_tile_city() */
+  tile_set_worked(ptile, pcity); /* partly redundant to server_set_tile_city() 
*/
   city_list_prepend(pplayer->cities, pcity);
 
   /* Claim the ground we stand on */
+  tile_set_owner(ptile, saved_owner);
   map_claim_ownership(ptile, pplayer, ptile);
 
   if (terrain_control.may_road) {
@@ -1060,7 +1066,7 @@
 
   update_tile_knowledge(ptile);
 
-  pcity->synced = FALSE;
+  pcity->server.synced = FALSE;
   send_city_info(NULL, pcity);
   sync_cities(); /* Will also send pcity. */
 
@@ -1556,7 +1562,7 @@
     return;
 
   if (!dest || dest == city_owner(pcity))
-    pcity->synced = TRUE;
+    pcity->server.synced = TRUE;
   if (!dest) {
     broadcast_city_info(pcity);
   } else {
@@ -1983,33 +1989,34 @@
 }
 
 /**************************************************************************
-Returns if a tile is available to be worked.
-Return true if the city itself is currently working the tile (and can
-continue.)
-city_x, city_y is in city map coords.
+  Returns TRUE when a tile is available to be worked, or the city itself is
+  currently working the tile (and can continue).
+  city_x, city_y is in city map coords.
 **************************************************************************/
 bool city_can_work_tile(struct city *pcity, int city_x, int city_y)
 {
-  struct tile *ptile;
+  struct player *powner = city_owner(pcity);
+  struct tile *ptile = city_map_to_map(pcity, city_x, city_y);
 
-  if (!(ptile = city_map_to_map(pcity, city_x, city_y))) {
+  if (NULL == ptile) {
     return FALSE;
   }
-  
-  if (is_enemy_unit_tile(ptile, city_owner(pcity))
-      && !is_free_worked_tile(city_x, city_y)) {
+
+  if (tile_owner(ptile) && tile_owner(ptile) != powner) {
     return FALSE;
   }
+  /* TODO: civ3-like option for borders */
 
-  if (!map_is_known(ptile, city_owner(pcity))) {
+  if (ptile->worked && ptile->worked != pcity) {
     return FALSE;
   }
 
-  if (ptile->worked && ptile->worked != pcity) {
+  if (!map_is_known(ptile, powner)) {
     return FALSE;
   }
 
-  if (tile_owner(ptile) && tile_owner(ptile) != city_owner(pcity)) {
+  if (is_enemy_unit_tile(ptile, powner)
+   && !is_free_worked_tile(city_x, city_y)) {
     return FALSE;
   }
 
@@ -2018,15 +2025,14 @@
 
 /**************************************************************************
 Sets tile worked status.
-city_x, city_y is in city map coords.
-You need to call sync_cities for the affected cities to be synced with the
-client.
+  city_x, city_y is in city map coords.
+  Call sync_cities() for the affected cities to be synced with the client.
 
 You should not use this function unless you really know what you are doing!
 Better functions are
 server_set_worker_city()
 server_remove_worker_city()
-update_city_tile_status()
+update_city_tile_status_map()
 **************************************************************************/
 static void server_set_tile_city(struct city *pcity, int city_x, int city_y,
                                 enum city_tile_type type)
@@ -2037,7 +2043,7 @@
   assert(current != type);
 
   set_worker_city(pcity, city_x, city_y, type);
-  pcity->synced = FALSE;
+  pcity->server.synced = FALSE;
 
   /* Update adjacent cities. */
   {
@@ -2058,9 +2064,8 @@
 }
 
 /**************************************************************************
-city_x, city_y is in city map coords.
-You need to call sync_cities for the affected cities to be synced with the
-client.
+  city_x, city_y is in city map coords.
+  Call sync_cities() for the affected cities to be synced with the client.
 **************************************************************************/
 void server_remove_worker_city(struct city *pcity, int city_x, int city_y)
 {
@@ -2070,9 +2075,8 @@
 }
 
 /**************************************************************************
-city_x, city_y is in city map coords.
-You need to call sync_cities for the affected cities to be synced with the
-client.
+  city_x, city_y is in city map coords.
+  Call sync_cities() for the affected cities to be synced with the client.
 **************************************************************************/
 void server_set_worker_city(struct city *pcity, int city_x, int city_y)
 {
@@ -2100,10 +2104,9 @@
 }
 
 /**************************************************************************
-Updates the worked status of a tile.
-city_x, city_y is in city map coords.
-You need to call sync_cities for the affected cities to be synced with the
-client.
+  Updates the worked status of a tile.
+  city_x, city_y is in city map coords.
+  Call sync_cities() for the affected cities to be synced with the client.
 **************************************************************************/
 static void update_city_tile_status(struct city *pcity, int city_x,
                                    int city_y)
@@ -2154,9 +2157,10 @@
 
   players_iterate(pplayer) {
     city_list_iterate(pplayer->cities, pcity) {
-      /* sending will set synced to 1. */
-      if (!pcity->synced)
+      if (!pcity->server.synced) {
+       /* sending will set to TRUE. */
        send_city_info(pplayer, pcity);
+      }
     } city_list_iterate_end;
   } players_iterate_end;
 }
Index: server/maphand.c
===================================================================
--- server/maphand.c    (revision 14397)
+++ server/maphand.c    (working copy)
@@ -1825,6 +1825,7 @@
 *************************************************************************/
 void map_claim_border(struct tile *ptile, struct player *powner)
 {
+  struct city *pcity = tile_city(ptile);
   struct vision_site *psite = map_get_player_site(ptile, powner);
   int range = game.info.borders;
 
@@ -1832,31 +1833,58 @@
     /* no borders */
     return;
   }
+
+  if (NULL == psite) {
+    /* should never happen! call map_claim_ownership() first! */
+    freelog(LOG_ERROR, "(%2d,%2d) border has NULL source for %s",
+            TILE_XY(ptile),
+            nation_rule_name(nation_of_player(powner)));
+    return;
+  }
+
   if (IDENTITY_NUMBER_ZERO == psite->identity) {
-    /* should never be called! */
-    freelog(LOG_ERROR, "Warning: border source (%d,%d) is unknown!",
-            TILE_XY(ptile));
+    /* TODO: maybe someday, but currently should never be called! */
+    freelog(LOG_ERROR, "(%2d,%2d) border has zero identity for %s",
+            TILE_XY(ptile),
+            nation_rule_name(nation_of_player(powner)));
     return;
   }
+
   if (IDENTITY_NUMBER_ZERO < psite->identity) {
     /* city expansion */
     range = MIN(psite->size + 1, game.info.borders);
+    /* TODO: expansion stages based on ruleset */
     if (psite->size > game.info.borders) {
       range += (psite->size - game.info.borders) / 2;
     }
   }
   range *= range; /* due to sq dist */
 
-  freelog(LOG_VERBOSE, "border source (%d,%d) range %d",
-          TILE_XY(ptile), range);
+  if (NULL != pcity) {
+    freelog(LOG_VERBOSE, "(%2d,%2d) border %2d \"%s\"[%d]",
+            TILE_XY(ptile),
+            range,
+            city_name(pcity), pcity->size);
+  } else {
+    freelog(LOG_VERBOSE, "(%2d,%2d) border %2d",
+            TILE_XY(ptile),
+            range);
+  }
 
   circle_dxyr_iterate(ptile, range, dtile, dx, dy, dr) {
+    struct city *dcity = tile_city(dtile);
     struct player *downer = tile_owner(dtile);
 
-    if (!map_is_known(dtile, powner)) {
-      /* border tile never seen */
+    if (NULL != dcity) {
+      /* cannot affect existing cities (including self) */
       continue;
     }
+
+    if (!map_is_known_and_seen(dtile, powner, V_MAIN)) {
+      /* TODO: border should expand vision */
+      continue;
+    }
+
     if (NULL != downer && downer != powner) {
       struct vision_site *dsite = map_get_player_site(dtile, downer);
       int r = sq_map_distance(dsite->location, dtile);
@@ -1913,15 +1941,18 @@
     cities_to_refresh = city_list_new();
   }
 
+freelog(LOG_VERBOSE,"map_calculate_borders() sites");
   /* base sites are done first, as they may be thorn in city side. */
   sites_iterate(psite) {
     map_claim_border(psite->location, vision_owner(psite));
   } sites_iterate_end;
 
+freelog(LOG_VERBOSE,"map_calculate_borders() cities");
   cities_iterate(pcity) {
     map_claim_border(pcity->tile, city_owner(pcity));
   } cities_iterate_end;
 
+freelog(LOG_VERBOSE,"map_calculate_borders() done");
 #ifdef OWNER_SOURCE
   /* First transfer ownership for sources that have changed hands. */
   whole_map_iterate(ptile) {
Index: server/sanitycheck.c
===================================================================
--- server/sanitycheck.c        (revision 14397)
+++ server/sanitycheck.c        (working copy)
@@ -235,13 +235,30 @@
   int delta;
   int citizens = 0;
   struct player *pplayer = city_owner(pcity);
+  struct tile *pcenter = city_tile(pcity);
 
   SANITY_CITY(pcity, pcity->size >= 1);
-  SANITY_CITY(pcity, !terrain_has_flag(tile_terrain(pcity->tile),
-                                       TER_NO_CITIES));
-  SANITY_CITY(pcity, tile_owner(pcity->tile) == NULL
-                     || tile_owner(pcity->tile) == pplayer);
 
+  SANITY_CITY(pcity, NULL != pcenter);
+
+  if (NULL != pcenter) {
+    SANITY_CITY(pcity, !terrain_has_flag(tile_terrain(pcenter), 
TER_NO_CITIES));
+
+    SANITY_CITY(pcity, NULL != tile_owner(pcenter));
+
+    if (NULL != tile_owner(pcenter)) {
+      if (tile_owner(pcenter) != pplayer) {
+       SANITY_("(%4d,%4d) tile owned by %s, "
+               "at %s \"%s\"[%d]%s"),
+               TILE_XY(pcenter),
+               nation_rule_name(nation_of_player(tile_owner(pcenter))),
+               nation_rule_name(nation_of_player(pplayer)),
+               city_name(pcity), pcity->size,
+               "{city center}");
+      }
+    }
+  }
+
   unit_list_iterate(pcity->units_supported, punit) {
     SANITY_CITY(pcity, punit->homecity == pcity->id);
     SANITY_CITY(pcity, unit_owner(punit) == pplayer);
Index: server/savegame.c
===================================================================
--- server/savegame.c   (revision 14397)
+++ server/savegame.c   (working copy)
@@ -2268,7 +2268,6 @@
     name = secfile_lookup_str_default(file, named, named);
     /* copied into city->name */
     pcity = create_city_virtual(plr, pcenter, name);
-    tile_set_owner(pcenter, plr);
 
     pcity->id = secfile_lookup_int(file, "player%d.c%d.id", plrno, i);
     identity_number_reserve(pcity->id);
@@ -2280,6 +2279,9 @@
     if (NULL != past) {
       pcity->original = past;
     }
+    past = tile_owner(pcenter);
+    tile_set_owner(pcenter, plr); /* for city_owner(), just in case? */
+    /* no city_choose_build_default(), values loaded below! */
 
     pcity->size = secfile_lookup_int(file, "player%d.c%d.size", plrno, i);
 
@@ -2456,7 +2458,7 @@
                                 "player%d.c%d.last_turns_shield_surplus",
                                 plrno, i);
 
-    pcity->synced = FALSE; /* must re-sync with clients */
+    pcity->server.synced = FALSE; /* must re-sync with clients */
 
     /* Fix for old buggy savegames. */
     if (!has_capability("known32fix", savefile_options)
@@ -2559,7 +2561,7 @@
       /* bypass set_worker_city() checks */
       base_map_to_city_map(&city_x, &city_y, pcenter, pcenter);
       pcity->city_map[city_x][city_y] = C_TILE_WORKER;
-      tile_set_city(pcenter, pcity); /* repair */
+      tile_set_worked(pcenter, pcity); /* repair */
 
       city_repair_size(pcity, -1);
     }
@@ -2619,7 +2621,7 @@
       }
     }
 
-    init_worklist(&pcity->worklist);
+    /* init_worklist() done in create_city_virtual() */
     worklist_load(file, &pcity->worklist, "player%d.c%d", plrno, i);
 
     /* after 2.1.0 new options format.  Old options are lost on upgrade. */
@@ -2660,6 +2662,7 @@
 
     /* After all the set_worker_city() and everything is loaded. */
     city_list_append(plr->cities, pcity);
+    tile_set_owner(pcenter, past);
     map_claim_ownership(pcenter, plr, pcenter);
     map_claim_border(pcenter, plr);
   }
@@ -2860,7 +2863,10 @@
 
       id = secfile_lookup_int(file, "player%d.dc%d.owner", plrno, i);
       if (id == plrno) {
-        freelog(LOG_ERROR, "player%d.dc%d has same owner (%d); skipping.",
+        /* Earlier versions redundantly saved the dummy for their own cities.
+         * Since 2.2.0, map_claim_ownership() rebuilds them at city load time.
+         */
+        freelog(LOG_VERBOSE, "player%d.dc%d has same owner (%d); skipping.",
                 plrno, i, id);
         free(pdcity);
         continue;
@@ -3531,13 +3537,13 @@
       /* Save improvement list as bitvector. Note that improvement order
        * is saved in savefile.improvement_order.
        */
+      assert(improvement_count() < sizeof(impr_buf));
       improvement_iterate(pimprove) {
         impr_buf[improvement_index(pimprove)] =
           BV_ISSET(pdcity->improvements, improvement_index(pimprove))
           ? '1' : '0';
       } improvement_iterate_end;
       impr_buf[improvement_count()] = '\0';
-      assert(strlen(impr_buf) < sizeof(impr_buf));
       secfile_insert_str(file, impr_buf,
                          "player%d.dc%d.improvements", plrno, i);
 
Index: common/city.c
===================================================================
--- common/city.c       (revision 14397)
+++ common/city.c       (working copy)
@@ -295,24 +295,23 @@
 void set_worker_city(struct city *pcity, int city_x, int city_y,
                     enum city_tile_type type)
 {
-  struct tile *ptile;
+  struct tile *ptile = city_map_to_map(pcity, city_x, city_y);
 
-  if ((ptile = city_map_to_map(pcity, city_x, city_y))) {
+  if (NULL != ptile) {
     if (pcity->city_map[city_x][city_y] == C_TILE_WORKER
        && ptile->worked == pcity) {
-      ptile->worked = NULL;
+      tile_set_worked(ptile, NULL);
     }
-    pcity->city_map[city_x][city_y] = type;
     if (type == C_TILE_WORKER) {
       /* No assert to check that nobody else is working this tile.
        * City creation relies on claiming tile to new city first,
        * and freeing it from another city only later. */
-      ptile->worked = pcity;
+      tile_set_worked(ptile, pcity);
     }
   } else {
     assert(type == C_TILE_UNAVAILABLE);
-    pcity->city_map[city_x][city_y] = type;
   }
+  pcity->city_map[city_x][city_y] = type;
 }
 
 /**************************************************************************
@@ -638,13 +637,61 @@
 }
 
 /**************************************************************************
+  Always tile_set_owner(ptile, pplayer) sometime before this!
+**************************************************************************/
+void city_choose_build_default(struct city *pcity)
+{
+  if (NULL == city_tile(pcity)) {
+    /* When a "dummy" city is created with no tile, then choosing a build 
+     * target could fail.  This currently might happen during map editing.
+     * FIXME: assumes the first unit is always "valid", so check for
+     * obsolete units elsewhere. */
+    pcity->production.kind = VUT_UTYPE;
+    pcity->production.value.utype = utype_by_number(0);
+  } else {
+    struct unit_type *u = best_role_unit(pcity, L_FIRSTBUILD);
+
+    if (u) {
+      pcity->production.kind = VUT_UTYPE;
+      pcity->production.value.utype = u;
+    } else {
+      bool found = FALSE;
+
+      /* Just pick the first available item. */
+
+      improvement_iterate(pimprove) {
+       if (can_city_build_improvement_direct(pcity, pimprove)) {
+         found = TRUE;
+         pcity->production.kind = VUT_IMPROVEMENT;
+         pcity->production.value.building = pimprove;
+         break;
+       }
+      } improvement_iterate_end;
+
+      if (!found) {
+       unit_type_iterate(punittype) {
+         if (can_city_build_unit_direct(pcity, punittype)) {
+           found = TRUE;
+           pcity->production.kind = VUT_UTYPE;
+           pcity->production.value.utype = punittype;
+         }
+       } unit_type_iterate_end;
+      }
+
+      assert(found);
+    }
+  }
+}
+
+#ifndef city_name
+/**************************************************************************
   Return the name of the city.
 **************************************************************************/
 const char *city_name(const struct city *pcity)
 {
-  assert(NULL != pcity);
   return pcity->name;
 }
+#endif
 
 /**************************************************************************
   Return the owner of the city.
@@ -656,15 +703,16 @@
   return pcity->owner;
 }
 
+#ifndef city_tile
 /**************************************************************************
   Return the tile location of the city.
   Not (yet) always used, mostly for debugging.
 **************************************************************************/
 struct tile *city_tile(const struct city *pcity)
 {
-  assert(NULL != pcity);
   return pcity->tile;
 }
+#endif
 
 /**************************************************************************
  Returns how many thousand citizen live in this city.
@@ -2436,6 +2484,8 @@
 /**************************************************************************
   Create virtual skeleton for a city.
   Values are mostly sane defaults.
+
+  Always tile_set_owner(ptile, pplayer) sometime after this!
 **************************************************************************/
 struct city *create_city_virtual(struct player *pplayer,
                                 struct tile *ptile, const char *name)
@@ -2443,97 +2493,109 @@
   int i;
   struct city *pcity = fc_calloc(1, sizeof(*pcity));
 
-  /* It does not register the city so the id is set to 0. */
-  pcity->id = IDENTITY_NUMBER_ZERO;
-
   assert(pplayer != NULL); /* No unowned cities! */
   pcity->original = pplayer;
   pcity->owner = pplayer;
 
   pcity->tile = ptile;
+
+  assert(name != NULL);
   sz_strlcpy(pcity->name, name);
 
 #ifdef ZERO_VARIABLES_FOR_SEARCHING
+  /* This does not register the city, so the identity defaults to 0. */
+  pcity->id = IDENTITY_NUMBER_ZERO;
+
   memset(pcity->feel, 0, sizeof(pcity->feel));
   memset(pcity->specialists, 0, sizeof(pcity->specialists));
-  memset(pcity->tile_output, 0, sizeof(pcity->tile_output));
-  pcity->was_happy = FALSE;
-  pcity->steal = 0;
-  for (i = 0; i < NUM_TRADEROUTES; i++) {
-    pcity->trade_value[i] = pcity->trade[i] = 0;
-  }
-  pcity->food_stock = 0;
-  pcity->shield_stock = 0;
 #endif
+
+  /* assume some non-working population: */
   pcity->specialists[DEFAULT_SPECIALIST] =
   pcity->size = 1;
 
-  /* Initialise improvements list */
-  for (i = 0; i < ARRAY_SIZE(pcity->built); i++) {
-    pcity->built[i].turn = I_NEVER;
+#ifdef ZERO_VARIABLES_FOR_SEARCHING
+  for (i = 0; i < NUM_TRADEROUTES; i++) {
+    pcity->trade_value[i] = pcity->trade[i] = 0;
   }
+  memset(pcity->tile_output, 0, sizeof(pcity->tile_output));
 
-  /* Set up the worklist */
-  init_worklist(&pcity->worklist);
+  memset(pcity->surplus, 0, O_COUNT * sizeof(*pcity->surplus));
+  memset(pcity->waste, 0, O_COUNT * sizeof(*pcity->waste));
+  memset(pcity->unhappy_penalty, 0,
+        O_COUNT * sizeof(*pcity->unhappy_penalty));
+  memset(pcity->prod, 0, O_COUNT * sizeof(*pcity->prod));
+  memset(pcity->citizen_base, 0, O_COUNT * sizeof(*pcity->citizen_base));
+  memset(pcity->usage, 0, O_COUNT * sizeof(*pcity->usage));
+#endif
 
-  if (!ptile) {
-    /* When a "dummy" city is created with no tile, then choosing a build 
-     * target could fail.  This currently might happen during map editing.
-     * FIXME: assumes the first unit is always "valid", so check for
-     * obsolete units elsewhere. */
-    pcity->production.kind = VUT_UTYPE;
-    pcity->production.value.utype = utype_by_number(0);
-  } else {
-    struct unit_type *u = best_role_unit(pcity, L_FIRSTBUILD);
+  output_type_iterate(o) {
+    pcity->bonus[o] = 100;
+  } output_type_iterate_end;
 
-    if (u) {
-      pcity->production.kind = VUT_UTYPE;
-      pcity->production.value.utype = u;
-    } else {
-      bool found = FALSE;
+#ifdef ZERO_VARIABLES_FOR_SEARCHING
+  pcity->martial_law = 0;
+  pcity->unit_happy_upkeep = 0;
 
-      /* Just pick the first available item. */
+  pcity->food_stock = 0;
+  pcity->shield_stock = 0;
+  pcity->pollution = 0;
 
-      improvement_iterate(pimprove) {
-       if (can_city_build_improvement_direct(pcity, pimprove)) {
-         found = TRUE;
-         pcity->production.kind = VUT_IMPROVEMENT;
-         pcity->production.value.building = pimprove;
-         break;
-       }
-      } improvement_iterate_end;
+  pcity->airlift = FALSE;
+  pcity->debug = FALSE;
+#endif
+  pcity->did_buy = TRUE; /* why? */
+#ifdef ZERO_VARIABLES_FOR_SEARCHING
+  pcity->did_sell = FALSE;
+  pcity->is_updated = FALSE;
+  pcity->was_happy = FALSE;
 
-      if (!found) {
-       unit_type_iterate(punittype) {
-         if (can_city_build_unit_direct(pcity, punittype)) {
-           found = TRUE;
-           pcity->production.kind = VUT_UTYPE;
-           pcity->production.value.utype = punittype;
-         }
-       } unit_type_iterate_end;
-      }
+  pcity->anarchy = 0;
+  pcity->rapture = 0;
+  pcity->steal = 0;
+#endif
 
-      assert(found);
-    }
-  }
   pcity->turn_founded = game.info.turn;
-  pcity->did_buy = TRUE;
-  pcity->did_sell = FALSE;
-  pcity->airlift = FALSE;
+  pcity->turn_last_built = game.info.turn;
 
-  pcity->turn_last_built = game.info.turn;
-  pcity->changed_from = pcity->production;
 #ifdef ZERO_VARIABLES_FOR_SEARCHING
   pcity->before_change_shields = 0;
+  pcity->caravan_shields = 0;
   pcity->disbanded_shields = 0;
-  pcity->caravan_shields = 0;
   pcity->last_turns_shield_surplus = 0;
-  pcity->anarchy = 0;
-  pcity->rapture = 0;
+#endif
+
+  /* Initialise improvements list */
+  for (i = 0; i < ARRAY_SIZE(pcity->built); i++) {
+    pcity->built[i].turn = I_NEVER;
+  }
+
+#ifdef ZERO_VARIABLES_FOR_SEARCHING
+  /* just setting the entry to zero: */
+  pcity->production.kind = VUT_NONE;
+  /* all the union pointers should be in the same place: */ 
+  pcity->production.value.building = NULL;
+  pcity->changed_from = pcity->production;
+#endif
+
+  /* Set up the worklist */
+  init_worklist(&pcity->worklist);
+
+#ifdef ZERO_VARIABLES_FOR_SEARCHING
   BV_CLR_ALL(pcity->city_options);
 
+  /* city_map; placeholder for searching */
+
+  pcity->client.occupied = FALSE;
+  pcity->client.walls = FALSE;
+  pcity->client.happy = FALSE;
+  pcity->client.unhappy = FALSE;
+  pcity->client.colored = FALSE;
+  pcity->client.color_index = FALSE;
+
   pcity->server.workers_frozen = 0;
   pcity->server.needs_arrange = FALSE;
+  pcity->server.synced = FALSE;
   pcity->server.vision = NULL; /* No vision. */
 
   pcity->ai.building_turn = 0;
@@ -2543,42 +2605,37 @@
   memset(pcity->ai.building_want, 0, sizeof(pcity->ai.building_want));
 
   /* pcity->ai.choice; placeholder for searching */
-  pcity->ai.founder_boat = FALSE;
-  pcity->ai.founder_turn = 0;
-  pcity->ai.founder_want = 0; /* calculating this is really expensive */
-  pcity->ai.settler_want = 0;
-#endif
-  pcity->ai.trade_want = 1; /* we always want some */
+  /* pcity->ai.worth; placeholder for searching */
 
-#ifdef ZERO_VARIABLES_FOR_SEARCHING
+  pcity->ai.invasion = 0;
+  pcity->ai.bcost = 0;
+  pcity->ai.attack = 0;
+
   pcity->ai.danger = 0;
+  pcity->ai.grave_danger = 0;
   pcity->ai.urgency = 0;
-  pcity->ai.grave_danger = 0;
   pcity->ai.wallvalue = 0;
+
   pcity->ai.downtown = 0;
-  pcity->ai.invasion = 0;
-  pcity->ai.bcost = 0;
-  pcity->ai.attack = 0;
+  /* pcity->ai.distance_to_wonder_city; placeholder for searching */
 
-  memset(pcity->surplus, 0, O_COUNT * sizeof(*pcity->surplus));
-  memset(pcity->waste, 0, O_COUNT * sizeof(*pcity->waste));
-  memset(pcity->unhappy_penalty, 0,
-        O_COUNT * sizeof(*pcity->unhappy_penalty));
-  memset(pcity->prod, 0, O_COUNT * sizeof(*pcity->prod));
-  memset(pcity->citizen_base, 0, O_COUNT * sizeof(*pcity->citizen_base));
+  /* pcity->ai.celebrate; placeholder for searching */
+  /* pcity->ai.diplomat_threat; placeholder for searching */
+  /* pcity->ai.has_diplomat; placeholder for searching */
+
+  pcity->ai.founder_boat = FALSE;
+  pcity->ai.founder_turn = 0;
+  pcity->ai.founder_want = 0; /* calculating this is really expensive */
+  pcity->ai.settler_want = 0;
 #endif
-  output_type_iterate(o) {
-    pcity->bonus[o] = 100;
-  } output_type_iterate_end;
+  pcity->ai.trade_want = 1; /* we always want some TRADE_WEIGHTING */
 
+  /* pcity->ai.act_value; placeholder for searching */
+  /* info_units_present; placeholder for searching */
+  /* info_units_supported; placeholder for searching */
+
   pcity->units_supported = unit_list_new();
 
-  pcity->client.occupied = FALSE;
-  pcity->client.happy = pcity->client.unhappy = FALSE;
-  pcity->client.colored = FALSE;
-
-  pcity->debug = FALSE;
-
   return pcity;
 }
 
Index: common/city.h
===================================================================
--- common/city.h       (revision 14397)
+++ common/city.h       (working copy)
@@ -199,24 +199,28 @@
 
   struct ai_choice choice;      /* to spend gold in the right place only */
 
+  int worth; /* Cache city worth here, sum of all weighted incomes */
+
+  int invasion; /* who's coming to kill us, for attack co-ordination */
+  int attack, bcost; /* This is also for invasion - total power and value of
+                      * all units coming to kill us. */
+
   unsigned int danger;          /* danger to be compared to assess_defense */
-  bool diplomat_threat;         /* enemy diplomat or spy is near the city */
-  bool has_diplomat;            /* this city has diplomat or spy defender */
+  unsigned int grave_danger;    /* danger, should show positive feedback */
   unsigned int urgency;         /* how close the danger is; if zero, 
                                    bodyguards can leave */
-  unsigned int grave_danger;    /* danger, should show positive feedback */
   int wallvalue;                /* how much it helps for defenders to be 
                                    ground units */
+
   int downtown;                 /* distance from neighbours, for locating 
                                    wonders wisely */
   int distance_to_wonder_city;  /* wondercity will set this for us, 
                                    avoiding paradox */
+
   bool celebrate;               /* try to celebrate in this city */
+  bool diplomat_threat;         /* enemy diplomat or spy is near the city */
+  bool has_diplomat;            /* this city has diplomat or spy defender */
 
-  /* Used for caching change in value from a worker performing
-   * a particular activity on a particular tile. */
-  int act_value[ACTIVITY_LAST][CITY_MAP_SIZE][CITY_MAP_SIZE];
-
   /* so we can contemplate with warmap fresh and decide later */
   /* These values are for builder (F_SETTLERS) and founder (F_CITIES) units.
    * Negative values indicate that the city needs a boat first;
@@ -227,11 +231,9 @@
   int settler_want;
   int trade_want;               /* saves a zillion calculations */
 
-  int invasion; /* who's coming to kill us, for attack co-ordination */
-  int attack, bcost; /* This is also for invasion - total power and value of
-                      * all units coming to kill us. */
-
-  int worth; /* Cache city worth here, sum of all weighted incomes */
+  /* Used for caching change in value from a worker performing
+   * a particular activity on a particular tile. */
+  int act_value[ACTIVITY_LAST][CITY_MAP_SIZE][CITY_MAP_SIZE];
 };
 
 enum citizen_category {
@@ -256,7 +258,7 @@
 
 struct city {
   char name[MAX_LEN_NAME];
-  struct tile *tile;
+  struct tile *tile; /* May be NULL in Editor! */
   struct player *owner; /* Cannot be NULL. */
   struct player *original; /* Cannot be NULL. */
   int id;
@@ -292,52 +294,52 @@
   /* the physics */
   int food_stock;
   int shield_stock;
-  int pollution;
+  int pollution;                /* not saved */
 
-  struct universal production;
+  /* turn states */
+  bool airlift;
+  bool debug;                   /* not saved */
+  bool did_buy;
+  bool did_sell;
+  bool is_updated;              /* not saved */
+  bool was_happy;
 
+  int anarchy;                  /* anarchy rounds count */ 
+  int rapture;                  /* rapture rounds count */ 
+  int steal;                    /* diplomats steal once; for spies, gets 
harder */
+  int turn_founded;
+  int turn_last_built;
+
+  int before_change_shields;    /* If changed this turn, shields before 
penalty */
+  int caravan_shields;          /* If caravan has helped city to build wonder. 
*/
+  int disbanded_shields;        /* If you disband unit in a city. Count them */
+  int last_turns_shield_surplus; /* The surplus we had last turn. */
+
   struct built_status built[B_LAST];
 
+  struct universal production;
+
+  /* If changed this turn, what we changed from */
+  struct universal changed_from;
+
   struct worklist worklist;
 
+  bv_city_options city_options;
+
   enum city_tile_type city_map[CITY_MAP_SIZE][CITY_MAP_SIZE];
 
-  struct unit_list *units_supported;
-
   struct {
     /* Only used at the client (the server is omniscient). */
     bool occupied;
-    bool happy, unhappy;
+    bool walls;
+    bool happy;
+    bool unhappy;
 
     /* The color is an index into the city_colors array in mapview_common */
     bool colored;
     int color_index;
-
-    bool walls;
   } client;
 
-  int steal;                 /* diplomats steal once; for spies, gets harder */
-  /* turn states */
-  bool did_buy;
-  bool did_sell, is_updated;
-  int turn_last_built;       /* The last year in which something was built */
-
-  /* If changed this turn, what we changed from */
-  struct universal changed_from;
-
-  int disbanded_shields;      /* If you disband unit in a city. Count them */
-  int caravan_shields;        /* If caravan has helped city to build wonder. */
-  int before_change_shields;  /* If changed this turn, shields before penalty 
*/
-  int last_turns_shield_surplus; /* The surplus we had last turn. */
-  int anarchy;               /* anarchy rounds count */ 
-  int rapture;                /* rapture rounds count */ 
-  bool was_happy;
-  bool airlift;
-
-  bv_city_options city_options;
-
-  /* server variable. indicates if the city map is synced with the client. */
-  bool synced;
   struct {
     /* If > 0, workers will not be rearranged until they are unfrozen. */
     int workers_frozen;
@@ -346,17 +348,19 @@
      * set inside auto_arrange_workers. */
     bool needs_arrange;
 
+    /* the city map is synced with the client. */
+    bool synced;
+
     struct vision *vision;
   } server;
 
-  int turn_founded;            /* In which turn was the city founded? */
+  struct ai_city ai;
 
   /* info for dipl/spy investigation -- used only in client */
   struct unit_list *info_units_supported;
   struct unit_list *info_units_present;
 
-  struct ai_city ai;
-  bool debug;
+  struct unit_list *units_supported;
 };
 
 struct citystyle {
@@ -468,6 +472,8 @@
 bool city_can_grow_to(const struct city *pcity, int pop_size);
 bool city_can_change_build(const struct city *pcity);
 
+void city_choose_build_default(struct city *pcity);
+
 /* textual representation of buildings */
 
 const char *city_improvement_name_translation(const struct city *pcity,
Index: common/tile.c
===================================================================
--- common/tile.c       (revision 14397)
+++ common/tile.c       (working copy)
@@ -32,6 +32,7 @@
 }
 #endif
 
+#ifndef tile_owner
 /****************************************************************************
   Return the player who owns this tile (or NULL if none).
 ****************************************************************************/
@@ -39,13 +40,14 @@
 {
   return ptile->owner;
 }
+#endif
 
 /****************************************************************************
   Set the owner of a tile (may be NULL).
 ****************************************************************************/
-void tile_set_owner(struct tile *ptile, struct player *owner)
+void tile_set_owner(struct tile *ptile, struct player *pplayer)
 {
-  ptile->owner = owner;
+  ptile->owner = pplayer;
 }
 
 /****************************************************************************
@@ -55,18 +57,18 @@
 {
   struct city *pcity = ptile->worked;
 
-  if (NULL != pcity && ptile == pcity->tile) {
+  if (NULL != pcity && city_tile(pcity) == ptile) {
     return pcity;
   }
   return NULL;
 }
 
 /****************************************************************************
-  Set the city on the tile (may be NULL).
+  Set the city/worker on the tile (may be NULL).
 ****************************************************************************/
-void tile_set_city(struct tile *ptile, struct city *pcity)
+void tile_set_worked(struct tile *ptile, struct city *pcity)
 {
-  ptile->worked = pcity;               /* probably duplicate effort */
+  ptile->worked = pcity;
 }
 
 #ifndef tile_terrain
Index: common/tile.h
===================================================================
--- common/tile.h       (revision 14397)
+++ common/tile.h       (working copy)
@@ -60,13 +60,14 @@
 int tile_index(const struct tile *ptile);
 
 struct city *tile_city(const struct tile *ptile);
-void tile_set_city(struct tile *ptile, struct city *pcity);
+void tile_set_worked(struct tile *ptile, struct city *pcity);
 
 #define tile_continent(_tile) ((_tile)->continent)
 /*Continent_id tile_continent(const struct tile *ptile);*/
 void tile_set_continent(struct tile *ptile, Continent_id val);
 
-struct player *tile_owner(const struct tile *ptile);
+#define tile_owner(_tile) ((_tile)->owner)
+/*struct player *tile_owner(const struct tile *ptile);*/
 void tile_set_owner(struct tile *ptile, struct player *pplayer);
 
 #define tile_resource(_tile) ((_tile)->resource)
Index: common/game.c
===================================================================
--- common/game.c       (revision 14397)
+++ common/game.c       (working copy)
@@ -155,23 +155,36 @@
   /* Opaque server-only variable: the server must free this earlier. */
   assert(punit->server.vision == NULL);
 
-  freelog(LOG_DEBUG, "game_remove_unit %d", punit->id);
-  freelog(LOG_DEBUG, "removing unit %d, %s %s (%d %d) hcity %d",
-         punit->id, 
-         nation_rule_name(nation_of_unit(punit)),
-         unit_rule_name(punit),
-         punit->tile->x,
-         punit->tile->y,
-         punit->homecity);
-
   pcity = player_find_city_by_id(unit_owner(punit), punit->homecity);
   if (pcity) {
     unit_list_unlink(pcity->units_supported, punit);
 
-    freelog(LOG_DEBUG, "home city %s, %s, (%d %d)",
-         city_name(pcity),
-         nation_rule_name(nation_of_city(pcity)),
-         TILE_XY(pcity->tile));
+    freelog(LOG_DEBUG, "game_remove_unit()"
+           " at (%d,%d) unit %d, %s %s home (%d,%d) city %d, %s %s",
+           TILE_XY(punit->tile),
+           punit->id, 
+           nation_rule_name(nation_of_unit(punit)),
+           unit_rule_name(punit),
+           TILE_XY(pcity->tile),
+           punit->homecity,
+           nation_rule_name(nation_of_city(pcity)),
+           city_name(pcity));
+  } else if (IDENTITY_NUMBER_ZERO == punit->homecity) {
+    freelog(LOG_DEBUG, "game_remove_unit()"
+           " at (%d,%d) unit %d, %s %s home %d",
+           TILE_XY(punit->tile),
+           punit->id, 
+           nation_rule_name(nation_of_unit(punit)),
+           unit_rule_name(punit),
+           punit->homecity);
+  } else {
+    freelog(LOG_ERROR, "game_remove_unit()"
+           " at (%d,%d) unit %d, %s %s home %d invalid",
+           TILE_XY(punit->tile),
+           punit->id, 
+           nation_rule_name(nation_of_unit(punit)),
+           unit_rule_name(punit),
+           punit->homecity);
   }
 
   unit_list_unlink(punit->tile->units, punit);
@@ -190,20 +203,27 @@
 **************************************************************************/
 void game_remove_city(struct city *pcity)
 {
-  freelog(LOG_DEBUG, "game_remove_city %d", pcity->id);
-  freelog(LOG_DEBUG, "removing city %s, %s, (%d %d)",
-         city_name(pcity),
+  struct tile *pcenter = city_tile(pcity);
+
+  freelog(LOG_DEBUG, "game_remove_city()"
+          " at (%d,%d) city %d, %s %s",
+         TILE_XY(pcenter),
+         pcity->id,
          nation_rule_name(nation_of_city(pcity)),
-         TILE_XY(pcity->tile));
+         city_name(pcity));
 
   /* Opaque server-only variable: the server must free this earlier. */
   assert(pcity->server.vision == NULL);
 
-  city_map_checked_iterate(pcity->tile, x, y, map_tile) {
+  /* always unlink before clearing data */
+  city_list_unlink(city_owner(pcity)->cities, pcity);
+
+  city_map_checked_iterate(pcenter, x, y, map_tile) {
     set_worker_city(pcity, x, y, C_TILE_EMPTY);
   } city_map_checked_iterate_end;
-  city_list_unlink(city_owner(pcity)->cities, pcity);
-  tile_set_city(pcity->tile, NULL); /* redundant to set_worker_city() above */
+
+  /* should be redundant to set_worker_city() above */
+  tile_set_worked(pcenter, NULL); /* is_free_worked_tile() */
   idex_unregister_city(pcity);
   destroy_city_virtual(pcity);
 }
Index: ai/aisettler.c
===================================================================
--- ai/aisettler.c      (revision 14397)
+++ ai/aisettler.c      (working copy)
@@ -114,9 +114,10 @@
                      struct cityresult *result)
 {
   struct city *pcity = tile_city(result->tile);
+  struct government *curr_govt = government_of_player(pplayer);
+  struct player *saved_owner = NULL;
   int sum = 0;
   bool virtual_city = FALSE;
-  struct government *curr_govt = government_of_player(pplayer);
   bool handicap = ai_handicap(pplayer, H_MAP);
 
   pplayer->government = ai->goal.govt.gov;
@@ -132,6 +133,9 @@
 
   if (!pcity) {
     pcity = create_city_virtual(pplayer, result->tile, "Virtuaville");
+    saved_owner = tile_owner(result->tile);
+    tile_set_owner(result->tile, pplayer); /* temporarily */
+    city_choose_build_default(pcity);  /* ?? */
     virtual_city = TRUE;
   }
 
@@ -262,6 +266,7 @@
   pplayer->government = curr_govt;
   if (virtual_city) {
     destroy_city_virtual(pcity);
+    tile_set_owner(result->tile, saved_owner);
   }
 
   assert(result->city_center >= 0);
Index: client/packhand.c
===================================================================
--- client/packhand.c   (revision 14397)
+++ client/packhand.c   (working copy)
@@ -84,7 +84,8 @@
 **************************************************************************/
 static struct unit * unpackage_unit(struct packet_unit_info *packet)
 {
-  struct unit *punit = create_unit_virtual(player_by_number(packet->owner), 
NULL,
+  struct unit *punit = 
create_unit_virtual(valid_player_by_number(packet->owner),
+                                          NULL,
                                           utype_by_number(packet->type),
                                           packet->veteran);
 
@@ -143,7 +144,8 @@
 **************************************************************************/
 static struct unit *unpackage_short_unit(struct packet_unit_short_info *packet)
 {
-  struct unit *punit = create_unit_virtual(player_by_number(packet->owner), 
NULL,
+  struct unit *punit = 
create_unit_virtual(valid_player_by_number(packet->owner),
+                                          NULL,
                                           utype_by_number(packet->type),
                                           FALSE);
 
@@ -409,15 +411,33 @@
   struct universal product;
   int caravan_city_id;
   int i;
+  bool popup;
   bool city_is_new = FALSE;
   bool city_has_changed_owner = FALSE;
   bool need_units_dialog_update = FALSE;
-  bool popup, update_descriptions = FALSE, name_changed = FALSE;
+  bool name_changed = FALSE;
+  bool update_descriptions = FALSE;
   bool shield_stock_changed = FALSE;
   bool production_changed = FALSE;
   struct unit_list *pfocus_units = get_units_in_focus();
   struct city *pcity = game_find_city_by_number(packet->id);
+  struct tile *pcenter = map_pos_to_tile(packet->x, packet->y);
+  struct player *pplayer = valid_player_by_number(packet->owner);
 
+  if (NULL == pplayer) {
+    freelog(LOG_ERROR,
+           "handle_city_info() bad player number %d.",
+           packet->owner);
+    return;
+  }
+
+  if (NULL == pcenter) {
+    freelog(LOG_ERROR,
+            "handle_city_info() invalid tile (%d,%d).",
+            TILE_XY(packet));
+    return;
+  }
+
   if (pcity && (player_number(city_owner(pcity)) != packet->owner)) {
     client_remove_city(pcity);
     pcity = NULL;
@@ -441,14 +461,25 @@
     }
   }
 
-  if(!pcity) {
+  if (NULL == pcity) {
     city_is_new = TRUE;
-    pcity = create_city_virtual(player_by_number(packet->owner),
-                               map_pos_to_tile(packet->x, packet->y),
-                               packet->name);
-    pcity->id=packet->id;
+    pcity = create_city_virtual(pplayer, pcenter, packet->name);
+    tile_set_owner(pcenter, pplayer);
+    pcity->id = packet->id;
     idex_register_city(pcity);
     update_descriptions = TRUE;
+  } else if (pcity->id != packet->id) {
+    freelog(LOG_ERROR, "handle_city_info()"
+            " city id %d != id %d.",
+            pcity->id,
+            packet->id);
+    return;
+  } else if (city_tile(pcity) != pcenter) {
+    freelog(LOG_ERROR, "handle_city_info()"
+            " city tile (%d,%d) != (%d,%d).",
+            TILE_XY(city_tile(pcity)),
+            TILE_XY(packet));
+    return;
   } else {
     name_changed = (strcmp(city_name(pcity), packet->name) != 0);
 
@@ -467,11 +498,8 @@
         is likely to have changed as well. */
       update_descriptions = TRUE;
     }
-    assert(pcity->id == packet->id);
   }
   
-  pcity->owner = player_by_number(packet->owner);
-  pcity->tile = map_pos_to_tile(packet->x, packet->y);
   sz_strlcpy(pcity->name, packet->name);
   
   /* check data */
@@ -660,7 +688,9 @@
     pcity->info_units_supported = unit_list_new();
     pcity->info_units_present = unit_list_new();
 
-    tile_set_city(pcity->tile, pcity);
+    /* redundant to set_worker_city() in handle_city_info(),
+     * but needed for handle_city_short_info() */
+    tile_set_worked(pcity->tile, pcity); /* is_free_worked_tile() */
     city_list_prepend(city_owner(pcity)->cities, pcity);
 
     if (city_owner(pcity) == game.player_ptr) {
@@ -725,31 +755,57 @@
   bool city_is_new = FALSE;
   bool update_descriptions = FALSE;
   struct city *pcity = game_find_city_by_number(packet->id);
+  struct tile *pcenter = map_pos_to_tile(packet->x, packet->y);
+  struct player *pplayer = valid_player_by_number(packet->owner);
 
-  if (pcity && (player_number(city_owner(pcity)) != packet->owner)) {
+  if (NULL == pplayer) {
+    freelog(LOG_ERROR,
+           "handle_city_short_info() bad player number %d.",
+           packet->owner);
+    return;
+  }
+
+  if (NULL == pcenter) {
+    freelog(LOG_ERROR,
+            "handle_city_short_info() invalid tile (%d,%d).",
+            TILE_XY(packet));
+    return;
+  }
+
+  if (pcity && city_owner(pcity) != pplayer) {
     client_remove_city(pcity);
     pcity = NULL;
     city_has_changed_owner = TRUE;
   }
 
-  if (!pcity) {
+  if (NULL == pcity) {
     city_is_new = TRUE;
-    pcity = create_city_virtual(player_by_number(packet->owner),
-                               map_pos_to_tile(packet->x, packet->y),
-                               packet->name);
-    pcity->id=packet->id;
+    pcity = create_city_virtual(pplayer, pcenter, packet->name);
+    tile_set_owner(pcenter, pplayer);
+    pcity->id = packet->id;
     idex_register_city(pcity);
+  } else if (pcity->id != packet->id) {
+    freelog(LOG_ERROR, "handle_city_short_info()"
+            " city id %d != id %d.",
+            pcity->id,
+            packet->id);
+    return;
+  } else if (city_tile(pcity) != pcenter) {
+    freelog(LOG_ERROR, "handle_city_short_info()"
+            " city tile (%d,%d) != (%d,%d).",
+            TILE_XY(city_tile(pcity)),
+            TILE_XY(packet));
+    return;
   } else {
     /* Check if city desciptions should be updated */
-    if (draw_city_names && strcmp(city_name(pcity), packet->name) != 0) {
+    if (draw_city_names
+     && 0 != strncmp(packet->name, pcity->name, strlen(pcity->name))) {
       update_descriptions = TRUE;
     }
 
-    pcity->owner = player_by_number(packet->owner);
+    tile_set_owner(pcenter, pplayer);
     sz_strlcpy(pcity->name, packet->name);
     
-    assert(pcity->id == packet->id);
-
     memset(pcity->feel, 0, sizeof(pcity->feel));
     memset(pcity->specialists, 0, sizeof(pcity->specialists));
   }
@@ -1550,13 +1606,20 @@
 **************************************************************************/
 void handle_player_info(struct packet_player_info *pinfo)
 {
-  int i;
-  bool poptechup, new_tech = FALSE;
   char msg[MAX_LEN_MSG];
-  struct player *pplayer = &game.players[pinfo->playerno];
-  struct player_research* research;
   bool is_new_nation;
+  bool new_tech;
+  bool poptechup;
+  int i;
+  struct player_research *research;
+  struct player *pplayer = valid_player_by_number(pinfo->playerno);
 
+  if (NULL == pplayer) {
+    freelog(LOG_ERROR,
+           "handle_player_info() bad player number %d.",
+           pinfo->playerno);
+    return;
+  }
   sz_strlcpy(pplayer->name, pinfo->name);
 
   is_new_nation = player_set_nation(pplayer, nation_by_number(pinfo->nation));
@@ -1765,10 +1828,7 @@
   } else {
     /* Add or update the connection.  Note the connection may refer to
      * a player we don't know about yet. */
-    struct player *pplayer =
-      ((pinfo->player_num >= 0 
-        && pinfo->player_num < MAX_NUM_PLAYERS + MAX_NUM_BARBARIANS)
-       ? player_by_number(pinfo->player_num) : NULL);
+    struct player *pplayer = player_by_number(pinfo->player_num);
     
     if (!pconn) {
       freelog(LOG_VERBOSE, "Server reports new connection %d %s",
@@ -1986,9 +2046,16 @@
 void handle_spaceship_info(struct packet_spaceship_info *p)
 {
   int i;
-  struct player *pplayer = &game.players[p->player_num];
-  struct player_spaceship *ship = &pplayer->spaceship;
-  
+  struct player_spaceship *ship;
+  struct player *pplayer = valid_player_by_number(p->player_num);
+
+  if (NULL == pplayer) {
+    freelog(LOG_ERROR,
+           "handle_spaceship_info() bad player number %d.",
+           p->player_num);
+    return;
+  }
+  ship = &pplayer->spaceship;
   ship->state        = p->sship_state;
   ship->structurals  = p->structurals;
   ship->components   = p->components;
@@ -2040,6 +2107,7 @@
   enum known_type old_known;
   bool known_changed = FALSE;
   bool tile_changed = FALSE;
+  struct player *powner = valid_player_by_number(packet->owner);
   struct terrain *pterrain = terrain_by_number(packet->type);
   struct tile *ptile = map_pos_to_tile(packet->x, packet->y);
   
@@ -2093,18 +2161,9 @@
   /* always called after setting terrain */
   tile_set_resource(ptile, resource_by_number(packet->resource));
 
-  if (packet->owner == MAP_TILE_OWNER_NULL) {
-    if (tile_owner(ptile)) {
-      tile_set_owner(ptile, NULL);
-      tile_changed = TRUE;
-    }
-  } else {
-    struct player *newowner = player_by_number(packet->owner);
-
-    if (tile_owner(ptile) != newowner) {
-      tile_set_owner(ptile, newowner);
-      tile_changed = TRUE;
-    }
+  if (tile_owner(ptile) != powner) {
+    tile_set_owner(ptile, powner);
+    tile_changed = TRUE;
   }
   if (old_known != packet->known) {
     known_changed = TRUE;
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to