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

Introduce snow/desert-coverage and custom terrain type, and move "maximum map height" to settings only #8891

Merged
merged 9 commits into from Mar 26, 2021

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Mar 24, 2021

Closes #8879

Preview available here: https://preview.openttd.org/pr8891/ (play this change in your browser!)
Binaries available here: https://www.openttd.org/downloads/openttd-branches/pr8891/latest.html (if you want to try large maps)

Motivation / Problem

Where even to start ...

If this thread made one thing clear, is that many people do not understand a few of the settings in the NewGame GUI, or have a hard time using them in a useful matter. And I have been very vocal that I do not think this is a user issue, but an UX one. This Pull Requests tries to correct the most common mistakes people make.

First off, I would like to serious thank reldrid for engaging in a constructive conversation about all of this. It makes such a difference to have such conversations; seriously, tnx. Also tnx to andythenorth for some gravedigging in old threads and frosch123 for sparring about solutions and possibilities. This hasn't been easy ;)
Sadly, there was also a lot of noise on the line, people using a lot of words to say very little, lot of screenshots without context, denying facts, etc. This was absolutely not helping .. it took 3 weeks before someone managed to tell me settings where 1.11-RC1 was producing less mountains than 1.10. It was a massive drain of energy to keep engaging in those conversations.

Anyway.

To start, lets dive in: the by-now famous "maximum map height" setting on the NewGame GUI. Looking through the MHL thread (tnx andythenorth for going through it), it got added "because it can be useful", under protest of the original author. It seems that it was not truly understood what it did, or what the effect would have on the player. Well, now 7 years later, we have the answer: not good :D
This setting set the height limit of the map to this value. It does not influence TGP, except that he too cannot build above this limit. So player increase the value, assuming the maps would produce higher mountains, and in some regards it is. But mostly, it isn't. People deduce all kind of weird conclusions based on it, and now we have two worlds: reality, and some vague misconception of reality. This is absolutely human, but makes the gap between perception and reality rather big.
So, let's stop annoying users, and lets split apart these settings. With this PR we now have the following settings:

  • map_height_limit - The true limit of the map. Nothing can build over this. Defaults to "auto" and is a setting only (no longer in NewGame GUI) More on this later.
  • heightmap_height - The brightest pixel in a heightmap will be this height. The rest is scaled in between. Replaces "maximum map height" in GUI.
  • custom_terrain_type - TGP is informed to build a map up to this height. It can fail in trying, as OpenTTD heavily limits the freedom TGP has, but it for sure will try to get there. Part of "Terrain Type" dropdown. Does what users expected "maximum map height" did.

These can all be controlled independently. They still influence each other, but hopefully the way they do is now more clear to the human.

Additionally, while we were considering how to untangle this mess, two other issues pop'd their ugly head:

  1. NewGRF snowline height. This is used in two ways: either a NewGRF says: 25% of the map should be snow, or a NewGRF let the user enter an "absolute" height level for the snow; in the background the NewGRF translates this to a relative value, which OpenTTD translates back to an absolute value. Huge tnx to Aegir for taking the time with me on Discord to talk about this, resulting in a nice conclusion.
    This is important as these values scale based on map_height_limit. As NewGRFs are initialized before map generation, this value has to be set correctly beforehand. Later on we will see that this constraint means we have to guestimate the height TGP will generate, rather than just look what it did.

  2. Snowline height is also a confusion setting. This often results in sub-optimal maps. When we looked at it, we mostly realised it is also a bit weird; most games nowedays use a "coverage", a percentage of the map that should be snow. This feels much more intuitive.

So, this PR also introduces two new settings:

  • snow_coverage - To set the % of the landmass that should be covered by snow (for desert climate)
  • desert_coverage - To set the % of the landmass that should be desert (for tropic climate)

Additionally, map_height_limit is now "auto" by default. When using this setting, it will guestimate what TGP is likely going to produce, and adjust the map_height_limit to that, plus a bit extra. The current limits are: never below 30, and always 15 above what-ever was estimated (but never over 255). The NewGRF snowline limitation makes it currently not possible to use the real "highest peak" TGP does. In theory it should be possible to do this, but it would require a refactoring of the terrain generation code.


