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

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

Raimar Falke wrote:
> 
> 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++)

> > > ./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;
> }

This is a larger cleanup than I had in mind; you're changing the order
in which the packets are placed in the buffer.

But, code like this might actually be better; for one thing it gives you
more flexibility on how often to flush the buffers.

jason