[Freeciv-Dev] (PR#12149) drawing is really slow

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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

The new fog code makes drawing really slow.  The main reason for this is 
that refresh_tile_mapcanvas() has to draw about 4x as much.  While this 
function should rarely be used (individual refreshes are rare) it is in 
actuality used often (because groups of refreshes are instead done one 
by one).  This makes things 4x-20x slower I suspect.

I ran some tests.  In each I started a game with default settings, 
maximized my civclient (my resolution is 1280x1024) then disbanded all 
my units.  At this point the server sends the client all tiles, one at a 
time.  In the current code the client calls refresh_tile_mapcanvas once 
for each of these packets.  This has always been very slow.  The 
flush_dirty() code makes it a little prettier since the packets aren't 
redrawn one-at-a-time, but doesn't really make it any faster.

So, I timed this process:

* CVS from one day ago: 8s.
* CVS from right now (with new fog and new refresh_tile_mapcanvas): 25s
* CVS from right now, with the refresh reverted (rev 1.176): 15s
* CVS from right now, with the fog reverted (rev 1.26): 13s

Now, how can this be improved?  Well we should follow a similar strategy 
to how the dirty/flush system works.  When an update is done (usually 
via refresh_tile_mapcanvas; occasionally directly through 
update_map_canvas)  we don't want to do the drawing immediately. 
Instead the areas to be drawn are stored.  Later when we have some free 
time or need to do animation we uqueue this drawing and do it all at 
once, with as little repitition as possible.

This patch is targeted toward handle_tile_info, but can be extended to 
work for just about everything else in packhand.c as well.  Looking at 
handle_tile_info the current situation is even worse than I thought.  A 
tile packet can cause up to 13 refresh_tile_mapcanvas calls (this is, I 
think, supposed to simulate the area-of-effect draw that 
refresh_tile_mapcanvas already handles and can now safely be removed). 
Each refresh_tile_mapcanvas call causes 13 tiles (in iso-view) to be 
redrawn.  So...it's a bit inefficient!!!

With this patch each of those calls (I didn't remove the extra ones yet; 
I'll look into that later) instead just queues up the tile for 
redrawing.  Later in the unqueue we loop over all tiles that need 
updating and take the bounding box of all of them.  This is the area 
that's updated.  Note that the area of redrawing is stored by tile not 
by canvas or GUI position, since the latter will break if the mapview is 
recentered in between the queue and unqueue.

* CVS from right now, with queue.diff: 1s

Most of this 1s I think is taken up inside the overview_tile_update() 
calls.  Initially I forgot this call (it's also called inside 
refresh_tile_mapcanvas) so the overview wasn't redrawn when I ran my 
little experiment.  However this caused the redrawing to be basically 
instantaneous (< 0.4s).  The way it is in the patch there is no 
acceleration over the old method, so this can definitely be improved.

I had to add a few more unqueue_mapview_updates() calls.  Any time we do 
animation we first have to make sure all drawing is up-to-date. 
Currently most places just do flush_dirty() to do this.  However the 
correct way to do it (even before the patch) is with 
unqueue_mapview_updates().  flush_dirty() should only be used inside 
loops, after unqueue_mapview_updates() has already been run.

Finally, I only changed handle_tile_info.  There are surely many other 
parts of packhand.c that can be improved.

-jason

? fog
? fog.c
? fog.png
? foo
? data/isotrident/grid.png
? data/isotrident/grid.xcf
Index: client/mapview_common.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.c,v
retrieving revision 1.177
diff -u -r1.177 mapview_common.c
--- client/mapview_common.c     6 Feb 2005 23:36:20 -0000       1.177
+++ client/mapview_common.c     7 Feb 2005 07:58:17 -0000
@@ -517,6 +517,8 @@
     gui_distance_vector(&diff_x, &diff_y, start_x, start_y, gui_x0, gui_y0);
     anim_timer = renew_timer_start(anim_timer, TIMER_USER, TIMER_ACTIVE);
 
+    unqueue_mapview_updates();
+
     do {
       mytime = MIN(read_timer_seconds(anim_timer), timing_sec);
 
@@ -1268,7 +1270,7 @@
   dirty_rect(canvas_x, canvas_y, width, height);
 
   /* Make sure everything is flushed and synced before proceeding. */
-  flush_dirty();
+  unqueue_mapview_updates();
   gui_flush();
 
   myusleep(1000000);
@@ -1803,6 +1805,7 @@
 
   set_units_in_combat(punit0, punit1);
 
+  unqueue_mapview_updates();
   while (punit0->hp > hp0 || punit1->hp > hp1) {
     const int diff0 = punit0->hp - hp0, diff1 = punit1->hp - hp1;
 
@@ -1891,6 +1894,7 @@
     return;
   }
 
+  unqueue_mapview_updates();
   if (tile_visible_mapcanvas(src_tile)
       || tile_visible_mapcanvas(dest_tile)) {
     int start_x, start_y;
@@ -2077,6 +2081,9 @@
 }
 
 static enum update_type needed_updates = UPDATE_NONE;
+struct city_list *city_updates = NULL;
+struct unit_list *unit_updates = NULL;
+struct tile_list *tile_updates = NULL;
 
 /**************************************************************************
   This function, along with unqueue_mapview_update(), helps in updating
@@ -2101,6 +2108,45 @@
 }
 
 /**************************************************************************
+  Queue this tile to be refreshed.  The refresh will be done some time
+  soon thereafter, and grouped with other needed refreshes.
+
+  Note this should only be called for tiles.  For cities or units use
+  queue_mapview_xxx_update instead.
+**************************************************************************/
+void queue_mapview_tile_update(struct tile *ptile)
+{
+  if (!tile_updates) {
+    tile_updates = tile_list_new();
+  }
+  tile_list_prepend(tile_updates, ptile);
+}
+
+/**************************************************************************
+  Queue this unit to be refreshed.  The refresh will be done some time
+  soon thereafter, and grouped with other needed refreshes.
+**************************************************************************/
+void queue_mapview_unit_update(struct unit *punit)
+{
+  if (!unit_updates) {
+    unit_updates = unit_list_new();
+  }
+  unit_list_prepend(unit_updates, punit);
+}
+
+/**************************************************************************
+  Queue this city to be refreshed.  The refresh will be done some time
+  soon thereafter, and grouped with other needed refreshes.
+**************************************************************************/
+void queue_mapview_city_update(struct city *pcity)
+{
+  if (!city_updates) {
+    city_updates = city_list_new();
+  }
+  city_list_prepend(city_updates, pcity);
+}
+
+/**************************************************************************
   See comment for queue_mapview_update().
 **************************************************************************/
 void unqueue_mapview_updates(void)
@@ -2108,10 +2154,87 @@
   freelog(LOG_DEBUG, "unqueue_mapview_update: needed_updates=%d",
          needed_updates);
 
-  if (needed_updates & UPDATE_MAP_CANVAS_VISIBLE) {
+  if ((needed_updates & UPDATE_MAP_CANVAS_VISIBLE)
+      || (needed_updates & UPDATE_CITY_DESCRIPTIONS)) {
     update_map_canvas_visible();
-  } else if (needed_updates & UPDATE_CITY_DESCRIPTIONS) {
-    update_city_descriptions();
+  } else {
+    int min_x = mapview_canvas.width, min_y = mapview_canvas.height;
+    int max_x = 0, max_y = 0;
+
+    if (tile_updates) {
+      tile_list_iterate(tile_updates, ptile) {
+       int x0, y0, x1, y1;
+
+       if (tile_to_canvas_pos(&x0, &y0, ptile)) {
+         x0 -= NORMAL_TILE_WIDTH / 2;
+         y0 -= NORMAL_TILE_HEIGHT / 2;
+         x1 = x0 + 2 * NORMAL_TILE_WIDTH;
+         y1 = y0 + 2 * NORMAL_TILE_HEIGHT;
+         min_x = MIN(min_x, x0);
+         min_y = MIN(min_y, y0);
+         max_x = MAX(max_x, x1);
+         max_y = MAX(max_y, y1);
+       }
+
+       /* FIXME: These overview updates should be batched as well.  Right
+        * now they account for as much as 90% of the runtime of the
+        * unqueue. */
+       overview_update_tile(ptile);
+      } unit_list_iterate_end;
+    }
+
+    if (unit_updates) {
+      unit_list_iterate(unit_updates, punit) {
+       int x0, y0, x1, y1;
+
+       if (tile_to_canvas_pos(&x0, &y0, punit->tile)) {
+         y0 += NORMAL_TILE_HEIGHT - UNIT_TILE_HEIGHT;
+         x1 = x0 + UNIT_TILE_WIDTH;
+         y1 = y0 + UNIT_TILE_HEIGHT;
+         min_x = MIN(min_x, x0);
+         min_y = MIN(min_y, y0);
+         max_x = MAX(max_x, x1);
+         max_y = MAX(max_y, y1);
+       }
+
+       /* FIXME: These overview updates should be batched as well.  Right
+        * now they account for as much as 90% of the runtime of the
+        * unqueue. */
+       overview_update_tile(punit->tile);
+      } unit_list_iterate_end;
+    }
+
+    if (city_updates) {
+      city_list_iterate(city_updates, pcity) {
+       int x0, y0, x1, y1;
+
+       if (tile_to_canvas_pos(&x0, &y0, pcity->tile)) {
+         y0 += NORMAL_TILE_HEIGHT - UNIT_TILE_HEIGHT;
+         x1 = x0 + UNIT_TILE_WIDTH;
+         y1 = y0 + UNIT_TILE_HEIGHT;
+         min_x = MIN(min_x, x0);
+         min_y = MIN(min_y, y0);
+         max_x = MAX(max_x, x1);
+         max_y = MAX(max_y, y1);
+       }
+
+       /* FIXME: These overview updates should be batched as well.  Right
+        * now they account for as much as 90% of the runtime of the
+        * unqueue. */
+       overview_update_tile(pcity->tile);
+      } unit_list_iterate_end;
+    }
+
+    update_map_canvas(min_x, min_y, max_x - min_x, max_y - min_y);
+  }
+  if (tile_updates) {
+    tile_list_unlink_all(tile_updates);
+  }
+  if (unit_updates) {
+    unit_list_unlink_all(unit_updates);
+  }
+  if (city_updates) {
+    city_list_unlink_all(city_updates);
   }
   needed_updates = UPDATE_NONE;
 
Index: client/mapview_common.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/mapview_common.h,v
retrieving revision 1.87
diff -u -r1.87 mapview_common.h
--- client/mapview_common.h     5 Feb 2005 19:26:38 -0000       1.87
+++ client/mapview_common.h     7 Feb 2005 07:58:17 -0000
@@ -288,6 +288,9 @@
                                      enum color_std *grwoth_color);
 
 void queue_mapview_update(enum update_type update);
+void queue_mapview_tile_update(struct tile *ptile);
+void queue_mapview_unit_update(struct unit *punit);
+void queue_mapview_city_update(struct city *pcity);
 void unqueue_mapview_updates(void);
 
 void map_to_overview_pos(int *overview_x, int *overview_y,
Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.463
diff -u -r1.463 packhand.c
--- client/packhand.c   4 Feb 2005 23:00:02 -0000       1.463
+++ client/packhand.c   7 Feb 2005 07:58:18 -0000
@@ -1975,14 +1975,14 @@
   if (can_client_change_view()) {
     /* the tile itself */
     if (tile_changed || old_known!=ptile->known)
-      refresh_tile_mapcanvas(ptile, FALSE);
+      queue_mapview_tile_update(ptile);
 
     /* if the terrain or the specials of the tile
        have changed it affects the adjacent tiles */
     if (tile_changed) {
       adjc_iterate(ptile, tile1) {
        if (tile_get_known(tile1) >= TILE_KNOWN_FOGGED)
-         refresh_tile_mapcanvas(tile1, FALSE);
+         queue_mapview_tile_update(tile1);
       }
       adjc_iterate_end;
       return;
@@ -1993,7 +1993,7 @@
     if (old_known == TILE_UNKNOWN && packet->known >= TILE_KNOWN_FOGGED) {     
       cardinal_adjc_iterate(ptile, tile1) {
        if (tile_get_known(tile1) >= TILE_KNOWN_FOGGED)
-         refresh_tile_mapcanvas(tile1, FALSE);
+         queue_mapview_tile_update(tile1);
       } cardinal_adjc_iterate_end;
     }
   }
Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.233
diff -u -r1.233 map.h
--- common/map.h        22 Jan 2005 19:45:43 -0000      1.233
+++ common/map.h        7 Feb 2005 07:58:18 -0000
@@ -55,6 +55,14 @@
   char *spec_sprite;
 };
 
+/* get 'struct tile_list' and related functions: */
+#define SPECLIST_TAG tile
+#define SPECLIST_TYPE struct tile
+#include "speclist.h"
+
+#define tile_list_iterate(tile_list, ptile) \
+    TYPED_LIST_ITERATE(struct tile, tile_list, ptile)
+#define tile_list_iterate_end  LIST_ITERATE_END
 
 /****************************************************************
 miscellaneous terrain information