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: Prevent tunnels from being built under mines #8495

Closed
wants to merge 1 commit into from

Conversation

@perezdidac
Copy link
Contributor

@perezdidac perezdidac commented Jan 4, 2021

Motivation / Problem

Currently, tunnels (road or rail) can be built under tiles with mines. The motivation is to make the game more realistic by preventing tunnels to be built where the raw material being extracted is supposed to come from: under ground.

Description

Prevent tunnels from being built under mines. The code changes adds a constrain to the code path that makes a tunnel to check that every tile along the tunnel is not under a mine industry.

I assume some players should be OK with this restriction as it makes sense in real life and makes the game more challenging in a good way. However, I am making it as an opt-in, given that the default value is set to 1.

Limitations

This code change does not prevent mines to be built on top of already existing tunnels.

Checklist for review

There is no problem with existing tunnels in previously saved games.

Testing

I have tested this change successfully with the original industry set as well as FIRS. Only mine industries prevent tunnels to be built under them.

@perezdidac perezdidac changed the title FR: Prevent tunnels from being built under extractive industries Feature: Prevent tunnels from being built under extractive industries Jan 4, 2021
@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Jan 4, 2021

Changing pull request name to pass checks.

@perezdidac perezdidac closed this Jan 4, 2021
@perezdidac perezdidac reopened this Jan 4, 2021
@James103
Copy link
Contributor

@James103 James103 commented Jan 4, 2021

Firstly, I would not want this restriction unless it was a voluntary (opt-in) setting as that breaks many optimization-based playstyles that make use of vertical height differences. Also, it would not make sense to be unable to build a massive tunnel very deep (100+ tiles) below a mine where it would most likely not reach that far down.

Secondly, The commit message needs to be changed as well. Replace FR: with Feature:.

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Jan 4, 2021

Hi James, I can work on make it an opt-in in settings (please can you provide code pointers on how to add a flag in settings?) and make it configurable so we can pick how deep we can build a tunnel under a mine. Seems to be a pretty easy thing to do. Thank you for the suggestions!

@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Jan 4, 2021

Just a note, some industry sets (including my Improved Town Industries) have mines which produce passengers as workers. I assume this won't be recognized as mines.

I don't know about the check for liquid cargos is needed — wouldn't a tunnel under an oil well be just as problematic as a coal mine?

src/station_cmd.cpp Outdated Show resolved Hide resolved
src/industry_map.h Outdated Show resolved Hide resolved
src/industry_cmd.cpp Outdated Show resolved Hide resolved
@Eddi-z
Copy link
Contributor

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

Changing pull request name to pass checks.

you can call the pull request whatever you like. it's the commit message that the CI check cares about.

@Eddi-z
Copy link
Contributor

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

I don't know about the check for liquid cargos is needed — wouldn't a tunnel under an oil well be just as problematic as a coal mine?

problem is, you can't change this check, as it would influence the original use of this function

@Eddi-z
Copy link
Contributor

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

Alternate Suggestion:
instead of relying on this heuristic about cargo types/classes, add a flag to https://newgrf-specs.tt-wiki.net/wiki/Action0/Industries#Special_industry_flags_to_define_special_behavior_.281A.29 for "cannot have a tunnel underneath" (and possibly also for "is considered a 'mine' for station names", replacing the original use of this function)

