Openrct2: Ride test crash prevention doesn't prevent crashes if ride status is closed ("single closed") at the moment the crash actually occurs

[jgottula]

OS: Windows 8.1
Version: 0.1.2
Commit/Build: 8fdec42 (x86-64 build)

Happens in vanilla RCT2: Not applicable.
Happens in multiplayer: Yep, both singleplayer and multiplayer.

Explanation

So, OpenRCT2 has this handy-dandy no_test_crashes feature that prevents rides that are undergoing testing from being able to crash. (Well... to be precise, it prevents "fall off the track" style crashes, not "vehicle-on-vehicle collision" style crashes; as per #5688.)

Out of habit (and in part to prevent potential vehicle-on-vehicle collisions during testing), on my coasters I'll usually hit the yellow "test" button once, allow a single train to leave the station, and then hit the red "close" button once, such that the ride is "single-closed" (it won't send more trains out) but not "double-closed" (it's not completely reset).

I found that this consistently leads to the test crash prevention feature not working: trains will fly off the end of the track and crash if the layout permits it, without the game instantly resetting the ride at the last moment as expected.

Video Demonstration

Here's a quick video demo I put together.

For the first two test runs, I leave the ride in testing mode, and the crash prevention reset mechanism (effectively: "close; close; test") works fine.

For the next two runs, I switch the ride from testing to closed mode after the first train leaves the station. Consequently, the crash prevention feature doesn't kick in as the train flies off the track.

For the last two runs, I leave the ride in testing mode, just as in the first two runs.

Code Analysis

Upon looking through the source code, it became fairly obvious what's going on here: the test crash prevention code checks the ride status (open/testing/closed) at the moment of the potential crash, and only prevents crashes if the current ride status at the moment that the vehicle is crashing is RIDE_STATUS_TESTING.
https://github.com/OpenRCT2/OpenRCT2/blob/8fdec42ff9b8956f6c502b683dec08141ec20fc7/src/openrct2/ride/ride.c#L2044

So obviously that's a bit overly simplistic. But having dug around the source code some more, I can see that the ride status field really is fairly useless for this purpose, given the dynamic manner in which players can switch rides between open/testing/closed at will, with various vehicles in motion on the track at any given moment that could have been from either the "open" or "testing" modes of operation.

And I'm not sure there really is a robust way to determine whether a particular ride vehicle was originally sent out of the station for testing purposes specifically. (I mean, yeah, sure, there's VEHICLE_UPDATE_FLAG_TESTING; but that can get invalidated by all kinds of stuff via invalidate_test_results.)

---

I get the strong sense that this is gonna end up being one of those "we'd need our own extended save format to add new bits so we could reliably track whether each vehicle was sent out for testing / what the ride's status was back at the time the vehicle was sent out" sort of things; and quite likely a healthy dose of "this feature is just a kludge anyway, which we really want to replace with a proper 'ghost train' thing at some point anyway" as well. Which I understand.

But I figured this was worth reporting, (a) so that it's at least a documented known issue; (b) to maybe increase motivation toward implementing proper "ghost trains" for ride testing; and (c) in case there does happen to be a currently-feasible, non-horrendous way of addressing this issue, that perhaps it could be implemented. :wink:

[Gymnasiast]

This is a duplicate of #1610. But since yours is more detailed, I'm not sure whether I should close the older one, or this one.

[Ryder17z]

I'd close the old one for that reason, but that's just my thoughts.

[jgottula]

This is a duplicate of #1610.

Ah, damn. I did look back for previous issue reports on the same subject but didn't catch that one. (It's surprisingly tough to craft a good search query string involving the word "crash", since most of the results end up being reports about game crashes, not ride vehicle crashes. :confused:)

Having now read through #1610, it sounds to me like, at that time (~2.5-2.0 years ago), this was more-or-less characterized as a bug that would be able to be addressed in a relatively straightforward manner (presumably not requiring massive milestones like the new save format) once enough of the vehicle code was RE'd and implemented.

https://github.com/OpenRCT2/OpenRCT2/issues/1610#issuecomment-121298688: "This will be addressed when vehicle update has been implemented. Don't expect that until after 0.0.3."

