[Freeciv-Dev] Re: [PATCH] aiunit.c ai_military_findvictim() cleanup (PR#1264)

[Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx>]

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

>   attached patch cleans the ai_military_findvictim function in aiunit.c
> of AI.

Good.  At last some real cleanup, not just tidying up ;)

>   As you can see, I've left some TODOs and dummy variables there, for
> one
> simple reason - aside of better idea how the equation actually works,

you can quote the literature too:
http://arch.freeciv.org/freeciv-dev-200201/msg00002.html
;)


> next patch to move this equation to separate function, possibly even
> preparing
> for some big cleanup of the fat functions (which seem to share also a

be careful, some fat function don't get the equation right (misplaced
brackets)

>   Happy reviewing ;),

oh thank you :)


/*************************************************************************
> -This looks at tiles neighbouring the unit to find something to kill or
> -explore. It prefers tiles in the following order:
> +This function looks at tiles neighbouring the unit in order to find
> something
> +to kill or explore.  It prefers tiles in the following order:
> +
>  1. Undefended cities
>  2. Huts
>  3. Enemy units weaker than the unit
>  4. Land barbarians also like unfrastructure tiles (for later pillage)
> -If none of the following is there, nothing is chosen.
>  
> -work of Syela - mostly to fix the ZOC/goto strangeness
> +If none of the following is there, nothing is chosen.

you should probably explain the flow of the function here too


>  static int ai_military_findvictim(struct player *pplayer, struct unit
> *punit,
> -                               int *dest_x, int *dest_y)
> +                                  int *dest_x, int *dest_y)
>  {
> -  int x, y;
> -  int best = 0, a, b, c, d, e, f;
> -  struct unit *pdef;
> -  struct unit *patt;
> -  struct city *pcity;
> -  x = punit->x;
> -  y = punit->y;

bellig is an awful name, but I have no better...

> +  int bellig = unit_belligerence_primitive(punit);
> +  int x = punit->x, y = punit->y;
> +  int best = 0;
> +  

[...]

> +  /* Ferryboats do not attack. -- Syela */
> +  
>    if (get_transporter_capacity(punit)) {

To please Raimar:
if (get_transporter_capacity(punit) > 0) {


> -    unit_list_iterate(map_get_tile(x, y)->units, aunit)
> -      if (!is_sailing_unit(aunit)) return(0);
> -    unit_list_iterate_end;
> -  } /* ferryboats do not attack.  no. -- Syela */

Is there no better way to say if a boat has passangers.
If the answer is "no" then you should write it, and separate the code
below into it.

> +    unit_list_iterate(map_get_tile(x, y)->units, aunit) {
> +      if (!is_sailing_unit(aunit)) return 0;
> +    } unit_list_iterate_end;
> +  }
>  
>    adjc_iterate(x, y, x1, y1) {
> -    pdef = get_defender(punit, x1, y1);
> +    struct unit *pdef = get_defender(punit, x1, y1);
> +    
>      if (pdef) {
> -      patt = get_attacker(punit, x1, y1);

you remove this comment but don't explain why patt should be used.
bad...

> -/* horsemen in city refused to attack phalanx just outside that was
> -bodyguarding catapult - patt will resolve this bug nicely -- Syela */
> -      if (can_unit_attack_tile(punit, x1, y1)) { /* thanks, Roar */
> -        d = unit_vulnerability(punit, pdef);

[...]

> +      struct unit *patt = get_attacker(punit, x1, y1);
> +
> +      if (!can_unit_attack_tile(punit, x1, y1))
> +        continue;
> +
> +      /* If we are in the city, let's deeply consider defending it. */
> +      if (map_get_city(x, y)

what is the meaning of the inequality?

> +          && get_total_defense_power(pdef, punit) *
> +             get_total_defense_power(punit, pdef) >= /* didn't like >
> -- Syela */
> +             get_total_attack_power(patt, punit) *
> +             get_total_attack_power(punit, pdef)
> +          && unit_list_size(&(map_get_tile(punit->x,
> punit->y)->units)) < 2
> +          && get_total_attack_power(patt, punit) > 0) {

[...]

> +      } else {
> +        int vuln = unit_vulnerability(punit, pdef);
> +        /* The total possible attack power we can throw on the victim.
> Note
> +         * that we will even square this. */
> +        int attack = reinforcements_value(punit, pdef->x, pdef->y)
> +                     + bellig;

Appetency is used wrongly here (although I understand what you mean)
I think attractiveness is better

> +        /* Something like 'appetency' of the victim, how nice it would
> be to
> +         * destroy it. Larger value, worse loss for enemy. */
> +        int benefit = unit_type(pdef)->build_cost;
> +        /* We're only dealing with adjacent victims here. */
> +        /* TODO: Remove this when moving the desire equation to
> separate
> +         * function. */
> +        int move_cost = 0;
> +        /* The possible loss when we would loose this unit. */
> +        /* TODO: Possibly remove this when moving the desire equiation
> to
> +         * separate function? */
> +        int loss = unit_type(punit)->build_cost;
> +        
> +        attack *= attack;
> +        

I think "health" part of your comment is wrong.

> +        /* If the victim is in the city, we increase the benefit and
> correct
> +         * it with our health because there may be more units in the
> city
> +         * stacked, and we won't destroy them all at once, so in the
> next
> +         * turn they may attack us. So we shouldn't send already
> injured
> +         * units to useless suicide. */
> +        if (map_get_city(x1, y1)) {
> +          benefit = (benefit + 40) * punit->hp / unit_type(punit)->hp;
>          }
> +        
> +        /* If we have non-zero belligerence... */
> +        if (attack && is_my_turn(punit, pdef)) {
> +          int desire;
> +          
> +          /* TODO: This equation is simplified version of much worse
> ones in
> +           * that long fat routines, but it's still a common pattern,
> so we
> +           * will can this equation to one separate readable function.
> */
> +         

well done!

> +          /*         appetency          danger */ 
> +          desire = ((benefit * attack - loss * vuln) *
> SHIELD_WEIGHTING
> +                    / (attack + vuln) - move_cost * SHIELD_WEIGHTING);
> +          
> +          /* No need to amortize! */
> +          
> +          if (desire > best && ai_fuzzy(pplayer, 1)) {
> +            freelog(LOG_DEBUG, "Better than %d is %d (%s)",
> +                          best, desire, unit_type(pdef)->name);

surely some style issues below

> +            best = desire; *dest_y = y1; *dest_x = x1;
> +            
> +          } else {
> +            freelog(LOG_DEBUG, "NOT better than %d is %d (%s)",
> +                          best, desire, unit_type(pdef)->name);
> +          }
>          }
>        }

[...]

> +      
> +    } else {
> +      struct city *pcity = map_get_city(x1, y1);
> +      
> +      /* No defender... */
> +     
> +      /* ...and free foreign city waiting for us. Who would resist! */
> +      if (pcity && pcity->owner != pplayer->player_no &&
> is_ground_unit(punit)
> +          && map_get_terrain(punit->x, punit->y) != T_OCEAN) {
> +        best = 99999; *dest_y = y1; *dest_x = x1;
> +      }
> +
> +      /* ...or tiny pleasant hut here! */
> +      if (map_has_special(x1, y1, S_HUT) && best < 99999
> +          && could_unit_move_to_tile(punit, punit->x, punit->y, x1,
> y1)
> +          && !is_barbarian(unit_owner(punit))
> +          && punit->ai.ai_role != AIUNIT_ESCORT
> +          && !punit->ai.charge /* Above line doesn't seem to work. :(
> */
> +          && punit->ai.ai_role != AIUNIT_DEFEND_HOME) {
>          best = 99998; *dest_y = y1; *dest_x = x1;
>        }
> -      if( is_land_barbarian(pplayer) && best == 0 &&
> +      

How about writing a separate function 
consider_pillaging(x, y, punit) ?
in general, not only barbarians should pillage.

> +      /* If we have nothing to do, we can at least pillage something,
> hmm? */
> +      if (is_land_barbarian(pplayer) && best == 0 &&
>            get_tile_infrastructure_set(map_get_tile(x1, y1)) &&
> -          could_unit_move_to_tile(punit, punit->x, punit->y, x1, y1) )
> {
> +          could_unit_move_to_tile(punit, punit->x, punit->y, x1, y1))
> {
>          best = 1; *dest_y = y1; *dest_x = x1;
> -      } /* next to nothing is better than nothing */
> +      }
>      }
>    } adjc_iterate_end;
>  

Verdict:  The patch improves the readability of the code very much.
However I believe that a lot of code should be separated into smaller
functions.  It can be done (1) in this patch, (2) before this patch or 
(3) after this patch.  Depending on this issue, patch is (1) not
complete,
(2) too early or (3) almost ready.  "Almost" refers to the fact that some
places still lack documentation.

Best,
G.

__________________________________________________
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