patchFreeciv - Patches: patch #3901, Rewrite get_MC callbacks for...

Show feedback again

patch #3901: Rewrite get_MC callbacks for complex nativity

Submitted by:  Emmet Hikory <persia>
Submitted on:  Wed 08 May 2013 08:09:33 AM UTC  
Category: aiPriority: 5 - Normal
Status: NonePrivacy: Public
Assigned to: pepeto <pepeto>Open/Closed: Open
Planned Release: 2.6.0

Add a New Comment (Rich MarkupRich Markup):

You are not logged in

Please log in, so followups can be emailed to you.


Sun 20 Apr 2014 08:59:37 PM UTC, comment #5:

Updated patch addressing identified issues from review. Overlap moves now have a separate cost function (set to param->move_rate, rather than SINGLE_MOVE to discourage attempting to land in two places in one turn). Overlap moves still consider attacking, but only if they would be moving to non-native territory (this may cause issues with paths post-combat, but pathfinding handles non-terminal combat poorly anyway, so not a regression). The order of conditionals for pf_is_ok_move_tile() was adjusted, which should speed resolution for most cases involving entering or leaving cities (while potentially slowing resolution when entering transport in an otherwise inaccessible city in games permitting city channels (should be rare)). Patch applies over r24782.

(file #20540)

Emmet Hikory <persia>
Project Member
Wed 16 Apr 2014 10:46:57 PM UTC, comment #4:

After some discussion on IRC, pepeto and I concluded that the comment "/* Entering port or bombardment */" introduced into sea_overlap_move() in r12065 was, while sensible, not the intended purpose of this use of SINGLE_MOVE. Rather, this use of SINGLE_MOVE is intended to represent the cost of moving to normally inaccessible territory as a result of overlap movement, to facilitate path planning for ferries. As a result, it is probably inappropriate to check pf_is_able_to_attack() in overlap moves, and there should probably be a new function overlap_move_cost, initially always returning SINGLE_MOVE, but used to indicate semantic construction for future developers encountering this issue, or needing to apply special values for this condition.

There was also some discussion about the appropriate ordering of the city_channel_tile() check and the potential_transport check in pf_is_ok_move_tile(), and interaction of this ordering with potential application of is_source_tile to city_channels and use in overlap moves.

Note that bug #21871 is now targeted only towards S2_5, so this patch must also address that issue in final form.

Review remains underway: those with rulesets that depend on complex nativity may feel free to test with the current patch (which addresses the vast majority of client-side GoTo issues), but be aware there may be additional changes as a result of deeper review.

Emmet Hikory <persia>
Project Member
Thu 03 Apr 2014 08:47:51 AM UTC, comment #3:

The attached patch is rebased on r24748 and only checks whether a transport is present without orders, rather than checking if it has available capacity (similar logic to that in bug #21871, which patch is rendered unnecessary if this is applied), and further only checks this for destination tiles, so that one may disembark from transport with orders.

It appears there is still potential for confusion: when performing an overlap move, transports are never checked for orders: this may cause the AI to be slightly more likely to try to use transports that will not be there when they arrive for certain sorts of moves, but in all cases these are attempts it would attempt with the logic in r24748, so do not represent a regression.

(file #20465)

Emmet Hikory <persia>
Project Member
Sun 23 Mar 2014 01:42:45 PM UTC, comment #2:

Found an oddity testing with this: If a transport is full, units in the transport cannot disembark from it with client-side GoTo pathfinding if the transport is on a tile that is otherwise non-native to the disembarking unit. This seems to be a result of checking pf_is_ok_move_tile() on the source, and excluding tiles where the transport has no available capacity. Possible solutions involve a) passing a boolean to indicate whether to test transport capacity, b) move the transport capacity test into the callers, c) split into pf_is_ok_src_tile() and pf_is_ok_dst_tile(), and d) don't bother checking capacity in pathfinding, and only block the moves at the actual move time (this may confuse the AI). I'll pick one after thinking about it, unless someone else has a preference, or a better solution than those listed above.

Note that the AI doesn't seem to have issues disembarking from ships (at least it successfully disembarks settlers and military forces on foreign shores), and players can work around this with direct movement (number keys, arrow keys, etc.).

Emmet Hikory <persia>
Project Member
Thu 20 Mar 2014 03:55:07 AM UTC, comment #1:

Rebased against r24699

(file #20392)

Emmet Hikory <persia>
Project Member
Wed 08 May 2013 08:09:33 AM UTC, original submission:

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 nativity.

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 actions.

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. Suggestions welcome.

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 overlap mode.
* 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 tiles.
* 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 pf_reverse_map_get_costs()).

Emmet Hikory <persia>
Project Member


(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):

Attached Files
file #20540:  native-get_MC-callbacks+firstreview.patch added by persia (21kB - application/octet-stream)
file #20465:  native-get_MC-callbacks+transports.patch added by persia (20kB - application/octet-stream)
file #20392:  native-get_MC-callbacks+rebase.patch added by persia (19kB - application/octet-stream)
file #17925:  native-get_MC-callbacks.patch added by persia (21kB - application/octet-stream)


Depends on the following items: None found

Items that depend on this one: None found


Carbon-Copy List
  • -unavailable- added by pepeto (Updated the item)
  • -unavailable- added by persia (Submitted the item)

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.


    Please enter the title of George Orwell's famous dystopian book (it's a date):



    Follow 6 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sun 20 Apr 2014 08:59:37 PM UTCpersiaAttached File-=>Added native-get_MC-callbacks+firstreview.patch, #20540
    Wed 16 Apr 2014 10:46:57 PM UTCpersiaPlanned Release=>2.6.0
    Thu 03 Apr 2014 08:47:51 AM UTCpersiaAttached File-=>Added native-get_MC-callbacks+transports.patch, #20465
    Thu 20 Mar 2014 03:55:07 AM UTCpersiaAttached File-=>Added native-get_MC-callbacks+rebase.patch, #20392
    Wed 08 Jan 2014 12:40:34 PM UTCpepetoAssigned toNone=>pepeto
    Wed 08 May 2013 08:09:33 AM UTCpersiaAttached File-=>Added native-get_MC-callbacks.patch, #17925
    Show feedback again

    Back to the top

    Powered by Savane 3.1-cleanup