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

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

At 10:52 PM 01/10/21 -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.

There was a reading problem here, I think.

The *specific* examples used should use whole_map_iterate(). There
may be other places that should use different techniques, like 
square_iterate, block_iterate, etc.

Whole_map_iterate() currently *guarantees* the map positions are all
not only real, but normalized ... no checks are needed until you start
to add new map types. I think Raimar just got carried away in his
enthusiasm to bog doen the code with checks here.

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

This is true, but it is easily remedied by making a fixed order part of
the interface, or providing versions that do have fixed orders.

[...]
>If you consider the looping the map iteration patch does too
>inefficient, why not reconsider the y_map_iterate and yx_map_iterate
>macros I suggested before?  Right now they would be the equivalent of
>whole_map_iterate, but (like whole_map_iterate) could be extended to be
>topology-specific:
>
>#define y_map_iterate(y_itr) \
>{  for(y_itr=0; y_itr<map.ysize; y_itr++) {
>
>#define yx_map_iterate(y, x_itr) \
>{
>  for(x_itr=0; x_itr<map.xsize; x_itr++) {
>    if (is_normal_map_pos2(x_itr, y)) {
>
>Doing this would consolidate code that uses this kind of loop into one
>place (map.h, with whole_map_iterate, y_map_iterate, and yx_map_iterate)
>where it can later be made topology-specific if desired.  It would keep
>the loops cleanly readable, and allow out-of-loop code to be placed
>anywhere.  A trick like Ross proposes with the "} else {" could be used
>for handling of non-normal positions (except that I'd like to make this
>a macro as well, for readability).

Two points.

1) You should try really hard to avoid doubly nested loops for 
programming reasons. It isn't really that much harder to convert
to a single iteration in most cases.

2) One of the main thrusts of recent changes is to avoid dealing with
coordinates separately. This is not technically a big deal here if
these are only used within map.h, but I don't think you want to have
x and y flavoured macros like this that leak back out into the general
code.

>Note, though, that this trick of Ross's would not work with a more
>efficient iteration algorithm, for instance if we have something like
>
>#define whole_map_iterate(x, y)
>  for (y=0; y<map.ysize; y++)
>    for (x=map_min_x(y); x<=map_map_x(y); x++) {
       if (1) {
>
>it will not work.

It isn't needed, but if you add the above line you can make the interface
look and feel identical to others from a coding standpoint.

I presume though that anyone who uses this does not expect to deal
with the unreal coordinates that it doesn't provide. And conversely
would choose a different macro form if they did.

>> > You want to use square_iterate() or block_iterate() in most of the
>> > other cases. These functions do the right thing by definition. They
>> > will also not change until they need to.
>
>Please explain.

Square and block iterate loop over real, normalized map positions that
are a subset of whole_map_iterate().

>jason

Cheers,
RossW
=====