[Freeciv-Dev] Re: [PATCH] Map cleanups (PR#1208)

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

At 03:45 PM 02/01/06 +0100, Raimar Falke wrote:
>On Sun, Jan 06, 2002 at 04:32:02AM -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>> Ross W. Wetmore wrote:
>> 
>> > This series of patches cleans up a number of cosmetic and real issues
>> > in map.c and map.h. It is split into a number of sub-patches for the 
                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > convenience of reviewers, but is intended as a single submission, and
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> > only tested in a fully installed state.

Just to reinforece the previous warning ...

You may need to be careful with some comments. Some things are present
in the subpatches to make things obvious, but go away in the full patch.

I could submit this as a single patch, but that tends to make people
crotchety. GB once asked how to get comments into a diff - think about
this as one possibility.

Certainly if these are not batched into CVS, i.e. applied to a sandbox
verified, then checked in at one time there will be problems. I will 
then need to go back to submitting single patches to prevent bugs in 
my submission from such activities as has occurred in the past :-).

>> > 0)  mapclean_00.dif
>> >     This is a pure cut and paste line rearrangement of the existing
>> > files intended to improve the grouping of related functionality and
>> > simplify/localize the subsequent changes. The line and character
>> > count (wc) should be unchanged. One can repeat this by doing cut and 
>> > paste operations on a clean CVS file to match side-by-side views with 
>> > the patch result, and use diff as a final verify step if you are
>> > consciencious or lacking in trust :-).
>> 
>> I trust you, and everything looks sane here.
>> 
>> There are advantages and disadvantages to this change. 

The rearrangement is the accumulated result of all the smaller changes
it is not really a "change" in itself. But it can be dealt with as a 
more global discussion provided you don't attach any more meaning to
this than it is the result of ... and not an intended goal in itself.

> In the long run, 
>> only the advantages matter.  As more and more functions have been 
>> added,there has been some consistency in their placement but no 
>> overriding organization.  Re-ordering them to match a completely 
>> consistent theme will certainly make future coding easier.

I assume a carefully designed rearrangement is an out-of-scope suggestion, 
and something to be dealt with as a separate task from merging corecleanup
fixes. Some minor adjustments are of course reasonable of consideration.
But a new key requirement for this patch is IMHO the sort of unreasonable
thing that tends to cause a lot of friction and delay of completed work.

As background considerations that were underlying rationale though ...

As people, like maintainers change things they have a tendency to delete 
the original and put the new function at the bottom. This doesn't always
make things the best over time, but here some of the clumping is a result
of collecting sets of functions most of which are diddled or would move
for some similar reason to reduce some of this scrambling. 

As one example, the ones that are potential macro candidates are put at 
the bottom in a meaningful defined order.

In the header file, macros are almost *always* best pushed to the bottom.
This has the advantage of keeping the static enums and defines or useful
reference material near the top, while the macro code can run on forever.
You can also get the function definitions out of the way, then replace
them with macros. #undef'ing will restore the function in any compile
hierarchy that specifically wants it.

Most of the header function reordering is from unscrambling the current
mess of mixed macros, supporting subsystems and function code, with some
attempt to keep things in the header grouped as in the "C" file when they
were moved. The corecleanups are also a slightly older branch point on
some things so there are changes being resolved from this realignment as
well.

>> On the other hand, this will make many existing patches to these files 
>> (map.[ch]) fail to apply.  In most cases this should be easily fixable. 
>>   It is in any case a short-term problem.

Almost any patch of more than trivial size has a pretty high probability 
of this :-). 

It is an interesting comment, but has it any relevance here, or are you 
advocating nothing be submitted to CVS that might overlap with other 
changes that may be in parallel development? 

This would be a bad policy to adopt or the general comment even be something 
to think about except in some very special cases, i.e. two large parallel 
sub-projects scheduling their final checkins.

If this is a new criteria for submission, we should probably make this a
separate discussion topic and thrash out the rules in a separate thread.

>> Summary: assuming you agree with the organization Ross introduces, this 
>> should be a beneficial change.  And if it's going to be applied, the 
>> sooner the better.

If you want to do a reorg for itself, there are many other groupings that
might be improved other than those touched here, or ones connected to 
them. 

