[Freeciv-Dev] Re: (PR#725) Production upgrade of newly-researched units deferred

["Jason Short via RT" <rt@xxxxxxxxxxxxxx>]

Per I. Mathisen via RT wrote:
> On Fri, 17 Jan 2003, Jason Short via RT wrote:
> 
>>>The proposed solution is to reorder the handling of cities so that all
>>>gold and science is generated first, then everything else (including
>>>unit upgrades) is considered.
>>
>>Upon further review, this really is the only solution.
> 
> 
> While we're at it, let's ensure that cities grow before producing
> settlers. Here's Thomas's patch for this issue, which I haven't yet gotten
> around to testing properly, let alone committing.

This patch looks good (although it doesn't fix PR#725, naturally).  I 
now see that the populate_city and city_build_stuff updates are not 
atomic - they should be split into two steps, as Thomas has done.

Doing that allows us to reorder the updates with a lot of flexibility. 
Combining Pille's ideas with my original patch gives me a new patch with 
the updates all reordered.  This should solve both the settlers bug and 
the upgrade bug without introducing any new problems that I can see.

The basic idea is that the order should be:

* Count up all resources for this turn (trade, food, shields).
* Update science for the civilization.
* Update the city populations (based on granary size).
* Update the city productions (upgrades & finished production).
* Update the city morale (rapture & disorder).
* Do misc. updates.

although this differs slightly in the code (as explained in comments).

I think the most important thing we need to do is make sure to comment 
all the dependencies in the ordering.  It's quite possible there are 
still some we've missed - but we don't want to have to figure it out 
from scratch every time this code is changed.

Note: I kept city_distribute_surplus_shields and 
nullify_caravan_and_disband_plus together, although I agree with Pille 
that this doesn't really matter.  nullify_caravan_and_disband_plus could 
go into update_city_flags instead.

Finally, if this patch is too complicated for consumption, it would be 
possible to split it up into several steps:

   1.  Introduce all the helper functions that are used.
   2.  Remove update_city_activity so all updates are done atomically.
   3.  Reorder the updates.

with steps 2 and 3 being interchangable.

jason

Index: server/cityturn.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/cityturn.c,v
retrieving revision 1.202
diff -u -r1.202 cityturn.c
--- server/cityturn.c   2003/01/09 02:36:37     1.202
+++ server/cityturn.c   2003/01/17 18:58:36
@@ -72,7 +72,6 @@
 static bool disband_city(struct city *pcity);
 
 static void define_orig_production_values(struct city *pcity);
-static void update_city_activity(struct player *pplayer, struct city *pcity);
 static void nullify_caravan_and_disband_plus(struct city *pcity);
 
 static void worker_loop(struct city *pcity, int *foodneed,
@@ -80,6 +79,10 @@
 
 static void advisor_choose_build(struct player *pplayer, struct city *pcity);
 
+static void update_city_granary(struct city *pcity);
+static void update_city_shield_stock(struct player *pplayer,
+                                    struct city *pcity);
+
 /**************************************************************************
 ...
 **************************************************************************/
@@ -404,6 +407,105 @@
 }
 
 /**************************************************************************
+  End-of-turn update for the city's celebration status.
+**************************************************************************/
+static void update_city_rapture(struct player *pplayer, struct city *pcity)
+{
+  /* reporting of celebrations rewritten, copying the treatment of disorder
+   * below, with the added rapture rounds count.  991219 -- Jing */
+
+  if (city_celebrating(pcity)) {
+    pcity->rapture++;
+    if (pcity->rapture == 1) {
+      notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_LOVE,
+                      _("Game: We Love The %s Day celebrated in %s."), 
+                      get_ruler_title(pplayer->government, pplayer->is_male,
+                                      pplayer->nation),
+                      pcity->name);
+    }
+  } else {
+    if (pcity->rapture != 0) {
+      notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_NORMAL,
+                      _("Game: We Love The %s Day canceled in %s."),
+                      get_ruler_title(pplayer->government, pplayer->is_male,
+                                      pplayer->nation),
+                      pcity->name);
+    }
+    pcity->rapture = 0;
+  }
+  pcity->was_happy = city_happy(pcity);
+}
+
+/**************************************************************************
+  End-of-turn update for the city's disorder status.
+**************************************************************************/
+static void update_city_disorder(struct player *pplayer, struct city *pcity)
+{
+  if (city_unhappy(pcity)) { 
+    pcity->anarchy++;
+    if (pcity->anarchy == 1) {
+      notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_DISORDER,
+                      _("Game: Civil disorder in %s."), pcity->name);
+    } else {
+      notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_DISORDER,
+                      _("Game: CIVIL DISORDER CONTINUES in %s."),
+                      pcity->name);
+    }
+  } else {
+    if (pcity->anarchy != 0) {
+      notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_NORMAL,
+                      _("Game: Order restored in %s."), pcity->name);
+      pcity->anarchy = 0;
+    }
+  }
+}
+
+/**************************************************************************
+  Check to see if this city causes the civilization to fall back into
+  anarchy.
+**************************************************************************/
+static void check_anarchy(struct player *pplayer, struct city *pcity)
+{
+  struct government *g = get_gov_pcity(pcity);
+
+  if (pcity->anarchy > 2
+      && government_has_flag(g, G_REVOLUTION_WHEN_UNHAPPY)) {
+    notify_player_ex(pplayer, pcity->x, pcity->y, E_ANARCHY,
+                    _("Game: The people have overthrown your %s, "
+                      "your country is in turmoil."),
+                    get_government_name(g->index));
+    handle_player_revolution(pplayer);
+  }
+}
+
+/**************************************************************************
+  Reset various per-turn flags for the city.
+**************************************************************************/
+static void update_city_flags(struct city *pcity)
+{
+  pcity->is_updated = TRUE;
+
+  pcity->did_sell = FALSE;
+  pcity->did_buy = FALSE;
+  if (city_got_building(pcity, B_AIRPORT)) {
+    pcity->airlift = TRUE;
+  } else {
+    pcity->airlift = FALSE;
+  }
+}
+
+/**************************************************************************
+  Handle income and expenses for the city this turn.
+**************************************************************************/
+static void update_city_income(struct player *pplayer, struct city *pcity)
+{
+  /* Keep income and expenses checks together, so that a city at least
+   * has the opportunity to pay for itself. */
+  pplayer->economic.gold += pcity->tax_total;
+  pay_for_buildings(pplayer, pcity);   
+}
+
+/**************************************************************************
 ...
 **************************************************************************/
 void update_city_activities(struct player *pplayer)
