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

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

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

> > 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 */
 
> 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.

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

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

> > ai_military_find_victim
> > 
> > Any ideas for something better than a,b,c,d,e,f
> > 
> > f should be unit_build_cost
> > 
> > what the hell is b? 
> >           b = unit_type(pdef)->build_cost;
> > b = (b + 40) * punit->hp / unit_type(punit)->hp;
> > 
> > find_something_to_kill is also pretty bad
> > 
> > m = max_move_rate
> > 
> 
> No idea.
> 
>       Raimar
> 
> -- 
>  email: rf13@xxxxxxxxxxxxxxxxx
>  Make a software that is foolproof, and only fools will want to use it.


__________________________________________________
Do You Yahoo!?
Yahoo! GeoCities - quick and easy web site hosting, just $8.95/month.
http://geocities.yahoo.com/ps/info1