[Freeciv-Dev] Re: (PR#3457) New sprite loading

["Raimar Falke" <rf13@xxxxxxxxxxxxxxxxx>]

On Sun, Mar 02, 2003 at 01:15:41AM -0800, Jason Short wrote:
> Raimar Falke wrote:
> > 
> > I wrote a nice explanation of the patch but than I hit 'q' in the mail
> > reader. ARGL. So here is the very short version:
> > 
> > Problems:
> >  - special case for not permanently loaded/used graphics needed
> >  - gui-sdl need sprites which other guis doesn't want to load
> > 
> > Solution has three functions load_sprite, unload_sprite and
> > finish_loading. See code for usage.
> 
> Sorry I haven't looked at this yet.
> 
> In general it looks pretty good.  But a few questions:

Keep in mind that this is a first version.

> > diff -Nurd -X clean/diff_ignore clean/client/gui-gtk-2.0/graphics.c 
> > tileset/client/gui-gtk-2.0/graphics.c
> > --- clean/client/gui-gtk-2.0/graphics.c     Mon Feb 17 19:32:12 2003
> > +++ tileset/client/gui-gtk-2.0/graphics.c   Mon Feb 17 20:49:44 2003
> > @@ -114,8 +114,8 @@
> 
> Why do you only make changes to gui-gtk-2.0?  How will other GUIs load 
> the intro sprites?  Answer: they won't.  Gui-gtk won't even compile.

At this time I had gtk2 autoconfigure'd.

> When I run it under gui-gtk-2.0, the client segfaults.

Any idea why? A backtrace?

> > +#define small_sprite_list_iterate(list, pitem) \
> > +    TYPED_LIST_ITERATE(struct small_sprite, list, pitem)
> > +#define small_sprite_list_iterate_end  LIST_ITERATE_END
> 
> Shouldn't LIST_ITERATE_END be TYPED_LIST_ITERATE_END?

There is no TYPED_LIST_ITERATE_END.

> > +    while (section_file_lookup(file, "%s.tiles%d.tag", gridnames[i], ++j)) 
> > {
> 
> > +      tags = secfile_lookup_str_vec(file, &num_tags, "%s.tiles%d.tag",
> > +                               gridnames[i], j);
> > +
> > +      /* there must be at least 1 because of the while(): */
> > +      assert(num_tags > 0);
> 
> Are you sure?  What if the tag contains garbage instead of a str_vec?

This code isnt't from me. I just moved it around.

> > +   if (!hash_insert(sprite_hash, mystrdup(tags[k]), ss)) {
> > +     die("already have a sprite for %s", tags[k]);
> > +   }
> 
> This error seems thoroughly impossible, right?  But it shouldn't be 
> fatal in any case...can we just turn it into an assert?

It should be a warning. It is an error that a tileset author can to.

> > +static void unload_all_sprites(void )
> 
> Why the inconsistent spacing?  There seems to be incorrect style 
> elsewhere as well.

First version.

> > +  specfile_list_iterate(specfiles, sf) {
> > +    small_sprite_list_iterate(sf->small_sprites, ss) {
> > +      small_sprite_list_unlink(&sf->small_sprites, ss);
> > +      assert(ss->sprite == NULL);
> > +      free(ss);
> > +    } small_sprite_list_iterate_end;
> > +
> > +    specfile_list_unlink(&specfiles, sf);
> > +    if (sf->big_sprite) {
> > +      free_sprite(sf->big_sprite);
> > +      sf->big_sprite = NULL;
> > +    }
> > +    free(sf);
> > +  } specfile_list_iterate_end;
> >  }
> 
> Are you sure about this code?  Why is the small_sprite part needed?

We have to save somewhere the info about the position of the small
sprites after the scanning of the spec files.

> > +void finish_loading(void)
> 
> This name needs to be more descriptive; finish_loading_sprites should do.

Yes.

> Also:
> 
> - Isotrident support is needed.  I guess this is why gui-gtk-2.0 crashes.

Yes this can be the cause of the crash.

> - All this output:
> 
>    2: freeing sprite 'r.c_rail_sw'
>    2: freeing sprite 'citizen.tax_collector'
> 
> is excessive.

Debug output.

> The design seems very good, but the patch is obviously incomplete.

Good. If you agree with the design I will make another version.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Any sufficiently advanced technology is indistinguishable from magic."
    -- Arthur C. Clarke