[freeciv-ai] Re: Patch Army

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Jordi Negrevernis i Font <jorneg@xxxxxxxxxxx> wrote:
> 
>     Well, i've been developing a patch for giving to the ai the capacity 
> to make coordinate mass attacks against a city. It does handle a total 
> of 8 attacking units, 4 defensive units and 4 naval units.

Not quite enough units. Since your code assesses the city's defence, attacking
units should be 2-3 times the amount of defending units in the city. 

This is a very good start Jordi. You have done some fantastic work with this
patch. Please keep going!
 
>     - How do i know if there is a city in the attack direction?

I don't know what you mean by this. Your code selects a dest city, and your
units move there. 

>     - Which are the wants for the units? While debugging i get want > 
> 101 many times, before i evaluate army_advisor_choice!! I knew they were 
> between 0 and 100!!

Ross? This is probably not your fault Jordi. 

>     - i need to calculate the mean of a couple of units. But i get a 
> wrong result! Which is the mean of (77, 15) and (5,12)  if the 
> coordinate of the map are (80,50) ???!!!!

I am guessing that you want to figure out the average travel time from the
co-ords (77,15) and (5,12) to (80,50) ? I'm afraid you will have to use warmap
for ground units. The algorithm will be

Iterate through units on 77,15
pick slowest unit 
generate warmap for dest co-ords(80,50)
return turns to dest

Do the same for 5,12

Add (result1 + result 2)/2, and you will have avg travel time. 
 
Note: This function may *not* work properly for your armies. Naval units can't
go everywhere. Only works for ground and air units. 
 
> +             aiarmy.c        \
> +             aiarmy.h        \

Jordi, your functions seem to remove the need for FSTK, PAW and some other ai
functions. You should probably re-read those functions and chop out everything
you've alread done in your code. 

