News:

Use the "Forum Search"
It may help you to find anything in the forum ;).

Height map loader refactor

Started by ceeac, October 30, 2019, 06:37:51 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

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

Quote from: makie on October 30, 2019, 09:47:48 PMUnfortunately 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  8)

Quote from: ceeac on May 21, 2020, 12:38:25 PM
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.

Quote from: prissi on July 28, 2020, 05:44:59 AMFirst, 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.

Quote from: prissi on July 28, 2020, 05:44:59 AMTwo 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.

Quote from: prissi on July 28, 2020, 05:44:59 AMAlso 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.

Quote from: prissi on July 28, 2020, 05:44:59 AMThird, several clamps has MAX_MODE instead MAX_MODE-1 as maximum.
Fixed now.

Quote from: prissi on July 28, 2020, 05:44:59 AMworld_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

Quote from: prissi on October 23, 2020, 01:49:24 AMmaybe 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.