This I presume to be useful discussion but out-of-scope for this merge.

<ASIDE>
Note, I am generally quite happy to see things move incrementally to a
given goal over time. Thus I don't mind if "some" style elements from
preexisting code aren't changed, or code uses the surrounding style
even though not the standard for small fixes. Local consistency (at
the function level) is better than none, and over time things will
aways move to the norm so a big onetime fight isn't particularly 
useful or necessary.

In the case of a more general structuring of files and code blocks,
I think it would be useful to keep raising such elements and achieving
a concensus and consistency with smaller changes that people are willing
to make over time than trying to achieve the one-true-way all at once.
</ASIDE>

>> I do disagree with some of the organization.  It takes code that is 
>> topology-dependent and is currently (mostly) clumped together and 
>> separates it all out.  Although is_normal / is_real / normalize / 
>> nearest_real_map_pos are all placed together, regular_is_normal and 
>> is_border have been separated from them, as has map_distance_vector. 

Actually, map_distance_vector() and related functions that use it are
the group just above is_real_tile() and its coordinate handling friends.
Which is where it was before ... I don't understand this.

The callers are below, because even with proper header definitions, it 
is still better to define things before use, than vice versa.

If you provide a specific list of things you think are grouped and the
criteria for that I will try to accommodate.

Until then I presume there is no tangible change here.

>> I 
>> do think these should eventually all go into a separate file 
>> topology.[ch], and if there is some agreement on this I'd provide a 
>> separate patch to accomplish this.  So it shouldn't stand in the way of 
>> inclusion.
>
>I think that it is hard to come up with a good (everybody agree to it)
>organization. Also a seperation like map_topology.[ch],
>map_tiles.[ch], map_part3.[ch],... may be better. The borders are just
>more clear this way. So I think it should be applied but there may be
>other opinions.

A clumping and any discussion of what is related to what, is a not 
unreasonable first step to some future split. Certainly if a file has
obvious subsections, gets big enough to be unweildy, or parts need to
be specialized for different architectures this is relatively easy
once the patterns and fault lines are established.

It is probably premature to "make" a split without some of this thought
and discussion of long term goals though.

Once there is a reasonable concensus to what sort of things will be
accepted, then someone should be given the responsibility and latitude
to just go and do it, and the fights that have been fought and decided
should not be allowed a rehash unless there is some new issue, or a 
real abuse of privilege.

>> > 1)  mapclean_01.dif
>> >     This is a series of local cosmetic and minor updates which should
>> > have no global scope or significant impact on game execution. They can
>> > largely be scanned in a superficial way, i.e. no heavy thinking.
>> 
>> For the most part, yes.  But you've slipped some comments in that do 
>> require thought.
>> 
>> This patch seems to consist of the following:

There is only one patch :-)
 
>> * A lot of places that have been changed only in spacing, i.e. to match 
>> freeciv's indentation rules.  Although purely formatting changes are 
>> frowned upon, most of it is pretty innocuous stuff (i.e. nothing 
>> controversial), and the rest is stuff that indent would probably mangle 
>> (by splitting lines at the wrong place).
>> 
>> You do have
>>    -  c=map_get_terrain(x,y);
>>    +  c = map_get_terrain(x,y);
>> in which the spacing on the second line should be fixed.

Sure.

As a note, I'm not style-police minded, but tend to do things for
readability or maintainability under the enough but not too much
policy :-).

I can easily get carried away once I get into standardizing though.

>> * A lot of places that replace map_get_tile(x, y) with MAP_TILE(x, y). 
>> This is faster and just as correct.  Some of them are in one-line if() 
>> statements which have not been separated out; Raimar may want them 
>> separated (although I imagine other maintainers won't care).  

There was an earlier partial update. The presumption here is that 
completing it for consistency within the file is not unreasonable.

>> It would be better IMO to get rid of MAP_TILE altogether and make
>> map_get_tile a macro, but that's easily done later.
>
>Ack.

It is trivial to do this. In fact the corecleanups had these sort of
global macros but the discussion at the time was not in favour of the
sort of widespread use.

Note, all one needs to do is add the appropriate map.h #define and
rename within this file in the same way that is_real_tile() or 
normalize_map_pos() are handled.

The local flavour of this was I thought part of the original decision.
Have the winds changed?

>> * 1 re-ordering of an if() statement.  This makes it both easier to read 
>> (IMO) and faster.

FYI: it is because in the corecleanups this is more complex, the change
is a merge with fixups for the fact that the whole process is being
split into bite sized pieces and redone several times as features are
added in. But in the corecleanup changes, order did play a role :-).