@@ -411,9 +513,103 @@
   int gold;
   gold=pplayer->economic.gold;
   pplayer->got_tech = FALSE;
-  city_list_iterate(pplayer->cities, pcity)
-     update_city_activity(pplayer, pcity);
-  city_list_iterate_end;
+
+  /* Specific updates for cities.  The order of these is very important!
+   * In some cases one update will affect other updates for that city;
+   * in others it will affect other updates for the whole civilization.
+   * Hopefully an update will never affect other updates from different
+   * civilizations! */
+
+  city_list_iterate(pplayer->cities, pcity) {
+    city_refresh(pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* the AI often has widespread disorder when the Gardens or Oracle
+     * become obsolete.  This is hack to prevent this.  Putting it lower
+     * would basically be cheating. */
+    /* FIXME: a smart player will sometimes let their population starve -
+     * but this code makes it impossible for the AI to do that. */
+    while (pplayer->ai.control && city_unhappy(pcity)) {
+      if (ai_make_elvis(pcity) == 0) {
+       break;
+      }
+    }
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* These three updates shouldn't have any side effects.  They go at
+     * the top so that they are dependent on the previous turn's settings. */
+    update_city_granary(pcity);
+    update_city_shield_stock(pplayer, pcity);
+    update_city_income(pplayer, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This update also goes near the top, but it can cause side effects.
+     * It must go above the update_production update since tech advances
+     * can cause a production upgrade. */
+    update_tech(pplayer, pcity->science_total);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This is handled above city_populate so rapture will take place
+     * immediately.  But there is a 1-turn delay build in before the
+     * city starts to celebrate.*/
+    update_city_rapture(pplayer, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This can result in the city disbanding, which just means it
+     * won't show up in the lower loops. Since this is handled below
+     * the rapture check a population boom will start immediately. It
+     * is also handled after food, shields, gold, and science are
+     * counted so those values will be based on the previous turn's
+     * city size. */
+    city_populate(pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This can result in the city disbanding, which just means it
+     * won't show up in the lower loops. It is below city_populate
+     * so that pop_cost will work properly, and below update_tech so that
+     * production upgrades will be handled fully. */
+    city_build_stuff(pplayer, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This is handled after city_populate and city_build_stuff, which 
+     * means they aren't influenced by disorder for this turn. */
+    update_city_disorder(pplayer, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* Note this happens after everything that influences pollution
+     * has already been updated. */
+    check_pollution(pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This is done after all the updates, although it probably
+     *  doesn't matter. */
+    update_city_flags(pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* Send out updated information about the city. */
+    send_city_info(NULL, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This comes last. */
+    check_anarchy(pplayer, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    sanity_check_city(pcity);
+  } city_list_iterate_end;
+
+  /* These checks don't really seem to belong here, but... */
   pplayer->ai.prev_gold = gold;
   if (gold-(gold-pplayer->economic.gold)*3<0) {
     notify_player_ex(pplayer, -1, -1, E_LOW_ON_FUNDS,
@@ -563,12 +759,20 @@
 }
 
 /**************************************************************************
+  Add on food grown this turn. This can be done before the city is
+  checked for growth in populate_city().
+**************************************************************************/
+static void update_city_granary(struct city *pcity)
+{
+  pcity->food_stock += pcity->food_surplus;
+}
+
+/**************************************************************************
   Check whether the population can be increased or
   if the city is unable to support a 'settler'...
 **************************************************************************/
 static void city_populate(struct city *pcity)
 {
-  pcity->food_stock+=pcity->food_surplus;
   if(pcity->food_stock >= city_granary_size(pcity->size) 
      || city_rapture_grow(pcity)) {
     city_increase_size(pcity);
@@ -1088,14 +1292,22 @@
 }
 
 /**************************************************************************
-return 0 if the city is removed
-return 1 otherwise
+  Add on the shield production for this turn.  This can be done before the
+  production is actually done.
 **************************************************************************/
-static bool city_build_stuff(struct player *pplayer, struct city *pcity)
+static void update_city_shield_stock(struct player *pplayer,
+                                    struct city *pcity)
 {
   city_distribute_surplus_shields(pplayer, pcity);
   nullify_caravan_and_disband_plus(pcity);
+}
 
+/**************************************************************************
+return 0 if the city is removed
+return 1 otherwise
+**************************************************************************/
+static bool city_build_stuff(struct player *pplayer, struct city *pcity)
+{
   if (!pcity->is_building_unit) {
     return city_build_building(pplayer, pcity);
   } else {
@@ -1257,94 +1469,6 @@
 {
   nullify_caravan_and_disband_plus(pcity);
   pcity->before_change_shields=0;
-}
-
-/**************************************************************************
- Called every turn, at end of turn, for every city.
-**************************************************************************/
-static void update_city_activity(struct player *pplayer, struct city *pcity)
-{
-  struct government *g = get_gov_pcity(pcity);
-
-  city_refresh(pcity);
-
-  /* the AI often has widespread disorder when the Gardens or Oracle
-     become obsolete.  This is a quick hack to prevent this.  980805 -- Syela 
*/
-  while (pplayer->ai.control && city_unhappy(pcity)) {
-    if (ai_make_elvis(pcity) == 0) break;
-  } /* putting this lower in the routine would basically be cheating. -- Syela 
*/
-
-  /* reporting of celebrations rewritten, copying the treatment of disorder 
below,
-     with the added rapture rounds count.  991219 -- Jing */
-  if (city_build_stuff(pplayer, pcity)) {
-    if (city_celebrating(pcity)) {
-      pcity->rapture++;
-      if (pcity->rapture == 1)
-       notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_LOVE,
-                        _("Game: We Love The %s Day celebrated in %s."), 
-                        get_ruler_title(pplayer->government, pplayer->is_male,
-                                        pplayer->nation),
-                        pcity->name);
-    }
-    else {
-      if (pcity->rapture != 0)
-       notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_NORMAL,
-                        _("Game: We Love The %s Day canceled in %s."),
-                        get_ruler_title(pplayer->government, pplayer->is_male,
-                                        pplayer->nation),
-                        pcity->name);
-      pcity->rapture=0;
-    }
-    pcity->was_happy=city_happy(pcity);
-
-    /* City population updated here, after the rapture stuff above. --Jing */
-    {
-      int id=pcity->id;
-      city_populate(pcity);
-      if(!player_find_city_by_id(pplayer, id))
-       return;
-    }
-
-    pcity->is_updated=TRUE;
-
-    pcity->did_sell=FALSE;
-    pcity->did_buy = FALSE;
-    if (city_got_building(pcity, B_AIRPORT))
-      pcity->airlift=TRUE;
-    else
-      pcity->airlift=FALSE;
-    update_tech(pplayer, pcity->science_total);
-    pplayer->economic.gold+=pcity->tax_total;
-    pay_for_buildings(pplayer, pcity);
-
-    if(city_unhappy(pcity)) { 
-      pcity->anarchy++;
-      if (pcity->anarchy == 1) 
-        notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_DISORDER,
-                        _("Game: Civil disorder in %s."), pcity->name);
-      else
-        notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_DISORDER,
-                        _("Game: CIVIL DISORDER CONTINUES in %s."),
-                        pcity->name);
-    }
-    else {
-      if (pcity->anarchy != 0)
-        notify_player_ex(pplayer, pcity->x, pcity->y, E_CITY_NORMAL,
-                        _("Game: Order restored in %s."), pcity->name);
-      pcity->anarchy=0;
-    }
-    check_pollution(pcity);
-
-    send_city_info(NULL, pcity);
-    if (pcity->anarchy>2 && government_has_flag(g, G_REVOLUTION_WHEN_UNHAPPY)) 
{
-      notify_player_ex(pplayer, pcity->x, pcity->y, E_ANARCHY,
-                      _("Game: The people have overthrown your %s, "
-                        "your country is in turmoil."),
-                      get_government_name(g->index));
-      handle_player_revolution(pplayer);
-    }
-    sanity_check_city(pcity);
-  }
 }
 
 /**************************************************************************