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: Allow automatically separating vehicles in shared orders #8342
Conversation
2999e3b
to
df56e94
Compare
Commit messages must follow a specified format. Instead of "Update ...", you should either merge that commit into your previous commit or use a commit message that's something like "Fix: grammar". |
Yes, that's Github's fault because I simply clicked to apply the suggested fix and it wasn't in that form. I fixed it locally and pushed an updated version now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Code looks fine on brief inspection. Will require some actual testing though :)
Testing this, I got an assert, but I do not know if that is related to the current mac build, or specific to this PR :) I've seen the assert twice in short succession. I can't get it to trigger in current master using same savegame. Crash log attached; the savegame will be pretty useless, it contains multiple unreleased grfs. |
You'll probably want to locally cherry-pick c98717c for testing on Apple/LLVM. |
Would it be helpful for people testing if I rebased this PR onto that commit (or another newer commit)? |
Always :) |
139dbb5
to
4e97ebd
Compare
@andythenorth Hopefully the builds just generated will be more successful for you. 🙂 |
No assert. UI seems straightforward. Simple case works as expected for me in test cases with ships, trams, trains. No full load orders, no explicit timetable, no conditional orders. Works great. Cases that were expected to 'break' the separation and do:
Cases I thought would break, but appear to work:
Cases I didn't try that I would expect to break:
|
I'm glad things went well; your results are mostly what I'd expect. To expand a bit on the expectations from me:
However, I'm particularly intrigued by this:
Are you referring to setting the stay time at a station in the timetable? In my testing, this seems to behave itself. It makes the vehicles wait for the timetabled time instead of their loading time, but that time is all included in the automatic separation measurement of journey time, so the automatic separation waits at the first station take account of that. Where you might have problems, is if the wait time causes vehicles to queue up on the approach to the first order station. In this scenario, automatic separation believes itself to be responsible for that queue and reduces the wait times accordingly. |
I agree about disabling the automatic separation button if there are full load orders. Is it possible to have a tooltip explaining why the button is greyed out? |
Yes this is exactly the case. I set a wait at the first station in the order, and the station (roadstop) has capacity for 1 vehicle. I don't have the savegame, and I can't remember the exact outcome, but I could try and repro if it helps. However I had no expectation that case would be handled, and I don't think it needs to be. |
Would you mind rebasing your Pull Request? It would allow us to create a preview (new feature). Tnx :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not test it yet; will do after you rebase. Some minor coding style thingies, but overall it looks really good :D Happy to see such a clean implementation of such feature :) Awesome work, tnx!
this->first_order_last_departure = max(last_departure + separation, now); | ||
|
||
/* Debug logging can be quite spammy as it prints a line every time a vehicle departs the first manual order */ | ||
if (_debug_misc_level >= 4) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who is going to be the user of this debug statement? I ask, as I wonder if it is useful to keep in now you did your initial work? Honest question, more of thinking out loud :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good question. 🙂
Honestly, I'd like to keep the code, both for future development and curious/bug reporting users, but debug misc=4 might well be the wrong door to put it behind.
A higher debug misc level, or a compile-time constant (if OpenTTD uses those for debugging code)?
c47f488
to
aac99f5
Compare
Thanks! Rebased onto latest code and fixed everything I can, further suggestions welcome. |
I saw this deadlock, with all separated vehicles waiting in stations. Using 'stop all vehicles' and 'start all vehicles' (for each vehicle type) cleared it. Happened after loading a savegame, but that might be coincidence. I can't repro by reloading the save. The savegame also uses unreleased grfs and grfs have been reloaded many times, so it's not helpful for debugging. Probably not much you can do with this report, but if I see it again, I'll try and get a repro. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Done. |
Unless I am mistaken, the remaining issues (excluding any bugs/issues spotted in the code itself) are: When should automatic separation be unavailable?
Integration with timetablesA timetable with all numbers filled in except the wait time at the first manual load/unload order is expected to function correctly with automatic separation as it is currently. If the timetable is completely filled in, it will stop working correctly. Should we disable automatic separation or try adjusting the timetable departure time/lateness to do separation? Queuing detection and behaviourThe algorithm for calculating queuing vehicles is certainly not completely accurate, but so far it is the best heuristic we have. We need to adjust for queuing road vehicles (playtesting shows this). But what about other vehicle types? |
Looks like if I update the |
That timetable implementation should work, yes. It is mostly unnecessary to use timetables and auto-separation in this way, but I see no reason to restrict users from using both at once. I could certainly see some "realism" gameplay that uses both and using both is somewhat error-proof since auto-separation just keeps timetabled vehicles spread out. Edit: You may be able to use lateness_counter to space out vehicles leaving the first station, instead of adding a second way of delaying vehicles. This could be cleaner if you're updating lateness_counter anyway. |
Hello, I hope you had a good holiday. Just as an update, we are looking at doing a 14.0-beta1 release as early as next week so we can start testing a few big PRs. That doesn't preclude this PR making it into 14.0-beta2, but I wanted to explain this all to you so you don't see beta1 and get discouraged. 🙂 In response to your comment above regarding outstanding issues, I think you are correct.
I don't know your availability over the next couple weeks, but I'm around and am happy to do my end of the work. I am very excited for this PR and would really like to see it in 14.0 if you have the time. 😃 |
Hi @twpol, I hadn't heard from you and was thinking about how I might help finish this when I had a spark of inspiration: Most of the attempts at autoseparation run into issues with gridlock at the point where vehicles wait. In your case, it's the first order and you have some logic that tries to break up logjams, but this is hard to test. So I thought, what if we did the autoseparation delays inside the infinite-capacity-wormhole of a depot? Instead of a toggle on the vehicle orders for autoseparation (we're rapidly running out of space for a button), add that behavior to the depot itself similar to how you select service and refit behaviors, where we have plenty of button space. I've started working on this approach, and although it's in rough state and doesn't work properly, I like the approach enough to continue working on it. Feel free to let me know your thoughts: https://github.com/2TallTyler/OpenTTD/tree/depot-autoseparation |
Oh, urgh, I did not see any of these recent messages. Not a fan of the depot approach, at all, but it seems I'm too late to avoid that being the chosen solution here? I really wanted a single button (which could be auto-on with a user setting) to do this. 😭 |
Yes, sorry. We released OpenTTD 14.0-beta1 yesterday, with the depot approach. |
Does that mean there is no way forward for this PR/approach now? |
No way that I can see, no. Sorry! |
Okay, that's devastating. We were so close to achieving a great outcome and now it looks like OpenTTD is stuck with an option that doesn't satisfy my desires, needs an extra hoop-jump for every route, and I think even needs more vehicles for the same throughput (at least for trains) due to the extra entry/exit time of the depot. It should be clear why I won't be attempting any further contributions to OpenTTD. Goodbye. |
This is my proposed code for allowing vehicles (of all types) to be automatically separated amongst their shared orders with the click of a single button (once for each shared order), to massively reduce the micromanagement needed for optimal passenger journeys.
I filed issue #8095 initially to check that the feature was acceptable, and it was suggested I go forward with a PR.
As this is my first-time contributing code to OpenTTD, even though I have read the contributing guidelines and linked materials, I will also probably have missed/misunderstood something so please let me know where and how I might have messed up.
As part of that, I have tried to document the algorithm itself and surrounding code with comments but if anything isn't clear, or the details would be better served elsewhere, let me know.
Outstanding issues - #8342 (comment)