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

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:

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

OK.
 
<snip>
> results: with -O2
> 
> integer (f1): 1.1s
> float (f2):   6.6s
>

Much bigger difference than I expected. If you want to continue on your
floating
point jehad.

ai/advmilitary.c, line 728 -- if (do_make_unit_veteran(acity, n)) m *= 1.5;
ai/aicity.c, line 298 -- if (do_make_unit_veteran(pcity, d_type)) m *= 1.5
ai/aiunit.c, line 1337 -- (do_make_unit_veteran(acity, n) ? 1.5 : 1) / 30;

There are some other 1.5's in the code.
 
> with -O3:
>  f1: 0.37s
>  f2: 5.9s
> 

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

Moves_left keeps track of how many moves are left. If it declines from
3(SINGLE_MOVE) to 2, this function will calculate the haste penalty correctly.
I don't see any code guaranteeing it will never go below SINGLE_MOVE.
 
> > Excellent work. I thought you did not like server side AI.
> 
> This is only to get a better understanding of combat.c ;)
>

I bet you still have nightmares about belligerence.
 
> 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".
>

OK. It's just that I'm conditioned to treat every * 3/2 as a vet bonus.
 
> > >      v = myunit->type;
> > 
> > You forgot to get rid of v.
> 
> v is used.

v is hardly used. myunit->type can do the job.
> > > -      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).
>

That's actively misleading. I thought do_make_unit_veteran checks after winning
a combat if the unit can be upgraded to veteran status. You're right about the
rename making it clearer. But, gazing into my crystal ball I see the dread
words

isn't in the scope of this patch.
  
> +/**************************************************************************
> > > + 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"
>

Doh. I misread the comment.
 
> > > +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?

Nothing really. It's just missing the words attack and defence.
 
> > 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.
>

OK.
 


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