All these changes together mean the defaults are a lot more sane for new players: arctic produces good snowy maps on all terrain types, and tropic looks good too.
And for the niche players that enjoy some extreme setting one way or the other, they can do this too:

  • Tropic maps without desert? Go ahead.
  • Arctic maps with no snow? Sure!
  • Extremely hilly map? If that is your thing.
  • Absolutely flat maps? What-ever, you do you.

There are some limitations, however. First, the coverage is translated into a heightlevel internally, and this means we cannot reach the value exactly. Sometimes it will be above, sometimes below. For tropic this is not needed, as every tile is assigned either desert or tropic; so strictly seen we can get the coverage exact. But given the time constraint (we want to include this in 1.11), that is currently not an option.
Second, "all snow" maps is not possible, simply because we do not have sprites for snow tiles touching water. So it is always at least 2.
Other than that, people can go completely nuts, as far as we care. I already created a map where the highest peak was 226. (screenshot is with a NewGRF loaded that influences snowline height, so no, that is not 40% coverage :P).

image

generated with:

image

Description

Review this commit by commit, really. It will make a lot more sense.

Most commits are mainly rename of variables.

Renaming map_height_limit is strictly seen not needed. But I did this with the following reasoning:

  1. the name is less confusing, hopefully helping future-us to understand this stuff.
  2. the changed name means it will use a new name from the openttd.cfg, but it loads the value from the savegame just fine. This means current savegames keep working correctly, but everyones configuration resets to auto. This should really help players who upgrade from 1.10 to 1.11.

Regarding the coverage algorithm to heightlevel, it took a while to get right. Especially desert has some weird rules: every water tile converts desert to tropic for a few tiles land inwards. And every tile above the desert/tropic line does the same towards sea. In other words: estimating the area that will be desert is more difficult than one would assume. edge_histogram is the trick here, it is a good average for the amount of tiles that convert desert to tropic in lower levels.

As I was incredible lazy, I just copied #8879 for the desert coverage, and changed it a bit to be a coverage. Tnx 2TallTyler for making it possible for me to do just that :D

Choices made

  • Besides variable-renaming, I fully optimized to "small diff". So there are related things that could also have been addressed, but I didn't want to make this impossible to review.
  • Coverage is based on landmass, as in: sea is subtracted. So 40% snow is 40% of the landmass, not of the full map. This felt more natural to me.
  • Coverage always finds the closest height line to the coverage value. This can be either too low or too high. This too felt more correct to me.
  • heightmap_height and custom_terrain_type could be a single variable. But I let it two, as I can understand people having different values for it, and changing one shouldn't influence the other, in my opinion.
  • custom_terrain_type follows custom_sea_level in implementation style.

Limitations

  • Snow height level is always 2+, because we do not have sprites for snow touching sea.
  • We should warn users if they try settings that are unlikely to work, like custom terrain types of 254 on a 64x64 map. We already know there is no way that is possible, so we should warn users.
  • Another place to warn, is when terrain type exceeds the map height limit (if not set to auto).

Questions

  • I do not store anything new in savegames. I think that is fine, as all these variables are just for generation only. But for example custom_sea_level is stored in the savegame, and I have no clue why. So I might be doing this completely wrong, and I should be storing this in the savegame. I would love to hear. Settings are now stored in the savegame.
  • How do you rate my defaults? 40% snow looks pretty good to me on all terrain types and map sizes. It is not overly present, and also not absent. Also, I haven't hit "can't place industry ..." yet with it. And 50% desert seems to fit the theme. Similar, no "can't place industry ...", and seems to strike a nice balance.

Apologizes

This PR will give translators a few more strings to translate, just before our release. I am really sorry about that. I tried to avoid it, but there really was no way to do that. SORRY!!! But it is for the best, really :D

Testing

Owh boy, this PR needs testing. And lots of it. So please help out, and test what-ever setting you fancy most. I did a ton of testing (in contrast to popular believe, I do tend to test my PRs :P), but more testing is better. The possible variables I am aware off and tested)

  • New Game / Scenario Editor / Flat World / Heightmap
  • Map size (influences terrain-types except for custom)
  • All climates
  • Original terrain generator vs TGP
  • Terrain type + custom values (2 / 254)
  • Snow coverage, 0 / 40 / 100
  • Desert coverage, 0 / 10 / 50 / 90 / 100
  • NewGRFs with settings for absolute snowline height (OpenGFX+landscape)
  • NewGRFs with settings for relative snowline height (alpinew.grf)
  • Sea level, all options + 1%