There will be a one line change to the separate pole handling when the
topology is extended to WRAP_Y cases (only one polar continent).

>> * You've added comments in some places.  This can only be a good thing, 
>> so long as they tell the truth.  In other places you've changed 
>> comments, which may be more questionable.  See below.
>
>> * You have moved the call to reset_move_costs down to the bottom of 
>> change_terrain().  Although this makes sense logically, I can't directly 
>> answer to its correctness.
>
>It is ok since S_MINE, S_FARMLAND nor S_IRRIGATION change the move
>costs.

It is one of those maintenance things. There are a lot of other changes
that probably need to go here. If they and or the intervening code 
actually did modify something then the reset_move_costs needs to be run 
after to make sure it reflects the valid state of the game.

This actually has no effect, but the reason is very real and the current
position actually a mistake, so it is a fix and not a convenience :-).

>> * You've changed several lines of the form "return foo;" to "return 
>> (foo);".  I can't say I agree with this change; any of my patches would 
>> most likely just change it back :-).  Perhaps this is a question for 
>> another style vote?  (Or did we already vote on it and I missed it?)
>
>It is definitely a style question. I'm also not sure if we have
>already covered it or not. I still have to apply the results of the
>first part.

There has been a flipflop of such things from the original branch point.
But in cases where this is a deliberate change you will probably find that
it brackets a boolean expression, and not just a return value.

Compilers give a lot of warnings about possible boolean bracket additions.
It helps to keep things straight even if not needed. Again it has 
maintenance vs purely subjective style aspects and is not a personal
preference so much as a general industry tilt.

>> * Some of your changed/added comments are very questionable to me.
>> 
>> The following added lines in the function header comment for 
>> map_distance_vector:
>>    +sq_map_distance is defined to be the square or the trigonometric 
>> distance.
>>    +
>>    +map_distance is the city grid distance, i.e. no diagonal moves allowed.
>> need to be improved on.  "sq_map_distance is the square of the 
>> trigonometric or Pythagorean distance, and the sum of the squares of the 
>> distances in the x and y direction", while "map_distance is the grid or 
>> 'Manhattan' distance, and the sum of the distances in the x and y 
>> direction".

Ok, added Pythagorean and Manhattan to the mix.
 
>> Then we have the following comments for normalize_map_pos:
>>    +FIXME: This should test for realness using is_real_tile() before 
>> touching
>>    +       coordinates and update only if needed, thus avoiding side 
>> effects.

>> normalize_map_pos() has _inherent_ side effects, and trying to pretend 
>> otherwise is foolishness IMO.

Clearly you haven't looked at the (whole) patch. Side effects have been 
removed from the current CVS bringing them into line with the original
corecleanup. "_inherent_" is another of those mental religious terms
not founded in reality here :-).

>>  As I've said before, there are three 
>> things we could legitimately do to unreal coordinates: (1) leave them as 
>> they are, (2) wrap them linearly as if they were real (the equivalent of 
>> Gaute's wrap_map_pos), or (3) find the nearest_normal_map_pos().  #1 is 
>> my preference and yours, but defining things this way would be bad.  I 
>> would write this change like so:

void_tile-ism is the philosophy of converting bogus values to pseudo-real
values so the local code does not break. The breakage is deferred to much
later in the code where the original culprit can not be identified and
thus held to blame. It therefore spawns a fatalistic philosophy wrt dealing
with error conditions as they occur :-).

void_tile-ism is also the sibling of printf error checking and the disdain
for dealing with error conditions in general.

It has been sufficiently blacklisted that clearly 2) and 3) can hardly
be considered the "optimal" solution for the primary interface.

