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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Feb 26, 2002 at 03:44:20AM -0800, Raahul Kumar wrote:
> 
> --- 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. 

Isn't in the scope of this patch.

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

Isn't in the scope of this patch.

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

$ cat test.c
int f1(int x)
{
  return (x * 3) / 2;
}

int f2(int x)
{
  return x * 1.5;
}

int main()
{
  int i, r = 0;
  for (i = 0; i < 30000000; i++) {
    r += f2(i & 63);
  }
  return r;
}

results: with -O2

integer (f1): 1.1s
float (f2):   6.6s

with -O3:
 f1: 0.37s
 f2: 5.9s

Asm version:

f1:
        pushl   %ebp
        movl    %esp, %ebp
        movl    8(%ebp), %eax
        leal    (%eax,%eax,2), %eax  // eax = eax * 3
        movl    %eax, %edx
        shrl    $31, %edx
        addl    %edx, %eax
        sarl    $1, %eax             // eax = eax / 2
        popl    %ebp
        ret

The other instructions take care of the sign of the result and go away
if you use unsigned.

.LC0:
        .long   0x0,0x3ff80000
f2:
        pushl   %ebp
        movl    %esp, %ebp
        fldl    .LC0                 // bad, a memory read
        subl    $8, %esp
        fimull  8(%ebp)
        fnstcw  -4(%ebp)
        movl    -4(%ebp), %edx
        movb    $12, -3(%ebp)
        fldcw   -4(%ebp)
        movl    %edx, -4(%ebp)
        fistpl  -8(%ebp)
        fldcw   -4(%ebp)
        movl    -8(%ebp), %eax
        leave
        ret

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

Why? The new code achieves the same as the old one.

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

This is only to get a better understanding of combat.c ;)

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

Mhh the condition says quite boldly that this doesn't have anything to
do with veterans. And we can't comment every occurrence of "x*=1.5".

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

v is used.

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

Don't ask me. There are people which are better at soothsaying. I can
comment that do_make_unit_veteran should be renamed to
is_city_producing_veteran_units (or similar).

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

"modified by moves left"

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

And what is wrong with my version?

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

This doesn't have to be true in the future. Nevertheless: isn't in the
scope of this patch.

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

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Of course, someone who knows more about this will correct me if I'm
  wrong, and someone who knows less will correct me if I'm right."
    -- David Palmer (palmer@xxxxxxxxxxxxxxxxxx)