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

[Kevin Brown <kevin@xxxxxxxxxxxxxx>]

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

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

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

* 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'?

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.


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.

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.

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

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

> > +  tile_cache = malloc(tile_cache_size * sizeof(*tile_cache));
> > +  if (tile_cache == NULL) {
> 
> fc_malloc will handle this.

Okay, I'll use it.


-- 
Kevin Brown                                           kevin@xxxxxxxxxxxxxx

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