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

[Kevin Brown <kevin@xxxxxxxxxxxxxx>]

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.

> > 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.

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!).

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?


> > * 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.

> 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.


-- 
Kevin Brown                                           kevin@xxxxxxxxxxxxxx

    It's really hard to define what "unexpected behavior" means when you're
                       talking about Windows.