Topic: Height map loader refactor

[ceeac]

This patch improves the code responsible for loading height maps by hardening it against malformed/corrupted input and by doing some refactoring and cleanup.

[makie]

In simutrans it is possible to set the "word_minimum_height" and the "world_maximum_height"
With world_maximum_height=100 you can build really high mountains.

Unfortunately height_map_loader_t works only with a fixed and limited range.
No support for extended "world_maximum_height" therefore you can not load a height map for this

It should better calculate the conversion so that black is "word_minimum_height" or the sea floor
and white is "world_maximum_height" or the highest mountain.

look at :
https://forum.simutrans.com/index.php/topic,19099.msg180941.html#msg180941

[ceeac]

Unfortunately height_map_loader_t works only with a fixed and limited range.
No support for extended "world_maximum_height" therefore you can not load a height map for this

Now there is support for this. Currently there are four modes:

• legacy (small heights): Equivalent to the previous behaviour with "maximize height levels" off.
• legacy (large heights): Equivalent to the previous behaviour with "maximize height levels" on.
• linear: The default mode. Linear interpolation of height values between minimum (black) and maximum (white) height.
• clamp: With this mode, 1 greyscale level = 1 height level. "Cuts off" mountains/valleys to between minimum and maximum height.

I am not convinced whether these names are the best so I'll leave that open for discussion. This is also the reason I have not yet added the strings to base.tab for translation.

[makie]

Oh this look fine. 
Need 

I am not convinced whether these names are the best so I'll leave that open for discussion. This is also the reason I have not yet added the strings to base.tab for translation.

Don't worry about best names. This can easy changed via translation.
The strings in the program should rather be seen as key into the table en.tab.

[ceeac]

Okay, I added the strings to base.tab and fixed an error which allowed heights to be larger than world_max_height when loading height maps in legacy mode.

[ceeac]

Updated patch to latest trunk. No functional changes.

[prissi]

I had some time to look at this. While I like the idea, some things need to be imporved.

First, which is the ENDIAN stuff gone? The heighmap loader most work on big endian ARM and PowerPC, since Simutrans can be compiled on those architektures as well. (There was/is an Amiga Simutrans community.)

Then all those cecks for fseek are somewhat superflous. Two check "fseek( file, 48, SEEK_SET )==48" and "fseek( file, data_offset, SEEK_SET ) != data_offset"would be equivalent and would generate much distraction. Also BMP files can end with incomplete lines, I have seen all kind of slightly off files out there.

Third, several clamps has MAX_MODE instead MAX_MODE-1 as maximum. This cannot lead to a valid parameter.

world_mniimum_height <= world_maximum_height should be tested after reading simuconf.tab and not be removed. Which is world_maximum_height enforced>=16 and world_miniimum_height<=-12. Especially the latter.

[ceeac]

Finally updated the patch to latest trunk.

First, which is the ENDIAN stuff gone?

It's not gone - see the start of read_bmp(). endian() is a no-op for little endian systems so there is no point in having separate code for big and little endian systems wrapped in an #ifdef SIM_BIG_ENDIAN.

Two check "fseek( file, 48, SEEK_SET )==48" and "fseek( file, data_offset, SEEK_SET ) != data_offset"would be equivalent and would generate much distraction.

The image data is not required to start at offset 48 so fseek( file, 48, SEEK_SET ) is wrong. See the Wikipeda page on the BMP file format for details.

Also BMP files can end with incomplete lines, I have seen all kind of slightly off files out there.

Changed the code to allow missing padding at the end of the last row.

Third, several clamps has MAX_MODE instead MAX_MODE-1 as maximum.

Fixed now.

world_mniimum_height <= world_maximum_height should be tested after reading simuconf.tab and not be removed. Which is world_maximum_height enforced>=16 and world_miniimum_height<=-12. Especially the latter.

world_minimum_height <= -12 and world_maximum_height >= 16 are enforced on load time so there is no need to check that world_minimum_height <= world_maximum_height. Or do you want to allow changing world_minimum_height/world_maximum_height freely?

[ceeac]

Updated the patch to latest trunk and replaced some C++11 specific code. I can commit the patch as-is by the end of the week if nobody has any objections.

[prissi]

Since the coming climate maps will also need to read bitmap files, maybe it would be a better idea to separate bitmap loading from heighmap calculation, if one is in for renovation. So one new file "utils/dr_readbmp_file.cc" (which also reads PPM into the same block structure as dr_readpng.cc), and just the bitmap to height code in the original location. In a second cleanup step, the BMP writing code from screenshots could go in there as well ...

That could then also easily add PNG support via libpng and finally enables the patch to have tags in simutrans html flowtext.

[ceeac]

maybe it would be a better idea to separate bitmap loading from heighmap calculation

I support the idea. However, imo this should be be done separately after the current patch is incorporated since the current patch is not only a refactor but also fixes some bugs with the 24 bit BMP importer, and also to prevent scope creep.

[prissi]

I understand. Still hope to have a bmp loader soon, then adding climate maps would be a fast thing. (This could even add rivers and lakes with them easily, a long standing request. But I get offtopic.)

[ceeac]

Okay, incorporated the patch in r9354, including an additional fix for non-divisible-by-4-width BMPs.