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

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Sat, Feb 16, 2002 at 09:29:21PM CET, I got a letter,
where Gregory Berkolaiko <gberkolaiko@xxxxxxxxxxx> told me, that...
>  --- Petr Baudis <pasky@xxxxxxxxxxx> wrote: 
> >   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
> ;)

Oh! I thought about mentioning some mails, but forgot to do that.. and I didn't
remember this one at all.. shame on me! :) BTW this was my very first touch of
the AI (now it's obviously a lot re-worked).

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

I'll have a look when doing the next patch. Thanks :).

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

Huh? Like?

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

Obviously shortened 'belligerence', but that's aawwffuuulllyyy lloooonngg.

> > +  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) {

Oh yes! I already changed one this if, but this is a good catch too :).

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

Hmm.. after all, shouldn't we just exit when we're a ferryboat, independently
on our charge? Why to waste them on the enemies? (if answer is "yes, we
should", i will leave this code as-is now and produce three-liner patch on the
top of this :). [NOT FIXED]

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

True. Comment added.

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

You mean !can_unit_attack_tile() ? It's just intended to remove completely
unneccessary indentation level... IIRC it was you who suggested doing this.

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

Thanks.

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

Can you elaborate, please? Why it is wrong, what would be more proper
interpretation of that formula etc.

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

Oh, ok :).

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

Is it used anywhere else?

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

Please elaborate.

> It can be done (1) in this patch, (2) before this patch or (3) after this
> patch.

In this patch or after this patch! :^) This patch should already at least a lot
help it moving the variables declarations to deeper blocks etc. And you have a
better idea what are you already doing :).

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

Sure. Can you be please more concrete?

Fixed patch attached.

-- 

                                Petr "Pasky" Baudis

* elinks maintainer                * IPv6 guy (XS26 co-coordinator)
* IRCnet operator                  * FreeCiv AI hacker
.
No one can feel as helpless as the owner of a sick goldfish.
.
Public PGP key && geekcode && homepage: http://pasky.ji.cz/~pasky/

findvictim-cleanup.patch
Description: Text document