OpenTTD

Tasklist

FS#6379 - YAPF cache/desync bug due to incorrect caching of unfollowable rail type change

Attached to Project: OpenTTD
Opened by J G Rennison (JGR) - Thursday, 15 October 2015, 18:06 GMT
Last edited by frosch (frosch) - Friday, 30 October 2015, 16:29 GMT
Type Bug
Category Vehicles → YAPF
Status Closed
Assigned To No-one
Operating System All
Severity Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

CFollowTrackT::Follow in src/pathfinder/follow_track.hpp unconditionally returns EC_NO_WAY for failure conditions due to CanEnterNewTile returning false, even when CanEnterNewTile returns the correct result of EC_RAIL_TYPE in the case where the current vehicle cannot use the new rail type.
When a result of EC_NO_WAY instead of EC_RAIL_TYPE is cached (via the test of the return value in PfCalcCost), subsequent vehicles which can use the new track type will erroneously avoid it if using the cache, as it is cached as a dead end, but will use it if the segment had not been cached (or had not been cached incorrectly), which leads to desyncs and can be caught by the cache checks in stChooseRailTrack etc.

See savegame for example.
Train 1 cannot use the track segment from Tile B to the station, Train 2 can.
If starting train 1 from the depot, it goes via Path 2 at Tile C, and caches the segment at Tile A towards Tile B incorrectly as a dead end.
If starting train 2 from the depot afterwards, it incorrectly goes via Path 2 instead of Path 1 at Tile C, and this triggers the desync check in stChooseRailTrack.
If starting train 2 without previously starting train 1, it correctly goes via Path 1 at Tile C.

See attached patch for proposed fix to avoid overwriting return code of CanEnterNewTile with EC_NO_WAY, the savegame performs correctly with this applied.
EC_NO_WAY only needs to be explicitly set in the changed line when `m_new_td_bits == TRACKDIR_BIT_NONE` (an actual dead end), as the ` || !CanEnterNewTile()` in the conditional already sets `m_err` to the correct value in the other cases.

The patch/testing was performed on r27409.
This task depends upon

Closed by  frosch (frosch)
Friday, 30 October 2015, 16:29 GMT
Reason for closing:  Fixed
Additional comments about closing:  in r27418. Thanks for the patch!

Loading...