[Freeciv-Dev] Re: Cleaning up find_a_direction. (PR#1132)

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

At 06:30 AM 02/01/07 -0800, Gregory Berkolaiko wrote:
> --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
>> On Thu, Dec 13, 2001 at 10:19:20AM -0800, Gregory Berkolaiko wrote:

>> > +    adjc_dir_iterate(punit->x, punit->y, x, y, dir) {
>> > +      if ((x == dest_x && y == dest_y)
>> > +    && warmap.vector[punit->x][punit->y] & (1 << dir)
>> > +    && !(restriction == GOTO_MOVE_CARDINAL_ONLY
>> > +         && !DIR_IS_CARDINAL(dir))) {
>> >    return dir;
>> 
>> Can this be expressed in terms of is_tiles_adjacent and
>> get_direction_for_step?
>
>Yes, but with the current implementation of those functions, the work
>would have to be done twice.  Maybe we can actually change
>is_tiles_adjacent to return 1+dir if the tiles are adjacent and 0
>otherwise?

is_tiles_adjacent() shouldn't have the iterator loop in it anymore so
it is actually not that inefficient to determine whether there is
anything doable before starting. You only have to win a few times
for it to pay off. 

There isn't enough context in this to figure out what is really going
on, but is it "almost" always adjacent here, or fairly random distance
to dest?

If it is the preliminary test before the main loop in one spot, then I 
think it will clearly payoff to do the fast test, then the loop. Raimar
is right on this part.

get_direction_for_step() is one of those Raimar looping things that 
could be done in 3 logical tests, but instead uses guess elimination
hidden behind a function call to boost the overhead. 

The fact that local code here is effectively equivalent means one
loses only the function call in performance, not a factor of two :-).

But it might be really neat to combine 3 tests with the distance check
and return DIR_UNKNOWN = -1, or the valid DIR for is_tiles_adjacent().
This should probably then be named 

  is_tiles_adjacent_and_get_direction_for_step_or_return_DIR_UNKNOWN()

or something, to document the changed interface. 
is_tiles_adjacent_dir(), or get_adjacent_dir() is probably too obtuse 
though.

This could then replace the two occurences of get_direction_for_step().

>>      Raimar
>
>Gregory
>__________________________________________________
>Do You Yahoo!?
>Everything you'll ever need on one web page
>from News and Sport to Email and Music Charts
>http://uk.my.yahoo.com

Cheers,
RossW
=====