[Freeciv-Dev] Re: AI cleanup Version 3

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Petr Baudis <pasky@xxxxxxxxxxx> wrote:

> >  ***********************************************************************/
> >  static int unit_move_turns(struct unit *punit, int x, int y)
> ...
> Cleaning up this functions appears useless to me, as we have probably better
> candidate for include, IIRC Greg made it, haven't we?
> >   
> >

Yes, but so far it is not in CVS. I'm not going to start ripping off Greg's
code to make mine better. When Greg's entire warmap series gets into CVS,
he can handle it then.
 
> /**************************************************************************
> > @@ -1162,7 +1163,7 @@
> > 
> **************************************************************************/
> >  int find_something_to_kill(struct player *pplayer, struct unit *punit, int
> *x, int *y)
> >  {
> > -  int a=0, b, c, d, e, m, n, v, i, f, a0, b0, ab, g;
> > +  int a=0, b, min_turns_to_dest, d, e, move_rate, n, v, i, f, a0, b0, ab,
> g;
> Ble - can you please put them at least to separate lines?
> min_turns_to_dest seems too long for me - what about 'delay'? ;) (used that
> in settlers code) - or maybe better 'move_time' :
> move_time = move_cost / move_rate ;p. Doesn't it look lovely? ;))

I've basically agreed with all your suggested changes. Not too sure
about the formatting ones though, I've got to go read up on the coding
style guide.



> > -  if (ferryboat) boatspeed = (unit_flag(ferryboat, F_TRIREME) ? 6 : 12);
> > -  else boatspeed = (get_invention(pplayer, game.rtech.nav) != TECH_KNOWN ?
> 6 : 12);
> > +  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);
> I think putting the statements to separate lines would be better. And I would
> probably rather do:
> 
> boatspeed = 2 * SINGLE_MOVE;
> 
> if ((ferryboat && unit_flag(ferryboat, F_TRIREME)) ||
>     get_invention(pplayer, game.rtech.nav) != TECH_KNOWN)
>    boatspeed *= 2;
> >

Yes! I've even got this in my next bunch of patches, but since you're demanding
it now so be it.  


Isn't the line too long? ;)

Yes.

> > +              if (boatspeed < 9 && c > 2) min_turns_to_dest = 999; /*
> tired of Kaput! -- Syela */
> please statement to separate line; obtw shouldn't it be move_time > 2 ?

OK.

> > +              if (ferryboat) min_turns_to_dest += (warmap.cost[bx][by] +
> move_rate - 1) / move_rate + 1;
> ditto

OK

> > +              else min_turns_to_dest += 1;
> > +              if (unit_flag(punit, F_MARINES)) min_turns_to_dest -= 1;
> ditto
> > +            } else min_turns_to_dest = (warmap.cost[acity->x][acity->y] +
> move_rate - 1) / move_rate;
>   +            } else {
>   +              move_time = (warmap.cost[acity->x][acity->y] + move_rate -
> 1) / move_rate;
>   +            }
> >            } else if (is_sailing_unit(punit))
>   +          } else if (is_sailing_unit(punit)) {
> > -             c = (warmap.seacost[acity->x][acity->y] + m - 1) / m;
> > -          else c = (real_map_distance(punit->x, punit->y, acity->x,
> acity->y)
> > -               * SINGLE_MOVE) / m;
> > -          if (c > 1) {
> > +             min_turns_to_dest = (warmap.seacost[acity->x][acity->y] +
> move_rate - 1) / move_rate;
> too long line ;)
> > +          else min_turns_to_dest = (real_map_distance(punit->x, punit->y,
> acity->x, acity->y)
> > +               * SINGLE_MOVE) / move_rate;
>   +          } else {
>   +            move_time =
>   +              (real_map_distance(punit->x, punit->y, acity->x, acity->y) *
>   +               SINGLE_MOVE) / move_rate;
>   +          }
>   +
> > +          if (min_turns_to_dest > 1) {
> >              n = ai_choose_defender_versus(acity, punit->type);
> >              v = get_virtual_defense_power(punit->type, n, acity->x,
> acity->y) *
> >                  unit_types[n].hp * unit_types[n].firepower *
> > @@ -1291,7 +1292,7 @@
> >                (!(acity->ai.invasion&1))) b -= 40; /* boats can't conquer
> cities */
> >            if (!punit->id
> >           && !unit_really_ignores_citywalls(punit)
> > -         && c > (player_knows_improvement_tech(aplayer, B_CITY) ? 2 : 4)
> > +         && min_turns_to_dest > (player_knows_improvement_tech(aplayer,
> B_CITY) ? 2 : 4)
> ETOOLONG
> >           && !city_got_citywalls(acity))
> >         d *= 9;
> >  
> > @@ -1304,7 +1305,7 @@
> >  
> >            if (!is_ground_unit(punit) && !is_heli_unit(punit) && !pdef) b0
> = 0;
> >  /* !d instead of !pdef was horrible bug that led to yoyo-ing AI warships
> -- Syela */
> > -          else if (c > THRESHOLD) b0 = 0;
> > +          else if (min_turns_to_dest > THRESHOLD) b0 = 0;
> What about explaining and elaborating b0 ? ;))
> >            else if ((is_ground_unit(punit) || is_heli_unit(punit)) &&
> >                     acity->ai.invasion == 2) b0 = f * SHIELD_WEIGHTING;
> >            else {
> ...
> 
> Well, honestly, I would welcome rather patches for smaller scope, but much
> higher focus, attempting to clean given function completely - this way it's
> hard for me to review something, as only de-facto details are changed (sure,
> it
> helps, but not much) and one can't see them in the context (as the context is
> messy ;) and you can easily change them when you will clean this function by
> yourself.
> 

I'm not quite sure what you are asking here. I believe you want me to finish
replacing all the one letter variables in a single function first, and make
minor
cleanups as I go.

What my plan of attack was:

Identify the uses of a one letter variable(m)
Where m is used to represent 2 or 3 different things get rid of them,
substitute
in better names, and make sure every time move_rate is referenced, it is the
same
thing as elsewhere.

Also get rid of hardcoded nos where I understand the consequences. I
deliberately
did not touch formatting, because I assumed the maintainer would let me know
how
he wanted the patch formatted.

__________________________________________________
Do You Yahoo!?
Send your FREE holiday greetings online!
http://greetings.yahoo.com