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

Feature: Permanent rivers #8461

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@SamuXarick
Copy link
Contributor

@SamuXarick SamuXarick commented Dec 28, 2020

Motivation / Problem

Players that make use of rivers for their ships can have another player come in and easily destroy the river just to lay their rail tracks or terraforming terrain without caring if there was any river there. The current solution is to build canals over the river to make them unclearable.

Accidental or not, terraforming rivers clears them. When it's not intentional, there's no way to bring the river back.

Some servers like citybuilders create an area that is exclusive to a player, and whenever another player tries to build something there, the server undoes it immediately. If it happens to be a canal being placed on a river, this would result in the canal being demolished and leave bare land instead of restoring the river.

Description

  • Rivers are indestructible, unless cheating or in the scenario editor.
  • Added a game setting that can turn on or off this feature.
  • Savegame conversion.

Other changes:

  • Demolishing a canal that was built on rivers will restore the river, regardless of setting value.
  • Can no longer use terraform tool to remove rivers, must demolish them first, regardless of setting value.

Limitations

I intentionally made terraforming rivers to come up with an error, warning the player that the river must be demolished first, assuming the setting allows demolishion of rivers.

  • Is this feature complete? Are there things that could be added in the future?
    Probably not complete, as there are things that could be added in the future. I suppose there could be added graphics for canalized river, extended to NewGRF support.

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

@stormcone stormcone left a comment

I think most of the
bool river = HasTileCanalOnRiver(tile_cur);
variables are unnecessary becuase you only use them once.

Loading

src/industry_cmd.cpp Show resolved Hide resolved
Loading
src/industry_cmd.cpp Show resolved Hide resolved
Loading
src/station_cmd.cpp Show resolved Hide resolved
Loading
src/station_cmd.cpp Show resolved Hide resolved
Loading
src/water_cmd.cpp Show resolved Hide resolved
Loading
src/water_cmd.cpp Show resolved Hide resolved
Loading
src/water_cmd.cpp Outdated Show resolved Hide resolved
Loading
src/water_cmd.cpp Outdated Show resolved Hide resolved
Loading
@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Dec 28, 2020

I think most of the
bool river = HasTileCanalOnRiver(tile_cur);
variables are unnecessary becuase you only use them once.

They are getting the state of the tile before a clear command is issued. The state of the tile is different afterwards.

Loading

@stormcone
Copy link
Contributor

@stormcone stormcone commented Dec 28, 2020

If I turn off the Allow removal of rivers setting, and try to demolish a river tile, I can hear the sound of the demolition however nothing happens. I would expect no sound and an error message stating that the river cannot be demolished.

Loading

@stormcone
Copy link
Contributor

@stormcone stormcone commented Dec 28, 2020

I think most of the
bool river = HasTileCanalOnRiver(tile_cur);
variables are unnecessary becuase you only use them once.

They are getting the state of the tile before a clear command is issued. The state of the tile is different afterwards.

I see. :)
In that case you use 2 variable names: river and canal_on_river. It would be better to use only one. (Personally I better like the latter.) :)

Loading

@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Dec 28, 2020

If I turn off the Allow removal of rivers setting, and try to demolish a river tile, I can hear the sound of the demolition however nothing happens. I would expect no sound and an error message stating that the river cannot be demolished.

Hmm, is it really needed that a message shows up? I'm unsure if I can do this for a single river tile. For multi-tile demolish, it seems better not to display anything. I'll see what I can do.

Loading

@SamuXarick SamuXarick force-pushed the permanent-rivers branch from d44cd53 to 8469ab0 Dec 29, 2020
@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Dec 29, 2020

I think most of the
bool river = HasTileCanalOnRiver(tile_cur);
variables are unnecessary becuase you only use them once.

They are getting the state of the tile before a clear command is issued. The state of the tile is different afterwards.

I see. :)
In that case you use 2 variable names: river and canal_on_river. It would be better to use only one. (Personally I better like the latter.) :)

I renamed to river. Done.

Loading

@SamuXarick
Copy link
Contributor Author

@SamuXarick SamuXarick commented Dec 30, 2020

If I turn off the Allow removal of rivers setting, and try to demolish a river tile, I can hear the sound of the demolition however nothing happens. I would expect no sound and an error message stating that the river cannot be demolished.

Hmm, is it really needed that a message shows up? I'm unsure if I can do this for a single river tile. For multi-tile demolish, it seems better not to display anything. I'll see what I can do.

