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

In the earlier reports (PR#14969), only part of the proposed patch
was committed to 2.1, as that was easiest to quickly review.

However, the remainder of the patch optimized the performance by a
significant change to the packet and map generation.  The patch was
fairly old and most patch hunks failed.  These issues were
problematic this close to a release candidate.

Instead, this patch optimizes performance with a different bit-test,
and also eliminates another test for NULL in each place, thus
outperforming the original:

-  if (ptile->resource
-   && terrain_has_resource(ptile->terrain, ptile->resource)) {
+  if (tile_resource_is_valid(ptile)) {

No changes to packet and map generation.  The change is compatible
with expected future map importation from commercial civ1/2/3.

Additionally, this patch fixes some potential problems with
settler/worker terrain+resource estimates, because the standard
functions were not used:

-    ptile->terrain = new_terrain;
+    tile_change_terrain(ptile, new_terrain);

In trunk and 2.2, a coding error is fixed.  The pillaging code used an
out-of-range constant in some enum variables.  The constant has been
added to the proper enum.

Other minor bug fixes, found by comparing 2.1 and 2.2 divergence.

Index: server/settlers.c
===================================================================
--- server/settlers.c   (revision 13728)
+++ server/settlers.c   (working copy)
@@ -312,7 +312,7 @@
     if (ptile->city && terrain_has_flag(new_terrain, TER_NO_CITIES)) {
       return -1;
     }
-    ptile->terrain = new_terrain;
+    tile_change_terrain(ptile, new_terrain);
     tile_clear_special(ptile, S_MINE);
     goodness = city_tile_value(pcity, city_x, city_y, 0, 0);
     ptile->terrain = old_terrain;
@@ -376,7 +376,7 @@
     if (ptile->city && terrain_has_flag(new_terrain, TER_NO_CITIES)) {
       return -1;
     }
-    ptile->terrain = new_terrain;
+    tile_change_terrain(ptile, new_terrain);
     tile_clear_special(ptile, S_IRRIGATION);
     tile_clear_special(ptile, S_FARMLAND);
     goodness = city_tile_value(pcity, city_x, city_y, 0, 0);
@@ -440,16 +440,7 @@
     return -1;
   }
 
-  ptile->terrain = new_terrain;
-
-  if (new_terrain->mining_result != new_terrain) {
-    tile_clear_special(ptile, S_MINE);
-  }
-  if (new_terrain->irrigation_result != new_terrain) {
-    tile_clear_special(ptile, S_FARMLAND);
-    tile_clear_special(ptile, S_IRRIGATION);
-  }
-    
+  tile_change_terrain(ptile, new_terrain);
   goodness = city_tile_value(pcity, city_x, city_y, 0, 0);
 
   ptile->terrain = old_terrain;
Index: server/savegame.c
===================================================================
--- server/savegame.c   (revision 13728)
+++ server/savegame.c   (working copy)
@@ -846,6 +846,15 @@
        set_savegame_old_resource(&ptile->resource, ptile->terrain, ch, 1));
   }
 
