[Freeciv-Dev] Re: advdomestic.c cleanup II. (PR#1157)

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Sat, Dec 29, 2001 at 01:21:29AM CET, I got a letter, where Mike
Kaufman <mkaufman@xxxxxxxxxxxxxx> told me, that...
> my comments are:
> 
> >>> comment here.
> 
> did you check to see if any speed up occurs by changing the order of the
> sections? granted it wouldn't be much.
I don't think it would help.

> /********************************************************************** 
> This function should assign a value, want and type to choice (that means what
> to build and how important it is).
> 
> If want is 0, this advisor doesn't want anything.
> ***********************************************************************/
> void domestic_advisor_choose_build(struct player *pplayer, struct city *pcity,
>                                  struct ai_choice *choice)
> {
>   /* Government of the player */
>   struct government *gov = get_gov_pplayer(pplayer);
>   /* Unit type with certain role */
>   Unit_Type_id unit_type;
>   /* Food surplus assuming that workers and elvii are already accounted for
>    * and properly balanced. */
>   int est_food = pcity->food_surplus +
>                  2 * pcity->ppl_scientist +
>                2 * pcity->ppl_taxman; 
>   /* Total count of caravans available or already being built */
>   int caravans = 0;
>   /* Continent where the city is - important for caravans */
>   int continent = map_get_continent(pcity->x, pcity->y);
> 
>   /* Had to add the scientist guess here too. -- Syela */
> 
> >> this is confusing: what does this relate to?
Moved in previous patch.
> 
>   choice->choice = 0;
>   choice->want   = 0;
>   choice->type   = CT_NONE;
> 
>   unit_type = best_role_unit(pcity, F_SETTLERS);
> 
> >> a section header comment would be nice: e.g.:
> >> /* Find out how much the city wants settlers. */
Ok.
> 
>   if (est_food > utype_food_cost(get_unit_type(unit_type), gov)) {
>     /* Allowing multiple settlers per city now. I think this is correct.
>      * -- Syela */
> 
> >> what does this comment have to do with the code in question?
This code just decides if build settlers or not ;p. Moved some lines lower.
>     
>     /* settler_want is calculated in settlers.c, called from 
> ai_manage_city(). */
> 
> >> don't let your comments wrap lines please. this is annoying.
Deleted fullstop and comma to satisfy you ;p
> 
> >> confusing. is settler_want calculated when whenever ai_manage_city()
> >> is called?
It's written in this way there, isn't it?
> 
>     int want = pcity->ai.settler_want;
> 
>     if (want > 0) {
>       freelog(LOG_DEBUG, "%s (%d, %d) desires settlers with passion %d",
>             pcity->name, pcity->x, pcity->y, want);
>       choice->want = want;
>       choice->type = CT_NONMIL;
>       ai_choose_role_unit(pplayer, pcity, choice, F_SETTLERS, want);
>       
>     } else if (want < 0) {
>       /* We need boats to colonize! */
>       choice->want = 0 - want;
>       choice->type = CT_NONMIL;
>       choice->choice = unit_type; /* default */
>       ai_choose_ferryboat(pplayer, pcity, choice);
>     }
>   }
> 
> >> this must be explained: what positive/negative wants mean...
> >> e.g. does _any_ negative value == boat want?
Ok, elaborated.
> 
>   /* Basically, copied from above and adjusted to handle city founders.
>    * -- jjm */
>   unit_type = best_role_unit(pcity, F_CITIES);
> 
>   if (est_food > utype_food_cost(get_unit_type(unit_type), gov)) {
>     /* founder_want is calculated in settlers.c, called from 
> ai_manage_city(). */
>     int want = pcity->ai.founder_want;
> 
> >> see above, settler_want
Saw ;)
> 
>     if (want > choice->want) {
>       freelog(LOG_DEBUG, "%s (%d, %d) desires founders with passion %d",
>             pcity->name, pcity->x, pcity->y, want);
>       choice->want = want;
>       choice->type = CT_NONMIL;
>       ai_choose_role_unit(pplayer, pcity, choice, F_CITIES, want);
>       
>     } else if (want < -choice->want) {
>       /* We need boats to colonize! */
>       choice->want = 0 - want;
>       choice->type = CT_NONMIL;
>       choice->choice = unit_type; /* default */
>       ai_choose_ferryboat(pplayer, pcity, choice);
>     }
>   }
> 
> >> section header
> >> e.g. /* find out how much the city wants to build a caravan */
What else is /* Count caravans */ ? ;)
> 
>   /* Count caravans */
>   unit_list_iterate(pplayer->units, punit) {
>     if (unit_flag(punit, F_CARAVAN) &&
>         map_get_continent(punit->x, punit->y) == continent) caravans++;
>   } unit_list_iterate_end;
>   city_list_iterate(pplayer->cities, acity) {
>     if (acity->is_building_unit && acity->shield_stock >= 50 &&
>         unit_type_flag(acity->currently_building, F_CARAVAN) &&
>         map_get_continent(acity->x, acity->y) == continent) caravans++;
>   } city_list_iterate_end;
> 
Aha - added heading here in previous patch as well.
>   city_list_iterate(pplayer->cities, acity) {
>     if (!acity->is_building_unit &&
>       is_wonder(acity->currently_building) &&
> 
> >> aren't these two mutually exclusive? why is the first test necessary?
> >> for caravan?
Because if the city isn't building unit, acity->currently_building numbers
means ids of _units_, not _buildings_, so you have to always test for this.
It's probably another candidate for description in README.AI.
> 
>         map_get_continent(acity->x, acity->y) == continent &&
>         acity != pcity &&
>       build_points_left(acity) > 50 * caravans) {
>       /* Desire for this building */
>       int want = pcity->ai.building_want[acity->currently_building];
>       /* Distance to wonder city was established after ai_manage_buildings()
>        * and before this.  If we just started building a wonder during
>        * ai_city_choose_build(), the started_building notify comes equipped
>        * with an update.  It calls generate_warmap(), but this is a lot less
>        * warmap generation than there would be otherwise. -- Syela */
>       int dist;
>       
>       unit_type = best_role_unit(pcity, F_CARAVAN);
>       
>       /* Value of 8 is a total guess and could be wrong, but it's still better
>        * than 0. -- Syela */
>       dist = pcity->ai.distance_to_wonder_city * 8 /
>            ((unit_type == U_LAST) ? SINGLE_MOVE
>                                   : get_unit_type(unit_type)->move_rate);
>       want -= dist;
>       
>       unit_type = get_role_unit(F_CARAVAN, 0);
>       if (can_build_unit_direct(pcity, unit_type)) {
>         if (want > choice->want) {
>           choice->want = want;
>           choice->type = CT_NONMIL;
>         ai_choose_role_unit(pplayer, pcity, choice, F_CARAVAN, dist / 2);
>         }
>       } else {
>       int tech_req = get_unit_type(unit_type)->tech_requirement;
>       
>       pplayer->ai.tech_want[tech_req] += want;
>       }
>     }
>   } city_list_iterate_end;
> 
> >> another section header here
Okok.
> 
>   {
>     struct ai_choice cur;
>     
>     ai_advisor_choose_building(pcity, &cur);
>     if (cur.want > choice->want) {
>       choice->choice = cur.choice;
>       choice->want = cur.want;
>       /* Allowing buy of peaceful units after much testing. -- Syela */
>       /* want > 100 means BUY RIGHT NOW */
>       /* if (choice->want > 100) choice->want = 100; */
> 
> >> why is this commented out? especially confusing with comment directly
> >> above it.
See the comment two lines above it ;).
> 
>       choice->type = CT_BUILDING;
>     }
>   }
> 
>   /* FIXME: rather (choice->want <= 0) --rwetmore */
>   if (!choice->want) {
>     /* Oh dear, better think of something! */
> 
> >> a better comment would give more insight here
Like? We can't be so bald everywhere ;).
> 
>     unit_type = best_role_unit(pcity, F_CARAVAN);
>     
>     if (unit_type == U_LAST) {
>       unit_type = best_role_unit(pcity, F_DIPLOMAT);
>       /* Someday, real diplomat code will be here! */
>     }
>     
>     if (unit_type != U_LAST) {
>       choice->want = 1;
>       choice->type = CT_NONMIL;
>       choice->choice = unit_type;
>     }
>   }
>   
>   /* Otherwise, we buy caravans in city X when we should be saving money to 
> buy
>    * defenses for city Y. -- Syela */
>   if (choice->want >= 200) choice->want = 199;
> 
> >> shed some insight on these magic numbers?
I proposed already they should be described centrally in README.AI - it's no
point in describing them near each their occurence.
> 
>   return;
> }

Attached new version of patch.

Thanks for review,

-- 

                                Petr "Pasky" Baudis

UN*X programmer, UN*X administrator, hobbies = IPv6, IRC, FreeCiv, (e)links
.
Firewall in a way is like the doorkeeper of a local pub. If you don't have your
I.D. on you, or if for some reason you do not qualify to enter, the doorkeeper
will not permit you to enter. In some extreme cases these doorkeepers will not
let you out, or at least give you a hard time before they finally let you out.
.
Public PGP key, geekcode and stuff: http://pasky.ji.cz/~pasky/

advdomestic.c.diff
Description: Text document