Update: I am unable to show up an error message. Couldn't figure out how to do it. Main issue is that I can't tell where the clear command was originally issued. A clear landscape command can be issued by a lot of things, like terraforming, building other infrastructure, objects, and not just demolishing. ClearTile_Water has always returned true for river and sea, and even with the changes in this PR, it still returns true, but the difference is that it may or may not remove the river. If I were to make it return an error, then many of the other commands that expect it to behave as it previously did, would simply not work. Like for example, building a ship depot, incurs in a clear landscape command. If it's built upon a river, the command has to return true, and not false with an error.

This limitation prohibits me to make it come up with an error. Or it could be that I just don't know better.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 7, 2021

I am not sure about this feature. It solves only a tiny portion of the issues with ships, while completely ignoring the others. You can still terraform sea, and just land-block a dock of a competitor. I do not see why rivers have to be more special that they require protection. Can you elaborate on the choice to only protect rivers?

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 7, 2021

So what I understand from talking to people, there are two issues:

  1. rivers are (one of the) only things that once destroyed, cannot be replaced. Which makes it annoying as ... when you do that accidentally.
  2. people don't like how canals look, and they cannot be put on slopes.

So by making them indestructible, we kinda solve those issues. But I keep wondering how that effects gameplay. All of a sudden a huge part of the map cannot be changed at all. This is not what the game currently is and does. You can terraform almost everything (exceptions are industries and transmitters/lighthouses; the latter everyone hates). So I am afraid people will, after 1 game being annoyed by this mechanic, just disable it and still run into the same problems they run into now. So I keep wondering if this is the right solution for the problem. It sounds like this is the easy way out.

An alternative I can imagine, is that we do some fluid-mechanics. Make the start of the river a "source" block, which can be indestructible for all I care, and make it flow through "trenches" to the sea. Now if you destroy a river, you can place back such "trench", and it will flow back through it again. It even allows you to move rivers, etc. This would also solve 1) and 2), but in a far less disruptive way.

I am still not sure if that was the intention of this PR btw; I just extrapolated this from what other people have been telling me.

tl;dr: I think this solution is way too disruptive for the gameplay for how most people play the game. But this is a very subjective opinion :)

Loading

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Jan 9, 2021

Issue is solved (differently) in JGR by providing river building tool usable in game, not just scenario editor.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 9, 2021

Issue is solved (differently) in JGR by providing river building tool usable in game, not just scenario editor.

We once had a patch for that, but in general, it feels weird from a gameplay mechanic. It is an easy solution, but not one that really fits in the game, in my opinion. Of course, realism is a relative thing in OpenTTD, so meh :)

Loading

@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Jan 9, 2021

Issue is solved (differently) in JGR by providing river building tool usable in game, not just scenario editor.

We once had a patch for that, but in general, it feels weird from a gameplay mechanic. It is an easy solution, but not one that really fits in the game, in my opinion. Of course, realism is a relative thing in OpenTTD, so meh :)

That feature is off by default in my branch. It's mainly there for the world-building/decoration players.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 9, 2021

Another thought came to mind:

Rivers currently appear not to be linked to town rating. What if we make destruction and construction of rivers based on town rating of the closest town? That might be a balance between all this: you cannot destroy endless amounts of rivers (so grieving etc is less likely), doing it by accident is okay-ish, and you can rebuild it, and you can't just drag&drop a full new river through the landscape as you ever so felt like it?

No clue if this is a good idea, just thinking out loud really :P

Loading

@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Jan 9, 2021

1. rivers are (one of the) only things that once destroyed, cannot be replaced. Which makes it annoying as ... when you do that accidentally.

I've also got a setting which disables removing rivers and sea/ocean tiles, but this is mainly to increase the difficulty and prevent silly ocean terraforming. The default is to allow removing, as otherwise most players really would be annoyed.

Another thought came to mind:

Rivers currently appear not to be linked to town rating. What is we make destruction and construction of rivers based on town rating of the closest town? That might be a balance between all this: you cannot destroy endless amounts of rivers (so grieving etc is less likely), doing it by accident is okay-ish, and you can rebuild it, and you can't just drag&drop a full new river through the landscape as you ever so felt like it?

No clue if this is a good idea, just thinking out loud really :P

Having town local authorities block actions outside of their authority zone would be inconsistent with the usual behaviour of towns, where egregious destruction outside of the authority zone is ignored.

