Summary: Rewrite get_MC callbacks for complex nativity
Submitted by: persia
Submitted on: Wed 08 May 2013 05:09:33 PM JST
Priority: 5 - Normal
Assigned to: None
Discussion Lock: Any
The pathfinding get_MC callbacks are selected based on utype_move_type(),
have inherent assumptions about which flags could apply to which move_types,
terrain nativity of cities, and binary nativity. For UMT_BOTH units, there is
additional awkwardness in that AI requests for different types of movement
with different restrictions are not honored. Aside from inconsistency, there
are probably bugs hiding in the conflation of various conditions, and it
certainly isn't suitable for highly complex rulesets with widely overlapping
This patch provides a complete rewrite, with three get_MC functions for
the three generic needs (normal, overlap, attack), with inline helper
functions providing logic to avoid the potential for partial updates in the
future. This makes it fairly easy to see the behaviour differences for the
different callbacks, and also independently verify the correctness of the
logic permitting attacks, permitting moves, and assigning costs for various
The logical tests are constructed in such a way that in most cases no
condition will be checked twice for a given set of starting conditions. The
exception is that whether the unit is native to either the source or
destination may be checked twice if the unit cannot attack non-native tiles,
cannot attack from non-native tiles, is on a non-native tile, and is
considering a move to a native tile containing non-allied units (source is
checked twice here: I'm not quite sure how to check destination twice,
although my tautological tables imply it should be possible). I chose not to
cache this value mostly because we usually don't need to call it more than
once. Suggestions on ways to rearrange the logic so that we don't either
force such calls when unnecessary or ever call it twice are appreciated:
perhaps in the event it is called once, the value can be cached to be used
later, although this is only useful if the cost of such caching is less than
the cost of checking tile nativity (which is fairly expensive). Note also
that we don't bother checking to see if the target tile is a city in
pf_able_to_attack() mostly because we know that if it happens to be a city, we
can't conquer it until the inhabitants have
been attacked: depending on the intent of the attack nativity conditionals, it
may be appropriate to check for native cities in the event the tiles in
question are not native (probably by implementing can_class_exist_at_tile()).
There may be other interesting tests for pf_able_to_attack() (e.g.
UTYF_CIVILIAN): I'm unsure how much to add here vs. how much processing should
be done at each step for pathfinding to behave both well and fast.
Some of the behaviour changes caused with this patch include:
* UMT_BOTH units now do not travel through unallied unit tiles when called
in overlap or attack modes.
* UMT_SEA units now do not travel through unallied unit tiles when called in
* UCF_MISSILE and UTYF_ONEATTACK units are now charged the remainder of
their movement within a turn when attacking.
* UMT_SEA and UMT_BOTH units are now able to use citychannel tiles (and must
have UCF_BUILD_ANYWHERE to use cities without adjacent nativity).
* UMT_SEA and UMT_BOTH units now check for available transport on non-native
* Several cases that were previously assigned SINGLE_MOVE now use
single_move_cost, and tile_move_cost_ptrs() may return different move costs
for these actions, depending on available terrain, transport, etc.
* Units attempting to conquer unoccupied non-allied cities now must check
UTYF_CAN_OCCUPY_CITY to do so.
This patch functionally depends on patch #3897 and patch #3900: without
the former, fueled units are incorrectly charged for UCF_MISSILE and
UTYF_ONEATTACK attacks, and without the latter, pathfinding loses even the
rudimentary UTYF_IGTER handling presently available. The patch itself isn't
very readable: reviewers are encouraged to look at pre-application and
post-application pf_tools.c side-by-side to more clearly see the differences
in the beginning of the file: changes near the end are more readable as diff.
Note that reverse_move() does not compile if the #ifdef UNUSED protection
is removed. While I've tried to preserve the intent of reverse_move_unit(), I
suspect that in nearly all cases it would be better to use get_MC(dest, dir,
src, param) to calculate the value: the exception being that slow_invasions
handling would not be correct with such logic (but this bug already exists in
Date: Wed 08 May 2013 05:09:33 PM JST Name: native-get_MC-callbacks.patch
Size: 21kB By: persia
Reply to this item at:
Message sent via/by Gna!
Freeciv-dev mailing list