[freeciv-ai] Re: ai bodyguard code cleanup part#1 v2

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- "Per I. Mathisen" <per@xxxxxxxxxxx> wrote:

> +#define LOG_BODYGUARD LOG_DEBUG
> +
> +enum bodyguard_enum {
> +  BODYGUARD_WANTED=-1,

I find this hard to read. I would prefer

BODYGUARD_WANTED = -1,




>      /* change homecity to this city */
>      /* FIXME: it is stupid to change homecity if the unit has no homecity
> @@ -1054,7 +1055,7 @@
>            && could_unit_move_to_tile(punit, punit->x, punit->y, x1, y1) != 0
>            && !is_barbarian(unit_owner(punit))
>            && punit->ai.ai_role != AIUNIT_ESCORT
> -          && punit->ai.charge == 0 /* Above line doesn't seem to work. :( */
> +          && punit->ai.charge == BODYGUARD_NONE /* Above line doesn't seem
> to work. :( */
>            && punit->ai.ai_role != AIUNIT_DEFEND_HOME) {
>          SET_BEST(99998);
>          continue;

> +  int x, y, id = punit->id;
>  
> -  if (aunit && aunit->owner == punit->owner) { x = aunit->x; y = aunit->y; }
> -  else if (acity && acity->owner == punit->owner) { x = acity->x; y =
> acity->y; }
> -  else { /* should be impossible */
> -    punit->ai.charge = 0;
> +  if (aunit && aunit->owner == punit->owner) { 
> +    /* protect a unit */
> +    x = aunit->x; 
> +    y = aunit->y; 
> +  } else if (acity && acity->owner == punit->owner) { 
> +    /* protect a city */
> +    x = acity->x; 
> +    y = acity->y;

This is weird. Why is bodyguard code being relied on to protect cities?
 





> +  /* is this insanity supposed to be a sanity check? -- per */
> +  if (aunit && unit_list_find(&map_get_tile(x, y)->units, id) 
> +      && aunit->ai.bodyguard != BODYGUARD_NONE) {
>      aunit->ai.bodyguard = id;
> +  }
>  }

Huh??? Explain what is happening here. I can't follow this code.


> +      punit->ai.bodyguard = BODYGUARD_NONE;
>  
>      if( is_barbarian(pplayer) )  /* barbarians must have more currage */
> -      punit->ai.bodyguard = 0;

currage? Are the barbarians dropping by India later? Courage, Per.

>
>      if (goto_is_sane(punit, dest_x, dest_y, TRUE) && punit->moves_left > 0
> &&
>         (!ferryboat || 
>         (real_map_distance(punit->x, punit->y, dest_x, dest_y) < 3 &&
> -       (punit->ai.bodyguard == 0 || unit_list_find(&(map_get_tile(punit->x,
> +       (punit->ai.bodyguard == BODYGUARD_NONE ||
> unit_list_find(&(map_get_tile(punit->x,
>         punit->y)->units), punit->ai.bodyguard) || (dcity &&
>         !has_defense(dcity)))))) {

GB, you have any comments to make on this line. I don't see why real map
distance is used. Since when have planes been bodyguards?

I'm really not at all ok with these lines. It seems a wrong estimate of
distance.


> +      if (punit->ai.bodyguard < BODYGUARD_NONE) { 
>       adjc_iterate(punit->x, punit->y, i, j) {
>         unit_list_iterate(map_get_tile(i, j)->units, aunit) {
>           if (aunit->ai.charge == punit->id) {
> @@ -1359,42 +1375,73 @@
>  }
>  

> +  int dist, def, best = 0;
> +  int toughness = unit_vulnerability_virtual(punit);
> +
> +  if (toughness == 0) {
> +    /* useless */
> +    return 0; 
> +  }
> +

Why unit_vulnerability? I can't remember off the top of my head what it does,
wouldn't defencepower be good enough?

> +  unit_list_iterate(pplayer->units, buddy) {
> +    if (buddy->ai.bodyguard == BODYGUARD_NONE /* should be !=
> BODYGUARD_WANTED */

This line is interesting. Any value besides bodyguard_none means we want a
bodyguard.

> +        || !goto_is_sane(punit, buddy->x, buddy->y, TRUE)
> +        || unit_type(buddy)->move_rate > unit_type(punit)->move_rate
> +        || unit_type(buddy)->move_type != unit_type(punit)->move_type) { 
> +      continue;
> +    }

I'm taking it the AI movement code makes faster bodyguards outpace slower
units.
Shouldn't that be the bug that is fixed, rather than kludging the AI code not
to use units with more move_rate as bodyguards?

I am very much not ok with this, even though this is an existing bug. Can you
please fix it Per? I'm thinking of Mech Inf not being used at all.

> +    dist = unit_move_turns(punit, buddy->x, buddy->y);
> +    def = (toughness - unit_vulnerability_virtual(buddy)) >> dist;
>      freelog(LOG_DEBUG, "(%d,%d)->(%d,%d), %d turns, def=%d",
> +         punit->x, punit->y, buddy->x, buddy->y, dist, def);
> +
> +    /* sanity check: if we already have a bodyguard, but don't know about
> it,
> +       we don't need a new one... this check should be superfluous
> eventually */
> +    unit_list_iterate(pplayer->units, body) {
> +      if (body->ai.charge == buddy->id) { 
> +        /* if buddy already has a bodyguard, ignore it */
> +        def = 0;
> +        /* we should  buddy->bodyguard = body->id;  here */
> +      }
> +    } unit_list_iterate_end;
> +    if (def > best && ai_fuzzy(pplayer, TRUE)) { 
> +      *aunit = buddy; 
> +      best = def; 
> +    }
> +  } unit_list_iterate_end;
> +

I don't see a reason why we have to stick with only one bodyguard. If the
square the guarded unit is a fortress or city tile we can have multiple
bodyguards.

<snip>

I like everything else about this patch. A much needed one in my opinion. Good
work Per.

Aloha,
RK.

I never killed a man that didn't deserve it. -Mickey Cohen

__________________________________________________
Do You Yahoo!?
HotJobs - Search Thousands of New Jobs
http://www.hotjobs.com