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

["Ross W. Wetmore" <rwetmore@xxxxxxxxxxxx>]

At 05:17 AM 01/10/30 -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>> 
>> There are a lot of places in the code that have loops like
>> 
>>   for (y=0; y<map.ysize; y++) {
>>     for (x=0; x<map.xsize; x++) {
>>       ...
>>     }
>>     ...
>>   }
>> 
>> These loops will generally not work under a general topology, since they
>> are intended to loop over only normal, real positions and for many
>> topologies that will not happen.
>> 
>> The easy solution is to do an is_normal_map_pos check, as in:
>> 
>>   for (y=0; y<map.ysize; y++) {
>>     for (x=0; x<map.xsize; x++) {
>>       if (is_normal_map_pos(x, y)) {
>>         ...
>>       } else {
>>         <possibly some extra handling>
>>       }
>>     }
>>     ...
>>   }

These are whole_map_iterate() loops. They should be replaced with
whole_map_iterate().

You need block_iterate() to get the ones that are a subset of
whole_map_iterate().
 
>> The cleanest example of this is in whole_map_iterate, where the
>> is_normal_map_pos check can just be added directly.

 ... when it is needed :-)

But you might find better ways to handle the implementation of 
whole_map than doing large amounts of looping and filtering out the
few cases you really need. So the change may never need to be added.

>So, now back to this patch...
>
>Questions:
>
>1.  Ross, do you see the necessity of these changes yet?

I have replaced whole_map_iterate in more places than you have found :-)

I have still not needed to fix manufactured regular problems :-) :-)

>2.  Are there any more loops that can be replaced by iterator macros? 
>Such replacement is IMO the best solution possible.  But, I don't see
>how to convert any more of them.

block_iterate

>3.  Is the debugging function print_landarea_map() in game.c (part of
>the LAND_AREA_DEBUG group) necessary?  If no consensus can be reached, I
>suppose it must be left in.

Someone removed a lot of these from mapgen at one time. I put new ones
back when I need to work in the code. There's probably a lesson in there
somewhere ...

>4.  Is regular_map_pos_is_normal good, or should we stick to
>is_normal_map_pos?  Again, regular_map_pos_is_normal provides identical
>functionality but assumes the map position it is given is "regular",
>i.e. within (0..map.xsize-1,0..map.ysize-1).  This makes it much faster.

I'm not much of a fan of regular. You need to demonstrate it a bit
more to show any real value. If it never does anything, then dubious
dregs like this are just noise.

Another maxim that comes to mind is premature optimization is the root
of all evil. I might be best to wait until you have demonstrated there
is a problem before optimizing. That shouldn't take too long, though.

>5.  For loops that cannot use any existing iterator macros, do we want
>to create new macros y_map_iterate and yx_map_iterate?  I am
>indifferent.

No. Come up with sensible iterators.

>6.  I have not touched any mapgen stuff, although some of the loops
>there could easily use whole_map_iterate.  Is this desired?

Raimar needs to decide what level of activity he wants here.

>7.  I have also not touched Overview_FillBuffer in gui-mui.  I don't
>know what the correct fix there might be.
>
>Once these issues are resolved, we should be able to think about closing
>this.