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

[rf13@xxxxxxxxxxxxxxxxxxxxxx]

On Mon, Jan 07, 2002 at 02:30:22PM +0000, Gregory Berkolaiko wrote:
>  --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote: 
> > On Thu, Dec 13, 2001 at 10:19:20AM -0800, Gregory Berkolaiko wrote:
> > > This is extensive cleanup of find_a_direction function from
> > gotohand.c
> > > A lot of variable renaming (from a,b,c to more meaningful names),
> > > commenting, putting braces in ifs.
> > > 
> > 
> > > The only significant change in the fabric of the code is trying to
> > make
> > > triremes safer at sea.  The previous implementation didn't work at
> > all,
> > > now it works if warmap is generated in the right way (it isn't, but
> > > that's will be fixed in future patches).  Even if warmap is "wrong",
> > the
> > > trireme is saved if there is a coast nearby.
> > 
> > Can I ask you to seperate this?
> 
> The existing trireme code in find_a_direction is not working at all.
> So when I separate the changes, should I
> 1. Update the wrong code too
> 2. Remove it?

About how many lines of code do we speak here? If it is small I would
prefer 1.

> > > This patch is highly recommended (if I can recommend my own patch) as
> > now
> > > it is actually possible to understand what find_a_direction is doing.
> > > 
> > > Tested, savegames differ from about 1500BC (due to trireme use, I
> > guess),
> > > visually AI behaves normally (that is "stupid as usual").

> > > +    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?

No:
 - makes it more expensive 
 - doesn't return a boolean anymore
 - the 1+ is ugly

A solution would be a 

int base_get_direction_for_step(int start_x, int start_y, int end_x, int end_y, 
int *dir2)
{
  adjc_dir_iterate(start_x, start_y, x1, y1, dir) {
    if (x1 == end_x && y1 == end_y) { 
       *dir2=dir; 
       return 1;
    }
  } adjc_dir_iterate_end;

  return 0;
}

> > > +    num_of_units = 1;
> > > +    unit_list_iterate(ptile->units, aunit) {
> > > +      if (!pplayers_allied(unit_owner(aunit), unit_owner(punit))) {
> > > + /* MINIMUM priority */
> > 
> > > + defence_of_ally = -1;
> > 
> > What means -1? What 0?
> 
> crap defender I guess
> i think the comment is adequate (it's not mine)
> would should I do about it?

> make a define?  it's stupid.

No.

I just don't understand it at a first glance. What does this if()
thing try to do? "MINIMUM priority" doesn't give me a hint. It is ok
that rating_of_ally may become <0 later? If not how is this prevented?

> > > + if (punit->moves_left <= base_move_cost) {
> > > +   /* Avoid the unknown */
> > 
> > > +   d[k] -= (d[k]/16);
> > 
> > Why 16?
> 
> arbitrary

This is a define.

> > > +       if (passenger && !is_ground_unit(aunit)) { 
> > 
> > > +         d[k] = -99;
> > 
> > Why -99?
> 
> In Syela's language it means "we don't want to go there _at_all_"

This is a define.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Are you saying that you actually used the Classpath Java AWT classes in 
  addition to the GTK peers and got them to display something?
  Wow.  That's way better than I did and I wrote the code!"
    -- Aaron M. Renn in the classpath mailing list