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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Oct 17, 2001 at 04:08:42PM -0400, Jason Dorje Short wrote:
> 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.

Since the 4 river_outlets are disjunct (don't share a pixel) this
isn't a problem.

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

Yes. It looks like this was the error.

New patch attached.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "There are three ways to get something done. Do it yourself, hire someone
  to do it for you or forbid your kids to do it."

tilespec_cleanup2.diff
Description: Text document