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

SamuXarick · 2021-07-03T16:28:29Z

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

OpenTTD/src/water_cmd.cpp

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:

OpenTTD/src/landscape.cpp

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.

Description

Limitations

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')
- newgrf_debug_data.h may need updating.
- PR must be added to API tracker

SamuXarick · 2021-07-14T14:28:07Z

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

TrueBrain · 2021-09-18T08:38:20Z

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.

Regarding DC_FORCE_CLEAR_TILE:
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

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.

TrueBrain added this to the 12.0 milestone Sep 14, 2021

TrueBrain reviewed Sep 18, 2021

View reviewed changes

src/water_cmd.cpp Show resolved Hide resolved

SamuXarick added 2 commits September 18, 2021 11:47

Fix: reduce cost of building canals over objects on sea

37c002d

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.

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

4c37d36

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

SamuXarick force-pushed the inconsistent-error-message-when-overbuilding-canal-on-a-lock/ship-depot branch from 128cbcd to 4c37d36 Compare September 18, 2021 10:47

TrueBrain approved these changes Sep 18, 2021

View reviewed changes

TrueBrain merged commit b335b05 into OpenTTD:master Sep 18, 2021

SamuXarick deleted the inconsistent-error-message-when-overbuilding-canal-on-a-lock/ship-depot branch December 5, 2022 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SamuXarick commented Jul 3, 2021

SamuXarick commented Jul 14, 2021 •

edited

TrueBrain commented Sep 18, 2021 •

edited

	/* 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);

	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]);
	}

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

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

Conversation

SamuXarick commented Jul 3, 2021

Motivation / Problem

Description

Limitations

Checklist for review

SamuXarick commented Jul 14, 2021 • edited

TrueBrain commented Sep 18, 2021 • edited

SamuXarick commented Jul 14, 2021 •

edited

TrueBrain commented Sep 18, 2021 •

edited