News:

Simutrans Wiki Manual
The official on-line manual for Simutrans. Read and contribute.

[BUG]: Rail vehicles not properly unreserving when reversing

Started by freddyhayward, January 24, 2020, 11:26:18 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

freddyhayward

This problem occurs when trains occupy new tiles upon reversing then fail to unreserve the tiles it previously occupied (see both saves). I made this pull request to deal with the issue: https://github.com/jamespetts/simutrans-extended/pull/130

jamespetts

Thank you for this. I notice that the proposed fix removes a large part of code, removing the first set of checks for reserve_own_tiles (so that this is called in every working method rather than some defined working methods), and removing the next call to this method with a different parameter passed.

I am not sure that I understand the intention of the first of those changes; I have not tested this extensively yet, but I expect that these checks are to prevent unreserving in cases where the train is in, e.g., the token block or the time interval working methods where unreserving is not desired.

As to the second block of code, although I do not immediately remember what this does, this would almost certainly be there for a reason, so I am doubtful that it is sensible to remove it. A good way of working out the reason that particular code was added is to search for the commit comments in the commit in which the lines of code in question were added.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

freddyhayward

On the restriction by working method, do you remember why only drive by sight and both time interval methods allow unreservations? In my view, all working methods except for token block and one train staff should allow it, as with many other parts of the file.

And on the second block of code, this only seems to undo the unreservation done by the first block. The assumption of this block seems to be that the convoy has already moved into its new position after calling reverse_order() -- but I'm not sure whether this actually occurs there or somewhere else.

jamespetts

I am afraid that the signalling code is so complex that I do not remember most of the detail of how it works in many cases, so I try to change as little as possible of the code that appears to be working whenever I try to fix something.

May I ask what degree of testing has been done on these proposed changes?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

freddyhayward

I haven't done much testing beyond the two saves linked here. In this case, the two problems with the code are:
1. too few working methods allowing unreservation (although I will re-add the restriction on one_train_staff and token_block)
2. the unreservation being immediately re-reserved
therefore I don't see how the problem could be fixed without this amount of change.

jamespetts

Apologies for not having had time to reply to this sooner.

The difficulty is that the re-reservation was written in because it is clearly necessary in some situations. The problem is that I do not now know what those situations are. The code in question was added by commit no. d4dc6d6211227e231db558149c98e1f43c64decf on the 27th of May 2018. The commit message for that commit was:

QuoteFIX: Tiles in some cases would remain reserved incorrectly when manually unreserving tiles

This commit added the ability to unreserve to the reserve_own_tiles() method. The idea is that the tiles will be unreserved before reversing and re-reserved after reversing. However, it appears that the re-reserving occurs before the position of the train has moved after reversing.

So, in order to preserve the existing logic but overcome the problem of the re-reserving being done too early, we have to replace the re-reserve command with setting a local variable and then use that local variable to re-reserve later.

I have just pushed a fix with this solution; I should be grateful if you could re-test.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.