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

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Raimar Falke <hawk@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> 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? 

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

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

The previous comment that you removed said that if moves_left == 1 or 2 that
the result would be wrong. That isn't the case with the current code(maybe the
comment was wrong before as well).

moves_left = 2

power =(power * moves_left)/ SINGLE_MOVE

That is the correct 2/3 penalty for attack power if the unit has only
fractional move points left.  There is no issue here. The code is behaving
correctly.
 
 
> > > > >      v = myunit->type;
> > 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.
>

Petr has renamed a lot of variables. Any comments?
 
> > > > > -      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.
> 

I can send you a todo list, or post it to freeciv-dev. I'm not willing to send
you a patch. So far, I've only sucessfully gotten one patch past you. And that
was into the CMA. I have not suceeded in getting any of my movement patches
into
CVS. I'm not eager to repeat the experience.

<snip>
> > > > > +    /* 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.

That is the best place for the comment.



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