patchFreeciv - Patches: patch #3886, Refactoring tile_move_cost_ptrs()

Show feedback again

patch #3886: Refactoring tile_move_cost_ptrs()

Submitted by:  Emmet Hikory <persia>
Submitted on:  Mon Apr 29 07:01:31 2013  
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Planned Release: 2.5.0, 2.6.0Contains string changes: None

Add a New Comment (Rich MarkupRich Markup):

You are not logged in

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


(Jump to the original submission Jump to the original submission)

Wed May 29 12:47:37 2013, SVN revision 22906:

Optimized tile_move_cost_ptrs(). Do not assume that move cost on
non-native terrain is at least SINGLE_MOVE.

Patch by Emmet Hikory

See patch #3886

(Browse SVN revision 22906)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed May 29 12:46:15 2013, SVN revision 22905:

Do not assume that move cost on non-native terrain is at least

Patch by Emmet Hikory

See patch #3886

(Browse SVN revision 22905)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed May 8 22:26:19 2013, comment #6:

Oops. Comment fixed. The assumption is indeed wrong: I was too caught up in the idea of moving from sea to land as part of an invasion. Fixed, and S2_5 patch attached also fixing that assumption (but not otherwise refactoring the function).

(file #17933, file #17934)

Emmet Hikory <persia>
Project Member
Wed May 8 21:39:03 2013, comment #5:

- Comment /* UCF_TERRAIN_SPEED units have a constant cost. */ is reversed. UCF_TERRAIN_SPEED units have no constant cost, but one from terrain.
- Isn't asumption "we expect the cost of the move as calculated below not to be less than SINGLE_MOVE." wrong? What about road that unit can use, but which has no "NativeTile" flag (i.e., ones that unit can use on already native tiles, but that do not make non-native terrain tiles native - for example alien ruleset roads don't make radiating tiles native for Earthly units)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Mon May 6 09:16:49 2013, comment #4:

The removed FC_INFINITY handling is now in patch #3897.

Emmet Hikory <persia>
Project Member
Sun May 5 09:40:31 2013, comment #3:

It appears to have been the inclusion of FC_INFINITY / pf_fuel_map_adjust_cost() changes, which I'll split into a separate ticket later once it no longer potentially causes the game to hang. The remainder of the changes (patch attached) are no longer able to replicate the hung games.

I did find some differences in further testing with the alien ruleset, which I attribute to different move costs for moves to non-native terrain in pathfinding, rather than discovery during the execution of the move step (the prior code assumed all tiles were native (bool native = TRUE) for calls from pathfinding (punit == NULL)).

(file #17890)

Emmet Hikory <persia>
Project Member
Sun May 5 05:58:56 2013, comment #2:

Just ran another autogame against this over SVN 22826 which didn't terminate. Will be breaking apart to identify unterminating code: although more testing is welcome, reviewers are discouraged from applying this patch as-is to their local trees.

Emmet Hikory <persia>
Project Member
Mon Apr 29 10:56:46 2013, comment #1:

And I made a mistake in pf_fuel_map_adjust_cost(): the correct value is the MINIMUM value, not the MAXIMUM value.

(file #17856)

Emmet Hikory <persia>
Project Member
Mon Apr 29 07:01:31 2013, original submission:

I've been looking at pathfinding, and many of my investigations fall back into tile_move_cost_ptrs(), making me think about ways it might be faster to execute (as it is called so often).

The attached patch tries to streamline the execution path, avoiding duplicate conditionals and unnecessary caching, and attempting to delay all operations as late as possible (especially the road iteration, in the hopes it need not be performed).

In passing, I addressed the FC_INFINITY TODO, as passing this correctly is essential for proper pathfinding, although the only situation in which it wasn't already handled correctly was for fueled units with nativity restrictions (which aren't present in shipping rulesets). This will introduce a behaviour change for pathfinding, correctly charging the remainder of the move when determining a path if slow invasions is enabled (currently pathfinding assumes units may continue to move in such situations if their move points are not all used, potentially leading to units standing on the coast in positions they might otherwise have avoided as final positions (e.g. next to enemy military units)).

Another introduced behaviour change is that with this logic UTYF_IGTER units entering non-native terrain that has a road native to the unit without the RF_NATIVE_TILE flag and with a move cost less than MOVE_COST_IGTER will be charged MOVE_COST_IGTER move points, rather than the cost from the road. Again, not something present in any shipping rulesets, and easily worked around by ruleset authors by adjusting RF_NATIVE_TILE for the road.

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 #17933:  disembark-costs-SINGLE_MOVE.S2_5.patch added by persia (2kB - application/octet-stream)
file #17856:  refactor-tile_move_cost_ptrs+MIN.patch added by persia (6kB - application/octet-stream)
file #17851:  refactor-tile_move_cost_ptrs.patch added by persia (6kB - application/octet-stream)


Depends on the following items: None found

Items that depend on this one: None found


Carbon-Copy List
  • -unavailable- added by cazfi (Posted a comment)
  • -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.


    Error: not logged in



    Follow 10 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed May 29 12:47:54 2013cazfiStatusReady For Test=>Done
    Tue May 14 06:47:59 2013cazfiStatusNone=>Ready For Test
      Assigned toNone=>cazfi
      Planned Release=>2.5.0, 2.6.0
    Wed May 8 22:26:19 2013persiaAttached File-=>Added disembark-costs-SINGLE_MOVE.S2_5.patch, #17933
      Attached File-=>Added refactor-tile_move_cost_ptrs+disembarking.patch, #17934
    Sun May 5 09:40:31 2013persiaAttached File-=>Added refactor-tile_move_cost_ptrs+no-infinity.patch, #17890
    Mon Apr 29 10:56:46 2013persiaAttached File-=>Added refactor-tile_move_cost_ptrs+MIN.patch, #17856
    Mon Apr 29 07:01:31 2013persiaAttached File-=>Added refactor-tile_move_cost_ptrs.patch, #17851
    Show feedback again

    Back to the top

    Powered by Savane 3.1-cleanup