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

[Jason Dorje Short <vze2zq63@xxxxxxxxxxx>]

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

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

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

jason