[Freeciv-Dev] (PR#13656) handle side effects when changing terrain

["Jason Short" <jdorje@xxxxxxxxxxxxxxxxxxxxx>]

<URL: http://bugs.freeciv.org/Ticket/Display.html?id=13656 >

This patch improves the handling of global side effects when changing 
terrain.

Local side effects are handled directly inside tile_change_terrain. 
Global side effects are handled independently, currently in an ad-hoc 
way.  In update_unit_activity when land and ocean are toggled a large 
series of checks are done.  Some are done when the change happens and 
some are done later.

In the new system the unit and continent checks are done all inside one 
function, check_terrain_change.  The functions are also much simplified 
because of improvements in the underlying code (safe unit iteration, 
consistent transported_by fields, better movement/terrain code).

Finally, this check is done inside the global-warming terrain change 
code.  This may not be the only place it's needed but it's the one that 
comes to mind first.  Currently if you edit the ruleset to allow global 
warming to create ocean then you can get server crashes (AFAICT).

-jason

? vgcore.pid14896
Index: server/maphand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/maphand.c,v
retrieving revision 1.171
diff -p -u -r1.171 maphand.c
--- server/maphand.c    4 Aug 2005 15:47:11 -0000       1.171
+++ server/maphand.c    12 Aug 2005 03:54:42 -0000
@@ -17,16 +17,18 @@
 
 #include <assert.h>
 
-#include "events.h"
 #include "fcintl.h"
-#include "game.h"
 #include "log.h"
-#include "map.h"
 #include "mem.h"
-#include "nation.h"
-#include "packets.h"
 #include "rand.h"
 #include "support.h"
+
+#include "events.h"
+#include "game.h"
+#include "map.h"
+#include "movement.h"
+#include "nation.h"
+#include "packets.h"
 #include "unit.h"
 
 #include "citytools.h"
@@ -233,6 +235,7 @@ void global_warming(int effect)
     if (new != T_NONE && old != new) {
       effect--;
       tile_change_terrain(ptile, new);
+      check_terrain_change(ptile, old);
       update_tile_knowledge(ptile);
       unit_list_iterate(ptile->units, punit) {
        if (!can_unit_continue_current_activity(punit)) {
@@ -275,6 +278,7 @@ void nuclear_winter(int effect)
     if (new != T_NONE && old != new) {
       effect--;
       tile_change_terrain(ptile, new);
+      check_terrain_change(ptile, old);
       update_tile_knowledge(ptile);
       unit_list_iterate(ptile->units, punit) {
        if (!can_unit_continue_current_activity(punit)) {
@@ -1517,32 +1521,72 @@ static void ocean_to_land_fix_rivers(str
   } cardinal_adjc_iterate_end;
 }
 
-/**************************************************************************
-  Checks for terrain change between ocean and land.  Handles side-effects.
-  (Should be called after any potential ocean/land terrain changes.)
-  Also, returns an enum ocean_land_change, describing the change, if any.
+/****************************************************************************
+  A helper function for check_terrain_change that moves units off of invalid
+  terrain after it's been changed.
+****************************************************************************/
+static void bounce_units_on_terrain_change(struct tile *ptile)
+{
+  unit_list_iterate_safe(ptile->units, punit) {
+    if (punit->tile == ptile
+       && punit->transported_by == -1
+       && !can_unit_exist_at_tile(punit, ptile)) {
+      /* look for a nearby safe tile */
+      adjc_iterate(ptile, ptile2) {
+       if (can_unit_exist_at_tile(punit, ptile2)
+           && !is_non_allied_unit_tile(ptile2, unit_owner(punit))) {
+         freelog(LOG_VERBOSE,
+                 "Moved %s's %s due to changing terrain at %d,%d.",
+                 unit_owner(punit)->name, unit_name(punit->type),
+                 punit->tile->x, punit->tile->y);
+         notify_player_ex(unit_owner(punit),
+                          punit->tile, E_UNIT_RELOCATED,
+                          _("Moved your %s due to changing terrain."),
+                          unit_name(punit->type));
+         (void) move_unit(punit, ptile2, 0);
+         if (punit->activity == ACTIVITY_SENTRY) {
+           handle_unit_activity_request(punit, ACTIVITY_IDLE);
+         }
+         break;
+       }
+      } adjc_iterate_end;
+      if (punit->tile == ptile) {
+       /* if we get here we could not move punit */
+       freelog(LOG_VERBOSE,
+               "Disbanded %s's %s due to changing land to sea at (%d, %d).",
+               unit_owner(punit)->name, unit_name(punit->type),
+               punit->tile->x, punit->tile->y);
+       notify_player_ex(unit_owner(punit),
+                        punit->tile, E_UNIT_LOST,
+                        _("Disbanded your %s due to changing terrain."),
+                        unit_name(punit->type));
+       wipe_unit_spec_safe(punit, FALSE);
+      }
+    }
+  } unit_list_iterate_safe_end;
+}
 
-  if we did a land change, we try to avoid reassigning
-  continent numbers.
-**************************************************************************/
-enum ocean_land_change check_terrain_ocean_land_change(struct tile *ptile,
-                                                struct terrain *oldter)
+/****************************************************************************
+  Handles global side effects for a terrain change.  Call this in the
+  server immediately after calling tile_change_terrain.
+****************************************************************************/
+void check_terrain_change(struct tile *ptile, struct terrain *oldter)
 {
   struct terrain *newter = tile_get_terrain(ptile);
-  enum ocean_land_change change_type = OLC_NONE;
+  bool ocean_toggled = FALSE;
 
   if (is_ocean(oldter) && !is_ocean(newter)) {
     /* ocean to land ... */
     ocean_to_land_fix_rivers(ptile);
     city_landlocked_sell_coastal_improvements(ptile);
-
-    change_type = OLC_OCEAN_TO_LAND;
+    ocean_toggled = TRUE;
   } else if (!is_ocean(oldter) && is_ocean(newter)) {
     /* land to ocean ... */
-    change_type = OLC_LAND_TO_OCEAN;
+    ocean_toggled = TRUE;
   }
 
-  if (change_type != OLC_NONE) {
+  if (ocean_toggled) {
+    bounce_units_on_terrain_change(ptile);
     assign_continent_numbers(FALSE);
 
     /* New continent numbers for all tiles to all players */
@@ -1550,8 +1594,6 @@ enum ocean_land_change check_terrain_oce
     
     map_update_borders_landmass_change(ptile);
   }
-
-  return change_type;
 }
 
 /*************************************************************************
Index: server/maphand.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/maphand.h,v
retrieving revision 1.60
diff -p -u -r1.60 maphand.h
--- server/maphand.h    4 Aug 2005 15:47:12 -0000       1.60
+++ server/maphand.h    12 Aug 2005 03:54:42 -0000
@@ -20,8 +20,6 @@
 
 #include "hand_gen.h"
 
-enum ocean_land_change { OLC_NONE, OLC_OCEAN_TO_LAND, OLC_LAND_TO_OCEAN };
-
 struct section_file;
 struct conn_list;
 
@@ -104,8 +102,7 @@ void map_update_borders_city_change(stru
 void map_update_borders_landmass_change(struct tile *ptile);
 void map_calculate_borders(void);
 
-enum ocean_land_change check_terrain_ocean_land_change(struct tile *ptile,
-                                              struct terrain *oldter);
+void check_terrain_change(struct tile *ptile, struct terrain *oldter);
 int get_continent_size(Continent_id id);
 int get_ocean_size(Continent_id id);
 
Index: server/unittools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/unittools.c,v
retrieving revision 1.374
diff -p -u -r1.374 unittools.c
--- server/unittools.c  4 Aug 2005 02:50:35 -0000       1.374
+++ server/unittools.c  12 Aug 2005 03:54:43 -0000
@@ -73,8 +73,6 @@ static void update_unit_activity(struct 
 static void wakeup_neighbor_sentries(struct unit *punit);
 static void do_upgrade_effects(struct player *pplayer);
 
-static void sentry_transported_idle_units(struct unit *ptrans);
-
 static bool maybe_cancel_patrol_due_to_enemy(struct unit *punit);
 static int hp_gain_coord(struct unit *punit);
 
@@ -622,7 +620,6 @@ static void update_unit_activity(struct 
   int id = punit->id;
   bool unit_activity_done = FALSE;
   enum unit_activity activity = punit->activity;
-  enum ocean_land_change solvency = OLC_NONE;
   struct tile *ptile = punit->tile;
   bool check_adjacent_units = FALSE;
   
@@ -779,7 +776,7 @@ static void update_unit_activity(struct 
       struct terrain *old = tile_get_terrain(ptile);
 
       tile_apply_activity(ptile, ACTIVITY_IRRIGATE);
-      solvency = check_terrain_ocean_land_change(ptile, old);
+      check_terrain_change(ptile, old);
       unit_activity_done = TRUE;
     }
   }
@@ -807,7 +804,7 @@ static void update_unit_activity(struct 
       struct terrain *old = tile_get_terrain(ptile);
 
       tile_apply_activity(ptile, ACTIVITY_MINE);
-      solvency = check_terrain_ocean_land_change(ptile, old);
+      check_terrain_change(ptile, old);
       unit_activity_done = TRUE;
       check_adjacent_units = TRUE;
     }
@@ -819,7 +816,7 @@ static void update_unit_activity(struct 
       struct terrain *old = tile_get_terrain(ptile);
 
       tile_apply_activity(ptile, ACTIVITY_TRANSFORM);
-      solvency = check_terrain_ocean_land_change(ptile, old);
+      check_terrain_change(ptile, old);
       unit_activity_done = TRUE;
       check_adjacent_units = TRUE;
     }
@@ -880,144 +877,6 @@ UNIT_LOG(LOG_ERROR, punit, "using old go
       handle_unit_activity_request(punit2, ACTIVITY_IDLE);
     }
   } unit_list_iterate_end;
-
-  /* Any units that landed in water or boats that landed on land as a
-     result of settlers changing terrain must be moved back into their
-     right environment.
-     We advance the unit_list iterator passed into this routine from
-     update_unit_activities() if we delete the unit it points to.
-     We go to START each time we moved a unit to avoid problems with the
-     tile unit list getting corrupted.
-
-     FIXME:  We shouldn't do this at all!  There seems to be another
-     bug which is expressed when units wind up on the "wrong" terrain;
-     this is the bug that should be fixed.  Also, introduction of the
-     "amphibious" movement category would allow the definition of units
-     that can "safely" change land<->ocean -- in which case all others
-     would (here) be summarily disbanded (suicide to accomplish their
-     task, for the greater good :).   --jjm
-  */
- START:
-  switch (solvency) {
-  case OLC_NONE:
-    break; /* nothing */
-
-  case OLC_LAND_TO_OCEAN:
-    unit_list_iterate(ptile->units, punit2) {
-      if (is_ground_unit(punit2)) {
-       /* look for nearby land */
-       adjc_iterate(ptile, ptile2) {
-         if (!is_ocean(ptile2->terrain)
-             && !is_non_allied_unit_tile(ptile2, unit_owner(punit2))) {
-           if (get_transporter_capacity(punit2) > 0)
-             sentry_transported_idle_units(punit2);
-           freelog(LOG_VERBOSE,
-                   "Moved %s's %s due to changing land to sea at (%d, %d).",
-                   unit_owner(punit2)->name, unit_name(punit2->type),
-                   punit2->tile->x, punit2->tile->y);
-           notify_player_ex(unit_owner(punit2),
-                            punit2->tile, E_UNIT_RELOCATED,
-                            _("Moved your %s due to changing"
-                              " land to sea."), unit_name(punit2->type));
-           (void) move_unit(punit2, ptile2, 0);
-           if (punit2->activity == ACTIVITY_SENTRY)
-             handle_unit_activity_request(punit2, ACTIVITY_IDLE);
-           goto START;
-         }
-       } adjc_iterate_end;
-       /* look for nearby transport */
-       adjc_iterate(ptile, ptile2) {
-         if (is_ocean(ptile2->terrain)
-             && ground_unit_transporter_capacity(ptile2,
-                                                 unit_owner(punit2)) > 0) {
-           if (get_transporter_capacity(punit2) > 0)
-             sentry_transported_idle_units(punit2);
-           freelog(LOG_VERBOSE,
-                   "Embarked %s's %s due to changing land to sea at (%d, %d).",
-                   unit_owner(punit2)->name, unit_name(punit2->type),
-                   punit2->tile->x, punit2->tile->x);
-           notify_player_ex(unit_owner(punit2),
-                            punit2->tile, E_UNIT_RELOCATED,
-                            _("Embarked your %s due to changing"
-                              " land to sea."), unit_name(punit2->type));
-           (void) move_unit(punit2, ptile2, 0);
-           if (punit2->activity == ACTIVITY_SENTRY)
-             handle_unit_activity_request(punit2, ACTIVITY_IDLE);
-           goto START;
-         }
-       } adjc_iterate_end;
-       /* if we get here we could not move punit2 */
-       freelog(LOG_VERBOSE,
-               "Disbanded %s's %s due to changing land to sea at (%d, %d).",
-               unit_owner(punit2)->name, unit_name(punit2->type),
-               punit2->tile->x, punit2->tile->y);
-       notify_player_ex(unit_owner(punit2),
-                        punit2->tile, E_UNIT_LOST,
-                        _("Disbanded your %s due to changing"
-                          " land to sea."), unit_name(punit2->type));
-       wipe_unit_spec_safe(punit2, FALSE);
-       goto START;
-      }
-    } unit_list_iterate_end;
-    break;
-  case OLC_OCEAN_TO_LAND:
-    unit_list_iterate(ptile->units, punit2) {
-      if (is_sailing_unit(punit2)) {
-       /* look for nearby water */
-       adjc_iterate(ptile, ptile2) {
-         if (is_ocean(ptile2->terrain)
-             && !is_non_allied_unit_tile(ptile2, unit_owner(punit2))) {
-           if (get_transporter_capacity(punit2) > 0)
-             sentry_transported_idle_units(punit2);
-           freelog(LOG_VERBOSE,
-                   "Moved %s's %s due to changing sea to land at (%d, %d).",
-                   unit_owner(punit2)->name, unit_name(punit2->type),
-                   punit2->tile->x, punit2->tile->y);
-           notify_player_ex(unit_owner(punit2),
-                            punit2->tile, E_UNIT_RELOCATED,
-                            _("Moved your %s due to changing"
-                              " sea to land."), unit_name(punit2->type));
-           (void) move_unit(punit2, ptile2, 0);
-           if (punit2->activity == ACTIVITY_SENTRY)
-             handle_unit_activity_request(punit2, ACTIVITY_IDLE);
-           goto START;
-         }
-       } adjc_iterate_end;
-       /* look for nearby port */
-       adjc_iterate(ptile, ptile2) {
-         if (is_allied_city_tile(ptile2, unit_owner(punit2))
-             && !is_non_allied_unit_tile(ptile2, unit_owner(punit2))) {
-           if (get_transporter_capacity(punit2) > 0)
-             sentry_transported_idle_units(punit2);
-           freelog(LOG_VERBOSE,
-                   "Docked %s's %s due to changing sea to land at (%d, %d).",
-                   unit_owner(punit2)->name, unit_name(punit2->type),
-                   punit2->tile->x, punit2->tile->y);
-           notify_player_ex(unit_owner(punit2),
-                            punit2->tile, E_UNIT_RELOCATED,
-                            _("Docked your %s due to changing"
-                              " sea to land."), unit_name(punit2->type));
-           (void) move_unit(punit2, ptile2, 0);
-           if (punit2->activity == ACTIVITY_SENTRY)
-             handle_unit_activity_request(punit2, ACTIVITY_IDLE);
-           goto START;
-         }
-       } adjc_iterate_end;
-       /* if we get here we could not move punit2 */
-       freelog(LOG_VERBOSE,
-               "Disbanded %s's %s due to changing sea to land at (%d, %d).",
-               unit_owner(punit2)->name, unit_name(punit2->type),
-               punit2->tile->x, punit2->tile->y);
-       notify_player_ex(unit_owner(punit2),
-                        punit2->tile, E_UNIT_LOST,
-                        _("Disbanded your %s due to changing"
-                          " sea to land."), unit_name(punit2->type));
-       wipe_unit_spec_safe(punit2, FALSE);
-       goto START;
-      }
-    } unit_list_iterate_end;
-    break;
-  }
 }
 
 /**************************************************************************
@@ -2058,25 +1917,6 @@ void send_all_known_units(struct conn_li
   flush_packets();
 }
 
-
-/**************************************************************************
-For all units which are transported by the given unit and that are
-currently idle, sentry them.
-**************************************************************************/
-static void sentry_transported_idle_units(struct unit *ptrans)
-{
-  struct tile *ptile = ptrans->tile;
-
-  unit_list_iterate(ptile->units, pcargo) {
-    if (pcargo->transported_by == ptrans->id
-       && pcargo->id != ptrans->id
-       && pcargo->activity == ACTIVITY_IDLE) {
-      pcargo->activity = ACTIVITY_SENTRY;
-      send_unit_info(unit_owner(pcargo), pcargo);
-    }
-  } unit_list_iterate_end;
-}
-
 /**************************************************************************
   Nuke a square: 1) remove all units on the square, and 2) halve the 
   size of the city on the square.