[Freeciv-Dev] Re: (PR#10702) RFC: removing ptile->move_cost
[Top] [All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index] [Thread Index]
<URL: http://bugs.freeciv.org/Ticket/Display.html?id=10702 >
Benoit Hudson wrote:
> <URL: http://bugs.freeciv.org/Ticket/Display.html?id=10702 >
>
> On Tue, Apr 12, 2005 at 12:06:58AM -0700, Jason Short wrote:
>
>><URL: http://bugs.freeciv.org/Ticket/Display.html?id=10702 >
>>
>>Here's an updated patch to remove ptile->move_costs. This one is
>>intended to be committed.
>>
>>Again, the reason for removing the move_costs is that it's a fairly
>>bug-prone cache that doesn't actually speed things up very much.
>>Various comments throughout the code (which are removed by the patch)
>>should make this clear.
>
> tile_move_cost_ai has nothing to do with the AI, so just call it
> tile_move_cost. In fact, it's not local to a single tile, so it should
> probably be map_move_cost.
That's not quite true. This function is only used by the AI. It has
some special cases like MOVE_COST_FOR_VALID_SEA_STEP and maxcost. There
is also the quite misnamed tile_move_cost_ptrs function, which is called
by tile_move_cost_ai and is static in map.c. The "actual" function that
returns the cost is also in map.c and is simply a wrapper for
tile_move_cost_ptrs. It's called - you guessed it - map_move_cost.
The reason tile_move_cost_ai is used instead of map_move_cost() is
because the latter requires a unit argument. The extra special cases
are for unittype-independent movement values. Back when this was cached
in ptile->move_costs[] this made sense since you can't cache values for
every unit type. But now that the calculation is be done by hand
everywhere it makes sense to call map_move_cost() in as many places as
possible - maybe everywhere. However I leave these changes for a later
patch.
Oh, I will rename it as map_move_cost_ai though. Here's that patch.
-jason
Index: ai/aiferry.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/ai/aiferry.c,v
retrieving revision 1.18
diff -u -r1.18 aiferry.c
--- ai/aiferry.c 5 Apr 2005 20:36:09 -0000 1.18
+++ ai/aiferry.c 12 Apr 2005 16:38:28 -0000
@@ -248,7 +248,7 @@
move_cost = PF_IMPOSSIBLE_MC;
} else {
/* Land-to-Land */
- move_cost = src_tile->move_cost[dir];
+ move_cost = map_move_cost_ai(src_tile, tgt_tile);
}
return move_cost;
Index: client/goto.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/goto.c,v
retrieving revision 1.82
diff -u -r1.82 goto.c
--- client/goto.c 19 Feb 2005 17:15:13 -0000 1.82
+++ client/goto.c 12 Apr 2005 16:38:28 -0000
@@ -102,7 +102,6 @@
DRAWN(ptile, dir) = 0;
}
} whole_map_iterate_end;
- initialize_move_costs();
is_init = TRUE;
}
Index: client/packhand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/packhand.c,v
retrieving revision 1.487
diff -u -r1.487 packhand.c
--- client/packhand.c 10 Apr 2005 23:55:24 -0000 1.487
+++ client/packhand.c 12 Apr 2005 16:38:28 -0000
@@ -219,7 +219,6 @@
ptile = pcity->tile;
client_remove_city(pcity);
- reset_move_costs(ptile);
/* update menus if the focus unit is on the tile. */
if (get_unit_in_focus()) {
@@ -625,8 +624,6 @@
get_nation_name(city_owner(pcity)->nation),
pcity->name, pcity->id, TILE_XY(pcity->tile));
}
-
- reset_move_costs(pcity->tile);
}
/**************************************************************************
@@ -1945,8 +1942,6 @@
}
}
- reset_move_costs(ptile);
-
if (ptile->known <= TILE_KNOWN_FOGGED && old_known == TILE_KNOWN) {
/* This is an error. So first we log the error, then make an assertion.
* But for NDEBUG clients we fix the error. */
Index: client/gui-gtk-2.0/menu.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/client/gui-gtk-2.0/menu.c,v
retrieving revision 1.70
diff -u -r1.70 menu.c
--- client/gui-gtk-2.0/menu.c 8 Apr 2005 06:32:27 -0000 1.70
+++ client/gui-gtk-2.0/menu.c 12 Apr 2005 16:38:29 -0000
@@ -1196,7 +1196,6 @@
/* Restore the original state of the tile. */
ptile->terrain = old_terrain;
ptile->special = old_special;
- reset_move_costs(ptile);
return text;
}
Index: common/map.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.c,v
retrieving revision 1.213
diff -u -r1.213 map.c
--- common/map.c 16 Mar 2005 19:37:48 -0000 1.213
+++ common/map.c 12 Apr 2005 16:38:29 -0000
@@ -967,7 +967,6 @@
* rivers in oceans, don't clear this! */
map_clear_special(ptile, S_RIVER);
}
- reset_move_costs(ptile);
}
map_clear_special(ptile, S_MINE);
}
@@ -994,7 +993,6 @@
* rivers in oceans, don't clear this! */
map_clear_special(ptile, S_RIVER);
}
- reset_move_costs(ptile);
}
map_clear_special(ptile, S_FARMLAND);
map_clear_special(ptile, S_IRRIGATION);
@@ -1013,8 +1011,6 @@
rivers in oceans, don't clear this!
*/
}
- reset_move_costs(ptile);
-
/* Clear mining/irrigation if resulting terrain type cannot support
that feature. (With current rules, this should only clear mines,
but I'm including both cases in the most general form for possible
@@ -1123,27 +1119,31 @@
return(get_tile_type(t2->terrain)->movement_cost*SINGLE_MOVE);
}
-/***************************************************************
- tile_move_cost_ai is used to fill the move_cost array of struct
- tile. The cached values of this array are used in server/gotohand.c
- and client/goto.c. tile_move_cost_ai returns the move cost as
+/****************************************************************************
+ map_move_cost_ai returns the move cost as
calculated by tile_move_cost_ptrs (with no unit pointer to get
unit-independent results) EXCEPT if either of the source or
destination tile is an ocean tile. Then the result of the method
shows if a ship can take the step from the source position to the
destination position (return value is MOVE_COST_FOR_VALID_SEA_STEP)
- or not (return value is maxcost).
+ or not. An arbitrarily high value will be returned if the move is
+ impossible.
- A ship can take the step if:
- - both tiles are ocean or
- - one of the tiles is ocean and the other is a city or is unknown
-***************************************************************/
-static int tile_move_cost_ai(struct tile *tile0, struct tile *tile1,
- int maxcost)
+ FIXME: this function can't be used for air units because it returns
+ sea<->land moves as impossible.
+****************************************************************************/
+int map_move_cost_ai(const struct tile *tile0, const struct tile *tile1)
{
+ const int maxcost = 72; /* Arbitrary. */
+
assert(!is_server
|| (tile0->terrain != T_UNKNOWN && tile1->terrain != T_UNKNOWN));
+ /* A ship can take the step if:
+ * - both tiles are ocean or
+ * - one of the tiles is ocean and the other is a city or is unknown
+ *
+ * Note tileX->terrain will only be T_UNKNOWN at the client. */
if (is_ocean(tile0->terrain) && is_ocean(tile1->terrain)) {
return MOVE_COST_FOR_VALID_SEA_STEP;
}
@@ -1159,6 +1159,9 @@
}
if (is_ocean(tile0->terrain) || is_ocean(tile1->terrain)) {
+ /* FIXME: Shouldn't this return MOVE_COST_FOR_VALID_AIR_STEP?
+ * Note that MOVE_COST_FOR_VALID_AIR_STEP is currently equal to
+ * MOVE_COST_FOR_VALID_SEA_STEP. */
return maxcost;
}
@@ -1166,65 +1169,6 @@
}
/***************************************************************
- ...
-***************************************************************/
-static void debug_log_move_costs(const char *str, struct tile *tile0)
-{
- /* the %x don't work so well for oceans, where
- move_cost[]==-3 ,.. --dwp
- */
- freelog(LOG_DEBUG, "%s (%d, %d) [%x%x%x%x%x%x%x%x]", str,
- tile0->x, tile0->y,
- tile0->move_cost[0], tile0->move_cost[1],
- tile0->move_cost[2], tile0->move_cost[3],
- tile0->move_cost[4], tile0->move_cost[5],
- tile0->move_cost[6], tile0->move_cost[7]);
-}
-
-/***************************************************************
- Recalculate tile->move_cost[] for (x,y), and for adjacent
- tiles in direction back to (x,y). That is, call this when
- something has changed on (x,y), eg roads, city, transform, etc.
-***************************************************************/
-void reset_move_costs(struct tile *ptile)
-{
- int maxcost = 72; /* should be big enough without being TOO big */
-
- debug_log_move_costs("Resetting move costs for", ptile);
-
- /* trying to move off the screen is the default */
- memset(ptile->move_cost, maxcost, sizeof(ptile->move_cost));
-
- adjc_dir_iterate(ptile, tile1, dir) {
- ptile->move_cost[dir] = tile_move_cost_ai(ptile, tile1, maxcost);
- /* reverse: not at all obfuscated now --dwp */
- tile1->move_cost[DIR_REVERSE(dir)] =
- tile_move_cost_ai(tile1, ptile, maxcost);
- } adjc_dir_iterate_end;
- debug_log_move_costs("Reset move costs for", ptile);
-}
-
-/***************************************************************
- Initialize tile->move_cost[] for all tiles, where move_cost[i]
- is the unit-independent cost to move _from_ that tile, to
- adjacent tile in direction specified by i.
-***************************************************************/
-void initialize_move_costs(void)
-{
- int maxcost = 72; /* should be big enough without being TOO big */
-
- whole_map_iterate(ptile) {
- /* trying to move off the screen is the default */
- memset(ptile->move_cost, maxcost, sizeof(ptile->move_cost));
-
- adjc_dir_iterate(ptile, tile1, dir) {
- ptile->move_cost[dir] = tile_move_cost_ai(ptile, tile1, maxcost);
- }
- adjc_dir_iterate_end;
- } whole_map_iterate_end;
-}
-
-/***************************************************************
The cost to move punit from where it is to tile x,y.
It is assumed the move is a valid one, e.g. the tiles are adjacent.
***************************************************************/
@@ -1324,10 +1268,6 @@
void map_set_special(struct tile *ptile, enum tile_special_type spe)
{
ptile->special |= spe;
-
- if (contains_special(spe, S_ROAD) || contains_special(spe, S_RAILROAD)) {
- reset_move_costs(ptile);
- }
}
/***************************************************************
@@ -1336,10 +1276,6 @@
void map_clear_special(struct tile *ptile, enum tile_special_type spe)
{
ptile->special &= ~spe;
-
- if (contains_special(spe, S_ROAD) || contains_special(spe, S_RAILROAD)) {
- reset_move_costs(ptile);
- }
}
/***************************************************************
Index: common/map.h
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/map.h,v
retrieving revision 1.235
diff -u -r1.235 map.h
--- common/map.h 13 Mar 2005 17:50:54 -0000 1.235
+++ common/map.h 12 Apr 2005 16:38:29 -0000
@@ -50,7 +50,6 @@
int assigned; /* these can save a lot of CPU usage -- Syela */
struct city *worked; /* city working tile, or NULL if none */
Continent_id continent;
- signed char move_cost[8]; /* don't know if this helps! */
struct player *owner; /* Player owning this tile, or NULL. */
char *spec_sprite;
};
@@ -168,9 +167,6 @@
void map_set_continent(struct tile *ptile, Continent_id val);
Continent_id map_get_continent(const struct tile *ptile);
-void initialize_move_costs(void);
-void reset_move_costs(struct tile *ptile);
-
/* Number of index coordinates (for sanity checks and allocations) */
#define MAP_INDEX_SIZE (map.xsize * map.ysize)
@@ -334,6 +330,7 @@
bool is_move_cardinal(const struct tile *src_tile,
const struct tile *dst_tile);
int map_move_cost(struct unit *punit, const struct tile *ptile);
+int map_move_cost_ai(const struct tile *tile0, const struct tile *tile1);
enum tile_special_type get_special_by_name(const char * name);
const char *get_special_name(enum tile_special_type type);
bool is_safe_ocean(const struct tile *ptile);
Index: common/aicore/pf_tools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/common/aicore/pf_tools.c,v
retrieving revision 1.29
diff -u -r1.29 pf_tools.c
--- common/aicore/pf_tools.c 31 Mar 2005 18:32:10 -0000 1.29
+++ common/aicore/pf_tools.c 12 Apr 2005 16:38:30 -0000
@@ -65,7 +65,7 @@
struct pf_parameter *param)
{
/* MOVE_COST_FOR_VALID_SEA_STEP means ships can move between */
- if (ptile->move_cost[dir] == MOVE_COST_FOR_VALID_SEA_STEP
+ if (map_move_cost_ai(ptile, ptile1) == MOVE_COST_FOR_VALID_SEA_STEP
&& !is_non_allied_city_tile(ptile1, param->owner)) {
return SINGLE_MOVE;
} else {
@@ -140,7 +140,7 @@
move_cost = get_tile_type(terrain1)->movement_cost * SINGLE_MOVE;
}
} else {
- move_cost = ptile->move_cost[dir];
+ move_cost = map_move_cost_ai(ptile, ptile1);
}
return move_cost;
@@ -189,7 +189,7 @@
move_cost = SINGLE_MOVE;
} else {
/* Normal move */
- move_cost = src_tile->move_cost[dir];
+ move_cost = map_move_cost_ai(src_tile, tgt_tile);
}
}
@@ -225,7 +225,7 @@
} else if (is_ocean(ptile->terrain)) {
move_cost = get_tile_type(terrain1)->movement_cost * SINGLE_MOVE;
} else {
- move_cost = ptile->move_cost[dir];
+ move_cost = map_move_cost_ai(ptile, ptile1);
}
return move_cost;
@@ -287,7 +287,8 @@
move_cost = MOVE_COST_ROAD;
}
} else {
- move_cost = (ptile->move_cost[dir] != 0 ? MOVE_COST_ROAD : 0);
+ move_cost = (map_move_cost_ai(ptile, ptile1) != 0
+ ? MOVE_COST_ROAD : 0);
}
return move_cost;
}
Index: server/citytools.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/citytools.c,v
retrieving revision 1.309
diff -u -r1.309 citytools.c
--- server/citytools.c 1 Apr 2005 00:42:27 -0000 1.309
+++ server/citytools.c 12 Apr 2005 16:38:30 -0000
@@ -800,7 +800,6 @@
/* The units themselves are allready freed by transfer_city_units. */
unit_list_unlink_all(old_city_units);
unit_list_free(old_city_units);
- reset_move_costs(pcity->tile);
if (resolve_stack) {
resolve_unit_stacks(pgiver, ptaker, transfer_unit_verbose);
@@ -994,13 +993,6 @@
map_clear_special(ptile, S_FORTRESS);
update_tile_knowledge(ptile);
- reset_move_costs(ptile);
-/* I stupidly thought that setting S_ROAD took care of this, but of course
-the city_id isn't set when S_ROAD is set, so reset_move_costs doesn't allow
-sea movement at the point it's called. This led to a problem with the
-warmap (but not the GOTOmap warmap) which meant the AI was very reluctant
-to use ferryboats. I really should have identified this sooner. -- Syela */
-
pcity->synced = FALSE;
send_city_info(NULL, pcity);
sync_cities(); /* Will also send pcity. */
@@ -1134,8 +1126,6 @@
map_fog_pseudo_city_area(pplayer, ptile);
- reset_move_costs(ptile);
-
/* Update available tiles in adjacent cities. */
map_city_radius_iterate(ptile, tile1) {
/* For every tile the city could have used. */
Index: server/gotohand.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/gotohand.c,v
retrieving revision 1.189
diff -u -r1.189 gotohand.c
--- server/gotohand.c 14 Mar 2005 20:26:25 -0000 1.189
+++ server/gotohand.c 12 Apr 2005 16:38:30 -0000
@@ -315,15 +315,18 @@
move_cost = igter ? MOVE_COST_ROAD : MIN(base_cost,
unit_move_rate(punit));
} else if (igter)
/* NOT c = 1 (Syela) [why not? - Thue] */
- move_cost = (ptile->move_cost[dir] != 0 ? SINGLE_MOVE : 0);
+ move_cost = (map_move_cost_ai(ptile, tile1) != 0
+ ? SINGLE_MOVE : 0);
else if (punit)
- move_cost = MIN(ptile->move_cost[dir], unit_move_rate(punit));
+ move_cost = MIN(map_move_cost_ai(ptile, tile1),
+ unit_move_rate(punit));
/* else c = ptile->move_cost[k];
This led to a bad bug where a unit in a swamp was considered too far
away */
else { /* we have a city */
- int tmp = tile1->move_cost[DIR_REVERSE(dir)];
- move_cost = (ptile->move_cost[dir] + tmp +
- (ptile->move_cost[dir] > tmp ? 1 : 0))/2;
+ int tmp = map_move_cost_ai(tile1, ptile);
+ int tmp2 = map_move_cost_ai(ptile, tile1);
+
+ move_cost = (tmp2 + tmp + (tmp2 > tmp ? 1 : 0)) / 2;
}
move_cost += cost;
@@ -342,7 +345,8 @@
can move between we allow for shore bombardment/transport
to inland positions/etc. */
WARMAP_SEACOST(tile1) = move_cost;
- if (ptile->move_cost[dir] == MOVE_COST_FOR_VALID_SEA_STEP) {
+ if (map_move_cost_ai(ptile, tile1)
+ == MOVE_COST_FOR_VALID_SEA_STEP) {
add_to_mapqueue(move_cost, tile1);
}
}
@@ -647,9 +651,11 @@
int base_cost = get_tile_type(pdesttile->terrain)->movement_cost *
SINGLE_MOVE;
move_cost = igter ? 1 : MIN(base_cost, unit_move_rate(punit));
} else if (igter)
- move_cost = (psrctile->move_cost[dir] != 0 ? SINGLE_MOVE : 0);
+ move_cost = (map_move_cost_ai(psrctile, pdesttile) != 0
+ ? SINGLE_MOVE : 0);
else
- move_cost = MIN(psrctile->move_cost[dir], unit_move_rate(punit));
+ move_cost = MIN(map_move_cost_ai(psrctile, pdesttile),
+ unit_move_rate(punit));
if (!pplayer->ai.control && !map_is_known(tile1, pplayer)) {
/* Don't go into the unknown. 5*SINGLE_MOVE is an arbitrary
deterrent. */
@@ -709,7 +715,8 @@
continue; /* No need for all the calculations */
/* allow ships to target a shore */
- if (psrctile->move_cost[dir] != MOVE_COST_FOR_VALID_SEA_STEP
+ if (map_move_cost_ai(psrctile, pdesttile)
+ != MOVE_COST_FOR_VALID_SEA_STEP
&& !same_pos(tile1, dest_tile)) {
continue;
} else if (unit_loss_pct(unit_owner(punit), tile1, punit) > 0) {
@@ -982,7 +989,7 @@
if (is_ground_unit(punit)) {
/* assuming move is valid, but what if unit is trying to board?
-- GB */
- base_move_cost = punit->tile->move_cost[dir];
+ base_move_cost = map_move_cost_ai(punit->tile, ptile);
} else {
base_move_cost = SINGLE_MOVE;
}
Index: server/srv_main.c
===================================================================
RCS file: /home/freeciv/CVS/freeciv/server/srv_main.c,v
retrieving revision 1.243
diff -u -r1.243 srv_main.c
--- server/srv_main.c 12 Apr 2005 14:44:36 -0000 1.243
+++ server/srv_main.c 12 Apr 2005 16:38:31 -0000
@@ -1908,7 +1908,6 @@
gamelog(GAMELOG_TEAM, pteam);
} team_iterate_end;
- initialize_move_costs(); /* this may be the wrong place to do this */
init_settlers(); /* create minimap and other settlers.c data */
ai_data_movemap_init();
|
|
