Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trains can crash through depots #8424

Open
BarryJRowe opened this issue Dec 24, 2020 · 14 comments
Open

Trains can crash through depots #8424

BarryJRowe opened this issue Dec 24, 2020 · 14 comments
Labels
bug Something isn't working

Comments

@BarryJRowe
Copy link

BarryJRowe commented Dec 24, 2020

Version of OpenTTD

1.10.3

Expected result

The train entering the depot should continue going into the depot.

Actual result

The train crashes into train on the other side of the depot.

Steps to reproduce

Make a straight line of track with depots at the end and two one-way signals facing either other in the middle. Create two trains going to the other depot. One train will wait at the signal while the other will go back and forth in the middle segment. Stop the train that's waiting for a path and delete the track in front of it. Place a depot on that square for the other train to go into. The second train will crash into the first train through the back wall of the depot.
openttd_bug

@James103
Copy link
Contributor

Do you have a save game that can reliably reproduce this?

@BarryJRowe
Copy link
Author

Depot train crashing.zip
Here's an example, unpause the game to get the trains to crash. I've noticed upon further testing that it only crashes when the train entering the depot is coming from the south east, or from the south west like in the example screenshot.

@UnsuspiciousGooball
Copy link
Contributor

I'm not sure if this is a rendering thing or if there's underlying game logic for it, but depots facing NW/NE don't seem to have rails in them:
grafik

Also, I put some debugging statements in the code that's supposedly responsible for making trains on depot tracks unable to crash (near the top of FindTrainCollideEnum() in train_cmd.cpp) and found that the conditional Train::From(v)->track == TRACK_BIT_DEPOT on its own doesn't ever seem to be true for a train pulling into a depot, at least with the bit of testing I've done.

@UnsuspiciousGooball
Copy link
Contributor

UnsuspiciousGooball commented Dec 28, 2020

After some further testing, it seems like it also depends on whether the train is a single or double loco. I've checked whether the game thinks that the train is on depot rails when pulling into a depot for both single and double engines and all 4 orientations:
grafik

Here's what I found, the directions are the way the exit of the depot is facing:

Single Loco (Ginzu A4):

  • NW: No
  • NE: No
  • SE: No
  • SW: Yes

Double Locos (SH'125', Front engine):

  • NW: No
  • NE: No
  • SE: Yes
  • SW: No

@glx22
Copy link
Contributor

glx22 commented Dec 28, 2020

@UnsuspiciousGooball the "missing" track is just a drawing issue. If you set to invisible instead of transparent, they'll be drawn.

@UnsuspiciousGooball
Copy link
Contributor

Checking (Train->track == TRACK_BIT_DEPOT) or even (Train->track & TRACK_BIT_DEPOT) does not seem to be a reliable indicator of whether a train part is actually on a depot rail because many other conditionals in the train code assume that only 1 bit of Train->track is ever set, and overwriting it with TRACK_BIT_DEPOT or setting TRACK_BIT_DEPOT independently would break them.

Perhaps we could instead replace (Train->track == TRACK_BIT_DEPOT) with IsRailDepot() on the train part's current tile in the collision detection code.

@Eddi-z
Copy link
Contributor

Eddi-z commented Dec 29, 2020

I'm pretty sure this "bug" has been in the game since TTO. also the "missing rail" drawing "bug".

@James103
Copy link
Contributor

How do you know? Did you have TTO, TTD, and/or TTDPatch and you tested them on all three?

@Eddi-z
Copy link
Contributor

Eddi-z commented Dec 29, 2020

Well, yes. I did have TTO. and when i encountered this "bug" some 20-ish years ago, i said to myself "well, don't build like this" and didn't think of it any further. (well, actually, back then you could use a variation of this to crash the trains of competitors by sticking a rail/depot at the end of their station and run a train into them. but i think we patched that out)

about the missing rail: since the depot sprite is shorter than a full tile, drawing the rail would show it sticking out the back of the depot, hence it is omitted.

@Eddi-z
Copy link
Contributor

Eddi-z commented Dec 29, 2020

well, i think the main issue is that a train approaching the end of a tile has a "crash box" that sticks out into the next tile. when two "crash boxes" overlap, there will be a crash. we should change the rail movement that on approaching end of track (including depot) the movement stops earlier, so the "crash box" never sticks out.

@Eddi-z
Copy link
Contributor

Eddi-z commented Dec 29, 2020

Checking (Train->track == TRACK_BIT_DEPOT) or even (Train->track & TRACK_BIT_DEPOT) does not seem to be a reliable indicator of whether a train part is actually on a depot rail

i'm pretty sure that's not what TRACK_BIT_DEPOT actually means. the depot tile itself has a regular track bit, and TRACK_BIT_DEPOT applies only to the part of the train that turned invisible because it has left the regular map

@UnsuspiciousGooball
Copy link
Contributor

UnsuspiciousGooball commented Dec 29, 2020

TRACK_BIT_DEPOT applies only to the part of the train that turned invisible because it has left the regular map

I checked and that seems to be true, that would explain why it can't and shouldn't be set independently of the other bits. So the purpose of that check in the collision code is probably just to dismiss train parts which are currently parked in depots, my bad.

Also, what's interesting is that FindTrainCollideEnum() isn't even called if there is a train right behind a depot facing NE/NW with another train pulling into the depot, probably hence why the collision only happens on depots facing SE/SW.

@UnsuspiciousGooball
Copy link
Contributor

well, i think the main issue is that a train approaching the end of a tile has a "crash box" that sticks out into the next tile. when two "crash boxes" overlap, there will be a crash. we should change the rail movement that on approaching end of track (including depot) the movement stops earlier, so the "crash box" never sticks out.

That's probably the best way to do it, because the collision logic itself does exactly what it's supposed to do in this scenario. In the provided savefile it determines the trains to be just 6 tile units apart right before the crash (the engines are 8 tile units long each), and if you turn on x-ray for the depot you can even briefly see that the trains are inside each other.
So we probably shouldn't change the collision logic but instead the train's movement on pulling into a depot/pulling up to the end of a track.

@PeterN
Copy link
Member

PeterN commented Nov 30, 2022

This is because it's possible to remove a rail tile if a vehicle is partially on that tile, as the length of the vehicle is not included when seeing if it's on the file.

Meanwhile collisions do check vehicle lengths. Boom.

TrevorShelton added a commit to TrevorShelton/OpenTTD that referenced this issue Apr 2, 2023
… correct incorrect crash circumstances.

Collision sensitivity was lowered in a way that both problematic circumstances
appear to be rectified while true collisions still seem to function fully. (An
idea of making the trains "park in" a little less also comes to mind, but this
patch seems sufficient to resolve the overall issue.)
TrevorShelton added a commit to TrevorShelton/OpenTTD that referenced this issue Apr 4, 2023
TrevorShelton added a commit to TrevorShelton/OpenTTD that referenced this issue Apr 4, 2023
…o Fix Wrongful Collision

This patch appears to fix the wrongful train crashing, and it changing two directions may
be a good sign because it was said the bug was happening in two directions. Counting steps
from 5 to 0 also match A to F in number. No process crashimg seems to be caused by this,
either. However, further testing would be wise.
TrevorShelton added a commit to TrevorShelton/OpenTTD that referenced this issue Apr 4, 2023
…o Fix Wrongful Collision

This patch appears to fix the wrongful train crashing, and it changing two directions may
be a good sign because it was said the bug was happening in two directions. Counting steps
from 5 to 0 also match A to F in number. No process crashing seems to be caused by this,
either. However, further testing would be wise.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants