[Freeciv-Dev] Re: (PR#7261) restructure tileset to avoid explicit terrain mention

["Raimar Falke" <i-freeciv-lists@xxxxxxxxxxxxx>]

<URL: http://rt.freeciv.org/Ticket/Display.html?id=7261 >

On Fri, Jan 23, 2004 at 04:06:46PM -0800, Jason Short wrote:
> 
> <URL: http://rt.freeciv.org/Ticket/Display.html?id=7261 >
> 
> Here's a third version of the patch that actually works.  Two bugs fixed:
> 
> - Matched terrain actually works in isoview.
> 
> - T_RIVER works in isoview.
> 
> I took snapshots of a game in isotrident and they were identical:
> 
> [jdorje@devon:~/src/freeciv/freeciv]$ md5sum ~/orig.png ~/new.png
> c448aebfa927606c73ac5d73c41b6f73  /home/jdorje/orig.png
> c448aebfa927606c73ac5d73c41b6f73  /home/jdorje/new.png
> 
> One thing the patch will need is a new _manditory_ tilespec capability.
>  This is a fairly significant change, but a small price to pay IMO.

> Index: client/tilespec.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/client/tilespec.c,v
> retrieving revision 1.137
> diff -u -r1.137 tilespec.c
> --- client/tilespec.c 2004/01/20 21:52:07     1.137
> +++ client/tilespec.c 2004/01/23 23:38:44
> @@ -78,7 +78,6 @@
>  
>  int num_tiles_explode_unit=0;
>  
> -static bool is_mountainous = FALSE;
>  static int roadstyle;
>  static int flag_offset_x, flag_offset_y;
>  
> @@ -133,6 +132,7 @@
>   * This hash table maps tilespec tags to struct small_sprites.
>   */
>  static struct hash_table *sprite_hash = NULL;
> +static struct hash_table *terrain_hash;
>  
>  #define TILESPEC_CAPSTR "+tilespec2 duplicates_ok roadstyle"
>  /*
> @@ -265,6 +265,8 @@
>  ***********************************************************************/
>  static void tilespec_free_toplevel(void)
>  {
> +  int entries;

> +  for (entries = hash_num_entries(terrain_hash); entries > 0; entries--) {

I used

  while(hash_num_entries(terrain_hash)>0) {

for such constructs in the past.

> +    const struct terrain_drawing_data *draw;
> +
> +    draw = hash_value_by_number(terrain_hash, 0);
> +    hash_delete_entry(terrain_hash, draw->name);
> +    free(draw->name);
> +    free((void *) draw);
> +  }
> +  hash_free(terrain_hash);
terrain_hash = NULL;

assert(terrain_hash==NULL);
> +  terrain_hash = hash_new(hash_fval_string, hash_fcmp_string);


> @@ -668,6 +712,7 @@
>    }
>  
>    sprite_hash = hash_new(hash_fval_string, hash_fcmp_string);
> +
>    specfile_list_init(&specfiles);
>    for (i = 0; i < num_spec_files; i++) {
>      struct specfile *sf = fc_malloc(sizeof(*sf));

Noise.

> +      freelog(LOG_FATAL, "No graphics %s or %s for %s terrain.",
> +           tt->graphic_str, tt->graphic_alt, tt->terrain_name);
> +      exit(-1);

exit(-1) ??

>      }
> -  } else {
> -    for(i=0; i<NUM_DIRECTION_NSEW; i++) {
> -      nsew = nsew_str(i);
> -      my_snprintf(buffer1, sizeof(buffer1), "%s_%s", tt->graphic_str, nsew);
> -      my_snprintf(buffer2, sizeof(buffer2), "%s_%s", tt->graphic_alt, nsew);
> +  }
>  
> -      tt->sprite[i] = lookup_sprite_tag_alt(buffer1, buffer2, TRUE, 
> "tile_type",
> -                                         tt->terrain_name);
> +  /* Currently ocean terrains have special handling.  Although a match type
> +   * may be specified it is ignored.  This is a hack. */

> +  if (draw->match_type == 0 || draw->is_layered || is_ocean(terrain)) {

MARK

> +    /* Load single sprite for this terrain. */
> +    my_snprintf(buffer1, sizeof(buffer1), "t.%s1", draw->name);
> +    draw->base = lookup_sprite_tag_alt(buffer1, "", TRUE, "tile_type",
> +                                    tt->terrain_name);
> +  }
> +

> +  if (draw->match_type != 0 && !is_ocean(terrain)) {

What if neither the MARK line or this lines matches? Isn't there
draw->base unset?

> +    /* Load 16 cardinally-blended sprites. */
> +    for (i = 0; i < NUM_DIRECTION_NSEW; i++) {

> +      nsew = nsew_str(i);

Variable can be removed. Only used once.

> -      assert(tt->sprite[i] != NULL);
> +      my_snprintf(buffer1, sizeof(buffer1), "t.%s_%s", draw->name, nsew);
> +      draw->blend[i] = lookup_sprite_tag_alt(buffer1, "", TRUE,
> +                                          "tile_type", tt->terrain_name);
> +      assert(draw->blend[i] != NULL);
>        /* should probably do something if NULL, eg generic default? */
>      }
> +
> +    if (!draw->base) {
> +      draw->base = draw->blend[0];
> +    }
>    }
>  
>    for (i=0; i<2; i++) {
>      char *name = (i != 0) ? tt->special_2_name : tt->special_1_name;
>      if (name[0] != '\0') {
> -      tt->special[i].sprite
> +      draw->special[i]
>       = lookup_sprite_tag_alt(tt->special[i].graphic_str,
>                               tt->special[i].graphic_alt,
>                               name[0] != '\0', "tile_type special", name);

The expression "name[0] != '\0'" will always evaluate to true here.

I really have no idea what match-type is. There is no enum, no docu
and no comment in the tilespecs. The same is true for is_blended and
is_layered. But you explained these on the list so I have an idea.

There should be a comment that terrain_hash maps from the name (not
the sprite name) to drawing-data.

Why is struct terrain_drawing_data declared in the header file when it
is used only in the .c file?

I really don't know enough about the drawing code to comment if the
semantic changes are ok. But if the patch moves hard-coded behavior
into data files and reproduce the old output it is ok from my side.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "I haven't lost my mind - it's backed up on tape somewhere."