[Freeciv-Dev] mapview doesn't update properly when we get a city update (PR#1303)

[jdorje@xxxxxxxxxxxxxxxxxxxxx]

Mike pointed this one out to me a little while ago, but the best fix I 
can come up with isn't very good.  Nonetheless, it's a pretty 
significant bug.
The problem is that any time a city update is sent, if the map grid is 
enabled the packet code (in client/packhand.c) calls a 
update_map_canvas() to redraw the city map.  This is sometimes 
necessary: if the citizens have moved the red-white grid will be drawn 
differently.  But, update_map_canvas does not redraw the city 
descriptions.  This means at a minimum that the updated city's 
description will be overdrawn, and may mean that other cities' 
descriptions are overdrawn too.
At end-of-turn, all cities seem to be updated from the server (no doubt 
this is usually one of those "unnecessary updates" Raimar is planning to 
get rid of), so all city descriptions vanish.  Ouch!
It is not enough just to call show_desc_at_tile when we update the city. 
  Descriptions of other cities may be overdrawn as well - in fact those 
other cities might not even lie within the city map for the updated 
city; only their descriptions have to.  This would be difficult to 
implement in any case, since show_desc_at_tile is a statically declared 
GUI-dependent function that does not exist for all GUIs.
We could call show_city_descriptions on these updates, but that would be 
extremely wasteful.  In particular, at end-of-turn if we get updates for 
100 cities it doesn't make much sense to call show_city_descriptions 100 
times.
The fundamental problem here is a problem in the design of the drawing 
system: tiles (including the map grid, etc) are drawn with one of the 
slew of put_***_tile*** functions (which all draw a _single_ tile), 
while city names are drawn with the show_city_descriptions() function 
(which draws _all_ city descriptions).  Most tile updating is done 
through update_map_canvas_visible(), which draws the city descriptions 
as you'd expect (albeit with a bit of flicker).  But a fair amount is 
done outside, and this usually doesn't call show_city_descriptions(). 
(For instance, moving units and nuke mushrooms do not update the city 
descriptions.)

I see two possible solutions.

The easy one (patch attached) doesn't call the updating code immediately 
when the city update is received, but queues it for later redrawing. 
When we're ready to leave the packet-reading code, if there are any 
queued updates we just redraw the _entire_ visible mapview, and all city 
descriptions.  (Although we do redraw the entire mapview, which isn't 
always necessary, it's probably still more efficient since we'll never 
redraw a single tile more than once.)
The harder, but more correct, one would hold the city descriptions (and 
any other "floating" graphics) in a separate pixmap.  Then any time we 
redraw a tile, we can just redraw the floating-graphics part of that 
tile over it.  This would require some changes to the structure of the 
drawing code, and would also involve having a masked pixmap (which may 
not be possible under all GUIs?).  But it would (help) clean up a lot of 
other problems, too (like the city descriptions being overdrawn by 
moving units, city description updates sometimes overwriting themselves, 
etc.).
jason

Index: client/clinet.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/clinet.c,v
retrieving revision 1.67
diff -u -r1.67 clinet.c
--- client/clinet.c     2002/03/05 15:46:19     1.67
+++ client/clinet.c     2002/03/06 05:48:52
@@ -332,6 +332,8 @@
   } else {
     close_socket_callback(&aconnection);
   }
+
+  unqueue_mapview_update();
 }
 
 /**************************************************************************
@@ -380,6 +382,8 @@
       close_socket_callback(&aconnection);
     }
   }
+
+  unqueue_mapview_update();
 }
 
 #ifdef WIN32_NATIVE
Index: client/mapview_common.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.c,v
retrieving revision 1.11
diff -u -r1.11 mapview_common.c
--- client/mapview_common.c     2002/02/26 19:57:08     1.11
+++ client/mapview_common.c     2002/03/06 05:48:52
@@ -350,3 +350,38 @@
     }
   }
 }
+
+static bool need_mapview_update = FALSE;
+
+/**************************************************************************
+  This function, along with unqueue_mapview_update(), helps in updating
+  the mapview when a packet is received.  Previously, we just called
+  update_map_canvas when (for instance) a city update was received.
+  Not only would this often end up with a lot of duplicated work, but it
+  would also draw over the city descriptions, which would then just
+  "disappear" from the mapview.  The hack is to instead call
+  queue_mapview_update in place of this update, and later (after all
+  packets have been read) call unqueue_mapview_update.  The functions
+  don't track which areas of the screen need updating, rather when the
+  unqueue is done we just update the whole visible mapqueue, and redraw
+  the city descriptions.
+
+  Using these functions, updates are done correctly, and are probably
+  faster too.  But it's a bit of a hack to insert this code into the
+  packet-handling code.
+**************************************************************************/
+void queue_mapview_update(void)
+{
+  need_mapview_update = TRUE;
+}
+
+/**************************************************************************
+  See comment for queue_mapview_update().
+**************************************************************************/
+void unqueue_mapview_update(void)
+{
+  if (need_mapview_update) {
+    update_map_canvas_visible();
+    need_mapview_update = FALSE;
+  }
+}
Index: client/mapview_common.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.h,v
retrieving revision 1.5
diff -u -r1.5 mapview_common.h
--- client/mapview_common.h     2002/02/26 10:27:37     1.5
+++ client/mapview_common.h     2002/03/06 05:48:52
@@ -65,4 +65,7 @@
 void get_city_mapview_production(struct city *pcity,
                                  char *buf, size_t buf_len);
 
+void queue_mapview_update(void);
+void unqueue_mapview_update(void);
+
 #endif /* FC__MAPVIEW_COMMON_H */
Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.225
diff -u -r1.225 packhand.c
--- client/packhand.c   2002/03/06 03:03:04     1.225
+++ client/packhand.c   2002/03/06 05:48:53
@@ -515,12 +515,7 @@
   }
 
   if (draw_map_grid && get_client_state() == CLIENT_GAME_RUNNING_STATE) {
-    int r = ((CITY_MAP_SIZE + 1) / 2);
-    int d = (2 * r) + 1;
-    int x = pcity->x - r;
-    int y = pcity->y - r;
-    normalize_map_pos(&x, &y);
-    update_map_canvas(x, y, d, d, TRUE);
+    queue_mapview_update();
   } else {
     refresh_tile_mapcanvas(pcity->x, pcity->y, TRUE);
   }