+  /* after the resources are loaded, indicate those currently valid */
+  whole_map_iterate(ptile) {
+    if (ptile->resource
+     && terrain_has_resource(ptile->terrain, ptile->resource)) {
+      /* cannot use set_special() for internal values */
+      BV_SET(ptile->special, S_RESOURCE_VALID);
+    }
+  } whole_map_iterate_end;
+
   /* Owner and ownership source are stored as plain numbers */
   if (has_capability("new_owner_map", savefile_options)) {
     int x, y;
Index: common/city.c
===================================================================
--- common/city.c       (revision 13728)
+++ common/city.c       (working copy)
@@ -30,7 +30,7 @@
 #include "mem.h"
 #include "movement.h"
 #include "packets.h"
-#include "unitlist.h"
+#include "unit.h"
 
 #include "cm.h"
 
@@ -594,31 +594,29 @@
                                int city_x, int city_y, bool is_celebrating,
                                Output_type_id otype)
 {
-  const struct terrain *pterrain = ptile->terrain;
   struct tile tile;
   int prod;
-  const struct output_type *output = &output_types[otype];
+  struct terrain *pterrain = tile_get_terrain(ptile);
 
   assert(otype >= 0 && otype < O_LAST);
 
-  if (ptile->terrain == T_UNKNOWN) {
+  if (T_UNKNOWN == pterrain) {
     /* Special case for the client.  The server doesn't allow unknown tiles
      * to be worked but we don't necessarily know what player is involved. */
     return 0;
   }
 
   prod = pterrain->output[otype];
-  if (ptile->resource
-   && terrain_has_resource(ptile->terrain, ptile->resource)) {
-    prod += ptile->resource->output[otype];
+  if (tile_resource_is_valid(ptile)) {
+    prod += tile_get_resource(ptile)->output[otype];
   }
 
   /* create dummy tile which has the city center bonuses. */
-  tile.terrain = tile_get_terrain(ptile);
+  tile.terrain = pterrain;
   tile.special = tile_get_special(ptile);
 
   if (pcity && is_city_center(city_x, city_y)
-      && ptile->terrain == pterrain->irrigation_result
+      && pterrain == pterrain->irrigation_result
       && terrain_control.may_irrigate) {
     /* The center tile is auto-irrigated. */
     tile_set_special(&tile, S_IRRIGATION);
@@ -659,6 +657,8 @@
   }
 
   if (pcity) {
+    const struct output_type *output = &output_types[otype];
+
     prod += get_city_tile_output_bonus(pcity, ptile, output,
                                       EFT_OUTPUT_ADD_TILE);
     if (prod > 0) {
Index: common/tile.c
===================================================================
--- common/tile.c       (revision 13728)
+++ common/tile.c       (working copy)
@@ -68,6 +68,14 @@
 void tile_set_terrain(struct tile *ptile, struct terrain *pterrain)
 {
   ptile->terrain = pterrain;
+  if (NULL != pterrain
+   && NULL != ptile->resource
+   && terrain_has_resource(pterrain, ptile->resource)) {
+    /* cannot use set_special() for internal values */
+    BV_SET(ptile->special, S_RESOURCE_VALID);
+  } else {
+    BV_CLR(ptile->special, S_RESOURCE_VALID);
+  }
 }
 
 /****************************************************************************
@@ -117,6 +125,14 @@
 void tile_set_resource(struct tile *ptile, struct resource *presource)
 {
   ptile->resource = presource;
+  if (NULL != ptile->terrain
+   && NULL != presource
+   && terrain_has_resource(ptile->terrain, presource)) {
+    /* cannot use set_special() for internal values */
+    BV_SET(ptile->special, S_RESOURCE_VALID);
+  } else {
+    BV_CLR(ptile->special, S_RESOURCE_VALID);
+  }
 }
 
 /****************************************************************************
@@ -283,7 +299,7 @@
   case S_RIVER:
   case S_AIRBASE:
   case S_FALLOUT:
-  case S_LAST:
+  default:
     break;
   }
 }
@@ -313,7 +329,7 @@
   case S_FARMLAND:
   case S_AIRBASE:
   case S_FALLOUT:
-  case S_LAST:
+  default:
     break;
   }
 }
@@ -447,9 +463,9 @@
     sz_strlcat(s, special_name_translation(S_RIVER));
   }
 
-  if (ptile->resource
-   && terrain_has_resource(ptile->terrain, ptile->resource)) {
-    cat_snprintf(s, sizeof(s), " (%s)", 
resource_name_translation(ptile->resource));
+  if (tile_resource_is_valid(ptile)) {
+    cat_snprintf(s, sizeof(s), " (%s)",
+                resource_name_translation(ptile->resource));
   }
 
   first = TRUE;
Index: common/tile.h
===================================================================
--- common/tile.h       (revision 13728)
+++ common/tile.h       (working copy)
@@ -17,7 +17,7 @@
 #include "fc_types.h"
 #include "player.h"
 #include "terrain.h"
-#include "unit.h"
+#include "unitlist.h"
 
 /* Convenience macro for accessing tile coordinates.  This should only be
  * used for debugging. */
@@ -28,14 +28,14 @@
   int x, y; /* Cartesian (map) coordinates of the tile. */
   int nat_x, nat_y; /* Native coordinates of the tile. */
   int index; /* Index coordinate of the tile. */
-  struct terrain *terrain; /* May be NULL for unknown tiles. */
+  Continent_id continent;
+  bv_player tile_known, tile_seen[V_COUNT];
   bv_special special;
   struct resource *resource; /* available resource, or NULL */
+  struct terrain *terrain; /* May be NULL for unknown tiles. */
   struct city *city;        /* city standing on the tile, NULL if none */
   struct unit_list *units;
-  bv_player tile_known, tile_seen[V_COUNT];
   struct city *worked;      /* city working tile, or NULL if none */
-  Continent_id continent;
   struct player *owner;     /* Player owning this tile, or NULL. */
   struct tile *owner_source; /* what makes it owned by owner */
   char *spec_sprite;
@@ -53,20 +53,27 @@
 /* Tile accessor functions. */
 struct player *tile_get_owner(const struct tile *ptile);
 void tile_set_owner(struct tile *ptile, struct player *pplayer);
+
 struct city *tile_get_city(const struct tile *ptile);
 void tile_set_city(struct tile *ptile, struct city *pcity);
+
 struct terrain *tile_get_terrain(const struct tile *ptile);
 void tile_set_terrain(struct tile *ptile, struct terrain *pterrain);
+
 bv_special tile_get_special(const struct tile *ptile);
 bool tile_has_special(const struct tile *ptile,
                      enum tile_special_type to_test_for);
 void tile_set_special(struct tile *ptile, enum tile_special_type spe);
+void tile_clear_special(struct tile *ptile, enum tile_special_type spe);
+void tile_clear_all_specials(struct tile *ptile);
+
+#define tile_resource_is_valid(vtile) BV_ISSET(vtile->special, 
S_RESOURCE_VALID)
 const struct resource *tile_get_resource(const struct tile *ptile);
 void tile_set_resource(struct tile *ptile, struct resource *presource);
-void tile_clear_special(struct tile *ptile, enum tile_special_type spe);
-void tile_clear_all_specials(struct tile *ptile);
+
+Continent_id tile_get_continent(const struct tile *ptile);
 void tile_set_continent(struct tile *ptile, Continent_id val);
-Continent_id tile_get_continent(const struct tile *ptile);
+
 enum known_type tile_get_known(const struct tile *ptile,
                              const struct player *pplayer);
 
Index: common/terrain.h
===================================================================
--- common/terrain.h    (revision 13728)
+++ common/terrain.h    (working copy)
@@ -18,7 +18,10 @@
 #include "fc_types.h"
 
 enum special_river_move {
-  RMV_NORMAL=0, RMV_FAST_STRICT=1, RMV_FAST_RELAXED=2, RMV_FAST_ALWAYS=3
+  RMV_NORMAL = 0,
+  RMV_FAST_STRICT = 1,
+  RMV_FAST_RELAXED = 2,
+  RMV_FAST_ALWAYS = 3,
 };
 
 enum tile_special_type {
@@ -33,13 +36,19 @@
   S_FARMLAND,
   S_AIRBASE,
   S_FALLOUT,
-  S_LAST
+
+  /* internal values not saved */
+  S_LAST,
+  S_RESOURCE_VALID = S_LAST,
+
+  /* internal values not saved and never set */
+  S_LAST_PLUS,
 };
 
 /* S_LAST-terminated */
 extern enum tile_special_type infrastructure_specials[];
 
-BV_DEFINE(bv_special, S_LAST);
+BV_DEFINE(bv_special, S_LAST_PLUS);
 
 #define T_NONE (NULL) /* A special flag meaning no terrain type. */
 #define T_UNKNOWN (NULL) /* An unknown terrain. */
Index: client/packhand.c
===================================================================
--- client/packhand.c   (revision 13728)
+++ client/packhand.c   (working copy)
@@ -1388,7 +1388,7 @@
   }
 
   if (packet->owner == game.info.player_idx) {
-    freelog(LOG_ERROR, "Got packet_short_unit for own unit.");
+    freelog(LOG_ERROR, "handle_unit_short_info() for own unit.");
   }
 
   punit = unpackage_short_unit(packet);
@@ -2012,7 +2012,14 @@
     }
   }
 
