[Freeciv-Dev] Re: [IAMBACK] [PATCH] [1.3] cleanup of proccess_*_want() (PR#1295)

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Mon, Mar 04, 2002 at 01:20:16PM CET, I got a letter,
where Raahul Kumar <raahul_da_man@xxxxxxxxx> told me, that...
> > > > +      int desire = unit_desirability(unit_type, TRUE);
> > > > +
> > > 
> > > The range of unit desirability is ? Does negative numbers have any 
> > > meaning?
> > 
> > Nope. AFAIK it's guaranteed to be -le 0.
> 
> It would be nice if that was a comment.

Then it should be a comment near unit_desirability() declaration. It doesn't
matter for us anyway.

> > This is good place for POWER_DIVIDER/2 ;).
> >
> 
> Yes, but why is it /2?

Because it's "good enough, no rounding errors" ? ;) There were probably some
overflows. I take this as an empirical value.

> > > > +      
> > > > +      if (can_build_unit(pcity, unit_type)) {
> > > > +        /* We can build the unit now... */
> > > > +        
> > > > +        if (walls && move_type == LAND_MOVING) {
> > > > +          desire *= pcity->ai.wallvalue;
> > > > +          desire /= 10;
> > > > +        }
> > > 
> > > Huh??? Is this trying to model the fact that one unit defending behind 
> > > city
> > > walls is better than 2 without?
> > 
> > Who knows, maybe...
> >
> 
> I think this is just plain wrong. Unless someone knows why it should stay, I
> vote for it to removed in your next patch.

Someone's next patch :).

> > > > +      bool will_be_veteran = (move_type == LAND_MOVING ||
> > > > +                              player_knows_improvement_tech(pplayer,
> > > > B_PORT));
> > > 
> > > If you're going to check that, why not check B_AIRPORT as well?
> > 
> > That's Greg's job :).
> >
> 
> You sir, are the offspring of a demented hyena and a lascivious scorpion.
> Poor Greg needs some limited assistance. Stick a todo at least.

I just happen to absolutely love your poetic language and brilliant
attributions.

As a meed, you've a TODO there now! :)

> > > > +      /* Cost (shield equivalent) of gaining these techs. */
> 
> > > > +      if (unit_type_flag(unit_type, F_IGTER)) {
> > > > +        /* Not quite right... */
> > > > +        move_rate *= SINGLE_MOVE;
> > > 
> > > Bad Petr. Create your own define, IGTER_MOVE_COST.
> > 
> > Why? This would mean changing on much more places. And this stuff is IIRC
> > your
> > or Greg's job, isn't it? ;))) (oh, I'm evil..)
> >
> 
> Evil does not describe it. Heh ;-). Well, at least stick a todo saying it
> should be replaced with IGTER_MOVE_COST.

I'm in so nice mood today.. :P

> > > > +      if (unit_types[unit_type].move_type == LAND_MOVING && acity
> > > > +          && move_cost >
> > (player_knows_improvement_tech(city_owner(acity),
> > > > +                                                        B_CITY) ? 2 : 
> > > > 4)
> > > > +          && !unit_type_flag(unit_type, F_IGWALL)
> > > > +          && !city_got_citywalls(acity)) {
> > > > +        /* FIXME: Use acity->ai.wallvalue? --pasky */
> > > > +        vuln *= 9;
> > > 
> > > I wish I knew why used 9 instead of 3. I can only assume this is due to
> > those
> > > goddamn scaling factors, and this probably explains why we divide results
> > by
> > > 30.
> > 
> > Why should we use 3?
> 
> Citywalls give a defensive bonus of 300%. So for units that lack the ability 
> to
> ignore city walls, the lack of a city wall makes them 3 times as dangerous. 
> Yet
> in this check we multiply by 9. WHY???????? (Pulls out hair and screams).

  if (badmojo == 0) pcity->ai.wallvalue = 90;
  else pcity->ai.wallvalue = (danger * 9 - badmojo * 8) * 10 / (danger);

> If you think the above comment makes anything clearer, stick it in there.

You've it there.

> > > > -  pcity->ai.a = val2;
> > > > -  pcity->ai.f = val3;
> > > > +  pcity->ai.attack = val2;
> > > > +  pcity->ai.bcost = val3;
> > > 
> > > Crap names.
> > 
> > Hm? Tell me better.
> 
> I got nothing. I don't have to be creative. I'm a critic. It's my job to mock,
> yours to do better.

Well, then what about being concrete when mocking? :^)