These are even worse than void_tile in that the returned values are
completely indistinguishable from real values. void_tile was at least
a recognizable reference.

If you do not build this into the normalize_map_pos() interface, and it
is trivial and cost-free to guarantee it's adherence, then there will be
numerous constructs of the form 
  if( is_real_tile() && normalize_map_pos(&x,&y) )
  if( am_i_sane() && normalize_map_pos(&x,&y) )
  save_values(x,y);
  if( !normalize_map_pos() && restore_values() )
plus all the places that don't do anything and make the wrong assumptions.
There are some already.

Unix errno is a good model here. It is set only when an error is detected
and not touched otherwise. This means that successful returning functions
do not trash an errno condition notification just because they were called
during the handling.

Initially, I was ambivalent about this part of the interface, partly
because there might be a compare operation or two saved if you only
needed to test as you went along. But I have seen places in the code 
where this causes even more effort to work around or recover from.

Making this a defined part of the interface does not break existing code
and fixes a lot more, including that yet to be written.

I don't consider there to be any real grounds for either alternative.

>> /**************************************************************************
>>   Attempt to normalize the map position (to wrap it into the set of
>>   "normal" positions.
>>     - If the position is real, the function returns TRUE and (*x, *y) are
>>       changed to be the equivalent normal position.
>>     - If the position is not real, the function returns FALSE and
>>       (*x, *y) become undefined.
>> **************************************************************************/
>> int normalize_map_pos(int *x, int *y)
>> {
>>    /*
>>     * FIXME: this should test for realness before touching coordinates
>>     * and update only if needed, thus avoiding side effects.
>>     */
>> 
>> You write comments of the form:
>>    /*  ===============...===============  */
>> and
>>    /******************...******************/
>> What's up with that?  Just stick to the second one.

I presume you refer to the section breaks in the map.h header file?

They aren't part of any specific comment, and the /***..***/ form is not
applicable as it is a bracketting construct.
 
>> The following segment
>>    +/* Map topology definitions and map dimension related macros */
>>    +/* The default definition reflects a cylindrical map wrapping at 
>> map.xsize  */
>> should be one comment, not two.

I actually disagree though not strongly. 

I could add a space to make the subsection title stand out more from the 
note that follows, or move it elsewhere as part of some larger block.
 
>> In the following comment
>>   /*
>> - * Sets (dest_x, dest_y) to the new map position or a real map
>> - * position near. In most cases it is better to use MAPSTEP instead of
>> - * SAFE_MAPSTEP.
>> + * Sets (dest_x, dest_y) to the new map position or a nearby real
position.
>> + *
>> + * In almost all cases it is better to use MAPSTEP and deal with the
unreal
>> + * coordinate case explicitly, instead of SAFE_MAPSTEP and the 
>> void_tile-ish
>> + * problems that produce surprises later.
>>    */
>
>> although I agree with you in principle ("in almost all cases it is 
>> better to use MAPSTEP"), the "void_tile-ish problems that produce 
>> suprises later" part is completely misplaced.

Anyone who sees SAFE_MAPSTEP() and the original description is clearly
going to make the wrong assumptions about the "safety" of using this.

>>  First of all, there will 
>> be no void_tile-ish problems by definition, since unreal coordinates 
>> will _never_ be returned by the macro.

And that is the crux of the void-tile mistake ... it doesn't identify
unreal conditions and returns a completely bogus real result which is
propagated until it causes a weird downstream problem.

For example, having a unit SAFE_MAPSTEP() until it runs out of movement
points would result in an infinite loop if it encountered an unreal
position.

>>  Secondly, this comment is aimed 
>> at convincing the casual user to not use this macro, but it will only be 
>> understood by someone who knows the history of the internals of map.c.

Any casual user that doen't understand the use of this shouldn't use it.
Anything to convey this message in clear terms is IMHO only beneficial :-).

>Ack. void_tile is gone and shouldn't been mentioned anymore.

It is a pity the philosophy hasn't gone. Until it has, remembering past
history can only help to avoid repeating past mistakes. I disagree quite
strongly with your opinion and expressed desire to suppress others here :-).

