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

Fix: Inconsistent error message when overbuilding canal on a lock/ship depot tile #9410


Copy link

@SamuXarick SamuXarick commented Jul 3, 2021

Motivation / Problem

This is a delicate issue.
It originated when my AI was overbuilding a canal on the entry/exit of a lock tile. If the waterclass was canal, I would get a 'Can't build canals here... ... already built' error message, but if the waterclass was sea, I would get 'Can't build canals here... Building must be demolished first'.


Lines 478 to 485 in d38ad7d

/* can't make water of water! */
if (IsTileType(current_tile, MP_WATER) && (!IsTileOwner(current_tile, OWNER_WATER) || wc == WATER_CLASS_SEA)) continue;
bool water = IsWaterTile(current_tile);
ret = DoCommand(current_tile, 0, 0, flags | DC_FORCE_CLEAR_TILE, CMD_LANDSCAPE_CLEAR);
if (ret.Failed()) return ret;
if (!water) cost.AddCost(ret);

First I discovered that IsTileType(current_tile, MP_WATER) can't be used here, due to Ship Depots and Locks also being tiles of the same type MP_WATER as Canals. I changed to IsWaterTile(current_tile) and I thought that would be enough to fix the problem, but it wasn't.

Now I was getting 'Can't build canals here... Must demolish canal first'.

I found that DC_FORCE_CLEAR_TILE was in the way. It would end up doing these checks:


Lines 696 to 702 in d38ad7d

bool do_clear = false;
/* Test for stuff which results in water when cleared. Then add the cost to also clear the water. */
if ((flags & DC_FORCE_CLEAR_TILE) && HasTileWaterClass(tile) && IsTileOnWater(tile) && !IsWaterTile(tile) && !IsCoastTile(tile)) {
if ((flags & DC_AUTO) && GetWaterClass(tile) == WATER_CLASS_CANAL) return_cmd_error(STR_ERROR_MUST_DEMOLISH_CANAL_FIRST);
do_clear = true;
cost.AddCost(GetWaterClass(tile) == WATER_CLASS_CANAL ? _price[PR_CLEAR_CANAL] : _price[PR_CLEAR_WATER]);

I still tried adding some extra checks to line 698, to exclude locks and ship depots, but that would mean I was going against the purpose of DC_FORCE_CLEAR_TILE.

Went back to the origin of this flag, at line 482 of water_cmd.cpp. Decided to remove the flag, and that finally solved the issue.

Now, the error message is consistent, regardless of waterclass: 'Can't build canals here... Building must be demolished first'. They reach ClearTile_Water to get the error string.

There is however, behaviour changes when experimenting with tiles of type MP_OBJECT. I Loaded NewGRF 'OpenGFX+ Landscape 1.1.2'. When we have a canal, and a rocky land on the canal, overbuilding the canal no longer comes up with an error message saying 'Can't build canals here... ... Must demolish canal first'. Now it overbuilds with no error. Also, if we build a canal on top of a rocky land on sea, the price is also different. It doesn't include the price of clearing sea.



Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, gs_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Copy link
Contributor Author

@SamuXarick SamuXarick commented Jul 14, 2021

Apparently, this also fixes inconsistent error messages when overbuilding on dock-water-parts and buoys on sea/canal.

@TrueBrain TrueBrain added this to the 12.0 milestone Sep 14, 2021
Copy link

@TrueBrain TrueBrain commented Sep 18, 2021

Wow. This PR is really hard to read .. "rambling by a madman" .. "a day in the life of Samu" .. please do not do this to us. It makes it really hard to understand what you are trying to fix.

Additionally, this PR is basically: I fiddled till it worked. And that brings me to: a broken clock is even right twice a day. So let's dive in, see what happened, and what this change actually does.

In 2010, 0e250f2 makes the change that building canals over seas also charge you for the price of removing the sea first, because .. logic? the reasoning is never explained, so I have no clue.
In 2013, 00530f4 makes the change to remove any water-based costs when building canals, with the reasoning: you are not going to drain the sea first, just to fill it back up again.

Those two commits kinda contradict each other. Given the second negates the use for DC_FORCE_CLEAR_TILE, the change in this PR is fine as far as I can tell.

As for the IsTileType(current_tile, MP_WATER), I think this is simply forgotten to be changed over the years into IsWaterTile, as that clearly is the intention of that line (given the comment above). It dates back to the first revision we can track, in a time where MP_WATER truly meant: this is water. So the change in this PR is fine there too, as the comment clearly states the intention is to only prevent making water into water.

But, that said, when we dive a bit deeper in what that if-statement does, we notice the function has changed so drastically, it no longer really means what it used to. Take this change for example, a12c748, here it was added that using CTRL when placing canals meant placing sea. For that this check is useful, to prevent water being made to water, to nudge the player in the right direction.
However, this functionality no longer exists. Only in the SE you can place sea, and even that is limited. More important, if you place sea over sea you get "Canal already build" error. Which is weird.
Edit: basically the only thing this check does these days, is prevent you from building canals on things owned by OWNER_WATER (or sea). Read: rivers and sea.

So I suggest we completely remove this if-statement. That brings building sea on-par with building rivers: if you build sea over sea, or river over river, no error is given.

Finally, this PR really should be two commits: one removing the DC_FORCE_CLEAR_TILE, indicating it didn't do anything anymore. The second removing the if-statement, stating it no longer does anything.

@SamuXarick : Are you up for that? Or are you like: way over my head, please take care of it?

Edit: I suggest two commits with messages something like this:

DC_FORCE_CLEAR_TILE remove commit:

Fix: reduce cost of building canals over objects on sea

It is not like we will drain the sea first, to put water back in it after.
Besides, the cost for draining the sea isn't calculated for all other cases either.

Changing to IsWaterTile:

Fix: wrong error message when building canals over ship depots / locks

IsTileType() also considers ship depots and locks water. IsWaterTile() does the right thing.

src/water_cmd.cpp Show resolved Hide resolved
SamuXarick added 2 commits Sep 18, 2021
It is not like we will drain the sea first, to put water back in it after.
Besides, the cost for draining the sea isn't calculated for all other cases either.
IsTileType() also considers ship depots and locks water. IsWaterTile() does the right thing.
@SamuXarick SamuXarick force-pushed the inconsistent-error-message-when-overbuilding-canal-on-a-lock/ship-depot branch from 128cbcd to 4c37d36 Compare Sep 18, 2021
@TrueBrain TrueBrain merged commit b335b05 into OpenTTD:master Sep 18, 2021
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
None yet

Successfully merging this pull request may close these issues.

None yet

2 participants