[Freeciv-Dev] Re: Civ 2 style happiness (PR#1436)

[Davide Pagnin <nightmare@xxxxxxxxxx>]

        Hi, all!
I've seen the notes to the specialist patch written by Ross.
I was the author of the patch, so I take a little bit of effort to
understand the objection from Ross...

On Sun, 12 May 2002 12:59:18 -0400 Ross W. Wetmore wrote:
>>static void citizen_happy_size(struct city *pcity)
>> {
>>-  int workers, tmp;
>>+  int specialists, tmp; 
>> 
>>-  workers = pcity->size - city_specialists(pcity);
>>
>>The above line introduces problems here! This makes it possible for
>>unhappy/angry people to be used as specialists. This is what I mean
>>when I say making scientists helps your happiness problems. If you have
>>one unhappy person in a city, making a scientist helps just as much
>>as making an elvis. You should not be able to turn a rioter unhappy
>>with the government into a content specialist churning out gold/lux.
>
>The code fix doesn't prevent turning rioters into specialists, it just
>delays it. As long as you don't push beyond this stage I'm happy.
>
>>+  specialists = city_specialists(pcity);
>>   tmp = content_citizens(city_owner(pcity));
>
>To really do what you say, you need to leave the initial code and
>change the above line to
>    tmp = content_citizens(city_owner(pcity)) - specialists;
>
>But this creates angry citizens when no content are found which PayCiv
>appears not to do.

You're right, perhaps Raahul has bad explained the goal of the patch.
Imagine that you have in a city a number of content citizen and a number
of unhappy. The patch assure that the specialists are taken first from
the happy number and THEN from the unhappy, this doesn't prevent to have
rioters turned to specialist, only prevent to spare already content
citizen. 

>>-  pcity->ppl_content[0] = MAX(0, MIN(workers, tmp));
>>+  pcity->ppl_content[0] = MAX(0, MIN(pcity->size, tmp) - specialists);
>>   if (game.angrycitizen == 0)
>>     pcity->ppl_angry[0] = 0;
>>   else
>>     pcity->ppl_angry[0] = MIN(MAX(0, -tmp), pcity->size);
>
>This line is wrong. It should look exactly like the content line except
>that it uses -tmp. The bug will be triggered when all new citizens are
>angry and you make one a specialist, This then creates a new negative
>unhappy citizen with your fix.

I've made an example, with city size = 5 and content_citizen = -7 and
1 specialist.

If you play with the old code, you get:
tmp = -7
workers = 5 - 1 = 4
content = MAX( 0, MIN(  4, -7)) = 0
angry = MIN( MAX( 0, 7), 5) = 5
unhappy = 4 - 0 - 5 = -1
happy = 0

If you play with the new code, you get:
specialist = 1
tmp = -7
content = MAX( 0, MIN ( 5, -7) - 1) = 0
angry = MIN( MAX( 0, 7), 5) = 5
unhappy = 5 - 1 - 0 -5 = -1
happy = 0

You've spotted a bug! But not in this patch... In the old code! ;-)
(Well... the angry patch was made by me, so I'm guilty, but I want
to point that the "specialist" patch is not wrong!

To fix the above line, which is wrong, a simple onliner patch is
required:

-     pcity->ppl_angry[0] = MIN(MAX(0, -tmp), pcity->size); 
+     pcity->ppl_angry[0] = MIN(MAX(0, -tmp), workers); 

I attach this patch to my mail, filename: patch
Which I urge to review and put in the CVS code!

>>   pcity->ppl_unhappy[0] =
>>-      workers - pcity->ppl_content[0] - pcity->ppl_angry[0];
>>+  ((pcity->size - specialists) - pcity->ppl_content[0]) -
>pcity->ppl_angry[0];
>>   pcity->ppl_happy[0] = 0;    /* no one is born happy */
>> }

On the "specialist patch" instead, this "patch" is different:
-     pcity->ppl_angry[0] = MIN(MAX(0, -tmp), pcity->size); 
+     pcity->ppl_angry[0] = MIN(MAX(0, -tmp), pcity->size -
specialists); 

I attach this as a second patch to my mail, with the complete code,
of the specialist patch that applies to the old cvs code, as 
filename patch-2 and a version that applies after the correction 
of the bug spotted by ross, filename patch-3.

As a last word, Ross stated that at least 2 AI function may misbehave
with this patch applied. I've tried to understand the problem but
my knowledge on the core of the AI code is very little and so I will
wait others that have proper knoledge to write the code on this problem.

My 2 cents are that the event of have an elvis taken from pool of
working
citizen and having more happiness problems, in very unlikely although
not impossible but can happen ONLY if you chose poorly which citizen
take out of work. 
(If I'm wrong, may you demonstrate me an example?)
Thanks in advance

        Ciao, Davide

--- city.c      Wed Feb 27 08:50:45 2002
+++ city.c.ok   Mon May 13 17:03:59 2002
@@ -1650,7 +1650,7 @@
   if (game.angrycitizen == 0)
     pcity->ppl_angry[0] = 0;
   else
-    pcity->ppl_angry[0] = MIN(MAX(0, -tmp), pcity->size);
+    pcity->ppl_angry[0] = MIN(MAX(0, -tmp), workers);
   pcity->ppl_unhappy[0] =
       workers - pcity->ppl_content[0] - pcity->ppl_angry[0];
   pcity->ppl_happy[0] = 0;     /* no one is born happy */

--- city.c      Wed Feb 27 08:50:45 2002
+++ city.c.new  Mon May 13 17:13:21 2002
@@ -1642,17 +1642,17 @@
 **************************************************************************/
 static void citizen_happy_size(struct city *pcity)
 {
-  int workers, tmp;
+  int specialists, tmp;
 
-  workers = pcity->size - city_specialists(pcity);
+  specialists = city_specialists(pcity);
   tmp = content_citizens(city_owner(pcity));
-  pcity->ppl_content[0] = MAX(0, MIN(workers, tmp));
+  pcity->ppl_content[0] = MAX(0, MIN(pcity->size, tmp) - specialists);
   if (game.angrycitizen == 0)
     pcity->ppl_angry[0] = 0;
   else
-    pcity->ppl_angry[0] = MIN(MAX(0, -tmp), pcity->size);
+    pcity->ppl_angry[0] = MIN(MAX(0, -tmp), pcity->size - specialists);
   pcity->ppl_unhappy[0] =
-      workers - pcity->ppl_content[0] - pcity->ppl_angry[0];
+    pcity->size - specialists - pcity->ppl_content[0] - pcity->ppl_angry[0];
   pcity->ppl_happy[0] = 0;     /* no one is born happy */
 }
 

--- city.c.ok   Mon May 13 17:03:59 2002
+++ city.c.new  Mon May 13 17:13:21 2002
@@ -1642,17 +1642,17 @@
 **************************************************************************/
 static void citizen_happy_size(struct city *pcity)
 {
-  int workers, tmp;
+  int specialists, tmp;
 
-  workers = pcity->size - city_specialists(pcity);
+  specialists = city_specialists(pcity);
   tmp = content_citizens(city_owner(pcity));
-  pcity->ppl_content[0] = MAX(0, MIN(workers, tmp));
+  pcity->ppl_content[0] = MAX(0, MIN(pcity->size, tmp) - specialists);
   if (game.angrycitizen == 0)
     pcity->ppl_angry[0] = 0;
   else
-    pcity->ppl_angry[0] = MIN(MAX(0, -tmp), workers);
+    pcity->ppl_angry[0] = MIN(MAX(0, -tmp), pcity->size - specialists);
   pcity->ppl_unhappy[0] =
-      workers - pcity->ppl_content[0] - pcity->ppl_angry[0];
+    pcity->size - specialists - pcity->ppl_content[0] - pcity->ppl_angry[0];
   pcity->ppl_happy[0] = 0;     /* no one is born happy */
 }