More height levels (in trunk since r27010)

[ChillCore]

... and "friends" ...

Including the friends that this patch introduces.
There are many functions not modified properly at the moment causing the glitches. Not only terrain is glitchy ... but vehicles also, either they are drawn too high or the terrain is drawn too low.

ic111, I am giving up for tonight but I am confident that everything will work correctly again soon.

[ic111]

Rubidium, thank you. Yes, I agree I should have done it step by step, and I will do so. The problem was rather realizing that there actually was a bigger refactoring

But one question: Because I use mq, I use a hg working copy, and not a svn working copy. Is there any way to tell hg to go to some particular svn version? I assume not, but what is then the best way to do that step-wise? I can think about fetching a svn working copy, applying the patch to it, and if I need to change something performing the change in the hg working copy, and applying the result again to the svn working copy and so on...

I also didn't take your patches, applied them and then modified trunk. I changed the behaviour of some functions to return the value without TILE_HEIGHT multiplication, so comparisons could be written down more easily.

Well, the idea is good I think, it just seems that I first underestimated the consequences for the patch...

After that I took the idea of changing the size of the z_pos variables and all functions I could find (by search and looking at the diffs of mhl). I did it this way as I didn't like all the casts to int in the diffs and finding those unneeded casts when starting from the patch would've been more difficult.

This kind of change was not the problem I think, as this actually results in me being able to remove parts of the patch.

But anyway, we will get it working again

[Alberth]

But one question: Because I use mq, I use a hg working copy, and not a svn working copy. Is there any way to tell hg to go to some particular svn version?

Yes there is. Say I want to go to r22351:

Code: Select all

$ hg qpop -a       # pop all from patch queue
$ hg log -k r22351     # Get the log data with 'r22351' in its description.
changeset:   17577:7911ca97c886
user:        smatz
date:        Tue Apr 19 18:20:34 2011 +0000
summary:     (svn r22351) -Revert (r17101): and reinstall r16546. Hash tables are much smaller now, so it shouldn't cause crash anymore
$ hg up -r 17577   # Copy the changeset number, you may want to add '-C'
$ hg qpush
# fix the problems until you can push everything cleanly, then go back to the first line for the next commit.

A user-interface like tortoisehg may be useful too, as it presents a simple list of descriptions, which is easy to read/select.

[ic111]

Seems that I made it working again

Attached is the new version. Feel free to test it.

[ChillCore]

Great news. Absolutely great news.

[ic111]

The next thing I will do is improving heightmap generation.

It is on the one hand side in a stable state regarding this patch, on the other hand side I am not content with the resulting maps. The problem IMHO is that it first sets heightlevel ignoring the rules of map geometry (i.e. max height difference between neighbor tiles), and then corrects the heightlevels to form a valid map. If, however, a steep mountain is near a valley, the valley will more or less dissapear, because the algorithm generates higher heightlevels than originally present in the heightmap. IMHO, the map should be built up from low heightlevels to high heightlevels. If a high mountain is just 50 instead of 80 heightlevels high, this will not alter the character of the map significantly, if instead of a valley of height 5, there is a mountain of height 30, the character of the map is actually altered. And tuning the max. heightlevel manually to avoid such effects would be a quite cumbersome process of trial and error.

So, I will adjust this algorithm. I plan to include the results into the patch set (replacing the current 170 patch), but the changes will stay local to heightmap generation. So, the rest of the patch except patch 170 should not be affected by this.

[Rubidium]

As far as I can see the current (trunk) algorithms go over the map from north to south lowering the tiles where needed and then from south to north in the same manner. So it seems to be only lowering the tiles.

[ic111]

Yes, it alters the tiles *after* it set them to the calculated height. The problem IMHO is that this can result in it setting a higher heightlevel than originally defined in the heightmap.

I have attached a heightmap of Central Europe I use as testcase. Start a patched game, and open that heightmap using Alpine settings. Then you will notice that the valleys in the Alps have almost completely disappeared below the mountains, simply because the stepness of the mountains in the heightmap can´t be represented in game.

