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 29 Apr 2013 07:01:31 AM UTC  
 
Category: generalPriority: 5 - Normal
Status: DonePrivacy: Public
Assigned to: Marko Lindqvist <cazfi>Open/Closed: Closed
Planned Release: 2.5.0, 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.

 

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

Wed 29 May 2013 12:47:37 PM UTC, 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 29 May 2013 12:46:15 PM UTC, SVN revision 22905:

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 22905)

Marko Lindqvist <cazfi>
Project AdministratorIn charge of this item.
Wed 08 May 2013 10:26:19 PM UTC, 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 08 May 2013 09:39:03 PM UTC, 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 06 May 2013 09:16:49 AM UTC, comment #4:

The removed FC_INFINITY handling is now in patch #3897.

Emmet Hikory <persia>
Project Member
Sun 05 May 2013 09:40:31 AM UTC, 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 05 May 2013 05:58:56 AM UTC, 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 29 Apr 2013 10:56:46 AM UTC, 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 29 Apr 2013 07:01:31 AM UTC, 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):
   
   
Comment:
   

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.

     

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

     

     

    Follow 10 latest changes.

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

    Back to the top


    Powered by Savane 3.1-cleanup