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

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

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:

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

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

> +#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?


> +    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?

> +     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?

> +static void unload_all_sprites(void )

Why the inconsistent spacing?  There seems to be incorrect style 
elsewhere as well.

> +  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?

> +void finish_loading(void)

This name needs to be more descriptive; finish_loading_sprites should do.

Also:

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

- All this output:

   2: freeing sprite 'r.c_rail_sw'
   2: freeing sprite 'citizen.tax_collector'

is excessive.


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

jason