Anyway, the algorithm I have in mind is quite straightforward, so I think we will see alternative results soon.

One technical question: I currently try to add this variable

Code: Select all

[SDT_VAR]
base	 = GameSettings
var      = game_creation.heightmap_max_heightlevel
type	 = SLE_UINT8
flags	 = SLF_NOT_IN_SAVE | SLF_NO_NETWORK_SYNC
def		 = 30
min		 = 1
max		 = 255

in settings.ini, just below heightmap_rotation (it will control the height scaling of the heightmap). My expectation was that I just have to call make, the corresponding files will be generated, and I can use it in the genworld_gui.cpp. However, it doesn´t know the variable:

Code: Select all

[SRC] Compiling genworld_gui.cpp
/home/wol/openttd/_trunk_mq/src/genworld_gui.cpp: In member function ‘virtual void GenerateLandscapeWindow::OnClick(Point, int, int)’:
/home/wol/openttd/_trunk_mq/src/genworld_gui.cpp:657:38: error: ‘struct GameCreationSettings’ has no member named ‘heightmap_max_heightlevel’
/home/wol/openttd/_trunk_mq/src/genworld_gui.cpp:658:49: error: ‘struct GameCreationSettings’ has no member named ‘heightmap_max_heightlevel’
/home/wol/openttd/_trunk_mq/src/genworld_gui.cpp:666:5: error: expected ‘;’ before ‘SetDParam’

(before changing genworld_gui.cpp parallel to the compile job, I got similar errors for settings.cpp / settings.h)
Where is my mistake? Did I miss some file I also have to change?

[Yexo]

You also need to add the new setting to settings_type.h. table/settings.ini is only for the saveload code.

[Eddi]

imho, you should decouple the maximum heightlevel from the "mountaineousness" of the map, i.e. don't add the "alpine" etc. options after "mountaneous", but instead add another dropdown "heightlevels" next to the map size (with options e.g. "16", "32", "64", "128", "256", possibly some intermediate values like "48" and "96").

also, there is an old "rounding error" that prevents the highest chosen heightlevel from actually being generated, creating a lot more flat mountain tops than necessary.

[ChillCore]

also, there is an old "rounding error" that prevents the highest chosen heightlevel from actually being generated, creating a lot more flat mountain tops than necessary.

You mentioned this before and I remember me posting a solution.
However it may have been lost in a mass of posts and even I do not even remember in which thread it was.
Anyway, here is the fix again.

Code: Select all

@@ -849,7 +1057,7 @@ static void HeightMapNormalize()
 {
 	int sea_level_setting = _settings_game.difficulty.quantity_sea_lakes;
 	const amplitude_t water_percent = sea_level_setting != (int)CUSTOM_SEA_LEVEL_NUMBER_DIFFICULTY ? _water_percent[sea_level_setting] : _settings_game.game_creation.custom_sea_level * 1024 / 100;
-	const height_t h_max_new = I2H(_max_height[_settings_game.difficulty.terrain_type]);
+	const height_t h_max_new = TGPGetMaxAllowedHeight() + 1;
 	const height_t roughness = 7 + 3 * _settings_game.game_creation.tgen_smoothness;
 
 	HeightMapAdjustWaterLevel(water_percent, h_max_new);

Note:
- Line numbers might not be correct and the code above is against clean trunk as it is extracted from my patchpack.



To have trunk generate 15 levels high maps instead of 14 (not counting water) and reduce flattened mountains tops, add +1 to the code like this.

Code: Select all

-	const height_t h_max_new = I2H(_max_height[_settings_game.difficulty.terrain_type]);
+	const height_t h_max_new = (I2H(_max_height[_settings_game.difficulty.terrain_type])) + 1;

Note:
- The above code might be a too dirty hack and maybe it needs to be fixed at another location (I have not tested as it works as is).

[Eddi]

I vaguely remember testing that and getting occasional crashes, i think in the histogram code. (out of bounds access)

[ic111]

imho, you should decouple the maximum heightlevel from the "mountaineousness" of the map, i.e. don't add the "alpine" etc. options after "mountaneous", but instead add another dropdown "heightlevels" next to the map size (with options e.g. "16", "32", "64", "128", "256", possibly some intermediate values like "48" and "96").

Indeed, this is the way the new variable above works. I will use it as a scaling factor, the values in the image are scaled to (max_heightlevel / 255) * image_value. Plus a little bit of extra logic to maintain the coastline (I will map all values of that fraction that are in the interval ]0,1] to heightlevel 1 to maintain coastline.

