[Freeciv-Dev] Re: Client crash when t on land worked by hidden city (PR#1198)

[Jason Short <vze2zq63@xxxxxxxxxxx>]

dspeyer@xxxxxxxxxxxxxxxxxxxxx wrote:


> If you use the 't' key on any tile which is being worked by an invisible (enemy)
> city, the client will crash with a bad assertion:
>
> civclient: mapview_common.c:296: find_city_near_tile: Assertion
> `get_worker_city
> (pcity, 5 - 1 - city_x, 5 - 1 - city_y) == C_TILE_EMPTY' failed.
> Aborted
This "feature" was introduced by me - when I fixed find_city_near_tile, 
I removed this check and changed it into an assertion because I didn't 
think it was possible for it to occur.
The previous code can be seen here: 
http://www.freeciv.org/lxr/source/client/gui-gtk/mapctrl.c#L339.

> I think this patch should fix it:
>
> --- freeciv-10-23/client/mapview_common.c       Sat Dec 22 02:06:08 2001
> +++ freeciv/client/mapview_common.c     Thu Jan  3 20:59:39 2002
> @@ -271,6 +271,7 @@
>          "looked at".
>      e.  If none of the cities were looked at last, choose "randomly".
>      f.  If no cities can work it, return NULL.
> +    g.  If an invisible (enemy) city is working it, return NULL
>  **************************************************************************/
>  struct city *find_city_near_tile(int x, int y)
>  {
> @@ -292,9 +293,11 @@
>    city_map_checked_iterate(x, y, city_x, city_y, map_x, map_y) {
>      pcity = map_get_city(map_x, map_y);
>      if (pcity && pcity->owner == game.player_idx) {
> +      /* rule g */
> +      if (get_worker_city(pcity, CITY_MAP_SIZE - 1 - city_x,
> +                            CITY_MAP_SIZE - 1 - city_y) != C_TILE_EMPTY)
> +       return(NULL);
>        /* rule c */
> -      assert(get_worker_city(pcity, CITY_MAP_SIZE - 1 - city_x,
> -                            CITY_MAP_SIZE - 1 - city_y) == C_TILE_EMPTY);
>        if (pcity == last_pcity) {
>         return pcity;           /* rule d */
>        }
This is different from the original code, and I think the original is 
more correct (although both should work).
As I understand it, a city tile will be marked C_TILE_UNAVAILABLE if any 
other city is working it.  In your version, we immediately return NULL 
any time any city has the tile marked as unavailable, whereas in the 
original if a city has the tile marked unavailable, we just skip that 
city.  Should the code change so that the tile may be available for some 
cities but not all, the original version should continue to work. 
(Also, note we don't really need an explicity rule G, as this falls 
under rule C.)
Patch attached.  Either one of these should work, I think, but one of 
them needs to be applied to fix the problem.
jason

Index: client/mapview_common.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.c,v
retrieving revision 1.4
diff -u -r1.4 mapview_common.c
--- client/mapview_common.c     2001/12/21 16:53:10     1.4
+++ client/mapview_common.c     2002/01/04 07:10:47
@@ -291,10 +291,16 @@
   pcity2 = NULL;               /* rule f */
   city_map_checked_iterate(x, y, city_x, city_y, map_x, map_y) {
     pcity = map_get_city(map_x, map_y);
-    if (pcity && pcity->owner == game.player_idx) {
+    if (pcity && pcity->owner == game.player_idx
+        && get_worker_city(pcity, CITY_MAP_SIZE - 1 - city_x,
+                          CITY_MAP_SIZE - 1 - city_y) == C_TILE_EMPTY) {
       /* rule c */
-      assert(get_worker_city(pcity, CITY_MAP_SIZE - 1 - city_x,
-                            CITY_MAP_SIZE - 1 - city_y) == C_TILE_EMPTY);
+      /*
+       * Note, we must explicitly check if the tile is workable (with
+       * get_worker_city(), above) since it is possible that another city
+       * (perhaps an unseen enemy city) may be working it, causing it to
+       * be marked as C_TILE_UNAVAILABLE.
+       */
       if (pcity == last_pcity) {
        return pcity;           /* rule d */
       }