And my absolute favorite: go to scenario editor and make a flat world at 250 height :D Just because we can!!

If you do test, and find something you do not expect, please be as exact in your report as possible. Show what settings you used (both the World Generation window and the World Generation settings would really help), and try to explain in short words what you expected vs what you get. This PR is about user experience, so if something feels off, I would love to hear. Not saying we will address everything .. but it does give an idea where there is still work to be done :)

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

@TrueBrain TrueBrain changed the title Fix heights Introduce snow/desert-coverage and custom terrain type, and move "maximum map height" to settings only Mar 24, 2021
@TrueBrain TrueBrain added the preview label Mar 24, 2021
@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 24, 2021 Inactive
@mattkimber
Copy link
Contributor

@mattkimber mattkimber commented Mar 25, 2021

IMO this is a huge improvement. The terrain window works so much more how I'd "expect" it to work.

As discussed on IRC, there's an oddity where variety distribution affects tropic in a different way to arctic, but I want to do more testing and get some heightmaps to quantify whether that's really happening and in what way.

One little comment if it's easy to change - Terrain Type > Custom brings up a dialogue box with a numeric input and a heading "Terrain type:". I think this would be clearer if the dialogue said something like "Peak Height" explaining what the numeric value is.

@reldred
Copy link

@reldred reldred commented Mar 25, 2021

I concur, this is a fantastic improvement that I think will satisfy some of the largest complainers in the community carrying on about the 'regression' in 1.11 vs 1.10. Heightmap generation behavior is now slightly different, but I think in reality it is far more predictable what it's going to do compared to before where a ridiculous maxheight of 255 had to be used to produce mountains far smaller because the highest white-point on the height map got nowhere near 255.

Overall, my complaints (that I've already voiced to TrueBrain seperately and don't require re-stating) about setting snow/rainforest height levels as percentages aside, I implore the rest of the team to grant this PR a speedy review and inclusion to 1.11. This is a good first impression to make for the Steam release.

@auge8472
Copy link
Contributor

@auge8472 auge8472 commented Mar 25, 2021

I wanted to change the snow coverage in the create game window and the game crashed. After reloading the game window the setting was grayed out.

Browser: Firefox 86.0.1, Ubuntu 18.04

@James103
Copy link
Contributor

@James103 James103 commented Mar 25, 2021

Binaries available here: https://www.openttd.org/downloads/openttd-branches/pr8891/latest.html (once build; currently still building, check back in 30 minutes if the link is still a 404).

The link no longer returns HTTP 404; therefore, the parenthetical phrase can be removed.

@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Mar 25, 2021

I wanted to change the snow coverage in the create game window and the game crashed. After reloading the game window the setting was grayed out.

Browser: Firefox 86.0.1, Ubuntu 18.04

Oops. Fixed. (that it is grayed out is most likely because it no longer selected Arctic when you reloaded)

One little comment if it's easy to change - Terrain Type > Custom brings up a dialogue box with a numeric input and a heading "Terrain type:". I think this would be clearer if the dialogue said something like "Peak Height" explaining what the numeric value is.

Absolutely. I did have to fiddle with wording a bit, as it is unlikely TGP will generate a map that high. Target peak height I picked for now. Other suggestions are welcome.

@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 25, 2021 Inactive
@LordAro LordAro added the backport requested label Mar 25, 2021
src/lang/english.txt Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 25, 2021 Inactive
Copy link
Member

@LordAro LordAro left a comment

Not actually tested anything yet, just some thoughts when browsing through the code. Not going to pretend to have understood everything either :)

src/cheat_gui.cpp Outdated Show resolved Hide resolved
src/landscape.cpp Outdated Show resolved Hide resolved
src/landscape.cpp Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
src/lang/english.txt Outdated Show resolved Hide resolved
src/widgets/genworld_widget.h Outdated Show resolved Hide resolved
src/genworld_gui.cpp Show resolved Hide resolved
src/genworld_gui.cpp Show resolved Hide resolved
@DorpsGek DorpsGek had a problem deploying to preview-pr-8891 Mar 25, 2021 Failure
@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 25, 2021 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 25, 2021 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 25, 2021 Inactive
@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Mar 25, 2021

Variety distribution seems to break the calculation of snow percentage with a high sea level.

