[Freeciv-Dev] Re: PATCH: map iteration (PR#1018)

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Mon, Oct 22, 2001 at 11:55:05AM -0400, Jason Dorje Short wrote:
> Raimar Falke wrote:
> > 
> > On Sun, Oct 21, 2001 at 10:52:51PM -0400, Jason Dorje Short wrote:
> > > Raimar Falke wrote:
> > > >
> > > > On Fri, Oct 19, 2001 at 09:15:46PM -0400, Ross W. Wetmore wrote:
> > > > > The is_normal_map_pos() solution is really just a bad hack because you
> > > > > aren't willing to think about the case and fix it right at the moment 
> > > > > :-).
> > > > >
> > > > > But in most cases, the right fix it is far easier than what you are
> > > > > proposing.
> > > > >
> > > > > The examples below should use whole_map_iterate(). When you get weird
> > > > > maps, then whole_map_iterate() will become weird, but until then it 
> > > > > will
> > > > > remain as is, i.e. efficient iteration over the whole map. The change
> > > > > when needed is to 1 line of code in a header.
> > > >
> > > > Yes whole_map_iterate should be prefered. However whole_map_iterate
> > > > doesn't allow an action if the map position is unreal or an extra
> > > > action for every new line.
> > >
> > > Indeed.  Adding such code to whole_map_iterate would be very unwieldy at
> > > best.
> > >
> > > My understanding is that whole_map_iterate also doesn't guarantee
> > > anything about the order in which the coordinates are traversed.  The
> > > stuff in server/savegame.c certainly needs such a guarantee.
> > 
> > Lets go a step back:
> > $ grep -Irn 'for.*map.xsize.*[+][+]' .
> > ./client/goto.c:224:  for (x_itr = 0; x_itr < map.xsize; x_itr++) {
> > ./client/gui-mui/overviewclass.c:205:    for (x = 0; x < map.xsize; x++)
> > 
> > ./common/game.c:125:      for (x = 0; x < map.xsize; x++)
> > ./common/game.c:133:          for (x = 0; x < map.xsize; x++)
> > ./common/game.c:144:      for (x = 0; x < map.xsize; x++)
> > ./common/game.c:152:          for (x = 0; x < map.xsize; x++)
> > ./common/game.c:167:  for (x = 0; x < map.xsize; x++)
> > ./common/game.c:175:      for (x = 0; x < map.xsize; x++)
> > ./common/game.c:183:  for (x = 0; x < map.xsize; x++)
> > ./common/game.c:191:      for (x = 0; x < map.xsize; x++)
> > 
> > We may remove this LAND_AREA_DEBUG stuff altogether.
> 
> Everything for LAND_AREA_DEBUG>=1 or just LAND_AREA_DEBUG>=2?
> 
> Seems easy enough...

I don't know. I have never used it.

> > ./common/map.h:475:    for (WMI_x_itr = 0; WMI_x_itr < map.xsize; 
> > WMI_x_itr++)
> 
> whole_map_iterate, of course.
> 
> > ./server/barbarian.c:96:        for( j = 0; j < map.xsize; j++ )
> 
> This should be a whole_map_iterate.
> 
> > ./server/gamelog.c:93:    for (x=0;x<map.xsize;x++) {
> 
> This cannot easily be done with whole_map_iterate.
> 
> > ./server/mapgen.c:111:    for (x=0;x<map.xsize;x++) {
> > ./server/mapgen.c:123:    for (x=0;x<map.xsize;x++) {
> > ./server/mapgen.c:138:  for (x=0;x<map.xsize;x++) {
> > ./server/mapgen.c:724:      for (i = 0; i < map.xsize * map.ysize; i++) {
> > ./server/mapgen.c:782:  for (x=0;x<map.xsize;x++) {
> > ./server/mapgen.c:801:    for (x=0;x<map.xsize;x++) {
> > ./server/mapgen.c:1406:    for (x=0;x<map.xsize; x++) {
> > ./server/mapgen.c:1765:    for (x = 0 ; x < map.xsize ; x++) {
> > ./server/mapgen.c:1769:  for (x = 0 ; x < map.xsize; x++) {
> > 
> > Some of them can be replaced with a new rectangle/block_iterate.
> 
> I think all of them can, and the new code should be cleaner as well. 
> There are other issues with converting this stuff, as discussed in the
> other thread.
> 
> > ./server/maphand.c:302: for (x=0; x<map.xsize; x++) {
> > ./server/maphand.c:309: for (x=0; x<map.xsize; x++) {
> > 
> > Difficult.
> 
> But easy using is_normal_map_pos2.  We just need to check whether the
> position is good before we send it; alternately this can be done by
> y_map_iterate/yx_map_iterate.  It would be impossible to do with just
> whole_map_iterate.  Using block_iterate would be conceivable but
> wouldn't really gain anything IMO.

counter=0;
conn_list_do_buffer(dest);
#define SOME_NUMBER map.xsize

whole_map_iterate(x,y)
{
   counter++;

   if(counter%SOME_NUMBER==0)
   {
          conn_list_do_unbuffer(dest);
          flush_packets();
          conn_list_do_buffer(dest);
   }

  conn_list_iterate(*dest, pconn) {
      struct player *pplayer = pconn->player;

      if (!pplayer && !pconn->observer) {       /* no map needed */
        continue;
      }

      if (pplayer) {
          if (map_get_known(x, y, pplayer)) {
            send_NODRAW_tiles(pplayer, &pconn->self, x, y, 0);
            send_tile_info_always(pplayer, &pconn->self, x, y);
          }
      } else {
          send_tile_info_always(pplayer, &pconn->self, x, y);
      }
    } conn_list_iterate_end;
}

> > ./server/savegame.c:70:    for (x = 0; x < map.xsize; x++) {                
> >    \
> > ./server/savegame.c:128:    for(x = 0; x < map.xsize; x++) {                
> >           \
> > 
> > Ok.
> 
> These have already been fixed up, but if we don't want to handle the
> others this same way then we should change these as well.

> > ./server/gotohand.c:192:    for (x = 0; x < map.xsize; x++) {
> > ./server/gotohand.c:205:    for (x = 0; x < map.xsize; x++)
> > ./server/gotohand.c:210:    for (x = 0; x < map.xsize; x++)
> > ./server/gotohand.c:824:  for (x = 0; x < map.xsize; x++)
> > 
> > Not topology depend.
> 
> Yes, these can be left as-is.  Later, it would be nice to replace these
> arrays with a single array using map_inx for indexing.

Good idea.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
  Two OS engineers facing a petri net chart:
        "dead lock in four moves!"