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

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Petr Baudis <pasky@xxxxxxxxxxx> wrote:

Love the patch. It's looking good. If it could cook and clean, I'd date it.

<snip>

> > I'd like a little consistency.
> 
> So would I. But then I would rather change loss to bcost in findvictim() - I
> really can't look into future..
>

I knew a crystal ball would come in handy ...
 
> > Replace this with unit_type_iter.
> 
> Where it lives? Grep doesn't know and I saw only a proposal on ml.
>

I was suggesting, perhaps too indirectly, that you create it.

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



> This is good place for POWER_DIVIDER/2 ;).
>

Yes, but why is it /2?
 
> > > +      
> > > +      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.
 

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

> Ok, here I should use POWER_
>

Yes.
 
> > > +      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 you think the above comment makes anything clearer, stick it in 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.


> "If you have acquired knowledge, what do you lack?
>     If you lack knowledge, what have you acquired?"


<snip>


> +      desire /= POWER_DIVIDER/2; /* Good enough, no rounding errors. */
> +      desire *= desire;
> +

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



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



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


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

> +  /* 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?
 



> +static void process_attacker_want(struct player *pplayer, struct city
> *pcity,
> +                                  int value, Unit_Type_id victim_unit_type,
> +                                  bool veteran, int x, int y, int unhap,
> +                                  int *best_value, int *best_choice,
> +                                  int boatx, int boaty, int boatspeed,
> +                                  int needferry)
>  { 


> -      else c = real_map_distance(pcity->x, pcity->y, x, y) * SINGLE_MOVE * q
> / m;



> +      int bcost_balanced = build_cost_balanced(unit_type);
> +      /* See description of kill_desire() about this variables. */
> +      int bcost = unit_types[unit_type].build_cost;
> +      int vuln;
> +      int attack = base_unit_belligerence_primitive(unit_type,
> will_be_veteran,
> +                     SINGLE_MOVE, unit_types[unit_type].hp);
> +      /* Computed values.. */
> +      int desire, want;
> +      
> +      if (acity) attack += acity->ai.attack;
> +      
> +      attack *= attack;
> +
> +      if (unit_type_flag(unit_type, F_IGTER)) {
> +        /* Not quite right... */
> +        move_rate *= SINGLE_MOVE;
> +      }
>  

> +      /* Set the move_cost appropriatelly. */
> +      
> +      if (unit_types[unit_type].move_type == LAND_MOVING) {
> +        if (boatspeed) {
> +          /* It's either city or too far away, so don't bother with
> +           * victim_move_rate. */
> +          move_cost = (warmap.cost[boatx][boaty] + move_rate - 1) /
> move_rate
> +                      + 1 + warmap.seacost[x][y] / boatspeed; /* kludge */
> +          if (unit_type_flag(unit_type, F_MARINES)) move_cost -= 1;
> +          
> +        } else if (warmap.cost[x][y] <= move_rate) {
> +          /* It's adjacent. */
> +          move_cost = 1;
> +
> +        } else {
> +          move_cost = (warmap.cost[x][y] * victim_move_rate + move_rate - 1)
> +                      / move_rate;
> +        }
> +        
> +      } else if (unit_types[unit_type].move_type == SEA_MOVING) {
> +        if (warmap.seacost[x][y] <= move_rate) {
> +          /* It's adjectent. */
> +          move_cost = 1;
> +          
> +        } else {
> +          move_cost = (warmap.seacost[x][y] * victim_move_rate + move_rate -
> 1)
> +                      / move_rate;
> +        }
> +        
> +      } else if (real_map_distance(pcity->x, pcity->y, x, y) * SINGLE_MOVE
> <=
> +                 move_rate) {
> +        /* It's adjectent. */
> +        move_cost = 1;
>  
> -      if (unit_types[i].move_type == LAND_MOVING && acity &&
> -          c > (player_knows_improvement_tech(city_owner(acity),
> -                                          B_CITY) ? 2 : 4) &&
> -          !unit_type_flag(i, F_IGWALL) && !city_got_citywalls(acity)) d *=
> 9; 
> +      } else {
> +        move_cost = real_map_distance(pcity->x, pcity->y, x, y) *
> SINGLE_MOVE
> +                    * victim_move_rate / move_rate;
> +      }
>  

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







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



__________________________________________________
Do You Yahoo!?
Yahoo! Sports - sign up for Fantasy Baseball
http://sports.yahoo.com