I can consistently break farm generation with 40% snow coverage, Medium variety distribution, Hilly terrain, and a High sea level. Tiles at height 1 don't show any snow, but the land area information tool says they are snow-covered land. (This is an OpenGFX quirk which I've seen before; original TTD graphics are snowier on these tiles).

@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Mar 25, 2021

Variety distribution seems to break the calculation of snow percentage with a high sea level.

I can consistently break farm generation with 40% snow coverage, Medium variety distribution, Hilly terrain, and a High sea level. Tiles at height 1 don't show any snow, but the land area information tool says they are snow-covered land. (This is an OpenGFX quirk which I've seen before; original TTD graphics are snowier on these tiles).

Has little to do with variety distribution, for a change :) Similar effects can be created by increasing the snow coverage to a higher percentage, for example.

Basically what you observe is the snowline that is on height level 2. This marks the land tiles on the lowest level (h=1) as "snow covered", which in terms means Farms cannot be generated. And OpenGFX doesn't visually show this, so it is a bit unclear to the user why these map settings don't generate farms.

The coverage algorithm currently brings the coverage to the percentage set to what you can see as snowy tiles. So not the h=1 tiles in this scenario. As otherwise it would always visually show a lot less snow than there really is. But this is a bug specific to OpenGFX; I checked zBase and abase, and they both do show this snowy tile ever so slightly (it is really subtle, but there).

Also, zBase renders this pretty poor:
image

The slopes just look ugly. abase has the same issue.

So, that means we have a few options. I can imagine we say:

  • we stop marking those tiles as snowy. That resolves both the visually uglyness, and brings OpenGFX on-par with the rest. Also solves the above mentioned issue.
  • we fix the coverage calculation to assumes this layer is snowy too; that will for sure result in bug-reports about snow not reaching N%, but this is because OpenGFX doesn't visually show it.

To me it sounds both are equally evil. So I need to consult a NewGRF guru here :D

@2TallTyler
Copy link
Member

@2TallTyler 2TallTyler commented Mar 25, 2021

snowline-2-ttd

For reference, this is what the original TTD base set shows with snow line 2.

When you say "stop marking those tiles as snowy," do you mean not drawing the snow sprites, or still drawing them but calling them grass?

Your second option, combined with fixing OpenGFX to actually draw snow on snowy tiles, sounds like the best solution in the long-term, but probably the most difficult in the short term. :(

Edit: The flat snowy tile which should have snow is the flat ground in the snow14 spritesheet. Perhaps it could be replaced with the snow24 flat tile sprite? (via an update to OpenGFX, of course)

@frosch123
Copy link
Member

@frosch123 frosch123 commented Mar 25, 2021

The snowline definition is fuzzy and different in various contexts.

  • Original graphics have hard snow-level transitions, which was improved years ago: https://www.tt-forums.net/viewtopic.php?t=34742 . Since then probably every snow NewGRF, including the baseset OpenGFX, has done the same.
  • None-landscape sprites (road, rail, depots, houses, ...) usually only have two sprites: no snow, full snow.
  • Industry building restrictions (arctic farm and arctic forest) add a threshold of 2 for "safely" above/below snowline.

So, in summary, the sprites are inconsistent, and have always been.

I considered gameplay effects more important than graphics, so I suggest to define the "real snowline" at the height, when arctic towns require food: Setting snow line to 50% should result in about 50% of towns requireing food (assuming a consistent distribution of towns over the map).

@mattkimber
Copy link
Contributor

@mattkimber mattkimber commented Mar 25, 2021

I'm not sure it's within the scope of this PR, but I notice what I think is a slight oddity with the subtropic generator.

Using these settings:

settings

Generating an arctic map (snow %: 40) gives me this heightmap:

arctic48

But in sub-tropic (sand: 40%), it's struggling to get near the same peak heights, and my terrain is much more "rounded" generally:

tropic48

The same happens to me in 1.10 so it's not a regression and I don't think it should be a blocker, but it is strange the sub-tropic map alone isn't getting near my target peak height.

@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Mar 25, 2021

I'm not sure it's within the scope of this PR, but I notice what I think is a slight oddity with the subtropic generator.

[Innocent whistling face] #7340

Not proposing gravedigging PR 7340 here btw 😄

@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Mar 25, 2021

I'm not sure it's within the scope of this PR, but I notice what I think is a slight oddity with the subtropic generator.

Can you make a separate issue out of this? Just so we don't forget to address it for 1.12 :) Cheers!

@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 25, 2021 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 25, 2021 Inactive
@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 25, 2021 Inactive
Copy link
Member

@LordAro LordAro left a comment

Perhaps the "100% doesn't actually give 100%" comment could be addressed by adding something like "shorelines are always without snow" to a tooltip?

src/lang/english.txt Outdated Show resolved Hide resolved
src/lang/english.txt Show resolved Hide resolved
src/saveload/saveload.h Outdated Show resolved Hide resolved
@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 26, 2021 Inactive
TrueBrain added 7 commits Mar 26, 2021
…es snow line height)

