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

Feature: Order flag to unbunch vehicles at depot #11945

Merged
merged 4 commits into from Feb 3, 2024

Conversation

2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Feb 1, 2024

Motivation / Problem

Background

"Bus bunching" is a real-world phenomena that we experience in OpenTTD, across all vehicle types. (It also exists in Cities: Skylines! 😄 )

A feature to unbunch these vehicles (typically called "automatic separation") is a feature that has been requested for years. It exists in a simple form in our current timetable system, but has a finicky setup, requires manual updating when new vehicles are purchased, and like timetables in general, cannot be easily updated when the vehicles on the route are upgraded to faster models. JGR's Patchpack implements automatic adaptation of timetable-based separation. I've used it and have never had problems, but its controls in the timetable window aren't the most intuitive to players.

Deadlock danger

Fundamentally, unbunching requires delaying vehicles in order to keep a consistent interval between trips. You can't make a late-running vehicle faster, but you can delay the next vehicle. This introduces the fundamental danger of this feature: deadlocks. Vehicles get out of order, they block each other waiting to be on time, and the player has no idea why.

We've had an automatic separation PR open since 2020, but Andy managed to find a deadlock situation. This PR has some complex code to try to detect deadlocks, but it's hard to evaluate and test. And you know what they say:

Make it idiot-proof, and someone will make a better idiot.

A new approach

There is one place where vehicles cannot deadlock: Depots.

With a goal of building the simplest possible implementation, I started with #8342 and eventually replaced most of it moving the unbunching point to the depot.

Description

How do players use it?

Depots gain a new action type marking them as the unbunching point for a vehicle, chosen from the depot action dropdown (which now shows the selection action instead of the Service toggle button).

unbunch

A vehicle can only have one order marked for unbunching, so if you try and add a second you get an error. In addition, unbunching cannot be used with full load or conditional orders. Trying to add one of these orders, or trying to mark a depot for unbunching when the order list already includes one of these orders, blocks the new change and gives a descriptive error.

How does it work?

When a vehicle enters a depot, it records the duration of its last trip, in ticks. When it tries to exit a depot, it checks if its next departure time has arrived yet. If we are past the departure time, it leaves and immediately calculates the departure time of the next vehicle.

This calculation is simple: Take the average last trip duration of all vehicles, then divide by the number of vehicles to get the interval between trips, then look that many ticks ahead. The averaging evens out inconsistencies in trip times due to traffic, (train) routing, etc., and also handles new or changed vehicles that don't have a recorded trip time. Inconsistencies take a few cycles to stabilize, but the system should be quite resilient.

The next departure time calculated by the exiting vehicle is given to every vehicle in the shared order group. We don't know which one is supposed to leave next, but we don't care: whichever is processed first attempting to leave the depot is the lucky bus, and it will set a new departure time for all the others to wait for.

What about timetables?

When a vehicle leaves a depot after being unbunched, we reset its late counter, making it "on time." The only oddity of overriding the timetable is that the expected arrival/departure times of orders after the depot may not be correct if the vehicle is held in the depot for unbunching.

Review notes

I know we're right up against 14.0 feature freeze, but I'd quite like this to make it in. From my own experience and conversations with other players, the lack of this feature in vanilla OpenTTD is one of the main motivators for people to switch to JGRPP. Let's win those sales back and earn ourselves a nice bonus on all the additional revenue. 😛

I split this into several commits for ease of review. I will squash when merging.

Please check out the preview and see if you can break this.

I wonder if @JGRennison and @twpol, the two people most familiar with automatic separation, would be willing to take a look for anything I missed or got wrong. ❤️

Closes #8342.

Limitations

  • If a vehicle is extremely delayed, the next trips of its shared order groups will be delayed to match this average trip duration. It eventually settles down, but if two vehicles are delayed for an hour, the next two departures will have an interval of 30 minutes. This is a bit of an extreme edge case, but worth noting for anyone who tries testing this. 😉

  • A vehicle loaded with cargo which spends time waiting in a depot for unbunching will lose some cargo value. Players who are bothered by this should unload the vehicle first.

  • NewGRF aircraft which use the range limit feature (Av8 shown below) display this error after the (wait for unbunching) instruction. There's an extra space between the airport name and the error because of how STR_ORDER_TEXT works. I could fix it with a duplicate string, but I'm really not sure it's worth the effort based on the rarity of this error.

range

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

@2TallTyler 2TallTyler added preview This PR is receiving preview builds candidate: needs considering This Pull Request needs more opinions labels Feb 1, 2024
@2TallTyler 2TallTyler added this to the 14.0 milestone Feb 1, 2024
@reldred
Copy link

