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

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

[jdorje - Fri Jan 17 07:30:23 2003]:

> [ChrisK@xxxxxxxx - Wed Mar 14 17:21:38 2001]:
> 
> > When Explosives are sucessfully researched, the
> > production is upgraded from settlers to engineers
> > in _some_ cities, but not in _all_.
> 
> This is an annoying bug.
> 
> 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.

Often one step of the update process will influence other steps.  There
are three kinds of influence:

1.  The update influences other updates in this city.
2.  The update influences other updates in a different city of the same
civilization.
3.  The update influences other updates in different cities of different
civilizations.

#1 is very common.  #2 happens in at least one place - research
influences production - and probably others.  Hopefully #3 doesn't
happen, since handling that would require even larger changes.

It is difficult to tell what influences what else.  So IMO it is safest
to break up the city update into its atomic parts, and conduct each
update for all cities before moving on to the next city.  The attached
patch does this, without changing the order of the updates.

Since the order of the updates isn't changed, this patch doesn't fix the
bug - it makes it worse.  Now no cities will get production upgrades
because of research, since all production is handled before research. 
But fixing this needs to be done very carefully, since the order of the
steps can make a big difference in the outcome of the update.

This is a relatively small patch, but very serious!  I encourage
everyone to look at it and try to determine what the order of the
updates *should* be.  The current order is wrong because it doesn't
correctly handle production upgrades.  But I suspect the order cannot be
changed without affecting the game rules in some other way.

Note the patch will affect gameplay even as it is, so comparing
autogames is not likely to be helpful.

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 08:25:16
@@ -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,
@@ -404,6 +403,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 update_city_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 +509,89 @@
   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. */
+    while (pplayer->ai.control && city_unhappy(pcity)) {
+      if (ai_make_elvis(pcity) == 0) {
+       break;
+      }
+    }
+  } 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.  Also note since it is above
+     * the technology check, we won't properly upgrade units when they
+     * become obsolete. */
+    city_build_stuff(pplayer, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    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. */
+    city_populate(pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* The order of this is unimportant. */
+    update_city_flags(pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This is below the rapture and population updates
+     * but above the disorder updates. */
+    update_tech(pplayer, pcity->science_total);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This is below the rapture and population updates
+     * but above the disorder updates. */
+    update_city_income(pplayer, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This is handled below the rapture check, which changes behavior
+     * slightly */
+    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) {
+    send_city_info(NULL, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    /* This comes last. */
+    update_city_anarchy(pplayer, pcity);
+  } city_list_iterate_end;
+
+  city_list_iterate(pplayer->cities, pcity) {
+    sanity_check_city(pcity);
+  } city_list_iterate_end;
+
   pplayer->ai.prev_gold = gold;
   if (gold-(gold-pplayer->economic.gold)*3<0) {
     notify_player_ex(pplayer, -1, -1, E_LOW_ON_FUNDS,
@@ -1257,94 +1435,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);
-  }
 }
 
 /**************************************************************************