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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Oct 18, 2001 at 02:29:36PM -0400, Jason Dorje Short wrote:
> Raimar Falke wrote:
> > 
> > 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.
> 
> > > < +      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.
> 
> I don't quite understand this.  Are you saying the ordering of elements
> in sprs doesn't matter?

In general: now. In this case: yes. The order in sprs define the
drawing order of the sprites. If the sprites doesn't overlap (draw
over each other) you can reorder them without harm. They overlap in
the engels tileset but this isn't a problem because you can't define a
"native" ordering.

> If so, that could perhaps allow a lot more cleanups of this
> function.

This may be possible but would need careful analysis and isn't worth it.

> I'm not entirely sure that a SAFE_MAPSTEP macro is a good idea.  It
> makes things a little cleaner, but if it's there people will be inclined
> to use it and that's usually not a good idea :-).  It should be ok,
> though.

I will add a comment.

> Oh, one other thought: nearest_real_pos strongly implies that it
> will create only a real position; it doesn't say it'll be a normal
> position.  However, it says in the comments for the function that it
> will normalize.  Should the calling code assume this, or should it
> call normalize_map_pos after calling nearest_real_pos?  Up to now we
> have been assuming the former.

nearest_real_pos is misnamed. I will also add a comment.

> Aside from that, everything looks good.  Your other changes are all
> just cleanups of what I did.

Cleanup of the cleanup ;)

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "From what I am reading Win98 and NT5.0 will be getting rid of all that
  crap anyway. Seems that Microsoft has invented something called TCP/IP and
  another really revolutionary concept called DNS that eliminates the
  netbios crap too. All that arping from browsers is going to go away.
  I also hear rumors that they are on the verge of breakthrough discoveries
  called NFS, and LPD too. Given enough time and money, they might
  eventually invent Unix."
    -- George Bonser in linux-kernel