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

["Per I. Mathisen" <per@xxxxxxxxxxx>]

On Sat, 17 Aug 2002, Raahul Kumar wrote:
> > +#define LOG_BODYGUARD LOG_DEBUG
> > +
> > +enum bodyguard_enum {
> > +BODYGUARD_WANTED=-1,
>
> I find this hard to read. I would prefer
>
> BODYGUARD_WANTED = -1,

Oh, sorry. My bad.

> > +  /* protect a city */
> > +  x = acity->x;
> > +  y = acity->y;
>
> This is weird. Why is bodyguard code being relied on to protect cities?

Because no other code does...? ;)

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

If we are a bodyguard, then aiunit is our charge. That is, if we are a
_unit_ bodyguard. A bit above this point in the code, we try a goto. So we
check if we are stacked with our charge. If we are, we teach our charge
that it has a bodyguard.

Why a charge shouldn't know that it has a bodyguard before it is stacked
with it, is beyond me, though.

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

Feel free to fix.

BTW, Greg, speaking of flying, can you post an updated version of your
flying AI patch?

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

It needs to be adjusted for hp and firepower and POWER_DIVIDER.

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

No, it means we _wanted_ a bodyguard. We could have gotten one that died.
Or was stolen by some other part of the code.

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

No, it doesn't.

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

You got it exactly backwards. It doesn't want to use bodyguards that are
slower than the charge, because that would slow down the primary unit.
Movement is too valuable to waste, I suppose. Though I think we may want
to make some exceptions.

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

What, are you crack?? The AI uses mech inf all the time! It even uses mech
inf as bodyguards for stinkin' riflemen! (You see, riflemen have att >
def, so they are attack type unit while mech inf has def <= att, so it is
a bodyguard type of unit...)

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

Bodyguards move around and thus become vulnerable. I really don't want
multiple bodyguards. The very few cases where it is useful does not
warrant the much bigger code complexity.

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

Thanks, good to hear :)

Yours
Per