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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Aug 20, 2001 at 03:22:53AM -0700, Kevin Brown wrote:
> rf13@xxxxxxxxxxxxxxxxxxxxxx <rf13@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> > Looking at the code I noticed some things:
> >  - take a look at common/hash.[ch]
> 
> Call me stupid, but it's not quite obvious to me how to make these
> work with an array of pointers as the key to be hashed.  More
> importantly, it's not obvious at all how to make it work with the kind
> of key I'm using (see below).
> 
> Additionally, this is a cache.  We don't want to store every possible
> sprite combination that ends up being laid down onto the map, we want
> to replace elements when the cache fills up.  I don't know if the
> stuff in hash.c behaves this way or not (probably not, its purpose is
> different).

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.

> >  - you should say what is key and what is hash in the cache
> 
> Okay, I'll fix that.
> 
> >  - _I_ would make the key a NULL terminated array of pointers to
> >  sprites. This would also make sure you can be sure that you got the
> >  correct entry.
> 
> This is more or less what I do.  What the code that this is supposed
> to replace (in pixmap_put_tile) actually does is get a list of sprites
> that are supposed to be combined, and then combine them.  How they get
> combined depends on the values of fill_bg and pplayer.  The code
> doesn't seem to assume a NULL terminated array of pointers, but rather
> gets a count of entries from fill_tile_sprite_array.  If one of them
> is NULL it skips it, but doesn't terminate the loop.
> 
> 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.

> >  - you may take a look at the sma3.diff.gz in ftp.freeciv.org
> >  especially the merge_sprites() method and the changes I made to
> >  tilespec.[ch]. I hope to get this included in maintree. This could
> >  make the cache benefitting all GUIs.
> 
> Sweet!!!  Looks like a good head-start on the problem of dealing with
> the masks.  A couple of things:
> 
> * We need to be able to deal with 'sprite' and 'other' having NULL
>   masks.

Feel free to add code.

> * 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? I would
expand sprite in this case.

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

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

> 2.  Things get interesting if the tiles aren't of the same size.
>     Rather than having code deal with special cases, we generalize
>     thusly:
> 
>     a.  Create a canvas sprite (which I'll refer to as "the canvas")
>       with dimensions x = max(x[0] ... x[n]), y = max(y[0] ... y[n]).
> 
>     b.  Set the mask of the canvas to all zeros.
> 
>     c.  Draw each sprite onto the canvas using the sprite's mask, or
>       directly if there is none.
> 
>     d.  Draw each sprite's mask onto the canvas mask using the
>         sprite's mask as the mask (as you've done in the current
>         implementation of merge_sprite()) or draw a rectangle of 1's
>         with the sprite's dimensions onto the canvas mask if the
>         sprite doesn't have a mask.
> 
>     e.  We're done!  Return the newly-formed canvas sprite.

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.

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

> This sound right?  If so, I'll make my tile cache do this and play
> with it, perhaps change the isometric code to make use of it too...

Go ahead.

> > > +  tile_cache = malloc(tile_cache_size * sizeof(*tile_cache));
> > > +  if (tile_cache == NULL) {
> > 
> > fc_malloc will handle this.
> 
> Okay, I'll use it.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "#!/usr/bin/perl -w
  if ( `date +%w` != 1 ) {
    die "This script only works on Mondays." ;
  }"
    -- from chkars.pl by Cornelius Krasel in de.comp.os.linux.misc