[Freeciv-Dev] Re: [Patch] Cleanup of attack power calculations

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Mon, Feb 25, 2002 at 05:27:18PM -0800, Raahul Kumar wrote:
> 
> --- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> > 
> > To understand combat.c better I have cleaned up the calculation of the
> > attack power. Best part: you can now tell the differnce between
> > unit_belligerence_basic and unit_belligerence_primitive.
> > 
> 
> It's a good patch. Thumbs up. Minor nitpicks follow.
> 
>       Raimar
<snip>
> > Notice that here the POWER_FACTOR is 15 instead of 10 if the player knows
> > how to build a port. That is not the case in your replacement below. I'm
> not
> > quite clear on why the power factor is necessary. Put in comments 
> > explaining the power factor.
> 
> See MARK.
>

Ah, I see.
 
> MARK:
> > > +      if (m == LAND_MOVING || player_knows_improvement_tech(pplayer,
> > > B_PORT)) {
> > > + a *= 1.5;


> > V is fairly bad. How about my_unit_type.
> 
> It could be resolved to an even simpler version.
> 
> > You can remove this now or rename base_unit_belligerence. I find two
> functions,
> > one which does the actual work and a wrapper which calls it annoying. It
> means
> > I have to read two functions instead of one to work out what is really
> > happening.
> 
> I leave it to you (the AI cleanup people) to choose the interface. It
> is also possible to merge them and add a flag "bool
> consider_moves_left".

I prefer the merge. If someone objects to this, speak now or forever hold your
peace. 
 

<snip>
> > >  int get_attack_power(struct unit *punit)
> > >  {
> > > +  return base_get_attack_power(punit->type, punit->veteran,
> > > +                        punit->moves_left);
> > > +}
> > > +
 
> > You can get rid of this as well.
> 
> There are three users left.
> 
> ./common/combat.c:300:  int attackpower = get_attack_power(attacker);
> ./server/gotohand.c:1109:         attack_of_enemy = get_attack_power(aunit);
> ./server/unittools.c:151:    return (get_attack_power(punit)>0);
>

I suggest renaming base_get_attack_power to get_attack_power, and removing the
original wrapper.
 
> > > +  power = get_unit_type(type)->attack_strength * POWER_FACTOR;
> > > +  if (veteran) {
> > > +    power = (power * 3) / 2;
> > > +  }
> > 
> > power = power * 1.5; /* 50% bonus to attack and defence gained by veteran  
>    
> >                          units*/
> 
 1.5 involves the FPU. Comment added.

I sharply disagree on avoiding the FPU. On quite a few modern processors,
PIII onwards, the FPU is possibly a little faster than using integer division.
I think this was mentioned in a previous mail by Jason. The advantages of
removing the annoying scaling factors beats a tiny loss in performance on
some CPU's.
 
> > > +  if (!unit_type_flag(type, F_IGTIRED) && moves_left < SINGLE_MOVE) {
> > > +    power = (power * moves_left) / SINGLE_MOVE;
> > 
> > Finally. I always found it annoying that the attack power calcs were wrong
> > if the unit had fractional moves left.
> 
> ??
> 

If the unit had only 1/3 or 2/3 movepoints left, the old power calcs were
wrong.

> > These lines below could be replaced with the equivalent of
> > 
> > int get_defence_power(Unit_Type_id type, bool veteran, int moves_left)
> 
> Defence comes in the next patch.
> 

Excellent work. I thought you did not like server side AI.

> ? all.diff
> ? test.s
> ? auto.rc
> ? test.c
> ? freeciv
> ? oilmine.patch
> Index: ai/advmilitary.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/ai/advmilitary.c,v
> retrieving revision 1.91
> diff -u -r1.91 advmilitary.c
> --- ai/advmilitary.c  2002/02/25 19:05:13     1.91
> +++ ai/advmilitary.c  2002/02/26 09:25:25
> @@ -161,19 +161,14 @@
>  {
>    int v;
>    bool sailing;
> -  struct unit_type *ptype;
>  
>    if (unit_flag(punit, F_NO_LAND_ATTACK)) return(0);
>    sailing = is_sailing_unit(punit);
>    if (sailing && !is_terrain_near_tile(pcity->x, pcity->y, T_OCEAN))
> return(0);
>  
> -  ptype = unit_type(punit);
> -/* get_attack_power will be wrong if moves_left == 1 || == 2 */
> -  v = ptype->attack_strength * 10 * punit->hp * ptype->firepower;
> -  if (punit->veteran) v += v/2;
> +  v = unit_belligerence_basic(punit);
>    if (sailing && city_got_building(pcity, B_COASTAL)) v /= 2;
>    if (is_air_unit(punit) && city_got_building(pcity, B_SAM)) v /= 2;
> -  v /= 30; /* rescaling factor to stop the overflow nonsense */
>    return(v);
>  }
>  
> @@ -571,10 +566,11 @@
>        l /= city_list_size(&pplayer->cities);
>  /* Katvrr advises that with danger high, l should be weighted more heavily
> */
>  
> -      a = unit_types[i].attack_strength *  ((m == LAND_MOVING ||
> -          player_knows_improvement_tech(pplayer, B_PORT)) ? 15 : 10) *
> -             unit_types[i].firepower * unit_types[i].hp;
> -      a /= 30; /* scaling factor to prevent integer overflows */
> +      a = base_unit_belligerence_primitive(i, FALSE, SINGLE_MOVE,
> +                                        unit_types[i].hp);
> +      if (m == LAND_MOVING || player_knows_improvement_tech(pplayer,
> B_PORT)) {
> +     a = (a * 3) / 2;

Power factor again. Maybe a comment so the reader does not wonder why the
multiplication of what looks like a vet bonus. 

> +      }
>        if (acity) a += acity->ai.a;
>        a *= a;
>        /* b is unchanged */
> @@ -701,9 +697,7 @@
>      v = myunit->type;

You forgot to get rid of v.

>      vet = myunit->veteran;
>  
> -    a = unit_types[v].attack_strength * (vet ? 15 : 10) *
> -             unit_types[v].firepower * myunit->hp;
> -    a /= 30; /* scaling factor to prevent integer overflows */
> +    a = unit_belligerence_basic(myunit);
>      if (acity) a += acity->ai.a;
>      a *= a;
>  
> Index: ai/aiunit.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.c,v
> retrieving revision 1.180
> diff -u -r1.180 aiunit.c
> --- ai/aiunit.c       2002/02/25 19:05:13     1.180
> +++ ai/aiunit.c       2002/02/26 09:25:27
> @@ -561,20 +561,23 @@
>  /**************************************************************************
>  ...
>  **************************************************************************/
> +int base_unit_belligerence_primitive(Unit_Type_id type, bool veteran,
> +                                  int moves_left, int hp)
> +{
> +  return (base_get_attack_power(type, veteran, moves_left) * hp *
> +       get_unit_type(type)->firepower / POWER_DIVIDER);
> +}
> +
>  static int unit_belligerence_primitive(struct unit *punit)
>  {
> -  int v;
> -  v = get_attack_power(punit) * punit->hp * 
> -            unit_type(punit)->firepower / 30;
> -  return(v);
> +  return (base_unit_belligerence_primitive(punit->type, punit->veteran,
> +                                        punit->moves_left, punit->hp));
>  }
>  
>  int unit_belligerence_basic(struct unit *punit)
>  {
> -  int v;
> -  v = unit_type(punit)->attack_strength * (punit->veteran ? 15 : 10) * 
> -          punit->hp * unit_type(punit)->firepower / 30;
> -  return(v);
> +  return (base_unit_belligerence_primitive(punit->type, punit->veteran,
> +                                        SINGLE_MOVE, punit->hp));
>  }
>  
>  int unit_belligerence(struct unit *punit)
> @@ -1039,7 +1042,7 @@
>  static int ai_military_gothere(struct player *pplayer, struct unit *punit,
>                              int dest_x, int dest_y)
>  {
> -  int id, x, y, boatid = 0, bx = 0, by = 0, j;
> +  int id, x, y, boatid = 0, bx = 0, by = 0;
>    struct unit *ferryboat;
>    struct unit *def;
>    struct city *dcity;
> @@ -1057,9 +1060,9 @@
>      d_val = stack_attack_value(dest_x, dest_y) * 30;
>      if ((dcity = map_get_city(dest_x, dest_y))) {
>        d_type = ai_choose_defender_versus(dcity, punit->type);
> -      j = unit_types[d_type].hp * (do_make_unit_veteran(dcity, d_type) ? 15
> : 10) *
> -          unit_types[d_type].attack_strength;
> -      d_val += j;
> +      d_val += base_get_attack_power(d_type,
> +                                  do_make_unit_veteran(dcity, d_type),
> +                                  SINGLE_MOVE) * unit_types[d_type].hp;
>      }

Explain what is happening in the lines above. It seems we are taking into
account the number of units in a stack, modifying by the chance of being made
veteran after winning a fight.

>      d_val /= (unit_type(punit)->move_rate / SINGLE_MOVE);
>      if (unit_flag(punit, F_IGTER)) d_val /= 1.5;
> Index: ai/aiunit.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.h,v
> retrieving revision 1.30
> diff -u -r1.30 aiunit.h
> --- ai/aiunit.h       2002/02/25 19:05:14     1.30
> +++ ai/aiunit.h       2002/02/26 09:25:27
> @@ -14,7 +14,16 @@
>  #define FC__AIUNIT_H
>  
>  #include "unittype.h"
> +#include "combat.h"
>  
> +/*
> + * To prevent integer overflows the product "power * hp * firepower"
> + * is divided by POWER_DIVIDER.
> + *
> + * The constant may be changed since it isn't externally visible used.
> + */
> +#define POWER_DIVIDER        (POWER_FACTOR * 3)
> +

I'd really prefer avoiding these useless scaling factors and just going for
FP math.

<snip>
>  int get_attack_power(struct unit *punit)
>  {
> +  return base_get_attack_power(punit->type, punit->veteran,
> +                            punit->moves_left);
> +}
> +
> +/**************************************************************************
> + Returns the attack power, modified by moves left, and veteran
> + status. Set moves_left to SINGLE_MOVE to disable the reduction of
> + power caused by tired units.
> +**************************************************************************/

You are ignoring the reduction of power caused my fractional move points. I
don't see why.

> +int base_get_attack_power(Unit_Type_id type, bool veteran, int moves_left)
> +{
>    int power;
> -  power=unit_type(punit)->attack_strength*10;
> -  if (punit->veteran) {
> -    power *= 3;
> -    power /= 2;
> +
> +  power = get_unit_type(type)->attack_strength * POWER_FACTOR;
> +  if (veteran) {
> +    /* Veterans get +50% bonus. */
> +    power = (power * 3) / 2;
> +  }
> +

Good. If you really want to win terse comment of the year, try

Veteran 50% bonus.

> +  if (!unit_type_flag(type, F_IGTIRED) && moves_left < SINGLE_MOVE) {
> +    power = (power * moves_left) / SINGLE_MOVE;
>    }

The units that are F_IGTIRED are ships and airplanes(heli). None of these units
can possibly have fractional move_points. If that does occur then someone
screwed up. You can probably get rid of the F_IGTIRED check.

There are no land units that are F_IGTIRED as far as I know.

> -  if (unit_flag(punit, F_IGTIRED)) return power;
> -  if ( punit->moves_left < SINGLE_MOVE )
> -     return (power*punit->moves_left)/SINGLE_MOVE;
> -    return power;
> +  return power;
>  }
>  
>  /**************************************************************************
> @@ -264,7 +277,7 @@
>    if (!punit || punit->type<0 || punit->type>=U_LAST
>        || punit->type>=game.num_unit_types)
>      abort();
> -  power=unit_type(punit)->defense_strength*10;
> +  power = unit_type(punit)->defense_strength * POWER_FACTOR;
>    if (punit->veteran) {
>      power *= 3;
>      power /= 2;
> Index: common/combat.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/combat.h,v
> retrieving revision 1.4
> diff -u -r1.4 combat.h
> --- common/combat.h   2002/02/14 15:17:09     1.4
> +++ common/combat.h   2002/02/26 09:25:33
> @@ -15,6 +15,14 @@
>  
>  #include "unittype.h"
>  
> +/*
> + * attack_strength and defense_strength are multiplied by POWER_FACTOR
> + * to yield the base of attack_power and defense_power.
> + *
> + * The constant may be changed since it isn't externally visible used.
> + */
> +#define POWER_FACTOR 10
> +
>  struct unit;
>  struct player;
>  
> @@ -34,6 +42,7 @@
>  struct city *sdi_defense_close(struct player *owner, int x, int y);
>  
>  int get_attack_power(struct unit *punit);
> +int base_get_attack_power(Unit_Type_id type, bool veteran, int moves_left);
>  int get_defense_power(struct unit *punit);
>  int get_total_defense_power(struct unit *attacker, struct unit *defender);
>  int get_simple_defense_power(Unit_Type_id d_type, int x, int y);
> 

I like the patch. I'd like to see some comments from the prospective users of
the patch. Greg, Petr.


__________________________________________________
Do You Yahoo!?
Yahoo! Sports - Coverage of the 2002 Olympic Games
http://sports.yahoo.com