> +/**********************************************************************
> +  Find safe city to atack another on the same continent.
                        ^
                       attack



> +/**********************************************************************
> +  Function that initializes the armies.
> +***********************************************************************/
> +void ai_init_armies(struct player *pplayer)
> +{ int ndx, i;

You need fewer armies of larger size. It is better to have 2 armies of size 10
than 4 armies of size 5.


> +
> +/**********************************************************************
> +  Function for compute the accept switches of an army.
                        ^
                  computing the type and amount of units allowed to join an
army.



> +/**********************************************************************
> +  Function for assigning continents to armies.
> +  its assigns the seven continents with more cities to player's armies.
> +***********************************************************************/

I disagree. You should assign the same continent to all your armies until there
are no more enemy units/cities on that continent.
 
> +void assign_army_continent(struct player *pplayer)
> +{ struct {
> +    int ncont;
> +    int count;
> +  } continent[50], x;   /* arbitrary value */
> +  struct pcity *pcity;
> +  int numcont, n, i, ndx;

You use a lot of magic numbers. 50 and 9999 keep popping up in the code.

> +/**********************************************************************
> +  Function for assign or not a unit to an army.
> +***********************************************************************/
> +bool assign_unit_to_army(struct player *pplayer, struct ai_army *parmy,
> struct unit *punit)
> +{ bool success = FALSE;
> +  int ndx;
> +
> +  if ( is_ground_unit(punit) &&
> +       ( parmy->continent == map_get_continent(punit->x, punit->y) ||
> parmy->continent == 0 ) ) {
> +    /* we try to assign to that army depending on type */
> +    if ( (unit_has_role(punit->type, L_ATTACK_FAST) ||
> +         unit_has_role(punit->type, L_ATTACK_STRONG)) &&
> parmy->accept_attack) {
> +      /* attacking units */
> +      for (ndx = 0; ndx < MAX_ATTACK_PER_ARMY; ndx++) {
> +        if ( parmy->attack_unit[ndx] == 0 ) {
> +          parmy->attack_unit[ndx] = punit->id;
> +          success = TRUE;
> +          break;
> +        }
> +      }
> +    } else if ( unit_has_role(punit->type, L_DEFEND_GOOD) &&
> parmy->accept_defense) {
> +      /* defending units */
> +      for (ndx = 0; ndx < MAX_DEFENSE_PER_ARMY; ndx++) {
> +        if ( parmy->defense_unit[ndx] == 0 ) {
> +          parmy->defense_unit[ndx] = punit->id;
> +          success = TRUE;
> +          break;
> +        }
> +      }
> +    }
> +  }
> +
> +  if (is_water_unit(punit->type) && parmy->accept_naval) {
> +    /* sea attacking units */
> +    for (ndx = 0; ndx < MAX_NAVAL_PER_ARMY; ndx++) {
> +      if ( parmy->naval_unit[ndx] == 0 ) {
> +        parmy->naval_unit[ndx] = punit->id;
> +        success = TRUE;
> +        break;
> +      }
> +    }
> +  }
> +
> +  (void)recalculate_accept_units(parmy);

I'm not sure I like your casting of recalculate_accept_units to void.
I could be wrong, but I didn't notice you using the return value anywhere.

> +/**********************************************************************
> +  Function for know if a unit goes near a city.
> +***********************************************************************/

know = knowing

> +/**********************************************************************
> +  Function for assigning units of ai to armies.
> +***********************************************************************/
> +void assign_units_to_army(struct player *pplayer, struct ai_army *parmy)
> +{ struct unit *punit;
> +  struct city *pcityobj = find_city_by_id(parmy->objective_id);
> +
> +  if (!parmy->active )
> +    return;
> +
> +  if ( parmy->state == ARMY_STATE_FORMING || parmy->state == ARMY_STATE_IDLE
> ) {
> +    /* it's ok */
> +  } else {
> +    return;
> +  }
> +
> +  if (!pcityobj) {
> +    return;
> +  }
> +
> +  unit_list_iterate(pplayer->units, punit) {
> +    if (is_military_unit(punit)) {
> +      if ( punit->ai.ai_role == AIUNIT_NONE ||
> +           punit->ai.ai_role == AIUNIT_PILLAGE ||
> +           punit->ai.ai_role == AIUNIT_ARMY ) {
> +        /* the unit is idle or not doing any damage, so take it! */
> +        if ( is_unit_in_an_army(pplayer, punit->id) )
> +          continue;
> +
> +        if ( stay_and_defend(punit) )

Is this in the style guide? Spaces after (.

> +/**********************************************************************
> +  Desire to attack that city, based on city trade, our attack force and
> +  its defence capability.
> +***********************************************************************/
> +double army_desire_attack_city(struct ai_army *parmy, struct city *pcity)
> +{ int ndx;
> +  struct unit *punit;
> +  int defense = 0;
> +  int attack = 0;
> +
> +  defense = assess_defense(pcity);
> +
> +  for (ndx = 0; ndx < MAX_ATTACK_PER_ARMY; ndx++) {
> +    if ( parmy->attack_unit[ndx] != 0 ) {
> +      punit = find_unit_by_id(parmy->attack_unit[ndx]);
> +      if (!punit)
> +        continue;
> +      attack += unit_belligerence_basic(punit);
> +    }
> +  }
> +
> +  if (attack == 0)
> +    attack = 1;
> +
> +  if ( defense == 0 ) {
> +    return 99999;
> +  } else {
> +   return ((pcity->trade_prod + (pcity->size * POPULATION_WEIGHT) * attack)
> / defense);
> +  }
> +
> +}


Good function. You don't consider wonders/capital city though. 

> +          desire /= real_map_distance(parmy->army_x, parmy->army_y,
> acity->x, acity->y);
> +          /* is that occupied city mine? */
> +          if (acity->original == pplayer->player_no) desire *= 1.50;
> +          /* favour cities in capital continent */
> +          if ( capital_continent == map_get_continent(acity->x, acity->y) )
> desire *= 1.50;

I though it was standard Freeciv policy not to use Floating point math. You may
have missed the discussion. 1.50 = 3/2

> +/**********************************************************************
> +  Function to group the atttacking units prior to attack a city.

attacking! Sorry to nitpick your spelling.

> +  Returns a TRUE if are all together and can proceded to attack.
> +***********************************************************************/
> +bool army_group_troups(struct player *pplayer, struct ai_army *parmy)
> +{ struct unit *punit_first = NULL;
> +  struct unit *punit = NULL;
> +  struct unit_type *punittype;
> +  struct city *pcityobj = NULL;         /* city victim */
> +  struct city *pcitynear = NULL;        /* our city nearest */
> +  int first_unit_id = 0;
> +  int ndx;
> +  int res;
> +  int result = FALSE;
> +
> +  /* first unit is the leader unit, find it */
> +  for (ndx = 0; ndx < MAX_ATTACK_PER_ARMY; ndx++) {
> +    if ( parmy->attack_unit[ndx] != 0 ) {
> +      first_unit_id = parmy->attack_unit[ndx];
> +      punit_first = find_unit_by_id(first_unit_id);
> +      break;
> +    }
> +  }
> +  /* sanity check */
> +  if ( !first_unit_id ) {
> +    return FALSE;
> +  }
> +
> +  switch (parmy->state)
> +  {
> +    case ARMY_STATE_ATTACKING:
> +      /* if we are attacking doesn't need to enter here */
> +      return TRUE;
> +
> +    case ARMY_STATE_FORMING:
> +
> +      /* we retrieve the objective */
> +      pcityobj = find_city_by_id(parmy->objective_id);
> +      if (!pcityobj ) {
> +        /* fall back */
> +        parmy->state = ARMY_STATE_FORMING;
> +        parmy->objective_id = 0;
> +        parmy->attack_needed = 0;
> +        parmy->meeting_id = 0;
> +        return FALSE;
> +      }
> +
> +      /* we retrieve the nearest city of objective */
> +      /* we find the nearest city of the objective to coordinate attack to a
> city.
> +       for a unit is not necessary */
> +      pcitynear = find_nearest_friendly_city(punit_first, pcityobj->x,
> pcityobj->y);
> +      if (!pcitynear ) {
> +        /* fall back */
> +        parmy->state = ARMY_STATE_FORMING;
> +        parmy->objective_id = 0;
> +        parmy->attack_needed = 0;
> +        parmy->meeting_id = 0;
> +        return FALSE;
> +      } else {
> +        parmy->meeting_id = pcitynear->id;
> +        if ( !pplayers_allied(city_owner(pcitynear), pplayer) ) {
> +          /* fall back */
> +          parmy->state = ARMY_STATE_FORMING;
> +          parmy->objective_id = 0;
> +          parmy->attack_needed = 0;
> +          parmy->meeting_id = 0;
> +          return FALSE;
> +        }
> +      }
> +
> +      if (army_power(parmy) > parmy->attack_needed ) {
> +        /* we are grouping the forces if we have an objective that we can
> attack! */
> +        parmy->state = ARMY_STATE_GROUPING;
> +      }
> +
> +      break;
> +
> +    case ARMY_STATE_GROUPING:
> +
> +      /* we retrieve the objective */
> +      pcityobj = find_city_by_id(parmy->objective_id);
> +      if (!pcityobj ) {
> +        /* fall back */
> +        parmy->state = ARMY_STATE_FORMING;
> +        parmy->objective_id = 0;
> +        parmy->attack_needed = 0;
> +        parmy->meeting_id = 0;
> +        return FALSE;
> +      }
> +
> +      /* we retrieve the nearest city of objective */
> +      pcitynear = find_city_by_id(parmy->meeting_id);
> +      if (!pcitynear ) {
> +        /* fall back */
> +        parmy->state = ARMY_STATE_FORMING;
> +        parmy->objective_id = 0;
> +        parmy->attack_needed = 0;
> +        parmy->meeting_id = 0;
> +        return FALSE;
> +      } else {
> +        if ( !pplayers_allied(city_owner(pcitynear), pplayer) ) {
> +          /* fall back */
> +          parmy->state = ARMY_STATE_FORMING;
> +          parmy->objective_id = 0;
> +          parmy->attack_needed = 0;
> +          parmy->meeting_id = 0;
> +          return FALSE;
> +        }
> +      }
> +
> +      break;
> +
> +    default:
> +      return FALSE;
> +  }
> +
> +  /* there is some unit far enough to wait? */
> +  res = TRUE;
> +  for (ndx = 0; ndx < MAX_ATTACK_PER_ARMY; ndx++) {
> +    /* attacking units */
> +    if ( parmy->attack_unit[ndx] != 0 ) {

> +      } else {
> +        /* go there */
> +        punit->goto_dest_x = pcitynear->x;
> +        punit->goto_dest_y = pcitynear->y;
> +        set_unit_activity(punit, ACTIVITY_GOTO);
> +        result = do_unit_goto(punit, GOTO_MOVE_ANY, FALSE);
> +        if (result == GR_DIED) {
> +          /* We're dead. */
> +          parmy->attack_unit[ndx] = 0;
> +        }
> +      }
> +    }
> +  }
> +
> +  for (ndx = 0; ndx < MAX_DEFENSE_PER_ARMY; ndx++) {
> +      /* go to nearest city! */
> +      if ( (punit->x == pcitynear->x) && (punit->y == pcitynear->y) ) {
> +        /* we are in the city, is the unit damaged?
> +           if true recover hit points, if not idle */
> +        punittype = get_unit_type(punit->type);
> +        if (punit->hp < punittype->hp) {
> +          res = FALSE;
> +        }
> +        /* does the unit have all movement points? */
> +        if (punit->moves_left < punittype->move_rate) {
> +          res = FALSE;
> +        }

You've done the hp/mp recovery twice. It maybe be worthy of being a function.

> +/**********************************************************************
> +  Function to find a unit of the army which needs protection.
> +***********************************************************************/
> +bool find_attacking_unit_whitout_defense
                            ^^^^^^^  
whitout = without

>> +/**********************************************************************
> +  Did the attack function finish & conquerer or killed
> +  the enemy position?
> +***********************************************************************/

conquerer=conquer

> +bool did_army_finish_attack(struct player *pplayer, struct ai_army *parmy)
> +{ struct city *pcityobj;
> +
> +  if ( parmy->state == ARMY_STATE_ATTACKING ) {
> +    /* conquering cities */
> +    pcityobj = find_city_by_id(parmy->objective_id);
> +    if (!pcityobj) {
> +      /* we erase the city */
> +      return TRUE;
> +    }
> +    if (city_owner(pcityobj) == pplayer) {
> +      /* we conquer the city */
> +      return TRUE;
> +    }
> +    return FALSE;
> +  } else {
> +    return FALSE;
> +  }

You seem to have an extra return false here.

> +}
> +
> +/**********************************************************************
> +  Function that disbands an army if we have allocated units without
> +  an objective.
> +***********************************************************************/
> +void eventually_disband_army(struct ai_army *parmy)
> +{
> +
> +  if ( recalculate_accept_units(parmy) > 0 ) {
> +    /* we have troups allocated */
> +    if ( parmy->objective_id == 0 ) {
> +      /* disband army */
> +      disband_army(parmy);
> +    }
> +  }
> +}

Why not just assign it another objective?

> diff -Nur -b -Xfreeciv-cvs-Oct-25/diff_ignore freeciv-cvs-Nov-16/ai/aiarmy.h
> freeciv-cvs-army/ai/aiarmy.h
> --- freeciv-cvs-Nov-16/ai/aiarmy.h    Thu Jan  1 01:00:00 1970

> +#ifndef FC__AIARMY_H
> +#define FC__AIARMY_H
> +
> +#include "shared.h"          /* bool type */
> +
> +/* defined in player.h
> +#define MAX_ARMIES_PER_PLAYER 6

Too many armies!

> +*/
> +#define MAX_ATTACK_PER_ARMY 8
> +#define MAX_DEFENSE_PER_ARMY 4
> +#define MAX_NAVAL_PER_ARMY 4
> +#define MAX_TRANSPORT_PER_ARMY 6

Transport per army should be calculated based on number of ground units in army
divided by transport capacity.

In general, I think you have too many defines. These numbers should be
calculated per target city, rather than defined. This is excellent work Jordi.
Hoping I'll see it in CVS.

Aloha,
RK.

A Vulcan can no sooner be disloyal than he can exist without breathing. -Kirk,
"The Menagerie", stardate 3012.4



__________________________________________________
Do you Yahoo!?
New DSL Internet Access from SBC & Yahoo!
http://sbc.yahoo.com