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
Prevent water construction where it could block nearby ships #6935
Conversation
4d9e867
to
9a4d803
Compare
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.
Most of the ship checks seem to be paired with CMD_LANDSCAPE_CLEAR
, wouldn't it be better to change that command to include the ship check itself, instead of all the call sites?
src/industry_cmd.cpp
Outdated
@@ -1395,6 +1395,7 @@ static CommandCost CheckIfIndustryTilesAreFree(TileIndex tile, const IndustryTil | |||
} | |||
} else { | |||
CommandCost ret = EnsureNoVehicleOnGround(cur_tile); | |||
if (ret.Succeeded()) ret = EnsureNoShipFromDiagDirs(cur_tile); |
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.
This one is different from the others, everywhere else just uses AddCost
to merge error states.
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.
I'm not sure what you mean here. It looks fine, for me. Doesn't look like it need to use AddCost
.
Additionally, I think a better commit message (especially first line) would talk about the problem being solved, instead of the tech being used to solve it. |
No. I'm not calling all sites, there are way more places with |
What situation clearing a water/river/channel tile does not warrant checking that a ship is about to enter it? To me it sounds straightfoward to make the checking as part of the "can clear tile?" logic |
For example, when building a lock, ship depot or dock, the diag_dirs to check are only a few. When removing/clearing most items, this check isn't needed at all. Also water tile types, when buiding a canal, it's not needed to check for a ship nearby. Then there are other very special cases where the command is used, like industries clearing trees for the lumber mil, clearing of tiles that are garanteed not having water tracks in the end result. Also, the bankrupting of a company doesn't and shouldn't make this ship check. |
With the current way ships are handled, I see no point in trying to do this. First of all, it just prevents building a ship depot if a ship is "close", but it doesn't check whether the ship can reverse and find its way to its destination. I see no real improvement over the current situation. By the way, what you are doing here is familiar to me. I implemented a different approach with ships and water tracks that needed to check bounding tiles, but it makes little sense in the vanilla case. |
We recently switched from Jenkins as CI to Azure Pipelines as CI. This means you need to rebase before this Pull Request will pass its checks. Sorry for the troubles! |
Additional checks which helps prevent ships from becoming stuck upon doing certain actions: building of industries, objects, rail depots, roads, road vehicle depots, town houses, bridge heads, airports, docks, ship depots, locks, and terraforming of tiles containing water
9a4d803
to
2d50578
Compare
I changed the title of this PR to reflect the purpose of it, instead of the name the function implementing the check happened to be given. |
it's not any ordinary block, but the block that makes the ship dead-locked, with no means to get out of there. Imagine red ship 499 and blue ship 498 being from another company. The ship owner would have no means to move away from that situation no matter what he do. I didn't make it check whether the ships belong to the same company because it's a situation that also happens in AI games. |
A malicious player would always be able to box in a ship with no way for its owner to rescue it. The case of Ship 498 in this picture, being constricted between a dock and a depot, looks more like ship movement being limited is the problem, though? If the ship was able to turn 90 degrees on the spot, it wouldn't be constricted. |
I'm going to make an Executive Decision(tm) and close this. It's basically just preventing malicious behaviour between players which ultimately is not something we can prevent (in the general sense), and as such it doesn't make sense to spend all the extra CPU cycles to try to. Feel free to |
Can I edit the code and keep it only for industries? Avoids #6273 from happening. |
#6273 looks like a FIRS bug to me. It was already closed as "won't fix in OpenTTD". I haven't fixed the bug in FIRS as I found it impossible to repro. Bugs that can't be reproduced reliably can't be fixed reliably. |
Additional checks which helps prevent ships from becoming stuck upon doing certain actions:
building of industries, objects, rail depots, roads, road vehicle depots, town houses, bridge heads, airports, docks, ship depots, locks, and terraforming of tiles containing water.
As an example, this would prevent the situation caused in #6273.
It happens in AI games, too.