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

[Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Feb 26, 2002 at 04:57:23AM -0800, Raahul Kumar wrote:
> <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.

I'm not on one.

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

We will see how many are left if the defence power calculation patch
is applied.

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

Ack.

> I don't see any code guaranteeing it will never go below SINGLE_MOVE.

I still not understand.

> > > >      v = myunit->type;
> > > 
> > > You forgot to get rid of v.
> > 
> > v is used.
> 
> v is hardly used. myunit->type can do the job.

I bet that someone has a cleanup (renaming/removing of variables,...) 
of this function which does this. If not you may start one.

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

Yes. But you see that this patch uncovers some other problems. Please
fix them now or write it down on a todo list (as detailed as
possible) and don't forget them as I usually do.

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

But such comment have to be placed on the "bool veteran" in struct
unit in unit.h.

        Raimar

-- 
 email: rf13@xxxxxxxxxxxxxxxxx
 "Any sufficiently advanced technology is indistinguishable from magic."
    -- Arthur C. Clarke