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

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- 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
> 
> -- 
>  email: rf13@xxxxxxxxxxxxxxxxx
>  "Life is too short for reboots."
> > Index: ai/advmilitary.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/ai/advmilitary.c,v
> retrieving revision 1.90
> diff -u -r1.90 advmilitary.c
> --- ai/advmilitary.c  2002/02/21 09:44:50     1.90
> +++ ai/advmilitary.c  2002/02/25 17:10:28
> @@ -168,12 +168,10 @@
>    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 */

I noticed you fixed that in base_get_attack_power. Well done.

> -  v = ptype->attack_strength * 10 * punit->hp * ptype->firepower;
> -  if (punit->veteran) v += v/2;
> +  v = base_unit_belligerence_primitive(punit->type, punit->veteran,
> +                                    SINGLE_MOVE, punit->hp);

I like the bool veteran. Finally the bools add to readability.

>    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 +569,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) *

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.

> -             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 *= 1.5;
> +      }
>        if (acity) a += acity->ai.a;
>        a *= a;
>        /* b is unchanged */
> @@ -703,9 +702,7 @@
>      v = myunit->type;
>      vet = myunit->veteran;

V is fairly bad. How about my_unit_type.

>  
> -    a = unit_types[v].attack_strength * (vet ? 15 : 10) *
> -             unit_types[v].firepower * myunit->hp;
> -    a /= 30; /* scaling factor to prevent integer overflows */
> +    a = base_unit_belligerence_primitive(v, vet, SINGLE_MOVE, myunit->hp);
>      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.179
> diff -u -r1.179 aiunit.c
> --- ai/aiunit.c       2002/02/24 14:20:57     1.179
> +++ ai/aiunit.c       2002/02/25 17:10:31
> @@ -544,20 +544,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 / 30);
> +}
> +
>  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));
>  }
>  

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.

>  int unit_belligerence(struct unit *punit)
> @@ -1040,8 +1043,8 @@
>      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;
> +      j = base_get_attack_power(d_type, do_make_unit_veteran(dcity, d_type),
> +                             SINGLE_MOVE) * unit_types[d_type].hp;

A better name than j would be appreciated.

>        d_val += j;
>      }
>      d_val /= (unit_type(punit)->move_rate / SINGLE_MOVE);
> Index: ai/aiunit.h
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/ai/aiunit.h,v
> retrieving revision 1.29
> diff -u -r1.29 aiunit.h
> --- ai/aiunit.h       2002/02/19 16:41:15     1.29
> +++ ai/aiunit.h       2002/02/25 17:10:31
> @@ -32,6 +32,8 @@
>                              int *x, int *y);
>  int find_beachhead(struct unit *punit, int dest_x, int dest_y, int *x, int
> *y);
>  
> +int base_unit_belligerence_primitive(Unit_Type_id type, bool veteran,
> +                                  int moves_left, int hp);
>  int unit_belligerence_basic(struct unit *punit);
>  int unit_belligerence(struct unit *punit);
>  int unit_vulnerability_basic(struct unit *punit, struct unit *pdef);
> Index: common/combat.c
> ===================================================================
> RCS file: /home/freeciv/CVS/freeciv/common/combat.c,v
> retrieving revision 1.19
> diff -u -r1.19 combat.c
> --- common/combat.c   2002/02/16 17:05:05     1.19
> +++ common/combat.c   2002/02/25 17:10:35
> @@ -236,20 +236,32 @@
>  }
>  
>  /**************************************************************************
> - returns the attack power, modified by moves left, and veteran status.
> + Convenience wrapper for base_get_attack_power.
>  **************************************************************************/
>  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.

> +/**************************************************************************
> + 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.
> +**************************************************************************/
> +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) {
> +    power = (power * 3) / 2;
> +  }

power = power * 1.5; /* 50% bonus to attack and defence gained by veteran      
 
                         units*/

> +
> +  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 (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 +276,7 @@
>    if (!punit || punit->type<0 || punit->type>=U_LAST
>        || punit->type>=game.num_unit_types)
>      abort();

These lines below could be replaced with the equivalent of

int get_defence_power(Unit_Type_id type, bool veteran, int moves_left)
{
  int power;
  power = get_unit_type(type)->attack_strength * POWER_FACTOR;
  if (veteran) {
    power = power * 1.5;/* Veteran bonus to attack and defence of 50% */
  }

> -  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/25 17:10:35
> @@ -15,6 +15,12 @@
>  
>  #include "unittype.h"
>  
> +/*
> + * attack_strength and defense_strength are multiplied by POWER_FACTOR
> + * to yield the base of attack_power and defense_power.
> + */
> +#define POWER_FACTOR 10
> +
>  struct unit;
>  struct player;
>  
> @@ -34,6 +40,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);
> 

Overall I think this is a very good patch.


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