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: Prevent vehicles to unload cargo in implicit stops in cargodist mode if the cargo is not accepted in such station #8552

Closed
wants to merge 1 commit into from

Conversation

perezdidac
Copy link
Contributor

@perezdidac perezdidac commented Jan 10, 2021

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

  • Start a game with cargodist enabled.
  • Create a train line with 3 stations A--->C--->B making sure A is placed in a location where it can load any cargo.
  • Create a depot before A so the train starts at A towards B through C.
  • Create 2 train to load that cargo.
  • Set 2 orders: load at A and stop/unload at B.
  • Start the trains and observe loading at A.
  • Observe second train unloads cargo at C rather than keeping the cargo to be unloaded at B.

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.

@perezdidac perezdidac force-pushed the cargodist-acceptance branch 2 times, most recently from a92b9b9 to 0f00b45 Compare January 10, 2021 07:09
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;
Copy link
Member

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!

@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions needs review size: small This Pull Request is small, and should be relative easy to process labels Jan 10, 2021
@@ -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);
Copy link
Member

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,
Copy link
Member

@frosch123 frosch123 Jan 10, 2021

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.

@LC-Zorg
Copy link

LC-Zorg commented Jan 11, 2021

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.

@Eddi-z
Copy link
Contributor

Eddi-z commented Jan 11, 2021

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.

@JGRennison
Copy link
Contributor

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.
This does not actually stop cargo being unloaded at C for transfer, it may however result in cargo being routing a completely different way and bypassing A -> C -> B entirely.

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.
At minimum this would require:

  • Changing the management of Vehicle::last_loading_station such that at C the value is left as A
  • Changing OrderList::GetNextStoppingStation
  • Changing LinkRefresher
  • Disabling the call to IncreaseStats in Vehicle::BeginLoading at C

Caveats:
This idea won't work if:

  • A train has a mix of cargoes such that some are accepted at intermediary implicit order stations and some are not.
  • The acceptance of intermediary implicit order stations changes as this will temporarily break routing until after the next graph update.

@perezdidac
Copy link
Contributor Author

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.

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!

@perezdidac
Copy link
Contributor Author

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.
This does not actually stop cargo being unloaded at C for transfer, it may however result in cargo being routing a completely different way and bypassing A -> C -> B entirely.

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.
At minimum this would require:

  • Changing the management of Vehicle::last_loading_station such that at C the value is left as A
  • Changing OrderList::GetNextStoppingStation
  • Changing LinkRefresher
  • Disabling the call to IncreaseStats in Vehicle::BeginLoading at C

Caveats:
This idea won't work if:

  • A train has a mix of cargoes such that some are accepted at intermediary implicit order stations and some are not.
  • The acceptance of intermediary implicit order stations changes as this will temporarily break routing until after the next graph update.

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.

@perezdidac perezdidac closed this Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions needs review size: small This Pull Request is small, and should be relative easy to process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants