[patch] Timetabling waiting time in depots

[keoz]

This patch gives the possibility of setting waiting times in depots, exactly as we can in station.

Screenshot-Wait_in_depot.png (106.87 KiB) Viewed 11965 times

In my view, this is particularly useful - even necessary - for some cases of timetabling. It is specially useful for timetabled trams. Even when they are properly timetabled, trams are often overtaken by those who come behind and after that, it's quite tedious/hard to reorder properly all the vehicles. Waiting some time in depots allows the vehicles to reorder properly. Setting waiting times in depots and not (or less) in stations, also allows vehicles to leave platforms sooner and hence, have some more fluid traffic.

In the way I coded it, there is still one difference with the behaviour a vehicle has in stations: when you set a travel time to the depot, but not a wait time in the depot, the vehicle will behave as without the patch: it means, that - in case the vehicle arrives at the depot sooner as scheduled - it will not wait there, as it would in stations. That was chosen to maintain the original behaviour if needed.

That's my first try in patching, so I only hope everything works. Normally the patch file should be applicable without problems to latest trunk (now r26706). As a side note, so far there is not any savegame compatibility problem.

Any bugs/problems report will be welcome. Also, any coding advice will be very useful ! Probably there is a lot to make better.

[bokkie]

\o/

Have been wanting this for years It's much easier to timetable this way and keep good separation/order. Now I sometimes used very very big stations on the end of the timetable, even though they were often in small places.

EDIT: saw it was over 6 years ago http://www.tt-forums.net/viewtopic.php? ... 20#p691994 .

[Eddi]

it might be a good idea in general to treat 0 wait time as "don't wait here" instead of "this is unscheduled" also for stations

[keoz]

it might be a good idea in general to treat 0 wait time as "don't wait here" instead of "this is unscheduled" also for stations

Indeed. That would allow no waiting stations in scheduled timetables - original behaviour could still be maintained by giving some, even minimal, waiting time in stations - and, in the scope of this patch, unify the behaviour between depots and stations. I'll think about that.

Anyway. In so far, everything seems to work fine without problems.

[keoz]

Update to v2. Some code cleaning and corrections (depot "stop" orders can not anymore be timetabled).

[Zuu]

First, I have never worked with orders in the C++ code so this comment is just from just now looking at the code. Before I have only worked programmatically with orders from AI side which is a bit simplified sometimes when not all complexity needs to be exposed.

I wonder though, why adding a new OT_WAITING? As I understand it, stations don't have a separate OT_* enum value for when to wait at the station. So why do this for depots? Is it not enough that the depot order has the depot flag set to ODATF_SERVICE_ONLY which I assume correspond to "go always to depot (but not stop there)", and set order->wait_time to a value > 0 for you as condition on when to wait?

From my quick look, dropping OT_WAITING would be more consistent with how it works for stations, but if I'm wrong on this (or that it will not work without it), please enlighten me.


You can find the OpenTTD coding style documented here: https://wiki.openttd.org/Coding_style . You have spaces as indentation and deviate from how multi-line comments should be made. Also if-statements with code on the next line should use curly brackets. This is all explained in the wiki. Your code is already readable as such, but before going into trunk it need to follow the coding style. Though this is less important than how you actually solved things in your patch.

[keoz]

Thank you for your remarks, Zuu.

The answers I can give so far:

I wonder though, why adding a new OT_WAITING?

I knew that this was going to be, eventually, the most discussed point. It's a quite a change. I'll try to explain my reasons for this choice.

As I understand it, stations don't have a separate OT_* enum value for when to wait at the station. So why do this for depots?

Actually, they do have specific orders for stations. When a train stops in station, it triggers some order-type changing. Specifically: when arriving, order type becomes OT_LOADING and after that, when the train is going to leave, it becomes OT_LEAVESTATION. So, internally, what looks as a single goto station order actually changes in at least 3 order-types during its processing (all this orders are listed in order_base.h, l. 37).

A lot of my choices were mainly guided exactly by what you mention as important: trying to make the in-depot behaviour as consistent as possible with what happens in stations. Some other choices also results from that. The HandleWaiting() function, i.e. (in vehicle.cpp), is largely inspired from the HandleLoading() function.

Is it not enough that the depot order has the depot flag set to ODATF_SERVICE_ONLY which I assume correspond to "go always to depot (but not stop there)", and set order->wait_time to a value > 0 for you as condition on when to wait?

Well. Actually, when I decided to introduce the OT_WAITING order, I had the feeling that this was necessary to make things properly. Now that I worked a lot more on the code, and that I better understand it, I think it is probably possible to make the thing without it. I'm trying to do that right at the moment and I'll be able to answer you more consistently.

But I also already see that avoiding this order seems to bring to some inconsistencies. A lot of tests can be better achieved if there is actually a clear way to check whether the train is waiting or not (example: when processing orders; when recalculating the arrival/departure board; also in the skip orders function). My thought also was that a such kind of order could be interesting to be used in a lot of cases other than depots (waiting at Waypoints ?), thus useful for future development.

You can find the OpenTTD coding style documented here: https://wiki.openttd.org/Coding_style . You have spaces as indentation and deviate from how multi-line comments should be made. Also if-statements with code on the next line should use curly brackets. This is all explained in the wiki. Your code is already readable as such, but before going into trunk it need to follow the coding style. Though this is less important than how you actually solved things in your patch.

