[Freeciv-Dev] Re: (PR#12781) Create AI guard API

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=12781 >

I'm not an AI expert but here are a few low-level comments.

Benedict Adamson wrote:

> +/**************************************************************************
> +  Do sanity checks on a guard, reporting error messages to the log
> +  if necessary.
> +**************************************************************************/
> +void aiguard_check_guard(const struct unit *guard)
> +{
> +  const struct unit *charge_unit = find_unit_by_id(guard->ai.charge);
> +  const struct city *charge_city = find_city_by_id(guard->ai.charge);
> +  assert(BODYGUARD_NONE <= guard->ai.charge);
> +  if (charge_unit && charge_unit->ai.bodyguard == BODYGUARD_WANTED
> +      && charge_unit->tile != guard->tile) {
> +    BODYGUARD_LOG(LOG_VERBOSE, (struct unit *)guard,
> +               "guard probably en route to charge");

Casts like this are bad.  BODYGUARD_LOG/UNIT_LOG should take a const 
unit if possible.

> +/**************************************************************************
> +  Remove assignment of bodyguard for a unit.
> +**************************************************************************/
> +void aiguard_clear_guard(struct unit *charge)
> +{
> +  CHECK_CHARGE_UNIT(charge);
> +  if (0 < charge->ai.bodyguard) {
> +    struct unit *guard = find_unit_by_id(charge->ai.bodyguard);
> +
> +    if (guard && guard->ai.charge == charge->id) {
> +      /* charge doesn't want us anymore */
> +      guard->ai.charge = BODYGUARD_NONE;
> +    }
> +  }
> +
> +  charge->ai.bodyguard = BODYGUARD_NONE;
> +}

Is it correct that a unit may only have one bodyguard but a city may 
have many?  So a similar function for a city wouldn't be useful - nor is 
there any quick way to find out what units are guarding a city...

> +  assert(charge);
> +  assert(guard);
> +  assert(guard != charge);

On some platforms you can't assert on a pointer value.  You need

   assert(charge != NULL);

> +/**************************************************************************
> +  Which unit (if any) has a guard been assigned to.
> +  Returns NULL if the unit is not the guard for a unit.
> +**************************************************************************/
> +struct unit *aiguard_charge_unit(struct unit *guard)
> +{
> +  CHECK_GUARD(guard);
> +  return find_unit_by_id(guard->ai.charge);
> +}
> +
> +/**************************************************************************
> +  Which city (if any) has a guard been assigned to.
> +  Returns NULL if the unit is not a guard for a city.
> +**************************************************************************/
> +struct city *aiguard_charge_city(struct unit *guard)
> +{
> +  CHECK_GUARD(guard);
> +  return find_city_by_id(guard->ai.charge);
> +}

Wait a minute.  Units and cities can have the same IDs right (even if 
it's not true it's not a safe assumption)?  So guard->ai.charge may 
return a different unit or city depending on which function gets called? 
  That's definitely not right.  Better to have a charge_unit and 
charge_city values.

Also, you handle the unit-dies/city-dies case because it returns NULL. 
But what if the unit or city changes hands?  The callers have to figure 
this out right?  Maybe it should use 
player_find_unit_by_id/player_find_city_by_id (same for other 
functions)?  Alternately the caller needs to make sure it doesn't 
control its enemies units!  Maybe a unit should consider bodyguarding 
its allies but this would have to be very carefully done.

> diff -ruN -Xvendor.freeciv.current/diff_ignore 
> vendor.freeciv.current/ai/aiguard.h freeciv.PR12781/ai/aiguard.h
> --- vendor.freeciv.current/ai/aiguard.h       1970-01-01 01:00:00.000000000 
> +0100
> +++ freeciv.PR12781/ai/aiguard.h      2005-04-13 23:54:53.000000000 +0100

> +#ifndef NDEBUG
> +#define CHECK_GUARD(guard) aiguard_check_guard(guard)
> +#define CHECK_CHARGE_UNIT(charge) aiguard_check_charge_unit(charge)
> +#else
> +#define CHECK_GUARD(guard) {}
> +#define CHECK_CHARGE_UNIT(charge) {}
> +#endif

I think this isn't quite right.  Consider code like:

do
   CHECK_GUARD(guard);
while (guard = find_guard());

the {} isn't like a normal expression so it doesn't substitute in 
cleanly.  I've been using "(void)0" here which I think is an old 
freeciv-ism:

   #define NOOP (void)0

but another common thing is "do { } while(0)".

> +enum bodyguard_enum {
> +  BODYGUARD_WANTED = -1,
> +  BODYGUARD_NONE
> +};


>    my_snprintf(buffer, sizeof(buffer),
>                "%s's bodyguard %s[%d] (%d,%d){%s:%d@%d,%d} ",
>                unit_owner(punit)->name, unit_type(punit)->name,
>                punit->id, punit->tile->x, punit->tile->y,
> -           s, id, ptile->x, ptile->y);
> +           s, id, charge_x, charge_y);

There's a TILE_XY macro that you should use here (it works with a NULL 
ptile).

-jason