[Freeciv-Dev] Re: PATCH: cleanup in fill_tile_sprite_array (PR#1016)

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> On Tue, Oct 16, 2001 at 03:14:13PM -0400, Jason Dorje Short wrote:
> > New patch attached.
> 
> I changed the patch and broke it. I will later search for the error.



< +      if(tspecial_near[DIR8_NORTH] & S_RIVER ||
ttype_near[DIR8_NORTH]==T_RIVER)
< +     *sprs++ = sprites.tx.river_outlet[DIR4_NORTH];
< +      if(tspecial_near[DIR8_WEST] & S_RIVER ||
ttype_near[DIR8_WEST]==T_RIVER)
< +     *sprs++ = sprites.tx.river_outlet[DIR4_WEST];
< +      if(tspecial_near[DIR8_SOUTH] & S_RIVER ||
ttype_near[DIR8_SOUTH]==T_RIVER)
< +     *sprs++ = sprites.tx.river_outlet[DIR4_SOUTH];
< +      if(tspecial_near[DIR8_EAST] & S_RIVER ||
ttype_near[DIR8_EAST]==T_RIVER)
< +     *sprs++ = sprites.tx.river_outlet[DIR4_EAST];
---
> +      int dir8;
> +
> +      for (dir8 = 0; dir8 < 8; dir8++) {
> +     if (!DIR_IS_CARDINAL(dir8)) {
> +       continue;
> +     }
> +     if (tspecial_near[dir8] & S_RIVER || ttype_near[dir8] == T_RIVER) {
> +       *sprs++ = sprites.tx.river_outlet[dir8_to_dir4(dir8)];
> +     }
> +      }

You cannot simply substitute the for loop here.  You should not safely
assume that the ordering will be the same (N, W, S, E).  In fact the
current ordering is N, W, E, S, but may be changed in the future. 
Unless the code using this data is changed as well, this must keep the
fixed ordering.

> +    if (!MAPSTEP(x, y, abs_y0, abs_x0, dir)) {
> +      /* nearest_real_pos is used to make the poles seamless */
> +      nearest_real_pos(&x, &y);
> +    }

This is unsafe.  The values of *x and *y are not well-defined after
normalize_map_pos returns 0.  One of these days I'm just going to set
them both to 0 (or large negative numbers) to see what breaks.

< +    if(tspecial_near[DIR8_NORTH] & S_RIVER || ttype_near[DIR8_NORTH]
== T_RIVER)
< +      *sprs++ = sprites.tx.river_outlet[DIR4_NORTH];
< +    if(tspecial_near[DIR8_WEST] & S_RIVER || ttype_near[DIR8_WEST] ==
T_RIVER)
< +      *sprs++ = sprites.tx.river_outlet[DIR4_WEST];
< +    if(tspecial_near[DIR8_SOUTH] & S_RIVER ||
ttype_near[DIR8_SOUTH]==T_RIVER)
< +      *sprs++ = sprites.tx.river_outlet[DIR4_SOUTH];
< +    if(tspecial_near[DIR8_EAST] & S_RIVER ||
ttype_near[DIR8_EAST]==T_RIVER)
< +      *sprs++ = sprites.tx.river_outlet[DIR4_EAST];
---
> +    for (dir8 = 0; dir8 < 8; dir8++) {
> +      if (!DIR_IS_CARDINAL(dir8)) {
> +     continue;
> +      }
> +      if (tspecial_near[dir8] & S_RIVER || ttype_near[dir8] == T_RIVER) {
> +     *sprs++ = sprites.tx.river_outlet[dir8_to_dir4(dir8)];
> +      }
> +    }

Same as above.

I like the CW/CCW change though.

jason