There seems to be an awful lot of redefining everything from variable 
names to concepts by those that seek to reimplement discredited notions 
or just put their stamp on things - not a good sign and too much is
being lost in the process. More history needs to be retained :-).

>> This comment
>>    +/* This is a temporary measure to track down
>>    +   non-normal cordinate use */
>>     #define CHECK_MAP_POS(x,y) assert(is_normal_map_pos((x),(y)))
>> seems quite wrong to me.  Although the current form of CHECK_MAP_POS may 
>> be quite temporary, it's neither feasible nor desirable IMO to go back 
>> to the old system of using normalize_map_pos everywhere and returning 
>> void_tile if we ever end up with an unreal tile.  One possible change 
>> would be something like

The old system used normalize_map_pos() and dealt with the unreal problem.
The old-old system and the new system would return pseudo-real values and 
just continue merrily along their way.
 
>> #ifdef DEBUG
>> # define CHECK_MAP_POS(x,y) assert(is_normal_map_pos((x), (y)))
>> #else
>> # define CHECK_MAP_POS(x,y)               \
>> #   if (!normalize_map_pos(&(x), &(y))) { \
>> #     freelog(...);                       \
>> #     abort();                            \
>> #   }
>> #endif

The use of normalize_map_pos() everywhere was decried for performance
reasons, it *is* the robust solution. By adding back these checks you are
forfeiting the performance benefit. In the discussion of the time it was 
argued that eventually there would be a benefit, so this overhead vs the
proper solution was only temporary. 

If you now think this needs to be permanent, then the solution is to return 
to using normalize_map_pos() which you have done here. But in the original
code it was often used properly in context which this has now lost in 
favour of fairly widespread generic constructs like map_inx().

I do agree that something like this is a reasonable default for dealing
with unreal conditions (note not unnormal ones) in a purely generic 
context, but once at the head of a routine, rather than on every map
or tile lookup was a much better solution.

>> but calling CHECK_MAP_POS "temporary" just seems like a misplaced 
>> attempt to make it so :-).

That was your statement and therefore a condition on the decision that 
was made at the time - just recording the history to make sure it doesn't
manage to lose itself :-).

>> Summary: this patch is a collection of disjointed changes.  Although the 
>> map_get_tile->MAP_TILE ones are good and the straight formatting changes 
>> are decent, I have objections to a lot of the comments you've changed. 
>> Another opinion is needed on this.

Yes, we agree on most of this :-). 

This patch is a general file cleanup and this subset of largely cosmetic and 
noise-level bugs. 

I think the discussion items are things that do need to be resolved as
they underlie a number of larger tasks that are bogging down :-).

>> > 2)  mapclean_02.dif
>> >     This contains a small number of more significant changes plus an
>> > extensive update of map.h non-directional macros and fixes outstanding
>> > bugs with normalize_map_pos() and friends. While the functional versions
>> > are both equivalent and fixed, most program use should now be via macro 
>> > for increased performance. These features have been in the corecleanups 
>> > for 6 months and are completely compatible with the current CVS codebase,
>> > so there should be little or no concern about risk.
>> 
>> Ahh, now we get into real disagreement :-).

It is one patch ... 

>> > a)  map_city_radius_iterate -> city_map_checked_iterate
>> >     This fixes unnormalized access to map_get_terrain which can no longer
>> >     deal with real but non-normal coordinates.
>> 
>> As a side note, I don't think either of these names are particularly 
>> good.  map_city?  checked?  huh?  I have to look the city macros up 
>> every time I use them.

This should be largely out_of_scope. The names are in CVS for the most
part.

If we open up the naming, then SAFE_MAPSTEP is one of the first I think 
we should decide on. I'm in favour of a Raimar name
  BOGUS_MAPSTEP_FOR_THOSE_THAT_LIKE_TO_GENERATE_RANDOM_SURPRISES()
:-).

>So what is the long term goal? remove map_city_radius_iterate altogether?

Yes, when this cleanup merge is done, the followup patches to clean out a 
number of the residual coordinate and iterator uses can be processed as 
batches of smaller local fixes. That is why map_adjust is left, but marked.

As I say, if you want I can do it all at once. It is certainly much faster,
less of a problem in duplication or side trips to generate the meta-stable 
intermediate states, etc. Look at the corecleanup to see where it is going 
in a close to final form or to try things out. This iterator does not exist 
there.

