[Freeciv-Dev] Re: [PATCH] advdomestic.c cleanup (PR#1149)

[Petr Baudis <pasky@xxxxxxxxxxx>]

Dear diary, on Sun, Dec 23, 2001 at 07:36:25AM CET, I got a letter,
where Mike Kaufman <mkaufman@xxxxxxxxxxxxxx> told me, that...
> I looked at your patch, and I have a couple of comments/questions:
> 
> patch doesn't apply properly. I have the latest version?
> line 505 : if(B_CURE) -> impr_happy_val
You will get patch against latest CVS :).

> impr_happy_val()
> ----------------
> put bounds that function is supposed to return in the comment header?
> perhaps explain the meaning of the magnitude of return values.
I put 'desire to build it' there - desire is terminus technicus in the AI and
we can't put explanation of it to each function. But we should compose some
common description of it and its bounds and put it to README.AI probably.

> makes less sense now that bored doesn't exist.
>  /* Usage of (happy && bored) led to a lack of foresight, especially
>    * re: Chapel -- Syela */
Fixed.

> pollution_benefit()
> ---------------------
> 
> confusing?
> /* Count bonuses for that production bonuses ;-). */
Alternative? I don't think it's so confusing...

> don't let this line wrap
>     pollution += (player_knows_techs_with_flag(pplayer,
> TF_POPULATION_POLLUTION_INC) * pcity->size) / 4;
Now I did what I could, but they are going to smash me that I do indent's job
:). But I suspect indent would really break it at wrong place.

> a comment on this? 64?  
> weight = (POLLUTION_WEIGHTING + pplayer->ai.warmth) * 64;
Not entirely sure. Appears as some kind of combination of rescaling factor and
yet another heuristic constant to me, but I need Ross' opinion here. I really
have no idea why it's there, as it appears to me that:

  weight = (POLLUTION_WEIGHTING + pplayer->ai.warmth) * 64;
  int amortized = amortize(weight, 100 / pcity->pollution);
  cost = ((amortized * weight) / (MAX(1, weight - amortized))) / 64;

is equivalent to

  weight = (POLLUTION_WEIGHTING + pplayer->ai.warmth);
  int amortized = amortize(weight, 100 / pcity->pollution);
  cost = (amortized * weight * 64 * 64) / (MAX(1, (weight * 64) - (amortized * 
64)) * 64);

which is equivalent to
  ...
  cost = (amortized * weight * 64 * 64) / (MAX(1, (weight - amortized) * 64) * 
64);

so

  cost = (amortized * weight * 64) / (MAX(1, (weight - amortized) * 64));

thus

  cost = (amortized * weight) / (MAX(1 / 64, weight - amortized));

What I missed? :)

> Substract -> Subtract
Oh, and to this moment I always thought it's "substract" :). But my dictionary
agrees with you.

> put a function comment header on ai_eval_buildings()
> or is this another patch?
This is another patch.

> that's all. Could you send me an updated patch with these things in
> mind? otherwise it does look very nice.
Here it is...

-- 

                                Petr "Pasky" Baudis

UN*X programmer, UN*X administrator, hobbies = IPv6, IRC, FreeCiv hacking
.
  "A common mistake that people make, when trying to design
   something completely foolproof is to underestimate the
   ingenuity of complete fools."
     -- Douglas Adams in Mostly Harmless
.
Public PGP key, geekcode and stuff: http://pasky.ji.cz/~pasky/

advdomestic.c.diff
Description: Text document