Yes, I tried to comply the best but obviously, I read that part a little bit to fast. Will fix that.

[Eddi]

waiting at a waypoint won't work properly, because the train doesn't know whether it needs to stop when approaching the waypoint. so either you always slow down at a waypoint (bad), or you come to a full stop immediately (bad).

[keoz]

waiting at a waypoint won't work properly, because the train doesn't know whether it needs to stop when approaching the waypoint. so either you always slow down at a waypoint (bad), or you come to a full stop immediately (bad).

Well, supposing that someone defines a waiting time at a waypoint, the train would know that it's gonna to stop and wait (just as in stations). And anyway, it also does not know whether it'll have to stop when approaching a signal, but it still stops if needed.

BTW, that's not exacly my point. By no way I am now proposing to further have Waypoint's wait time. I just meant that it's not impossible that this kind of order may be useful for other further/corner cases. I may be wrong, of course.

[Eddi]

Well, supposing that someone defines a waiting time at a waypoint, the train would know that it's gonna to stop and wait (just as in stations).

well, that can easily be achieved currently by placing a station and using it with a "go via" order (without stopping) or a "no loading and no unloading" order (with stopping and timetabling)

And anyway, it also does not know whether it'll have to stop when approaching a signal, but it still stops if needed.

well, that is some giant hack that is supposed to be removed at some point in the future, so it makes for a very bad argument. also, the case is different, because you know that you need to stop at a signal at least one tile in advance, but you don't really know whether the wait time on the waypoint will be over until you are on the exact stopping location. or you need to figure out a formula that easily predicts the amount of time it will take, including current speed and acceleration, to travel X steps. (1 tile = 16 steps).

[keoz]

Zuu, I've been exploring some time about your suggestion (getting rid of the introduced OT_WAITING order type).

The problem is that if I remove it, I can't find a proper way to check whether the train is - or is not - waiting in a depot at a given time.

Some other possibilities have been explored to check that "waiting in depot" state:
- the function "bool IsInDepot()": but this one returns true even if a vehicle is only partially - and still not entirely - entered in depot and still after that, when it is leaving the depot, but still not entirely out.
- the function "bool IsChainInDepot()": this one returns true only when the vehicle completely arrived in depot, but keeps returning true when it's going out from the depot and still not completely out.
- the state ODATF_SERVICE_ONLY (which you mentioned over): the problem with this state flag, is that it is true during the entire processing of the OT_GOTO_DEPOT order, which means it is true already when the train is travelling TO the depot. Hence, it is unreliable to determine whether the train already arrived in the depot and is waiting there.

Now, the point is that at some moments, I need to have an accurate way to make that check. An example:

Code: Select all

bool travelling = (!v->current_order.IsType(OT_LOADING) || v->current_order.GetNonStopType() == ONSF_STOP_EVERYWHERE);

This variable definition is found in the function used to build the arrival/departure board. The variable "travelling" is necessary to properly make the calculations. Note that here, this check is performed using the OT_LOADING state for vehicles waiting in stations. But I also need this variable to be true when a vehicle is waiting in depot. Here, the OT_WAITING order lets make that check :

Code: Select all

bool travelling = (!(v->current_order.IsType(OT_LOADING) || v->current_order.IsType(OT_WAITING)) ||
v->current_order.GetNonStopType() == ONSF_STOP_EVERYWHERE);

Which is clear and neat, IMO. In other places too, I need that check to be made properly. In the vehicle controllers, (example RoadVehController(), in roadveh_cmd.cpp), when calling the waiting handling function (HandleWaiting()), I need to perfectly know whether the vehicle is waiting or not.

So far, all of this let's me think that trying to remove the OT_WAITING order is a dead end.

[Alberth]

The other direction is concluding that IsChainInDepot is broken, and must be fixed.
The Doxygen documentation excludes going out of the depot for returning true.

Either the code or the documentation is wrong thus at least.

[keoz]

The other direction is concluding that IsChainInDepot is broken, and must be fixed.
The Doxygen documentation excludes going out of the depot for returning true.

Either the code or the documentation is wrong thus at least.

I'll have better look on that, then. I'm maybee wrong on this point and badly interpreted the tests I did.

[Alberth]

I'll have better look on that, then. I'm maybee wrong on this point and badly interpreted the tests I did.

To be clear on this, I was not saying your tests were wrong.

The name "IsChainInDepot" suggests to me all vehicles of a train should be in the depot for the function to return true. This is also what the documentation of the function says. If you have a test that shows a train can partially drive out of the depot while the function still returns true, something is wrong.

I don't know train code well enough to understand the function, so I cannot quickly check what the function is actually doing. It seems to be used at least in autoreplace, where having fully entered the depot is important. I don't know what the other uses do exactly, but it may well be that currently only the "having fully entered" case matters, and the "not started leaving" case is never relevant currently. (You should check that probably.)

In that situation, nobody checks what the function does in the "not started leaving" case, and nobody would find a contradiction between the documentation and the actual behavior.

[Eddi]

it may very well be that the "chain is leaving the depot" case is a "don't care", as the front vehicle won't be in the depot during that phase anyway. a vehicle is unlisted from the depot view as soon as the front vehicle leaves.

[ic111]

Thank you very much for this patch, I started integrating it into my own patch, as I never got that particular feature running before.

If anyone has some clue into which problem I might be running here http://www.tt-forums.net/viewtopic.php? ... 5#p1156165 your hints would be highly welcome.