Much of this changes with the topology fixes, since it is these few basic
elements that form the valid interface, and the implementation here is where 
the core topology changes will largely go. But first one needs to get the 
interfaces in so the code can be updated to use them and there is something
to build on.

Note, this is in the 5-10% range of easy things to get out of the way.

The only thing of contention here is the agreement to go and removal of the 
standard CVS blockages and systemmic breakdown of the processes.

>> > b)  && -> || mistake
>> >     One T_UNKNOWN state can be handled, two cannot, and none is excessive
>> >     restriction on the server AI.
>> 
>> Gotta ask the AI people on this one...

It is good ... 

>> > c)  is_normal_map_pos() -> normalize_map_pos()
>> >     Returned values must be normalized, so just do it and return.
>> 
>> 
>>     do {
>>       *x = myrand(map.xsize);
>>       *y = myrand(map.ysize);
>> -  } while (!is_normal_map_pos(*x, *y));
>> +  } while (!normalize_map_pos(x, y));
>>   }
>> 
>> This change is incorrect.  Consider an iso-rectangular cylinder.  In 
>> this case, all positions will be real, and so will be accepted by 
>> normalize_map_pos.  However, it is only normal positions that we are 
>> interested in.  Changing the code like this will result in some 
>> positions being picked more often than others.

Consider the iso-rectangular cylinder, it has a map.xsize x map.ysize
native coordinate set where all values are treated exactly the same
as the current standard rectangular cylinder.

But what you are missing is the native coordinate conversion because 
that isn't in yet. Look at the prototype/corecleanup version.
    do {
      *x = myrand(map.xsize);
      *y = myrand(map.ysize);
      native_to_map_pos()&x,&y);
    } while (!normalize_map_pos(x, y));
which typically takes one pass per coordinate requested. Native 
coordinates are by definition normal as they are currently in the 
standard map. By normal one means no real coordinate appears twice.

For now, there is only standard == native coordinates, so this is correct.

This solution takes the tack of properly defining the random selection
to use the native normal space. Other solutions do things in cruder
more expeensive ways but hopefully not in Freeciv.
 
>> If you want to rewrite the function entirely to take topology into 
>> account, that is a valid change.  (I'd wait to get a second topology 
>> before doing this, however :-).)  Doing it like this is just asking for 
>> trouble.

No, it is perfectly safe. Since in fact is_normal_map_pos() just calls 
normalize_map_pos() and throws away the values, using an extra function 
call in the process, this change is actually more efficient so it is 
worth doing this part now.

>> > d)  is_real_tile() is a test, not a full normalization, make it so.
>
>This spreads out knowledge about the topology. Provide numbers which
>show how much faster it gets this way.

Accept it. Numbers aren't needed for the obvious.

Clearly, the test will be more efficient than the function call to
the full algorithm that includes the test.

This restores the original CVS behaviour that the corecleanups did not 
change.

Calling a full normalization function to return is_real, vs separating
the is_real test condition of normalize_map_pos() into a separate 
element that can be called on its own or as part of the full process
is just a better partitionning of the interface.

Or to follow your logic get rid of is_normal_map_pos() for the same reason, 
that it spreads the topology knowledge too far. Plus the fact that it only 
tests for the soft condition that doesn't matter, whereas is_real_map_pos()
at leasts tests for the correct hard condition of unrealness.

>> > e)  normalize_map_pos() should test for realness before it undertakes
>> >     any transformation. This fixes any potential bugs from side effects.
>> 
>> Heh.  The bug is not here, but you should fix it anyway :-).

Touche ...

But actually, there are GUI spots that had comments about not adjusting
the values for unreal tile positions. The fix to map_adjust_*() at these
points is normalize_map_pos() and this preserves that characteristic so
workarounds aren't needed.

>> > f)  nearest_real_pos() is defined consistently with normalize_map_pos()
>> >     to return FALSE for unreal coordinates and TRUE for real. The only
>> >     difference is that it returns arbitrary but normal results in the 
>> >     unreal case, i.e. controlled side effects.
>
>The changes are ok.
>
>> This is sensible.
>
>> Note one alternative would be to get rid of nearest_real_pos
>> entirely and give this functionality to normalize_map_pos.
>
>Ack. However I have no idea how hard it is to convert the remaining 3
>(three) cases.