reldred commented Feb 1, 2024

Love it! The idea of using an order destination (depots currently) for the auto separation is an interesting work around but I have some thoughts about this:

  • This would synergize well with Feature: Extended depots #8480 or Feature: Multi-tile depots #9577 for allowing this auto separation point to be 'roll on, roll off' in terms of network design.
  • In lieu of this, could we use a dedicated station as well for the auto separation order? We already have lots of newgrf's for constructing aesthetically pleasing yards. This would of course put the onus on the player to build an appropriately sized yard for buffering/auto separating vehicles.

Copy link
Member

@TrueBrain TrueBrain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best PR description I ever read. Well written, clearly worded. Impressive <3

src/saveload/saveload.h Outdated Show resolved Hide resolved
@zephyris
Copy link
Contributor

zephyris commented Feb 1, 2024

To summarise my Discord comments... I think "wait to unbunch" is much better than "auto-separate" as the order label, and "unbunching" is more accessible than "auto-separation".

First, anything on the order should say "wait to...". That way, the user knows that the effect of this order may be to wait in the depot.

Second, "unbunch" is a much more commonly used term than "auto-separate". Unbunch is used by the news media, by maths articles about the phenomenon, by other games (eg. Cities Skylines), and a query on TT-Forums (https://www.tt-forums.net/viewtopic.php?t=85121). As far as I can tell, auto-separate is never really used, except for in a OpenTTD context.

Auto-separate is already used by JGRPP. But, using the same name probably isn't a good idea because: First, it's a different implementation, so giving it the same name is confusing. Second, use of a jargon term is used in a patch pack doesn't mean that it is the most accessible term to a new user of the feature in vanilla - you want the best name for a feature that they're seeing for the first time.

@PeterN
Copy link
Member

PeterN commented Feb 1, 2024

From the description it sounds like this still works even without timetables being set up? If so that's a major win.

@zephyris

This comment was marked as resolved.

@zephyris

This comment was marked as resolved.

@zephyris

This comment was marked as resolved.

@zephyris

This comment was marked as resolved.

@2TallTyler 2TallTyler changed the title Feature: Automatically separate vehicles with shared orders Feature: Order flag to unbunch vehicles at depot Feb 1, 2024
@2TallTyler
Copy link
Member Author

2TallTyler commented Feb 1, 2024

Thanks to testers and reviewers. I've made quite a few changes.

First off, thanks to @zephyris for the testing and wording suggestions. I've adopted your "unbunching" language. We can point to Cities: Skylines to justify the wording choice. 😉 Let me know if there are any more language tweaks, I appreciate the attention to detail. 😃

@reldred and others on Discord suggested using stations as the unbunching point. This can be a later PR, potentially. It will be problematic with road vehicles since they so easily block each other, and I'm hesitant to introduce an inconsistency of allowing it for some vehicle types and not others. Indeed, #8480 would solve this inconsistency. 😉

That said, while I play on a few JGRPP servers with Scheduled Dispatch, using 24-hour timetables and Peaks & Troughs, and am as much of a scheduling nerd as anyone who likes to see a yard full of trains awaiting service... I don't see why this can't coexist with depot unbunching. Just send the vehicle to the depot first for unbunching, then give it a timetabled wait at the yard. In the US, many passenger trains go to a Service & Inspection building when they come out of service for cleaning, inspection, and washing, before heading to the yard.

@PeterN Yes, this works without touching timetables. It also works if you set up timetables, and the timetable still works too! 😄

As for the bugs:

  • Unbunching does not work if the depot is the last order: Fixed, was an issue with how the previous order was calculated. It now wraps around properly. This still needs some testing with implicit orders, though...
  • Trains not leaving depots properly: Fixed, was an issue with trains being allowed to leave the depot, setting the next departure time, and then being blocked by something else like an inability to reserve a path. That vehicle then had to wait for the departure time it just set for the next vehicle. I fixed this by splitting the "leave depot" actions in two: one for checking if it's allowed to leave yet, and another for when all other checks have passed, right before it honks and starts moving.

I also added more unbunching data invalidations to just about every player action, including modifying timetable data. And enforcement errors to ensure that the unbunching depot is a service order, not a "service if needed" order that is often skipped.

@zephyris
Copy link
Contributor

zephyris commented Feb 1, 2024

Looks good. Bugs confirmed as fixed. I've been trying to break it again, haven't managed to yet.

Sharing/unsharing orders, adding orders, demolishing stations, demolishing depots, etc. didn't cause problems, presumably just triggers unbunching wait time recalculation. I couldn't find any issue with implicit orders, looks like it works well.

I've followed three main strategies to break things:

  • Artificially cause a very large delay of one vehicle in a group, leading to an long wait for unbunching. Lost vehicles, stopped vehicles, vehicles where you constantly click turn around and similar seem to be correctly ignored for unbunching. The remaining cases I could contrive were trapping trains in a signal deadlock, then releasing them, or forcing one vehicle in a group around a circuitous route. I think these are OK - there's no deadlock in the unbunching, just an erroneously long wait.
  • Force a visit to a depot (only possible for trains), which might confuse things. Tricking trains into either the unbunching or other depots doesn't seem to cause issues, tricked using cunningly timed manual vehicle reversing or by designing the track to force a depot visit. There is one case where the unbunching wait time calculation might be wrong, where you contrive a track which forces a train to visit the unbunching depot two or more times over the course of a timetable - but there's no significant problem, there might just be some incorrect spacing.
  • Massively overload things with lots of vehicles and long loading times. Overall seems absolutely fine. For trains, road vehicles and ships you generally get a buildup of vehicles in the depot rather than on the track/road/water - however the number of vehicles on the move is still very large and seems to be limited by the throughput of the stations and infrastructure of the route. In my hands it reduced train signal deadlocks and city-wide traffic jams, which is great. For aeroplanes, it's still easy to get a huge cloud of planes trying and failing to get a landing slot. That's because it's easier for a plane to exit a hangar than land at an overloaded airport, but that's a separate problem.

Well done for getting to the end of a wall of text. Take-home message: I like it, I'd love to use it. Couldn't break it. The only minor issues (arguably non-issues) were very contrived scenarios.

@M3Henry
Copy link
Contributor

M3Henry commented Feb 2, 2024

It seems you can't have an Unload all order active when enabling unbunching. But it can be enabled afterwards.

@zephyris
Copy link
Contributor

zephyris commented Feb 2, 2024

Further testing of my forced depot visit scenario confirms that it works as expected - my previous suspicion of a problem was wrong.
image

@2TallTyler
Copy link
Member Author

It seems you can't have an Unload all order active when enabling unbunching. But it can be enabled afterwards.

Thanks, Unload all actions are supposed to be allowed but were being detected as something else. It is now fixed. 🙂

@M3Henry
Copy link
Contributor

M3Henry commented Feb 2, 2024

I note that this uses up a button slot. But I'm not convinced that it should:

  • Go to depot (wait to unbunch) Is the intended usage
  • Service at depot (wait to unbunch) Is not allowed
  • Go to depot (stop) (wait to unbunch) Is allowed but doesn't make sense.

IMO this button is not orthogonal to the depot order type dropdown. Perhaps it malkes more sense to add Go and unbunch to the dropdown instead of the Unbunch toggle button.

@2TallTyler
Copy link
Member Author

Yes, good point. I've made the change and updated the screenshot in the description.

@zephyris
Copy link
Contributor

zephyris commented Feb 2, 2024

I agree it can make sense to have the unbunch with the service etc., but the implementation is not intuitive! I genuinely got quite confused.

It might be more than unintuitive, it might be buggy too eg. Why does selecting "unbunching" in this state give an error?
image

Some comments:

  • The non-stop toggle/dropdown makes sense - it has two states in the dropdown (go and go non-stop) which correspond to the toggle state (not indented = go, indented = go non-stop).
  • The service toggle/dropdown probably made sense once. I'm guessing it once had two states (go and go if service needed) which correspond to the toggle stage (not indented = go always(?), indented = go if service needed(?)).
  • The extension of the service toggle/dropdown has lost the intuitive meaning of the toggle state. Not indented has been extended to mean two, now three different things.

Punchline is that there are four depot functions: Service, service if needed, stop, and wait to unbunch (with a service as a side-effect). These are not all to do with servicing, so service isn't a good label for the button. This is not a binary state, so a toggle button isn't intuitive.

I realise this is negative and not constructive! I'll have a think about specific suggestions on the bus home. Ironically, it'll probably be bunched up because it's rush hour.

edit. Maybe kill the toggle/indent function and change to a click to cycle through options? Or just change click to open the dropdown? Not sure...

@TrueBrain TrueBrain removed the candidate: needs considering This Pull Request needs more opinions label Feb 2, 2024
src/order_gui.cpp Fixed Show fixed Hide fixed
@2TallTyler
Copy link
Member Author

Thanks for the continued help. You no longer get an error if you select Unbunch on an order that already has that action.

I've also reworked the interface slightly, as you suggest. The Service button now shows the selected action, and has no toggle effect -- clicking on it always opens the dropdown. I've updated the screenshot in the PR description.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview This PR is receiving preview builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants