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

Prevent water construction where it could block nearby ships #6935

Closed

Conversation

SamuXarick
Copy link
Contributor

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.
nonocab - v5 2051-01-26
nonocab - v5 - 2 2051-01-26

@SamuXarick SamuXarick force-pushed the EnsureNoShipFromDiagDirs branch 5 times, most recently from 4d9e867 to 9a4d803 Compare October 9, 2018 18:51
Copy link
Contributor

@nielsmh nielsmh left a 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?

@@ -1395,6 +1395,7 @@ static CommandCost CheckIfIndustryTilesAreFree(TileIndex tile, const IndustryTil
}
} else {
CommandCost ret = EnsureNoVehicleOnGround(cur_tile);
if (ret.Succeeded()) ret = EnsureNoShipFromDiagDirs(cur_tile);
Copy link
Contributor

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.

Copy link
Contributor Author

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.

src/object_cmd.cpp Outdated Show resolved Hide resolved
@nielsmh
Copy link
Contributor

nielsmh commented Oct 31, 2018

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.

@SamuXarick
Copy link
Contributor Author

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?

No. I'm not calling all sites, there are way more places with CMD_LANDSCAPE_CLEAR that wouldn't make sense to do the ship check. I prefer a case by case approach, only on situations I believe are really necessary.

@nielsmh
Copy link
Contributor

nielsmh commented Oct 31, 2018

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

@SamuXarick
Copy link
Contributor Author

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.

@J0anJosep
Copy link
Contributor

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.
And secondly, there are many other situations were a ship depot can be built and block hundreds of ships, with these ships not being close to the newly built object.

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.

@TrueBrain
Copy link
Member

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
@SamuXarick SamuXarick changed the title Add: EnsureNoShipFromDiagDirs Add: EnsureNoShipOnDiagDirs Jan 12, 2019
@nielsmh nielsmh changed the title Add: EnsureNoShipOnDiagDirs Prevent water construction where it could block nearby ships Jan 12, 2019
@nielsmh
Copy link
Contributor

nielsmh commented Jan 12, 2019

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.

@SamuXarick
Copy link
Contributor Author

SamuXarick commented Jan 19, 2019

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.

@nielsmh
Copy link
Contributor

nielsmh commented Jan 19, 2019

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.

@LordAro
Copy link
Member

LordAro commented Jan 20, 2019

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 complain talk about it in the IRC channel

@LordAro LordAro closed this Jan 20, 2019
@SamuXarick
Copy link
Contributor Author

Can I edit the code and keep it only for industries? Avoids #6273 from happening.

@andythenorth
Copy link
Contributor

#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants