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

[Raahul Kumar <raahul_da_man@xxxxxxxxx>]

--- Petr Baudis <pasky@xxxxxxxxxxx> wrote:
> Hello,
<snip>

>   I think it's ready to actually go in, altough I'm open to objections and
> ideas how to extend/correct it even more (I expect many hints for better and
> more comments, and possibly also for better variable names - in fact, all
> those
> one-letter variables have usually same mean in the whole AI - but I didn't
> i.e.
> renamed f to 'loss', but to 'bcost' here, contrary to findvictim()).
>

I'd like a little consistency.
 

>   Oh, I want to ask for a long time already, but now finally I remembered ;).
> It seems to me that unit_vulnerability() (and assess_danger_unit() etc) are
> returning _higher_ values for _less_ vulnerability (as the defense rate is
> taken as a base) - isn't their name a bit confusing then?
>

Yes. Unit_toughness would be better.
 

>   Enjoy your reviewing ;),
>

Reviewing patches is where all the glamour is ;).
 

> Public PGP key && geekcode && homepage: http://pasky.ji.cz/~pasky/
> > Index: ChangeLog



> +/************************************************************************** 
> +What would be the best defender for that city?


> +  bool defended = has_a_normal_defender(pcity);
> +  /* Technologies we would like to have. */
> +  int tech_desire[U_LAST];
> +  /* Our favourite unit. */
> +  int best = 0;
> +  Unit_Type_id best_unit_type = 0; /* FIXME: 0 is legal value! */
> +  /* Iterator. */
> +  Unit_Type_id unit_type;
> +
> +  memset(tech_desire, 0, sizeof(tech_desire));
> +  
> +  for (unit_type = 0; unit_type < game.num_unit_types; unit_type++) {

Replace this with unit_type_iter.

> +    int move_type = unit_types[unit_type].move_type;
> +

Wahoo.    

> +    if (!is_ai_simple_military(unit_type)) continue;
> +    
> +    if (move_type == LAND_MOVING || move_type == SEA_MOVING) {
> +      /* How many technologies away it is? */
> +      int tech_dist = num_unknown_techs_for_goal(pplayer,
> +                        unit_types[unit_type].tech_requirement);
> +      /* How much we want the unit? */
> +      int desire = unit_desirability(unit_type, TRUE);
> +

The range of unit desirability is ? Does negative numbers have any meaning?
      
> +      /* We won't leave the castle empty when driving out to battlefield. */
> +      if (!defended && unit_type_flag(unit_type, F_FIELDUNIT)) desire = 0;
> +      
> +      desire /= 15; /* Good enough, no rounding errors. */
> +      desire *= desire;
> +      
> +      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?


> +        
> +        if ((desire > best ||
> +             (desire == best && get_unit_type(unit_type)->build_cost <=
> +                                get_unit_type(best_unit_type)->build_cost))
> +            && unit_types[unit_type].build_cost <= pcity->shield_stock + 40)
> {
> +          best = desire;
> +          best_unit_type = unit_type;
>          }
> -      } else if (k > 0 && (shore || m == LAND_MOVING) &&
> -                unit_types[i].tech_requirement != A_LAST) {
> -        if (m == LAND_MOVING) { j *= pcity->ai.wallvalue; j /= 10; }
> -
> -     /* cost (shield equiv) of gaining these techs */
> -     l = total_bulbs_required_for_goal(pplayer,
> -                                       unit_types[i].tech_requirement);
> -     l /= 4;
> -        l /= city_list_size(&pplayer->cities);
> -/* Katvrr advises that with danger high, l should be weighted more heavily
> */
> -        desire[i] = j * danger / (unit_types[i].build_cost + l);
> +        
> +      } else if (tech_dist > 0 && (shore || move_type == LAND_MOVING)
> +                 && unit_types[unit_type].tech_requirement != A_LAST) {
> +        /* We first need to develop a tech required by the unit... */
> +
> +        /* Cost (shield equivalent) of gaining these techs. */
> +        /* FIXME? Katvrr advises that this should be weighted more heavily
> in
> +         * big danger. */
> +        int tech_cost = total_bulbs_required_for_goal(pplayer,
> +                          unit_types[unit_type].tech_requirement) / 4
> +                        / city_list_size(&pplayer->cities);
> +

Beautiful.
        
> +        /* 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;
> +        }
> +
> +        /* Yes, there's some similarity with kill_desire(). */
> +        tech_desire[unit_type] = desire * danger /
> +                                (unit_types[unit_type].build_cost +
> tech_cost);
>        }
>      }
>    }
> -  if (!walls && unit_types[bestid].move_type == LAND_MOVING) {
> +  
> +  /* 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;
> -  } /* was getting four-figure desire for battleships otherwise. -- Syela */
> -/* Phalanx would be 16 * danger / 20.  Pikemen would be 36 * danger / (20 +
> l) */
> +  }
>  
> -  if (best == 0) best = 1;   /* avoid divide-by-zero below */
> +  if (best == 0) best = 1; /* Avoid division by zero bellow. */
>  
> -/* multiply by unit_types[bestid].build_cost / best */
> -  for (i = 0; i < game.num_unit_types; i++) {
> -    if (desire[i] && is_ai_simple_military(i)) {
> -      tech_req = unit_types[i].tech_requirement;
> -      n = desire[i] * unit_types[bestid].build_cost / best;
> -      pplayer->ai.tech_want[tech_req] += n;
> +  /* 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;
> +      
>        freelog(LOG_DEBUG, "%s wants %s for defense with desire %d <%d>",
> -                 pcity->name, advances[tech_req].name, n, desire[i]);
> +              pcity->name, advances[tech_req].name, desire,
> +              tech_desire[unit_type]);
>      }
>    }
> -  choice->choice = bestid;
> +  
> +  choice->choice = best_unit_type;
>    choice->want = danger;
>    choice->type = CT_DEFENDER;
>    return;
>  }
>  
>  /************************************************************************** 
> -n is type of defender, vet is vet status, x,y is location of target
> -I decided this funct wasn't confusing enough, so I made k_s_w send
> -it some more variables for it to meddle with -- Syela
> +This function decides, what unit would be best for erasing enemy. It is
> called,
> +when we just want to kill something, we've found it but we don't have the
> unit
> +for killing that built yet - here we'll choose the type of that unit.
> +
> +I decided this funct wasn't confusing enough, so I made kill_something_with
> +send it some more variables for it to meddle with. -- Syela
> +
> +TODO: Get rid of these parameters :).
> +
> +(x,y) is location of the target.
> +(best_value,best_choice) is pre-filled with our current choice.
>  **************************************************************************/
> -static void process_attacker_want(struct player *pplayer,
> -                         struct city *pcity, int b, Unit_Type_id n,
> -                            bool vet, int x, int y, int unhap, int *e0, int
> *v,
> -                            int bx, int by, int boatspeed, int needferry)
> +
> +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)
>  { 
> -  Unit_Type_id i;
> -  Tech_Type_id j;
> -  int a, c, d, e, b0, f, g, fprime;
> -  int k, l, m, q;
> -  bool shore = is_terrain_near_tile(pcity->x, pcity->y, T_OCEAN);
>    struct city *acity = map_get_city(x, y);
> -  int movetype = unit_types[*v].move_type;
> -
> -  for (i = 0; i < game.num_unit_types; i++) {
> -    if (!is_ai_simple_military(i)) continue;
> -    m = unit_types[i].move_type;
> -    j = unit_types[i].tech_requirement;
> -    if (j != A_LAST) k = num_unknown_techs_for_goal(pplayer,j);
> -    else k = 0;
> -    if ((m == LAND_MOVING || (m == SEA_MOVING && shore)) && j != A_LAST &&
> -         (k || !can_build_unit_direct(pcity, unit_types[i].obsoleted_by)) &&
> -         unit_types[i].attack_strength && /* otherwise we get SIGFPE's */
> -         m == movetype) { /* I don't think I want the duplication otherwise
> -- Syela */
> -      bool will_be_veteran = (m == LAND_MOVING
> -                           || player_knows_improvement_tech(pplayer,
> -                                                            B_PORT));
> -
> -      /* cost (shield equiv) of gaining these techs */
> -      l = total_bulbs_required_for_goal(pplayer,
> -                                     unit_types[i].tech_requirement);
> -      l /= 4;
> -
> -      l /= city_list_size(&pplayer->cities);
> -/* Katvrr advises that with danger high, l should be weighted more heavily
> */
> -
> -      a = base_unit_belligerence_primitive(i, will_be_veteran, SINGLE_MOVE,
> -                                        unit_types[i].hp);
> -      if (acity) a += acity->ai.a;
> -      a *= a;
> -      /* b is unchanged */
> -
> -      m = unit_types[i].move_rate;
> -      q = (acity ? 1 : unit_types[n].move_rate * (unit_type_flag(n, F_IGTER)
> ? 3 : 1));
> -      if (unit_type_flag(i, F_IGTER)) m *= SINGLE_MOVE; /* not quite right
> */
> -      if (unit_types[i].move_type == LAND_MOVING) {
> -        if (boatspeed) { /* has to be a city, so don't bother with q */
> -          c = (warmap.cost[bx][by] + m - 1) / m + 1 +
> -                warmap.seacost[x][y] / boatspeed; /* kluge */
> -          if (unit_type_flag(i, F_MARINES)) c -= 1;
> -        } else if (warmap.cost[x][y] <= m) c = 1;
> -        else c = (warmap.cost[x][y] * q + m - 1) / m;
> -      } else if (unit_types[i].move_type == SEA_MOVING) {
> -        if (warmap.seacost[x][y] <= m) c = 1;
> -        else c = (warmap.seacost[x][y] * q + m - 1) / m;
> -      } else if (real_map_distance(pcity->x, pcity->y, x, y) * SINGLE_MOVE
> <= m) c = 1;
> -      else c = real_map_distance(pcity->x, pcity->y, x, y) * SINGLE_MOVE * q
> / m;
> +  bool shore = is_terrain_near_tile(pcity->x, pcity->y, T_OCEAN);
> +  int orig_move_type = unit_types[*best_choice].move_type;
> +  int victim_move_rate = (acity ? 1 : unit_types[victim_unit_type].move_rate
> *
> +                          (unit_type_flag(victim_unit_type, F_IGTER) ? 3 :
> 1));
> +  int victim_count = (acity ? unit_list_size(&(map_get_tile(x, y)->units)) +
> 1
> +                            : 1);
> +  Unit_Type_id unit_type;
> +
> +  for (unit_type = 0; unit_type < game.num_unit_types; unit_type++) {
> +    Tech_Type_id tech_req = unit_types[unit_type].tech_requirement;
> +    int move_type = unit_types[unit_type].move_type;
> +    int tech_dist;
> +    
> +    if (!is_ai_simple_military(unit_type)) continue;
> +    
> +    if (tech_req != A_LAST) {
> +      tech_dist = num_unknown_techs_for_goal(pplayer, tech_req);
> +    } else {
> +      tech_dist = 0;
> +    }
> +    
> +    if ((move_type == LAND_MOVING || (move_type == SEA_MOVING && shore))
> +        && tech_req != A_LAST
> +        && (tech_dist > 0 ||
> +            !can_build_unit_direct(pcity,
> unit_types[unit_type].obsoleted_by))
> +        && unit_types[unit_type].attack_strength /* or we'll get SIGFPE */
> +        && move_type == orig_move_type) {
> +      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?

> +      /* Cost (shield equivalent) of gaining these techs. */
> +      /* FIXME? Katvrr advises that this should be weighted more heavily in
> big
> +       * danger. */
> +      int tech_cost = total_bulbs_required_for_goal(pplayer,
> +                        unit_types[unit_type].tech_requirement) / 4
> +                      / city_list_size(&pplayer->cities);
> +      int move_rate = unit_types[unit_type].move_rate;
> +      int move_cost;
> +      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;

Bad Petr. Create your own define, IGTER_MOVE_COST.

> +      }
>  
> -      m = get_virtual_defense_power(i, n, x, y);

You're removing those lines. Explanation why they are not needed?

> -      m *= unit_types[n].hp * unit_types[n].firepower;
> -      if (vet) m *= 1.5;
> -      m /= 30;
> -      m *= m;
> -      d = m;
> +      /* 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;
> +      }
>  
> -      f = unit_types[i].build_cost;
> -      fprime = build_cost_balanced(i);
> +      /* 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;
> +      vuln /= 30;
> +      vuln *= vuln;
> +
> +      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.

> +      }
>  
> -      if (acity) g = unit_list_size(&(map_get_tile(acity->x,
> acity->y)->units)) + 1;
> -      else g = 1;
> -      if (unit_types[i].move_type != LAND_MOVING && !d) b0 = 0;
> -/* not bothering to s/!d/!pdef here for the time being -- Syela */
> -      else if ((unit_types[i].move_type == LAND_MOVING ||
> -              unit_types[i].move_type == HELI_MOVING) && acity &&
> -              acity->ai.invasion == 2) b0 = f * SHIELD_WEIGHTING;
> -      else {
> -     if (!acity) {
> -       b0 = kill_desire(b, a, f, d, g);
> -     } else {
> -       int a_squared = acity->ai.a * acity->ai.a;
> -
> -       b0 = kill_desire(b, a, f + acity->ai.f, d, g);
> -       if (b * a_squared > acity->ai.f * d) {
> -         b0 -= kill_desire(b, a_squared, acity->ai.f, d, g);
> -       }
> -     }
> +      /* Not bothering to s/!vuln/!pdef here for the time being. -- Syela
> +       * (this is noted elsewhere as terrible bug making warships yoyoing)
> */
> +      if (unit_types[unit_type].move_type != LAND_MOVING && !vuln) {
> +        desire = 0;
> +        
> +      } else if ((unit_types[unit_type].move_type == LAND_MOVING ||
> +                  unit_types[unit_type].move_type == HELI_MOVING)
> +                 && acity && acity->ai.invasion == 2) {
> +        desire = bcost * SHIELD_WEIGHTING;
> +        
> +      } else {
> +        if (!acity) {
> +          desire = kill_desire(value, attack, bcost, vuln, victim_count);
> +          
> +        } else {
> +          int city_attack = acity->ai.attack * acity->ai.attack;
> +
> +          /* See aiunit.move_cost:find_something_to_kill() for comments. */
> +          
> +          desire = kill_desire(value, attack, (bcost + acity->ai.bcost),
> vuln,
> +                               victim_count);
> +          
> +          if (value * city_attack > acity->ai.bcost * vuln) {
> +            desire -= kill_desire(value, city_attack, acity->ai.bcost, vuln,
> +                                  victim_count);
> +          }
> +        }

> +                                     : SHIELD_WEIGHTING);



> -  pcity->ai.a = val2;
> -  pcity->ai.f = val3;
> +  pcity->ai.attack = val2;
> +  pcity->ai.bcost = val3;

Crap names.


> +        pcity->ai.attack += a;
> +        pcity->ai.bcost += unit_type(aunit)->build_cost;
>        }
<snip>
> +            int a_squared = acity->ai.attack * acity->ai.attack;
Good.




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