As an aside, a large fraction of players already find town authorities quite constricting, and are quite happy to demolish rivers, towns, mountains, etc. to fit perfectly flat and straight mainlines. I think that such a change would not be well appreciated by such players.

Loading

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 9, 2021

1. rivers are (one of the) only things that once destroyed, cannot be replaced. Which makes it annoying as ... when you do that accidentally.

I've also got a setting which disables removing rivers and sea/ocean tiles, but this is mainly to increase the difficulty and prevent silly ocean terraforming. The default is to allow removing, as otherwise most players really would be annoyed.

Another thought came to mind:
Rivers currently appear not to be linked to town rating. What is we make destruction and construction of rivers based on town rating of the closest town? That might be a balance between all this: you cannot destroy endless amounts of rivers (so grieving etc is less likely), doing it by accident is okay-ish, and you can rebuild it, and you can't just drag&drop a full new river through the landscape as you ever so felt like it?
No clue if this is a good idea, just thinking out loud really :P

Having town local authorities block actions outside of their authority zone would be inconsistent with the usual behaviour of towns, where egregious destruction outside of the authority zone is ignored.

As an aside, a large fraction of players already find town authorities quite constricting, and are quite happy to demolish rivers, towns, mountains, etc. to fit perfectly flat and straight mainlines. I think that such a change would not be well appreciated by such players.

You make valid points :) And you confirm basically what I am afraid with if a PR like this hits without having it off by default. Hmm .. this is more tricky than it looks :D

Loading

@Eddi-z
Copy link
Contributor

@Eddi-z Eddi-z commented Jan 9, 2021

Random Idea: (might be unrelated to this PR)

  • 5 classes of water tiles: Ocean, Natural river, Natural lake, Canalized river and Canal
  • On map generation, river tiles get assigned a flow direction, and if necessary a cached spring tile and a mouth tile (stored in map array).
  • Canals turn into "Canalized River" if there is any flow through them. "Canalized River" can be built by players on slopes.
  • Natural river tiles without flow turn into lake, Canalized River tiles without flow turn into Canal
  • Demolishing a river calculates an alternative flow route, and is only allowed if flow can connect spring to mouth through an alternate path.

Loading

@SamuXarick SamuXarick force-pushed the permanent-rivers branch from 8469ab0 to 8d9b80c Jan 9, 2021
/* Canals can't be terraformed */
if (IsWaterTile(tile) && IsCanal(tile)) return_cmd_error(STR_ERROR_MUST_DEMOLISH_CANAL_FIRST);
/* Canals and rivers can't be terraformed */
if (IsWaterTile(tile)) {
Copy link
Contributor

@Eddi-z Eddi-z Jan 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change feels a bit weird. might be better restructured as:

if (water) {
    if (!cheat) return error;
    if (canal) return error;
    if (river) return error;
}

Loading

@SamuXarick SamuXarick force-pushed the permanent-rivers branch 2 times, most recently from a289dd0 to 4066eb9 Jan 11, 2021
@btzy
Copy link
Contributor

@btzy btzy commented May 31, 2021

Gameplay-wise, canals and flat rivers are identical, and locks are just sloped rivers that boats can pass through. How about this (just a suggestion):

  • Source tiles of rivers are indestructible
  • A river/canal/lock/waterway can be demolished if and only if every source tile still has a path (via other rivers/canals/locks/waterways) to some ocean tile.

So this means in practice that the player has to build an alternative route for the river before demolishing the original one.

As for whether the player should be able to build rivers or canals: It doesn't really matter since they behave in exactly the same way. We could leave it as it is currently, where players can only build canals. Or we could simply merge them and use the canal sprite for rivers within cities and the river sprite for everywhere else (we would need a new sloped canal sprite for this though).

I like the latter way better though - this makes waterway networks become more similar to road networks - the city owns some rivers/canals at first, but you can build more of them for your ships to use. The canal vs river sprites are paralleled by the road sprites with pavement vs without pavement.

Loading

- Rivers are indestructible, unless cheating or in the scenario editor.
- Added a game setting that can turn on or off this feature.
- Savegame conversion.

Other changes:
- Demolishing a canal that was built on rivers will restore the river, regardless of setting value.
- Can no longer use terraform tool to remove rivers, must demolish them first, regardless of setting value.
@SamuXarick SamuXarick force-pushed the permanent-rivers branch from 5ebf642 to 3da8440 Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants