[Freeciv-Dev] Re: remove nearest_real_pos uses (PR#1211)

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

At 09:23 PM 02/01/08 -0800, jdorje@xxxxxxxxxxxxxxxxxxxxx wrote:
>Ross W. Wetmore wrote:
>
>> At 01:01 PM 02/01/07 +0100, Raimar Falke wrote:
[...] 
>> Callers used to pass unnormal ones though, which is a soft condition that
>> can always be fixed. In this case normalize_map_pos() is guaranteed to 
>> fix things and doesn't need to be checked. 
>
>So is CHECK_MAP_POS, if we implement it that way.  But as long as the 
>soft condition is met we will be safe.
>
>Again, if we implemented CHECK_MAP_POS as:
>
>#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(...);
>       nearest_real_pos(&x, &y);
>     }
>#endif
>
>Then we'll have exactly the same effective code, plus the additional 
>assertion to help us catch bugs, and it will be uniform throughout.

The effect now, is that instead of using normalize_map_pos() once at the
entry of a function or when you computed a new coordinate and were going 
to make an access to something that required normal coordinates, you now
make it everytime you do such accesses which can be dozens or hundreds of 
times in a routine. This is through the map_inx CHECK_MAP_POS() in 
addition to all the other random ones.

You have just replaced (previously) correct local code (optimizations) 
with a hidden global performance pig providing uniform de-optimization 
throughout.

Remember that normalize_map_pos() was the naughty one after warmap.

And remember this was the main argument for starting all this 
CHECK_MAP_POS() activity in the beginning - the need not to have to 
do these checks all over the place, just at source. 

And remember, you were warned this was neither particularly useful nor 
reasonable.

>  It's a choice between speed and safety - you 
>can't have both.

>> There was an extensive test period for realness fixes and fair amount
>> of code scrutiny at one point before the corecleanups were distributed
>> publicly and there has been a much longer settling time for these
>> things there. 
>
>In that case the assertion definitely shouldn't fail - all the more 
>reason to put it in :-).

I do find it hilarious that Raimar needs "numbers" to permit an obviously
more efficient code construct into CVS, but the two of you repeatedly 
find all these reasons to downgrade the performance with nary a whiff
of impact analysis.

It can be 50-fold increases in savegame sizes, 2% hit rates in regular 
whole_map_iterate loops, going through the above cycle to increase the 
number of redundant normalize_map_pos() calls in the code, replacing 
array lookups with function calls in all punit access, looping over all 
directions instead of computing the one you want ...

I really do have things backwards about what is important here, don't I :-?

>>>>If this patch is applied, the only remaining nearest_real_pos use will 
>>>>be in map_pos_to_canvas_pos in mapview_common.  If one of these places 
>>>>does need to use nearest_real_pos (which seems inconceivable), a comment 
>>>>should be added explaining why.
>> 
>> If this function returns status, and get_canvas_xy propagates this to
>> its callers who then deal with the situation, this one can go.
>
>Unfortunately no, since the only way the callers can deal with it is to 
>call nearest_real_pos.  We can let it go if we agree to give up the 
>functionality it provides.

The limitation is again in your imagination, not reality. Please stop
making such foolish statements :-).

Certainly they don't justify anything you might be trying to convince
anyone of.

>>>Can't nearest_real_pos then be moved into mapview_common? So nobody
>>>can use this "dangerous tool". However this would spread topology
>>>knowledge (which is also bad). What about just ignoring clicks which
>>>are outside the map?
>> 
>> Better fix this case, then let it join map_adjust_y() and SAFE_MAPSTEP
>> in the bit bucket.
>
>map_adjust_y is almost gone, and SAFE_MAPSTEP can be implemented locally 
>(i.e. moved to tilespec.c).  Only nearest_real_pos provides 
>functionality that isn't availibe outside the topology backend.
>
>jason

Just these last 2 void_tile elements to go ... I can't wait for the
next mutation to spring up as soon as it looks like these are finally 
squarely in the crosshairs. Bad habits die so hard ...

Cheers,
RossW
=====