I'd be curious to know if that's still an accurate characterization now (and whether I even got the right impression in the first place, for that matter). @duncanspumpkin? @IntelOrca?

I know that in #485, it was determined that robustly implementing the ghost train feature would require adding a new vehicle flag; and as such would require the new save format to persist properly across save/load; and so is therefore a feature that probably won't happen for a while. (0.3+, if the roadmap is correct.)

If it is feasible to merely determine whether a given train left the station for testing purposes rather than normal operational purposes (basically, whether the ride was in testing mode or open mode at the time that train left the station) via the existing ride+vehicle state/flags (something I'm still not completely sure is doable), then I'll be happy to open a PR implementing better crash-prevention logic that looks at the vehicle ("was it sent out for testing") rather than the ride ("is it currently testing"). That'd at least be an incremental improvement for the current feature, I would think.

[Gymnasiast]

Saving whether a train was sent out for testing _also_ requires a vehicle flag, just like having ghost trains.

Back in those days we expected that we would have moved away from SV6 by now (or even last year). Obviously, this has proven to be more difficult and more work than expected.

[jgottula]

Saving whether a train was sent out for testing also requires a vehicle flag, just like having ghost trains.

Okay. Had kinda been hoping that there might be a way to kludge it with existing state information; but I trust you guys to know the game's limitations a whole lot better than I do.

Back in those days we expected that we would have moved away from SV6 by now (or even last year). Obviously, this has proven to be more difficult and more work than expected.

I see.

Though that still doesn't really explain @duncanspumpkin's comment on #1610; nor why that issue is marked "bug" as opposed to "requires new save format" (or even "wontfix because by the time we have the prerequisites needed to fix this, we'll also have the things we need to just go ahead and do ghost trains anyway", for that matter).

[Gymnasiast]

Okay. Had kinda been hoping that there might be a way to kludge it with existing state information

Exactly: kludge into. But we have only done so in an extremely limited number of occasions, as it makes importing old saves harder (especially now RCTC has also started kludging stuff into SV6). Some stuff is possible to hack into SV6, but not always a good idea. In any case: yes, this feature will be replaced with a ghost train.

I can't really speak for Duncan, but sometimes we have different opinions about the best course of action. After all, this is a loosely-organised project.

[spacek531]

Are we not able to create data that is not saved in the save files? It seems to me that saving and loading a game while an incomplete ride is being tested is a serious edge case and I do not care if the property was not saved.

[duncanspumpkin]

Back then we did not split the issues into multiple tags. Also at the time i thought we might be able to achieve this without using any new variables. The vehicle update code is still a mess that needs attention and improvement to make this code better. In my opinion this is a bug in the feature hence why i marked it as bug. I think we do need a variable to indicate a train was sent out as a tester to do this feature properly.

[jgottula]

@duncanspumpkin Good clarification, thanks.

[jgottula]

I'm thinking I may start getting involved in the development of this project. I'm impressed by the huge list of things you guys have been able to improve over the vanilla game (even in spite of some roadblocks, like the save format transition, not yet being surpassed); the code quality seems quite good, from what I've seen looking through it; and, most significantly to me, the whole concept of reverse engineering and then reimplementing a game, on the scale involved here, is in itself really just fantastic.

I've done similar work myself (spending literally hundreds of hours in IDA, reverse engineering the entire bot AI system of another game from x86 assembly code into working C++ code; ~20-30k lines of C++ when all was said and done, depending on how you count it); and it's an incredibly interesting, difficult, fun, frustrating, and rewarding thing, all at the same time.

Of course I'm sure that RE'ing Chris Sawyer's semi-archaic hand-written assembly code, in this project, is probably quite a different experience and set of challenges compared to RE'ing optimized assembly code emitted by a modern C++ compiler, as I was doing; but I bet the feeling of putting all the puzzle pieces together is similarly enjoyable. :sunglasses:

[rwjuk]

@jgottula We will absolutely always welcome new contributors, but just so you're aware the RE work is now done, and we're now focusing on developing the project further (bug fixes, code quality, new features, etc, etc.)

[Gymnasiast]

Since this feature has been cut altogether with the introduction of ghost train simulation, I'll close this issue.