[Freeciv-Dev] Re: RFC: Fixing movement code

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Wed, Nov 21, 2001 at 06:17:50PM -0800, Raahul Kumar wrote:
> 
> --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > On Wed, Nov 21, 2001 at 01:35:15AM -0800, Raahul Kumar wrote:
> > > This patch is tidying up the AI for my further patches. The aim is to 
> > > allow
> > > the replacement of 3 = Single move cost with 30. This will allow finer
> > grained
> > > moAck. That is going to make the code even more messy. Are you sure you
> want this?vement costs such as 1/6 for an igter unit upgrade, as well as 
> maglev
> > costs.
> > > 
> > > List of proposed changes
> > > 
> > > If there are any other places in the AI code where there are changes to be
> > made
> > > let me know. 
> > > 
> > > 
> > >  if (ferryboat) boatspeed = (unit_flag(ferryboat, F_TRIREME) ? 6 : 12);
> > >   else boatspeed = (get_invention(pplayer, game.rtech.nav) != TECH_KNOWN ?
> > 6 :
> > > 12);
> > > 
> > > Not very portable at all. What is supposed to be the move rate for
> > triremes?
> > 
> > I don't know the answer but I want to point out
> > 
> > ./ai/advmilitary.c:676
> > 
> >  if (ferryboat) boatspeed = (unit_flag(ferryboat, F_TRIREME)
> >                  ? 2*SINGLE_MOVE : 4*SINGLE_MOVE);
> >   else boatspeed = (get_invention(pplayer, game.rtech.nav) != TECH_KNOWN
> >             ? 2*SINGLE_MOVE : 4*SINGLE_MOVE);
> > 
> 
> Thanks. Any other places like this in the code where changes have not
> been completed?

None that I know.

> > > warmap.seacost[acity->x][acity->y] <= 6 * THRESHOLD)))
> > > 
> > > I have to get rid of all these 6 * threshold nos. I could always
> > > just multiply threshold by 10.
> > 
> > Please make it a symbol like in:
> > 
> > /* docu */
> > #define SOME_MOVE_THRESHOLD 6
> > 
> > Also the original THRESHOLD in server/gotohand.h has no docu.
> >
> 
> Yes. Naming suggestions? 
> My idea of docu
> 
> /* Threshold is an arbitrary limit on the amount of moves considered */

For what?

> > Also this
> >  ./server/gotohand.c:275:  int maxcost = THRESHOLD * 6 + 2; /* should be big
> > enough without being TOO big */
> > looks like rather like some arbitrary value.
> > 
> > > if (boatspeed < 9 && c > 2)
> > > Have to multiply that by single move cost. What does c do in this 
> > > function?
> > 
> > I have no clue. This function is 290 lines long. These are 250 too
> > much IMHO.
> >
> 
> Two possibilities
> 
> a) You rewrite it to a 40 line function. Let me know if this is what you
> are going to do.

No.

> b) You let me know how I could rewrite it to a 40 line function. I look at it,
> and I am just disgusted. If you let me know what exactly it is supposed to
> do, I could probably write a better replacement.

I was more assuming a splitting of this function. Or another way is to
rewrite it as

void foobar(....)
{
   {
      int ....;
      code;
   }
   {
      int ....;
      code;
   }
   {
      int ....;
      code;
   }
   {
      int ....;
      code;
   }
   {
      int ....;
      code;
   }
}

So there are blocks which are more or less independent of each
other. You don't have to think of function names if you compare this
to the split-into-multiple-function way.

> > > total_distance in ai_tools.c
> > > 
> > > total_distance = 36 This must be 12 squares. Therefore my patch will have
> > > 360 as total distance.
> > > 
> 
> Do you agree with the above comment? This is quite important, I do not want
> to make changes that break code.

