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

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

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

> > 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").

> >  {
> > -  int k, d[8], n, a, best = 0, d0, d1, h0, h1, u, c;
> > -  struct tile *ptile, *adjtile;
> > -  int nearland;
> > +  int k, d[8], best = 0;
> 
> Rename k to dir(ection).

ok

> > +    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 it an allowed direction?  is it marked on the warmap? */
> 
> Full sentences please with the first word capitalized.

No nitpicks please ;)

> > +    /* determine the cost of the proposed move */
> > +    if (is_ground_unit(punit))
> 
> > +      /* assuming the move is valid, but what if unit is trying to
> board? -- GB*/
> 
> Is this a "I don't know what the code does" question? Or more a "if it
> is a ground_unit than it is trying to board"?

sorry.
it's a POTENTIAL BUG warning.  It says "the code here assumes that the
move is a valid land move, but there could be problems if the unit is
actually trying to board a ship".

> > +      base_move_cost = map_get_tile(punit->x,
> punit->y)->move_cost[k];
> > +    else 
> > +      base_move_cost = SINGLE_MOVE;
> 
> Add extra {}.

oops :(

> > +    /* find everybody's defence stats */
> > +    ptile = map_get_tile(x, y);
> > +    defence_of_unit = get_simple_defense_power(punit->type, x, y);
> > +    pcity = map_get_city(x, y);
> > +    defence_multiplier = 2;
> > +    if (pcity) { /* this code block inspired by David Pfitzner --
> Syela */
> > +      if (city_got_citywalls(pcity)) defence_multiplier += 2;
> > +      if (city_got_building(pcity, B_SDI)) defence_multiplier++;
> > +      if (city_got_building(pcity, B_SAM)) defence_multiplier++;
> > +      if (city_got_building(pcity, B_COASTAL)) defence_multiplier++;
> > +    }
> > +    defence_of_unit = (defence_of_unit * defence_multiplier) / 2;
> 
> > +    hp_of_unit = punit->hp;
> > +    /* find the best friendly defensive unit at the target tile */
> > +    hp_of_ally = 0; 
> 
> Please seperate such sections by an empty line. Maybe even some fancy
> comment style like:
> 
> /********* find the best friendly defensive unit at the target tile
> ************/

ok

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

> > +      if (!((adjtile->known)&(1u<<punit->owner))) {
> 
> Isn't there a function which does this?

sure, my fault

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

arbitrary

> > +         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_"

>       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