void_tile-ism is so hard to stamp out ... it truly amazes me, this
tenacity in its adherents.

The solution is of course to stomp out this function period, just like
void_tile, map_adjust_y(), SAFE_MAPSTEP and the rest of the mutations.

>> I don't advocate doing this (it will _very_ easily lend itself to
>> misuse), but it would allow us to eliminate one function.

I commend your first preference ... the latter mistake I waffled on at
one point myself.

I view nearest_real_pos() as map_adjust_*() with unfortunately a longer
lifetime because it is a newer mutation.

>> > g)  RANGE_CHECK macros are added and used to improve performance by
>> >     reducing execution in most cases where applicable by a factor of two.
>> 
>> I find the whole RANGE_CHECK_0 thing pretty unreadable and worthless. 
>> Even if such a thing were included in CVS, I'd still write
>>    if (0 <= a && a < b)
>> instead of
>>    if (RANGE_CHECK_0(b, a))
>> Although I haven't checked it, I find it hard to believe that any 
>> worthwhile compiler couldn't optimize this.

All the ones I've looked at don't. 

The double test is an endemic operation in all the code, normalization
for instance, it was worth a couple percent overall :-).

>This is a chicken-and-egg problem. Ross can't show the nubmers until
>all code is converted. I won't accept it until I have some
>numbers. Ross however won't start until he has the promise that it is
>accepted.

From the mailing lists ...

<Ross>
If there is agreement that RANGE_CHECK is the name and function that
should be used in all BOUNDS_CHECK or MAP_CHECK operations I will make 
the necessary changes and put out a patch for this. Do you want a simple 
zero based change or the more complicated arbitraty integer range?

<Raimar>
Can you make a patch with RANGE_CHECK and RANGE_CHECK_0 and the
changes to is_valid_city_coords?

This was at one time a decided issue that died along with other parts
that weren't - the macro name is Raimar's I believe :-).

I don't see any reason to repopen the debate - we can find enough things
to disagree on that will likely keep debate alive in useless wrangling
and kill patch submissions for the forseeable future :-).

The two steps back before one makes even one forward is definitely an
interesting process strategy though.

>> > h)  Wrap and clipping macros are added to standardize code use. Overtime
>> >     all map code that does local flavours should be updated to use these.
>
>> > i)  map_adjust_x() and map_adjust_y() are redefined to be explicit
>> >     wrap and clip operations for the standard cylindrical map, as the
>> >     first instance of standardization :-).
>> 
>> This is very bad IMO.  Standardization requires that 
>> map_adjust_x/map_adjust_y be removed entirely.  This change just adds a 
>> lot of importance to their definitions - you take 7 lines of 
>> #definitions that are currently only used in a very few places 
>> (hopefully soon to be removed), and change them into 30 lines of 
>> #definitions within #definitions with #definitions, plus extensive 
>> comments explaining how they should be used (including saying that 
>> they're about to be removed, fortunately :-).
>
>Ack. map_adjust_[xy] should die/definition moved out of
>common/map.h. Move it (in its current form) to client/mapview_common.c
>I will be happy.

I think we all agree these need to go (along with SAFE_MAPSTEP, ... :-).

map_adjust_*() goes as soon as the code doesn't need it, which may be 
blocked on Andreas right now. 

It is gone in the corecleanups, so this is just one of those intermediate 
inefficiencies because everyone want things done in small steps :-). 

Discouraging people from using it meanwhile is IMHO a good thing.

>> Also, the "void_tile-ish" comment here is in a similar position to the 
>> one mentioned above.

Too bad you don't react as strongly to the philosophy or implementation
of it as you do to the name :-).

>> > j)  map_to_index() is defined as the start of the map_inx() removal.
>> >     When the corecleanup and topology fixes are all applied, this will
>> >     be completely gone. index_to_map_pos() as the reverse operation is
>> >     defined in the standard manner, i.e. returns updated coordinates
>> >     and a status return indicating success or failure.
>> 
>> I definitely like the name change (although I would call it 
>> map_pos_to_index).  I'm not sure what you mean by map_inx removal, but I 
>> don't think I'm going to like it :-).