No. IMHO this 36 is the same as in common/city.c:city_corruption. You
don't have to change it. However the code in ai_tools.c isn't up to
date because of the recently introduced game.notradesize and
game.fulltradesize. So the best would be to let ai_tools.c use
city_corruption.

> I need a review of my code, agreeing that the changes I make are
> correct from either you, Tony, GB, or Ross. Are there any other
> active developers who know anything about the AI?

I would say that I know the AI. What I know I gather by reading the
code (which is quite every time).

> > > File aiunit.c
> > > 
> > > static int unit_move_turns(struct unit *punit, int x, int y)
> > > {
> > >   int m, d;
> > >   m = unit_type(punit)->move_rate;
> > >   if (unit_flag(punit, F_IGTER)) m *= SINGLE_MOVE;
> > >   if(is_sailing_unit(punit)) {
> > >     struct player *pplayer = unit_owner(punit);
> > >     if (player_owns_active_wonder(pplayer, B_LIGHTHOUSE))
> > >       m += SINGLE_MOVE;
> > >     if (player_owns_active_wonder(pplayer, B_MAGELLAN))
> > >       m += (improvement_variant(B_MAGELLAN)==1) ? SINGLE_MOVE : 2 *
> > SINGLE_MOVE;
> > >     m += player_knows_techs_with_flag(pplayer,TF_BOAT_FAST) * SINGLE_MOVE;
> > >   }
> > > 
> > >   if (unit_type(punit)->move_type == LAND_MOVING)
> > >     d = warmap.cost[x][y] / m;
> > >   else if (unit_type(punit)->move_type == SEA_MOVING)
> > >     d = warmap.seacost[x][y] / m;
> > >   else d = real_map_distance(punit->x, punit->y, x, y) * SINGLE_MOVE / m;
> > >   return(d);
> > > 
> > > m = max_move_rate
> > > d = min_turns_to_dest
> > 
> > I would use for a variable like d the name "result". But this is a
> > personal thing. You could write the "min turns to dest" in the header
> > of the function as comment.
> >
> 
> No. My comment is going to be along the lines of 
> 

> /* min turns to dest calculates the min cost to move to a
> destination 

> assuming the move cost a tile is SINGLE_MOVE_COST.

This is so by definition.

> It will only be accurate for air units, for all other units such as
> igter, it is probably going to be wrong. Things like railroad,roads
> will make the actual move cost lower than min est.  Mountains,
> swamps will increase actual move cost.  */

> > I want to point out that "is_sailing_unit(punit)" ==
> > "unit_type(punit)->move_type == SEA_MOVING". This means that the
> > if(is_sailing_unit(punit)) part can be moved into the "if
> > (unit_type(punit)->move_type == SEA_MOVING)" test.
> > 
> 
> Ack. That is going to make the code even more messy. Are you sure you
> want this?

static int unit_move_turns(struct unit *punit, int x, int y)
{
  int m, result;
  m = unit_type(punit)->move_rate;

  if (unit_type(punit)->move_type == LAND_MOVING) {
    if (unit_flag(punit, F_IGTER)) m *= SINGLE_MOVE;
    result = warmap.cost[x][y] / m;
  }
  else if (unit_type(punit)->move_type == SEA_MOVING) {
    struct player *pplayer = unit_owner(punit);
    if (player_owns_active_wonder(pplayer, B_LIGHTHOUSE))
      m += SINGLE_MOVE;
    if (player_owns_active_wonder(pplayer, B_MAGELLAN))
      m += (improvement_variant(B_MAGELLAN)==1) ? SINGLE_MOVE : 2 * SINGLE_MOVE;
    m += player_knows_techs_with_flag(pplayer,TF_BOAT_FAST) * SINGLE_MOVE;

    result = warmap.seacost[x][y] / m; 
  } else /* air unit */ {
    retult = real_map_distance(punit->x, punit->y, x, y) * SINGLE_MOVE / m;
  }
  return resukt;
}

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "If at first you don't succeed... well so much for skydiving."