-  ptile->resource = resource_by_number(packet->resource);
+  if (ptile->resource
+   && ptile->resource->index != packet->resource) {
+    tile_changed = TRUE;
+  }
+
+  /* always called after setting terrain */
+  tile_set_resource(ptile, resource_by_number(packet->resource));
+
   if (packet->owner == MAP_TILE_OWNER_NULL) {
     if (ptile->owner) {
       ptile->owner = NULL;
@@ -2047,7 +2054,9 @@
     case TILE_UNKNOWN:
       break;
     default:
-      freelog(LOG_NORMAL, "Unknown tile value %d.", packet->known);
+      freelog(LOG_ERROR,
+              "handle_tile_info() unknown tile value %d.",
+              packet->known);
       break;
     }
   }
@@ -2273,8 +2282,8 @@
 **************************************************************************/
 void handle_ruleset_building(struct packet_ruleset_building *p)
 {
+  int i;
   struct impr_type *b = improvement_by_number(p->id);
-  int i;
 
   if (!b) {
     freelog(LOG_ERROR,
@@ -2399,8 +2408,8 @@
 **************************************************************************/
 void handle_ruleset_terrain(struct packet_ruleset_terrain *p)
 {
+  int j;
   struct terrain *pterrain = terrain_by_number(p->id);
-  int j;
 
   if (!pterrain) {
     freelog(LOG_ERROR,
Index: client/gui-sdl/mapview.c
===================================================================
--- client/gui-sdl/mapview.c    (revision 13728)
+++ client/gui-sdl/mapview.c    (working copy)
@@ -495,8 +495,7 @@
     sz_strlcat(s, special_name_translation(S_RIVER));
   }
 
-  if (ptile->resource
-   && terrain_has_resource(ptile->terrain, ptile->resource)) {
+  if (tile_resource_is_valid(ptile)) {
     cat_snprintf(s, sizeof(s), " (%s)",
                 resource_name_translation(ptile->resource));
   }
Index: client/tilespec.c
===================================================================
--- client/tilespec.c   (revision 13728)
+++ client/tilespec.c   (working copy)
@@ -4142,8 +4142,7 @@
   case LAYER_SPECIAL1:
     if (ptile && client_tile_get_known(ptile) != TILE_UNKNOWN) {
       if (draw_specials) {
-       if (ptile->resource
-        && terrain_has_resource(ptile->terrain, ptile->resource)) {
+       if (tile_resource_is_valid(ptile)) {
          ADD_SPRITE_SIMPLE(t->sprites.resource[ptile->resource->index]);
        }
       }
_______________________________________________
Freeciv-dev mailing list
Freeciv-dev@gna.org
https://mail.gna.org/listinfo/freeciv-dev

Reply via email to