Setting the snow coverage (in % of the map) makes a lot more sense
to the human, while still allowing the niche player to set (by
finding the correct %) a snow line height they like. This makes for
easier defaults, as it decoupled terrain height from amount of snow.

Maps can never be 100% snow, as we do not have sprites for coastal
tiles.

Internally, this calculates the best snow line height to approach
this coverage as close as possible.
This is an indication value; the game tries to get as close as it
can, but due to the complex tropic rules, that is unlikely to be
exact.

In the end, it picks a height-level to base the desert/tropic
line on. This is strictly seen not needed, as we can convert any
tile to either. But it is the simplest way to get started with
this without redoing all related functions.
This setting influence the max heightlevel, and not as the name
suggests: the height of the generated map.

How ever you slice it, it is a very weird place to add this
setting, and it is better off being only in the settings menu.

Commits following this commit also make it more useful, so users
no longer have to care about it.
This better reflects what it is, and hopefully removes a bit of
the confusion people are having what this setting actually does.

Additionally, update the text on the setting to better inform
users what it is doing exactly, so they can make an educated
decision on how to change it.

Next commit will introduce an "auto" value, which should be the
new default. The rename has as added benefit that everyone will
start out on the "auto" value.
This opens up the true power of the TGP terrain generator, as it
is no longer constrainted by an arbitrary low map height limit,
especially for extreme terrain types.

In other words: on a 1kx1k map with "Alpinist" terrain type, the
map is now really hilly with default settings.

People can still manually limit the map height if they so wish,
and after the terrain generation the limit is stored in the
savegame as if the user set it.

Cheats still allow you to change this value.
It will add some slack to the map height limit if that was set
to auto.
At least, TGP will try to reach it. It heavily depends on the map
if it is reachable at all. But for sure it will do its atmost to
get there!
@DorpsGek DorpsGek temporarily deployed to preview-pr-8891 Mar 26, 2021 Inactive
TrueBrain added 2 commits Mar 26, 2021
This allows us to later on see what someone did, and makes sure
that "restart" command still knows how the game was created.
…r of this value

Before this commit, it scaled to map-height-limit. Recently this
could also be set to "auto", meaning players don't really know
or care about this value.

This also means that if a player exported a heightmap and wanted
to import it again, looking like the exact same map, he did not
know what value for "highest peak" to use.
Copy link
Member

@LordAro LordAro left a comment

Ship it before we get cold feet

@TrueBrain TrueBrain merged commit d0ed656 into OpenTTD:master Mar 26, 2021
12 checks passed
@TrueBrain TrueBrain deleted the fix-heights branch Mar 26, 2021
@andythenorth
Copy link
Contributor

@andythenorth andythenorth commented Mar 26, 2021

Ship it before we get cold feet

What % cold feet?

@TrueBrain TrueBrain added backported and removed backport requested labels Apr 1, 2021
@JGRennison
Copy link
Contributor

@JGRennison JGRennison commented Apr 10, 2021

This is a bit late, but it's also a bit small to make a whole new issue over.

The return value of CalculateDesertLine is also assigned to _settings_game.game_creation.snow_line_height. It doesn't look like this should be there, unless I am missing something?

static uint8 CalculateDesertLine()
{
	/* CalculateCoverageLine() runs from top to bottom, so we need to invert the coverage. */
	return _settings_game.game_creation.snow_line_height = CalculateCoverageLine(100 - _settings_game.game_creation.desert_coverage, 4);
}

@TrueBrain
Copy link
Member Author

@TrueBrain TrueBrain commented Apr 10, 2021

Nice catch And no, you are not missing something. I just copy/pasted too much, and didn't realise. And review failed to spot it too :P

Fix in #8989

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

Successfully merging this pull request may close these issues.

None yet