The International Simutrans Forum

 

Author Topic: [BUG]: Rail vehicles not properly unreserving when reversing  (Read 382 times)

0 Members and 1 Guest are viewing this topic.

Offline freddyhayward

  • Devotee
  • *
  • Posts: 437
  • Languages: EN
[BUG]: Rail vehicles not properly unreserving when reversing
« on: January 24, 2020, 11:26:18 PM »
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

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 20274
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: [BUG]: Rail vehicles not properly unreserving when reversing
« Reply #1 on: January 25, 2020, 01:20:36 AM »
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.

Offline freddyhayward

  • Devotee
  • *
  • Posts: 437
  • Languages: EN
Re: [BUG]: Rail vehicles not properly unreserving when reversing
« Reply #2 on: January 25, 2020, 07:45:23 AM »
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.

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 20274
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: [BUG]: Rail vehicles not properly unreserving when reversing
« Reply #3 on: January 25, 2020, 07:50:57 PM »
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?

Offline freddyhayward

  • Devotee
  • *
  • Posts: 437
  • Languages: EN
Re: [BUG]: Rail vehicles not properly unreserving when reversing
« Reply #4 on: January 25, 2020, 10:50:31 PM »
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.

Offline jamespetts

  • Simutrans-Extended project coordinator
  • Moderator
  • *
  • Posts: 20274
  • Cake baker
    • Bridgewater-Brunel
  • Languages: EN
Re: [BUG]: Rail vehicles not properly unreserving when reversing
« Reply #5 on: August 20, 2020, 03:13:49 PM »
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:

Quote
FIX: 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.