[Freeciv-Dev] Re: GTK client draws more slowly than it needs to (PR#899)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Aug 20, 2001 at 02:29:36PM -0700, Kevin Brown wrote:
> Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > How large will the cache get, if it doesn't get pruned? It should be
> > possible to prune a hash if it gets above a certain limit.
> 
> The cache is fixed in size, set either by default to 1024 entries or
> whatever you specify with --tile-cache-size.

I know. But what happens if you set the size really high like 100000
and play a bit. How much cache entries will be used?

> > > So my code has to do the same thing, and the hash value (which we mod
> > > against the cache size to get an index) also has to reflect the values
> > > of fill_bg and pplayer (since I'm assuming that these things can be
> > > changed at runtime) since the code that this is replacing alters its
> > > behavior based on these values.
> > 
> > _I_ would still use something like
> > 
> > struct key
> > {
> >    int fill_bg;
> >    struct player *pplayer;
> >    struct sprite *sprites[MAX_SPRITES];
> > };
> > 
> > This will just made is explicit what a key consists of. You can now
> > define a hash and a compare method. This all assumes using a hash.
> 
> I guess I could do that.  But note that, based upon my reading of the
> isometric equivalent of pixmap_put_tile, the key will be quite
> different for isometric view versus top-down view.

I have looked only at the non isometric part. 

> Most importantly, however, is that how the sprites get combined
> depends on some of these values, so it's not just a straightforward
> tile combination right now (wish it were!).

So leave out isometric in the first version.

> That being the case, it's not clear how much of a benefit I'd get from
> storing the key.  Right now I just store the hash value (prior to the
> mod) itself in the cache along with the tile so that if there's a
> different hash value that gets me the same slot, I can detect the
> collision.
> 
> And what should MAX_SPRITES be set to?  The number of sprites that
> build up a tile can potentially be quite large.  

> Each unit you put onto a tile increases the number of sprites
> associated with it, right?

No. From client/tilespec.c:fill_tile_sprite_array():

    punit = get_drawable_unit(abs_x0, abs_y0, citymode);
    if (punit && (draw_units || (draw_focus_unit && pfocus == punit))) {
      sprs += fill_unit_sprite_array(sprs, punit, solid_bg);

Overall the amount depends which sprites you collapse. The sprites of
a single unit are limited. Replace it with a dynamic NULL terminated
list if you don't like MAX_SPRITES thing.

> > > * We need to be able to deal with 'sprite' and 'other' being different
> > >   sizes, especially in conjunction with NULL masks.  Except for the
> > >   case of NULL masks, it looks like this is already partially taken
> > >   care of by merge_sprites().  But what happens if 'sprite' is smaller
> > >   than 'other'?
> > 
> > This case didn't happen in my code. At least I hope so, because this
> > case isn't handled at all. Does it happen in your testing? 
> 
> Yes.  In particular, it happens on any tile on which a moveable unit
> exists, since units are different in size than the tiles upon which
> they reside.
> 
> > > I need to make sure that my understanding of masks is correct.  The
> > > mask is a bitmap where 0 corresponds to a pixel that is NOT drawn, and
> > > 1 corresponds to a pixel that is drawn, correct?  If so, then a NULL
> > > mask means that the sprite is expected to completely overwrite the
> > > part of the tile it would occupy, so its mask is all 1s for the
> > > purpose of the merge.
> > 
> > AFAIK correct.
> 
> OK, excellent.  I'll proceed then.
> 
> > > So here's how I think the merge should work:
> > > 
> > > 1.  Instead of merging one sprite onto the other, perhaps it should
> > >     return a newly created sprite?  If so, then instead of just taking
> > >     two sprites, it should take a NULL-terminated array of sprite
> > >     pointers, or an array of pointers with a count.
> > 
> > Take a look at get_unit_sprite(). Yes some method has to perform the
> > allocation. However my intend was to encapsulate the gui-specific code
> > in merge_sprites and leave the rest to other methods.
> 
> If we don't do all the sprites at once, then multiple calls have to be
> made to merge_sprites(), which means that allocation of resources on
> the X server (of GCs if nothing else) happens every time.  Depending
> on the situation, this could slow things down considerably.

I still like the idea of encapsulate the gui-specific code in a
method. The GC and other stuff can be done at some init time or as
static method variables.

> > The canvas idea sounds good. This won't make merge_sprites more
> > complicated as it should. Lets call the new method collapse_sprites()
> > or something.
> 
> I'll call it combine_sprites().  :-)
> 
> > >     f.  A microoptimization to the mask handling is if, for each
> > >         sprite with a NULL mask that we encounter, we remember its
> > >         dimensions.  If, at the end, one the sprites with NULL masks
> > >         has the same dimensions as the tile we just built, then we
> > >         free up the mask we built and set it to NULL, so that code
> > >         which uses what we return can just draw it directly without a
> > >         mask (which could be a significant speed improvement in the
> > >         drawing phase for all I know).
> > 
> > I want to first know how common the case of different sized sprite
> > is.
> 
> It's not terribly common, but it does happen.  It happens often enough
> that I noticed drawing artifacts when I used my tile cache for tiles
> built from differing-sized sprites.  It's why I started thinking about
> how to combine masks.

I would like to see such a all-in-one combine_sprites to compare the
size and extra complexity to merge_sprite.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "These download files are in Microsoft Word 6.0 format. After
  unzipping, these files can be viewed in any text editor, including
  all versions of Microsoft Word, WordPad, and Microsoft Word Viewer."
    -- http://www.microsoft.com/hwdev/pc99.htm