if we add this to default industries, and the NewGRF authors used property 8 in a sane way (which they maybe didn't) it should propagate to new industries properly

@perezdidac perezdidac force-pushed the perezdidac:tunnel_under_mine branch from 5773b90 to 0be8e96 Jan 4, 2021
@perezdidac perezdidac force-pushed the perezdidac:tunnel_under_mine branch from 0be8e96 to 9fc56a3 Jan 4, 2021
@perezdidac perezdidac changed the title Feature: Prevent tunnels from being built under extractive industries Feature: Prevent tunnels from being built under mines Jan 4, 2021
@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Jan 4, 2021

Firstly, I would not want this restriction unless it was a voluntary (opt-in) setting as that breaks many optimization-based playstyles that make use of vertical height differences. Also, it would not make sense to be unable to build a massive tunnel very deep (100+ tiles) below a mine where it would most likely not reach that far down.

Secondly, The commit message needs to be changed as well. Replace FR: with Feature:.

James, I have addressed both comments and updated the code change. Please kindly review.

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Jan 4, 2021

I don't know about the check for liquid cargos is needed — wouldn't a tunnel under an oil well be just as problematic as a coal mine?

problem is, you can't change this check, as it would influence the original use of this function

Updated, thank you!

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Jan 4, 2021

Alternate Suggestion:
instead of relying on this heuristic about cargo types/classes, add a flag to https://newgrf-specs.tt-wiki.net/wiki/Action0/Industries#Special_industry_flags_to_define_special_behavior_.281A.29 for "cannot have a tunnel underneath" (and possibly also for "is considered a 'mine' for station names", replacing the original use of this function)

if we add this to default industries, and the NewGRF authors used property 8 in a sane way (which they maybe didn't) it should propagate to new industries properly

I looked into this but I ended up finding out that there is a flag for extractive industries already, and I wanted to prevent adding new flags since I would expect complains from people about it...

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Jan 4, 2021

Are there any instructions on how to update regression files? They seem to fail with this change.

@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Jan 4, 2021

I looked into this but I ended up finding out that there is a flag for extractive industries already, and I wanted to prevent adding new flags since I would expect complains from people about it...

I suspect you're looking at the industry type, 0B. This suggestion is for a new special flag in 1A. #8079 is a recent precedent for adding flags to support NewGRF behaviors not used by vanilla industries.

@Eddi-z
Copy link
Contributor

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

Alternate Suggestion:
instead of relying on this heuristic about cargo types/classes, add a flag to https://newgrf-specs.tt-wiki.net/wiki/Action0/Industries#Special_industry_flags_to_define_special_behavior_.281A.29 for "cannot have a tunnel underneath" (and possibly also for "is considered a 'mine' for station names", replacing the original use of this function)
if we add this to default industries, and the NewGRF authors used property 8 in a sane way (which they maybe didn't) it should propagate to new industries properly

I looked into this but I ended up finding out that there is a flag for extractive industries already, and I wanted to prevent adding new flags since I would expect complains from people about it...

well, the main problem is that this "extractive" flag is more general than "mine", which makes this heuristic about cargo types/classes in this function necessary, which is problematic enough if you use it for one purpose (station name), but even more problematic if you use it for two unrelated purposes

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Jan 4, 2021

Looked into it, that's not what I found yesterday so I will work on having it as a behavior actually, will update code change this week hopefully, thank you!

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 5, 2021

Hi @perezdidac ,

Thank you for your PR!

First of all, sorry you spend time on adding a setting for this, as that suggestion is not in line with what we maintainers of this game have as vision for the game. In general, we really frown upon new settings. OpenTTD already has many many many many settings, of which most are never used by anyone, and over the years we have found out that having this many settings always leads to more bug reports as in: but if I flip these 3 exotic settings, then your patch breaks! It is horrible :P So we try to stay away from settings in general, and as policy, we basically do not add settings, unless it really benefits a bigger group as a whole; not the itch of a few :)

That doesn't mean we add code that is "enabled for all", like the initial version of this PR. For this, we found that using either NewGRFs or GameScript is a much more appropriate solution. For example, if NewGRFs could control, as suggested by others, if a tunnel is allowed below them or not, that means a user can pick a NewGRF that does this or not. So the "setting" becomes part of the NewGRF you choose to download. This is much more maintainable for us, easier for the user to understand, and up to the content creator to have fun with :)

See our project goals for more details on this.

However, in this case, I truly wonder if we should add this to the base game. I am just one opinion, so take it with what-ever salt is needed, but my reasoning is as follow:

  • If you want realism, simply do not build tunnels under mines. There is no need to enforce this, really.
  • If you don't allow tunnels to be build under mines, you also have to add code that doesn't allow mines to be build over tunnels.
  • If it is realism you are after, how high can a mine be on a hill before a tunnel is valid? Is there ever a moment? Feels kinda harsh if for all mines the height is the same. I can imagine that certain mines are shallow, while others are deep. Should this influence the behaviour? (honest questions; I do not know the answers :D)

To me this feels like a lot of change for only a very small group of people (but I can be completely wrong of course; I can be convinced otherwise :D). In short, in my opinion, if we do this, this PR should:

  • Change the NewGRF specs, with a clear goal and plan
  • Conditionally disallow tunnels to be build
  • Conditionally disallow industries to be build because there is a tunnel

This increases the scope of this PR immensely. So I really wonder if that is worth it .. especially as such realism can also just be enforced by agreement, not by the game. A bit like role-playing .. nothing is holding you back from breaking character, but if you are true to yourself, you don't need a game to enforce it on you :)

What do you think? (again, read this more as thinking out loud than anything else; just trying to voice my opinion to start a discussion :D)

PS: on a personal opinion, this would annoy the hell out of me; I often had the case I was building a mainline, to have some industries exactly on top of it .. I was happy I could tunnel under it to continue my pretty mainline. The idea that I cannot under some industries would tick me off :P But that is really just a personal game-style opinion, and not as much relevant for this discussion :D

@Eddi-z
Copy link
Contributor

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

* If you want realism, simply do not build tunnels under mines. There is no need to enforce this, really.

i heavily oppose this opinion. in my history of playing this game, occasionally i have come across certain restrictions, that i would never even have considered before.

i'm a bit sceptical of offloading this to NewGRFs, because for every player type i can imagine, the decisions "should tunnels be restricted this way" and "should industry chains be different" are completely orthogonal.

as for the "bigger picture" approach: i do like the thought of combining this with a potential "bridges over everything", so my idea would be to add a z-extent property to houses and house-like things (objects, industrytiles, stations, etc.), that would define how many levels above and below ground level are blocked for other construction. in a distant relation this could also allow for airport runway approach/takeoff patterns to define how high buildings can be.

@Eddi-z
Copy link
Contributor

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

Are there any instructions on how to update regression files? They seem to fail with this change.

regression tests are there to find out whether you've inadvertendly broken something, and the output should be carefully checked whether that is the case, before you even consider updating the regression test itself.

Broken savegame - Invalid chunk size'

this probably means you forgot something while adding the setting, like put in invalid min/max savegame versions

guiflags = SGF_NO_NETWORK
def = 1
min = 1
max = MAX_MAX_HEIGHTLEVEL

This comment has been minimized.

@Eddi-z

Eddi-z Jan 5, 2021
Contributor

so, to add a new setting, you first add a new saveload version to the end of the enum in saveload.h, just before the line that says
SL_MAX_VERSION, ///< Highest possible saveload version
and to your entry in settings.ini you add a line that says
from = <insert new enum entry here>

this way, the game can figure out that a savegame is older, and can insert the new setting at the correct position with its default value upon loading

@perezdidac
Copy link
Contributor Author

@perezdidac perezdidac commented Jan 5, 2021

Thanks everyone for all suggestions and information! This is awesome. I will hold on on this for now, I believe it might be hard to reach an agreement on this matter. For situations like this, is there a way to get consensus on developers and players somehow?

@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 5, 2021

We talked it over some more with the developers, and how ever we twist or turn it, we don't see a place for this at the moment in OpenTTD. Not because we are against the feature itself, but because of the work involved in making it happen.

This for sure can and should be revisited later, especially when we are a bit further along in figuring out how we want to make other parts of the game scriptable. For a bit of context, we are looking how to make things like Towns scriptable, so people can make their own town-growth algorithms and stuff like that. There is some hope, but this might be wishful thinking, that this can extend to other places too, like: can you build here. That would be a far better place to fit in this feature :)

Either way, thank you for this Pull Request! I hope you understand our reasoning; if you feel up for more work on OpenTTD, our issue-tracker is full of small (and big) problems and bugs that need solving, as we are slowly working our way to a 1.11 release :) Feel free to hit me up on Discord if you have other ideas and want to talk over first if it fits in OpenTTD, or if you just have any other question in general :)

Tnx again!

@TrueBrain TrueBrain closed this Jan 5, 2021
@TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Jan 5, 2021

PS: https://www.tt-forums.net/viewtopic.php?f=33&t=21438 :D Seems history has a tendency on coming back around :)

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

5 participants