Complete.Org: Mailing Lists: Archives: freeciv-dev: March 2002:
[Freeciv-Dev] mapview doesn't update properly when we get a city update
Home

[Freeciv-Dev] mapview doesn't update properly when we get a city update

[Top] [All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
To: freeciv-dev@xxxxxxxxxxx
Cc: bugs@xxxxxxxxxxxxxxxxxxx
Subject: [Freeciv-Dev] mapview doesn't update properly when we get a city update (PR#1303)
From: jdorje@xxxxxxxxxxxxxxxxxxxxx
Date: Tue, 5 Mar 2002 21:54:19 -0800 (PST)

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);
   }

[Prev in Thread] Current Thread [Next in Thread]