The leading "pos" was I believe debunked by others, (Tony, maybe?).

map_inx was long ago to be renamed, but it will live on and continue to
multiply as map_to_index().

<ASIDE>
There is a very standard and consistent naming in the corecleanups. When 
the merge is complete hopefully CVS will have a similar standardization.

All these "coordinates" are basically interconvertible with the internal
standard game coordinates. There are thus two functions to write for any
new one, <blort>_to_map_pos() and map_to_<blort_pos>.

A key feature of this is that if something is easier done in a particular
coordinate representation, you convert, do the operation and convert back.

For example, it gets rid of all the vector arithmetic in normalize_map_pos()
and all but the special case part for mismatched axes in
unnormalize_map_pos().
</ASIDE>

>> > k)  Macro forms for is_real_tile() and family are defined for the
>> >     cylindrical map. These will be upgraded to full topological forms
>> >     when further capabilities (i.e. topological state) are added.
>> 
>> This is something Gaute has wanted to do for a while, and as you've 
>> found the speedup is very significant.  It makes the code pretty 
>> ugly/unreadable, but it's tough to argue with the results.

Luckily, it is broken down into small steps. Each step is not that
difficult to work out. Treat the steps as atomic operations once you
have understood them. The entire process is thus quite digestible and
very true to the conceptual explanation.

>> > l)  The generic border tile macro is added and used in the current case.
>> 
>> Ugh.
>> 
>> Summary: I don't like most of these changes at all.  So we need another 
>> opinion (aside from the is_normal->normalize change which must be fixed).

Reread the above counter. *must* is such a strong word for something
that is technically very straightforward and provably correct, more
efficient, etc.

We need to get past the religious debate sometime though (sigh).

>> > The changes have been sanity tested and client and server autogames run
>> > for a combined 24 hour period without a problem.
>> > 
>> > In one case, savegames chksummed identically through end in 1885. 
>> 
>> I believe it.  I see nothing which could change the behavior.
>> 
>> 
>> > Times were ...
>> >   9:05  CVS-Jan04
>> >   7:03  patched
>> 
>> Very impressive.  But a greater speedup can be achieved just by changing 
>> is_real_tile, is_normal_map_pos, normalize_map_pos, AND map_get_tile 
>> into macros - without the need for RANGE_CHECK or any of the other 
>> implementation hacks you use.
>> 
>> In my test, I get the following times for an autogame (compiled with 
>> DEBUG and -O2 and only run once, so maybe not a very good benchmark...):
>> 
>> CVS: 3m39s = 219 sec
>> mapclean patch: 3m6s = 186s ~ 15% speedup
>> simple macro patch: 2m36s = 156s ~ 28% speedup

In my books, 2 in 7 is about 28%. I'm not sure I understand the "greater" 
comment. map_get_tile() is left out here. It is one of the key functions
identified in the corecleanup section of map.h that used to be macro-ized.

BTW: the corecleanup runs at about half this speed on the same topology.
     But it has iterator loop and other things that are going to be much
     more controversial when they come out, even without the first 
     exposure backlash, i.e. 6 month settling time.
 
>> savegames were identical for all three, so I can back you up on that.
>
>I feel not ready for another thread about performance, macros and
>inline usage. So Ross please drop the RANGE_CHECK thing for now or
>show some drastic numbers (with RANGE_CHECK only).

It is so nice to work with someone that is so flexible and consistently 
willing to change his mind on things. It makes progress so much easier
to deal with.

>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
> "Last year, out in California, at a PC users group, there was a demo of
>  smart speech recognition software. Before the demonstrator could begin
>  his demo, a voice called out from the audience: "Format c, return. Yes,
>  return." Damned short demo, it was.

In summary, there are 2 minor fixes (will do in next pass), 4 major religious 
wars and one skirmish to be fought over two comments and three functional 
changes, and one previously agreed on and requested feature Raimar now wants 
dropped.

Does this summarize the current status?

Cheers,
RossW
=====