[Freeciv-Dev] Re: Angry citizen [PATCH UPDATED] (PR#656)

[Davide Pagnin <nightmare@xxxxxxxxxx>]

        Hi!

I've to say only one thing.... How the hell is possible to made so many
mistakes??? Shame on me...
(And I'm also not patient at all... maintainers work is very hard!...
thanks to every one of you, guys!) 

>>    sad += i; /* if units are making us unhappy, count that too. */
>> +  sad += pcity->ppl_angry[0] * 2;
>>    freelog(LOG_DEBUG, "In %s, unh[0] = %d, unh[4] = %d, sad = %d",
>>         pcity->name, pcity->ppl_unhappy[0], pcity->ppl_unhappy[4], sad);
>
>I don't know what building_value does. What is max and val?

Well, is hard to say how building_values work. I tried to figure out,
but it is unclear to me.
In this case I applied the rule: 1 angry = 2 unhappy. If someone can
explain me this function 
I will be very grateful. Moreover, I think that if angry citizen are
disabled, this modification
don't have side effect, otherside, it looks reasonable to me to apply
the rule I write before.

>
>> +        pcity->food_surplus > 0 && !(pcity->ppl_unhappy[4] + 
>> pcity->ppl_angry[4]) && 
>
>Maybe: pcity->ppl_unhappy[4]==0 && pcity->ppl_angry[4]==0

unhappy + angry has to be 0, and unhappy>=0 and angry>=0, so it is the
same. But I changed
it to keep the code more clear.

>
>> +        wants_to_be_bigger(pcity) && ai_fuzzy(pplayer,1)) {
>
>> -    pcity->is_building_unit    = 0;
>> +    pcity->is_building_unit   = 0;
>
>Noise.

eliminated

>
>> +  return (pcity->ppl_happy[4]>=(pcity->size+1)/2 && 
>> >pcity->ppl_unhappy[4]+pcity->ppl_angry[4]==0);
>
>Maybe: pcity->ppl_unhappy[4]== && pcity->ppl_angry[4]==0

Same as before

>
>> -      values[B_COURTHOUSE]=pcity->ppl_unhappy[4]*200+pcity->ppl_elvis*400;
>> +  
>> values[B_COURTHOUSE]=(pcity->ppl_angry[4]*2+pcity->ppl_unhappy[4])*200+pcity->ppl_elvis*400;
>
>Maybe the 200 can be factored out.

I don't think so. It is more clear that the 2 factors are distinct, and
moreover there is similar code
some lines below, so I prefer to keep it this way. Feel free to change
it, this don't impact the game.

>
>> -     values[B_TEMPLE]=pcity->ppl_unhappy[4]*200+pcity->ppl_elvis*600;
>> +     
>> values[B_TEMPLE]=(pcity->ppl_angry[4]*2+pcity->ppl_unhappy[4]*200)+pcity->ppl_elvis*600;
>
>Error.

Dammit!!!! Shame on me...

>
>> -    values[B_COLOSSEUM]=pcity->ppl_unhappy[4]*200+pcity->ppl_elvis*300;
>> +    
>> values[B_COLOSSEUM]=(pcity->ppl_angry[4]*2+pcity->ppl_unhappy[4]*200)+pcity->ppl_elvis*300;
>
>Error.

Cut & paste... (Of an error!!!!)

>
>> -    values[B_CATHEDRAL]=pcity->ppl_unhappy[4]*201+pcity->ppl_elvis*300;
>> +    
>> values[B_CATHEDRAL]=(pcity->ppl_angry[4]*2+pcity->ppl_unhappy[4]*201)+pcity->ppl_elvis*300;
>
>Error.

again!

>
>> -    if (pcity->ppl_happy[4]>=pcity->ppl_unhappy[4]) {
>> +    if (pcity->ppl_happy[4]>=pcity->ppl_unhappy[4]+2*pcity->ppl_angry[4]) {
>
>In which way is this related to city_happy()?

If you look at the code, it will be clear that at this point the server
have to send the information
on city status. (whether the city is revolting or not). So it make the
same calculation done by city_unhappy.
So I introduced the use of this function (that already exist!) to made
consistent coding.

>
>Sorry to the long delay.
>
>        Raimar

I know you have many patch to review, and I REALLY appreciate your work! 
        Thanks, Raimar!

P.S. The corrected version is attached to this...

angrycitizen.diff.bz2
Description: Binary data