Beside this, I think the problem I described yesterday was not in the algorithm but in the test data. The generated valleys were so steep that they disappear after the mountains have been flattened for reasons of map geometry.

[ic111]

Version 18 of the patch. Contains a small change in 140_Core (adjustment for a recent trunk change) and the revised heightmap generation code. 170_Heightmaps is from my point of view finished also. Comments and feedback welcome

[frosch]

Since the GRF v8 changes have been committed, here some hint for updating: newgrf.cpp now uses MAX_TILE_HEIGHT in four places. These should all be replaced with a variable indicating the maximal allowed heightlevel, i.e. not necessary 255.
That is, like eddi says above, there always needs to be a fixed maximum heightlevel to define the Z scaling of the map, which is independent from other mapgen settings. This is needed to define "climate" based on the height, snow an similar. This is needed in general, and not only for heightmaps. (well, that is what I understood from the queue, when looking very briefly at it; might be all wrong )

[ic111]

Thanks for the hints!

I am not quite sure what consequences this has for the patch. You say, I always need a fixed maximum heightlevel to define the z scaling of the map. Should this be regarded as a hard limit for heightlevels (i.e. can´t terraform higher), or just as a calculation basis for certain aspects of the game? Currently, in any game the max. allowed heightlevel is MAX_ALLOWED_HEIGHTLEVEL.

Regarding z scaling: The terraforming algorithm already produces maps of different max. heightlevel, depending on its settings.

And you say, this is needed for defining climate based on height: Well, of course one can think about a parameter that influences this (in fact, the snow line height is such a parameter). But, aren´t further parameters beside the snow line height overcomplicate in terms of this patch? IMHO, its aim is making more heightlevels technically work, while preserving the ability to generate maps, import heightmaps, etc.

And furthermore, which aspects of the game (beside snow line) are interesting with respect to this? Or do you have the vision of introducing things like an alpine timberline?

Sorry for asking so much, but for me, the point behind this is not yet clear.

[ChillCore]

And furthermore, which aspects of the game (beside snow line) are interesting with respect to this? Or do you have the vision of introducing things like an alpine timberline?

The concept of a treeline is nice and it is a nice addition to the game but I would not include it in this patch, IMHO, neither should you worry about this (for now) as there is no such thing in trunk.

In the arctic climate there are industries that are only generated above the snowline (¿sawmills and forests?).
With a treeline there would be industries that should not be generated above it (again forests and sawmills come to mind).

When you combine the two you have a nice little mess to think about and take care of. eg. Treeline may not be below the (variable) snowline ¿but by how much to keep the game playable?

The same aplies to the tropical climate where the snowline is re-used for the desertline but I do not remember what the industries are there that have or not have restrictions based on it in regards of being generated.

[Eddi]

Should this be regarded as a hard limit for heightlevels (i.e. can´t terraform higher)

i'd vote yes on that. if the map was generated with a max level of e.g. 32, then the player should not be able to terraform beyond that.

[ChillCore]

I'd vote no on that, I am against limiting players freedom in any way (unless the choice causes buggy behaviour).

Ooh e-fight coming up ...

Seriously though having different opinions is good.
I guess we would both be happy if there were a separate (advanced setting) to set the max allowed height for terraforming ingame (set by the server in case of MP).
¿generated_height_of_map <-> custom?

[Eddi]

From a user's point of view, it's way better explainable that if X and Y spread cannot be changed, Z spread cannot be changed either. Even if that has totally different internal reasons.