> > +      if (can_build_unit(pcity, unit_type)) {
> > +        /* We can build the unit now... */
> > +      
> > +        /* XXX? We've bigger desire for land units when city walls can
> > +         * protect them (?). */
> > +        if (walls && move_type == LAND_MOVING) {
> > +          desire *= pcity->ai.wallvalue;
> > +          desire /= 10;
> 
> POWER_FACTOR here I suspect.

blabla :) yes, GREG and you convienced me ;)).

> > +        /* Contrary above, we don't care if walls are actually built - 
> > we're
> > +         * looking into future now. */
> > +        if (move_type == LAND_MOVING) {
> > +          desire *= pcity->ai.wallvalue;
> > +          desire /= 10;
> 
> Another POWER_FACTOR.

You won't give my poor soul a rest..

> > +  /* I was getting four-figure desire for battleships otherwise. -- Syela 
> > */
> > +  if (!walls && unit_types[best_unit_type].move_type == LAND_MOVING) {
> >      best *= pcity->ai.wallvalue;
> >      best /= 10;
> 
> Repetition get boring fast, so yet another POWER_FACTOR.

*gasp*

> > +  if (best == 0) best = 1; /* Avoid division by zero bellow. */
> 
> You persist in introducing me to new sentences Petr. Until today I did not
> know the number zero could yell. I suspect bellow = below.

Maybe I blessed you too much when I was fabling about your imagination and soul
of poet.. :P Oh well, so I fixed that for you poor mortals.

> > +  /* Develop appropriate techs for units we want to build. */
> > +  
> > +  for (unit_type = 0; unit_type < game.num_unit_types; unit_type++) {
> > +    if (tech_desire[unit_type] && is_ai_simple_military(unit_type)) {
> > +      Tech_Type_id tech_req = unit_types[unit_type].tech_requirement;
> > +      int desire = tech_desire[unit_type]
> > +                   * unit_types[best_unit_type].build_cost / best;
> > +      
> > +      pplayer->ai.tech_want[tech_req] += desire;
> > +     
> 
> Any way to move this to advscience.c later?

I fear that won't be easy. Can't tell just now.

> > +      /* Appreciate resistance of the enemy. */
> > +      /* It's like assess_defense_unit(), but get_virtual_defense_power()
> > +       * instead of get_defense_power(). */
> > +      
> > +      vuln = get_virtual_defense_power(unit_type, victim_unit_type, x, y);
> > +      vuln *= unit_types[victim_unit_type].hp *
> > +              unit_types[victim_unit_type].firepower;
> > +      if (veteran) vuln *= 1.5;
> 
> Raimar will go on the warpath here. He prefers  * 3/2. It's part of his credo
> against floating point numbers.

Let's see.

> >                /* If there're enough units to do the job, we don't need this
> >                 * one. */
> > -              /* FIXME: The problem with ai.f is that bigger it is, less is
> > our
> > -               * desire to go help other units.  Now suppose we need five
> > +              /* FIXME: The problem with ai.bcost is that bigger it is, 
> > less
> > is
> > +               * our desire to go help other units.  Now suppose we need
> > five
> >                 * cavalries to take over a city, we have four (which is not
> >                 * enough), then we will be severely discouraged to build the
> >                 * fifth one.  Where is logic in this??!?! --GB */
> 
> There is no logic in this.

Hey, C comments aren't IRC! ;))

Attached patch is sync'd to latest CVS, improved few comments and variable
names, used POWER_FACTOR more frequently.

-- 
 
                                Petr "Pasky" Baudis
 
* ELinks maintainer                * IPv6 guy (XS26 co-coordinator)
* IRCnet operator                  * FreeCiv AI hacker
.
Teamwork is essential -- it allows you to blame someone else.
.
Public PGP key && geekcode && homepage: http://pasky.ji.cz/~pasky/

want.patch
Description: Text document