Correctly handle wares carried into harbors with no path to goal #1852
Conversation
|
it seems as if I drove the logistics into an interesting corner case? :-) |
| && carried_ware->GetGoal() != wh) | ||
| // Put ware down at flag if enough space. | ||
| // Might need to take it back in if goal was destroyed or changed to the warehouse | ||
| if(flag->HasSpaceForWare() && carried_ware->GetGoal() && carried_ware->GetGoal() != wh) |
There was a problem hiding this comment.
can there be no flag? (e.g flag being null?)
we should at least assert it
There was a problem hiding this comment.
If there was no flag, there is no warehouse and the worked would have been notified to abort the work. So yes there is always a flag
| if(flag->HasSpaceForWare() && carried_ware->GetGoal() && carried_ware->GetGoal() != wh) | ||
| { | ||
| carried_ware->WaitAtFlag(world->GetSpecObj<noFlag>(pos)); | ||
| // TODO: Remove assert. Was added to verify prior condition |
There was a problem hiding this comment.
Keep it for a short while, then remove. Possible to remove now as the auto-play tests succeeded but might be worth collecting a bit more evidence although I'm very sure this is correct.
| static_cast<nobHarborBuilding*>(goal)->WareDontWantToTravelByShip(this); | ||
| } else | ||
| { | ||
| // TODO(Replay) This should calculate the next dir even when carried |
There was a problem hiding this comment.
Yes: "TODO(Replay)" marks places that should be addressed when the replay version is increased, i.e. breaking old replays. I added this because it would be correct to do but can't be done as replays will go async
| // Else it wants to go somewhere else, so add to waiting wares | ||
| if(!carried_ware->GetGoal() || carried_ware->GetGoal() == wh) | ||
| { | ||
| // TODO: Remove assert. Was added to verify prior condition |
The check `GetGoal() != GetLocation()` is always met because the location is the flag and the goal is NULL if it was destroyed while the ware is being carried out.
When the goal for a ware becomes unreachable while it is carried into a harbor `RecalcRoute` will call `FindRouteToWarehouse`. Usually the selected warehouse will be the harbor as it is trivially the closest. However when the harbor does not accept this ware type another one might be chosen and the ware needs to be moved out instead of stored. But `FindRouteToWarehouse` does NOT set `next_dir` while the ware is carried even when called from `RecalcRoute` where the caller assumes it is set. Changing this causes replays to go async, so work around it by calling `RecalcRoute` again which now succeeds as it reached the goal in the previous call. This bug also affects `WantInBuilding` which also calls `RecalcRoute` assuming it does so in all cases. Fixes Return-To-The-Roots#1843
3 participants
When the goal for a ware becomes unreachable while it is carried into a harbor
RecalcRoutewill callFindRouteToWarehouse.Usually the selected warehouse will be the harbor as it is trivially the closest.
However when the harbor does not accept this ware type another one might be chosen
and the ware needs to be moved out instead of stored.
But
FindRouteToWarehousedoes NOT setnext_dirwhile the ware is carried even whencalled from
RecalcRoutewhere the caller assumes it is set.Changing this causes replays to go async, so work around it by calling
RecalcRouteagainwhich now succeeds as it reached the goal in the previous call.
This bug also affects
WantInBuildingwhich also callsRecalcRouteassuming it does so in all cases.Fixes #1843
The test case was a bit tricky to write and I added the case where no goal is possible in which case the ware should be accepted in the ware house anyway.