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: Prevent vehicles to unload cargo in implicit stops in cargodist mode if the cargo is not accepted in such station #8552
Conversation
a92b9b9
to
0f00b45
Compare
if the cargo is not accepted in such station
0f00b45
to
8c1ebc3
Compare
if (current_order_is_implicit && !HasBit(GetAcceptanceMask(next_station), v->cargo_type)) { | ||
/* Prevent node links to be created for implicit stops | ||
* whose stations do not accept the provided cargo. */ | ||
break; |
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.
break
or continue
? (what if there is a wagon with cargo that is accepted by the station?). I don't know much about cargodist, so this really is a question; I can be completely wrong!
@@ -3794,6 +3794,8 @@ void IncreaseStats(Station *st, CargoID cargo, StationID next_station_id, uint c | |||
*/ | |||
void IncreaseStats(Station *st, const Vehicle *front, StationID next_station_id) | |||
{ | |||
bool current_order_is_implicit = front->current_order.IsType(OT_IMPLICIT); |
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.
Testing for "implicit" orders looks wrong to me. Doesn't the same happen for explicit orders?
I would expect testing for "not forced unload/transfer".
* whose stations do not accept the provided cargo. */ | ||
break; | ||
} | ||
|
||
IncreaseStats(st, v->cargo_type, next_station_id, v->refit_cap, |
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.
As I read IncreaseStats, it records the capacity between two stations.
I think you want to change the "cargo demand at a station" in the flow planner.
This seems to be done in UpdateStationAcceptance(), but that already tests for station acceptance.
So, I do not understand how this issue of unloading stuff at C manifests, and how this fixes it.
It seems to me that the description of recreating the situation is incomplete. I have never encountered a situation where a vehicle unloading at a station that doesn't accept the cargo without explicit order. Most likely, this may only apply to a station where different lines carrying the same load intersect. I would love to see a save with such a situation. I don't know if this change is really necessary. Certainly, preventing the unloading with disabled cargodist would be a very bad "fix". If the change also affected the regular mode, I would be strongly against. There are times when these orders are really helpful. Among other things, they enable unloading at any station after the designated waypoint(helpful when the limit of the station size is too small). They can also serve as a stop sign before connecting routes. In any case, I believe the basic solution would be to consider non-stop orders as default, as suggested in the topic of changing defaults. I think the absolute majority of players use this type of orders only because they are default and don't understand the difference in operation between non-stop orders. |
I think the analysis of the problem that serves as a foundation for this PR is fundamentally wrong. Let's stick with the A-->C-->B example. From the view of cargodist, this becomes 2 lines: A-->C and C-->B, and at this graph level, the information that is a continuing train gets completely ignored. So, because cargodist is not a simulation of each piece of cargo having a destination, but a statistical model, at C we now get the situation that some fraction of the cargo gets off the train, trying to get the next train that goes C-->B. At no point here does it matter whether C is an implicit stop, so you're just messing with an arbitrary symptom, with the potential to break some legitimate uses of such a setup. |
If A -> C -> B is the entirety of the graph, and C does not accept the cargo, no cargo should ever be unloaded at C, and the C node is harmless anyway. If you have A -> C -> B and D -> C -> E, then cargo may be unloaded at C for transfer/routing purposes. Changing the behaviour of IncreaseStats in this way is not a correct solution as it only results in an incorrect estimate of the capacity of the A -> C link. If you want A -> C -> B and D -> C -> E to be interpreted as A -> B and D -> E with no C node and therefore no unload/transfer, you will need a more involved solution.
Caveats:
|
HI! I believe you are right. First of all, thank you so very much for your detailed comment. I can only repro if I create a second train in the same line. I don't quite know why, but I will check if the code change prevents it from happening. I can confirm this change does not affect the regular mode (cargodist disabled). I will dive deep more into this and provide an update, thanks again! |
Amazing comment, thank you so very much. I will do more testing and understand if this is what indeed happens. I can confirm with 2 trains I can repro the issue. |
Motivation / Problem
When using cargodist, there can be situations where vehicles set to load cargo at A and unload at B can potentially go through a third station C that is not in the order list. If that's the case, the extra station C gets added as an implicit stop, which is a correct behavior. However, in cargodist that station will be added as a node and will make the vehicle unload its cargo even if that type of cargo is not accepted.
While some players use non-stop orders to explicitly prevent this from happening, other players can play simpler by setting just two stops, A to load and B as destination, without worrying about intermediate stops since they assume the cargo won't be unloaded if the cargo is not accepted in those. As an example, this happens if coal trains go through common train paths with passenger stations in towns/cities.
Repro steps
Description
This code change prevent vehicles to unload cargo in implicit stops in cargodist mode if the cargo is not accepted in such station.
Limitations
This code change is only solving the above mentioned situation and not meant to fix any other one. I am not aware of potential edge cases and I would appreciate suggestions. This topic got raised over the Discord chat.
Checklist for review
This code change has the potential of breaking players who have vehicles transferring cargo at implicit stops. While this is unlikely, it is possible that players have gotten used to this situation and proceeded with it. I still believe this code change clarifies the behavior of vehicles in cargodist given that we expect transfers to be explicit stops.