[Freeciv-Dev] Re: directional system: more magic code cleanups

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

I notice you left the two normalize_map_pos() operations in for the
Ross code, but neglected to add them for the others. This will change
the profiling results somewhat, as function calls will probably
dominate the time, and even the extra double compare in a macro
is probably significant here.

Since these are there really to handle the truly oddball cases where
the src and destination are separated by more than a full map in the
wrapped case, and none of the others are checking for this it is
redundant.

The code appears to have been reformatted as well since I generally
lineup the things like if clauses in ways that enhance similarities
rather than obscure them. I don't believe the multi-line if's are 
broken in quite the way that the sample code was in when sent to you.
So I take some offense at your "readability" comments.

I suspect, that your "bugs" are really just the fact that the scalar
product cases don't prefer the diagonal move when diagonal or cartesian
are equivalent. This is preferable because the subsequent move is less
constrained if you go diagonal first. This was also mentionned as a
problem with the trig method in earlier emails.

But they will quite possibly be different when the delta is map.xsize/2
since in this case you can go either way.

But you should really explain what you mean here instead of just 
hinting or making innuendos.

Certainly if you can show me a real problem as opposed to cases where 
there are just equivalent path differences I'll happily fix the code. 
It is not much more than 3 basic comparison operations.

Cheers,
RossW
=====

At 10:47 PM 01/09/23 +0200, Raimar Falke wrote:
>On Fri, Sep 21, 2001 at 10:56:52PM -0400, Ross W. Wetmore wrote:
>> At 09:11 AM 01/09/21 +0200, Raimar Falke wrote:
>> >On Thu, Sep 20, 2001 at 08:29:12PM -0400, Ross W. Wetmore wrote:
>> >> But at least this is an O(0) solution instead of the fully fledged
>> >> O(n) one you started with.
>> >> 
>> >> We are getting there! even if by process of elimination :-)
>> >
>> >If you make a benchmark of Jason's scalar product method (float and
>> >sqrt), my scalar product method (int), my int tan(pi/8) method and
>> >your bit method AND check that each method returns the same value I
>> >will consider this. If you (or somebody) else will do this I will
>> >apply my int scalar product method tomorrow.
>> 
>> So your default choice is to go with the obviously worst implementation
>> over fairly minor modifications to the previous CVS code if someone else
>> doesn't prove that your method and a lot of other unrelated noise are
>> better?
>
>I found the time to run the proposed algorithms.
>
>Here is a profile:
>Flat profile:
>
>Each sample counts as 0.01 seconds.
>  %   cumulative   self              self     total           
> time   seconds   seconds    calls  us/call  us/call  name    
> 26.83      5.98     5.98  3260000     1.83     1.83
jason_straightest_direction
> 11.44      8.53     2.55  3260000     0.78     0.78
raimar_straightest_direction
>  9.24     10.59     2.06                             normalize_map_pos
>  4.62     11.62     1.03                             is_real_tile
>  3.99     12.51     0.89  3260000     0.27     0.27
raimar2_straightest_direction
>  3.81     13.36     0.85     1643   517.35   656.31  really_generate_warmap
>  2.69     13.96     0.60  3260000     0.18     3.25  straightest_direction
>  2.65     14.55     0.59                             tech_exists
>  1.97     14.99     0.44                             map_get_tile
>  1.66     15.36     0.37                             map_get_terrain
>  1.44     15.68     0.32                             get_invention
>  1.35     15.98     0.30  3260000     0.09     0.09
ross_straightest_direction
>  1.21     16.25     0.27  3260000     0.08     0.08
orig_straightest_direction
>
>The attached patch was used. 
>
>Ross your version is buggy. Remove the comment in
>
>+  if ( /*jason != orig || */ /*jason != ross || */jason != raimar
>+      || jason != raimar2) {
>
>to show the problem cases.
>
>The current version of Ross is buggy and unreadable. Jason, Ross do
>you find raimar2_straightest_direction acceptable?
>
>Does anybody else want to comment?
>
>       Raimar
>
>-- 
> email: rf13@xxxxxxxxxxxxxxxxx
>  +#if defined(__alpha__) && defined(CONFIG_PCI)
>  +       /*
>  +        * The meaning of life, the universe, and everything. Plus
>  +        * this makes the year come out right.
>  +        */
>  +       year -= 42;
>  +#endif
>    -- Patch for 1.3.2 (kernel/time.c) from Marcus Meissner
>
>Attachment Converted: "c:\program
files\eudora\attach\straightest_direction.diff"
>