Support heightmaps on Ogre 2

[chapulina]

Terrain support is being added for Ogre 1 on #180 . The implementation for Ogre 2 is being left empty for now.

Here's a tutorial using terrain on Ogre 2 upstream:

https://github.com/OGRECave/ogre-next/tree/v2-1/Samples/2.0/Tutorials/Tutorial_Terrain

[iche033]

the terrain component is not enabled by default when building ogre2 so I believe our ogre-2.1 debs do not ship the terrain component. Last I tried, I ran in to a build error compiling the ogre2 terrain component on ubuntu bionic but the ogre2 dev says it should be a finished product.

[darksylinc]

We talked this via video conference and email, but just in case it wasn't clear or the information got lost:

The sample at Samples/2.0/Tutorials/Tutorial_Terrain implements the terrain system entirely (aka Terra) in a few C++ and shader files and does not depend on the Terrain Component.

The terrain component is currently abandoned.

The reason for this being is that Ogre 2.1+ targets newer rendering APIs which allow for much simpler yet more powerful terrain systems; for example Terra does not even have vertex buffers. The vertices are made procedural and the only memory it needs (excluding colouring/shading) is the heightmap.

This was impossible in older APIs, which led to much higher complexity in the Terrain Component where a lot of work went into managing vertex buffers of the current pages and LODs.

The reason Terra was implemented as a self-contained sample and not a Component is that requirements for Terrain are very specific to each project, and users are very likely to customize it for their own needs. Given the simplicity of the code, it made more sense to do it as a sample exposing the source files directly; rather than adding a new library

[iche033]

The sample at Samples/2.0/Tutorials/Tutorial_Terrain implements the terrain system entirely (aka Terra) in a few C++ and shader files and does not depend on the Terrain Component.

ah thanks for clarifying this. In that case, we'll need to copy this Terra directory and relevant shader files over to ignition?

[darksylinc]

Yep. That is correct.

[chapulina]

This is great to know! So we can add heightmap support to earlier versions of Ignition as well :smile:

[darksylinc]

I'm tackling this issue and I have a few questions, based on my analysis of the ogre1 version:

1. ImageHeightmap is in 8 bpp! (the only implementation I could find for HeightmapData) This is rarely enough unless the size_in_km / resolution ratio is really big. Otherwise you get terrains with mountains that look like staircases. Usually 16bpp (R16_UNORM) is enough. I guess users can plug their own custom version of HeightmapData? Is that it?
2. Ogre1 contains minElevation because the old Terrain component had a bug if the terrain's height data had negative heights; so the terrain is offsetted as an object instead.
   • Ogre2's Terra needs a similar workaround because it was written assuming most heightmap sources are UNORM (it supports floats), therefore no height value in the height data is below 0
   • However minElevation is always set to 0!
   • I'm also confused because in point 1 it's clear you don't use anything that'd give negative heights since ImageHeightmap is in 8 bpp (aka R8_UNORM)
3. HeightmapData::FillHeightMap has _subSampling to increase the resolution. What's the rationale behind it?
4. OgreHeightmap split the terrain into multiple smaller terrains if the resolution was higher than 4096x4096 due to some LOD bug apparently. Terra has no such problem; however all GPUs have a hard cap on max texture size of 16384x16384 (which in VRAM would consume 1GB for heightmap + 1GB for normal data + 1GB for shadow data) is that a problem? IMHO at such large sizes, dynamic streaming (e.g. "infinite terrain") at 8192x8192 makes more sense.

Remarks

Now I understand @iche033 insistence on questions about LODs and paging. It seems Ogre1 did a lot of work to cache:

1. Vertex geometry
2. LODs
3. Baked textures

Terra has no such things. Everything that was baked either no longer exists (Terra has no vertex geometry, and without it, LODs are trivial as well) or is calculated in realtime.

This is like a hover car using antigravity devices and asking what happens when you get a flat tire: This is a hover car, it has no tires :smile:

Integration

The way I set it up was to copy-paste the Terra files into your own projects, and modify what you need (although for upgrade-ability sake, keep them to a minimum or submit contributions to upstream)

But I noticed the coding standards and naming conventions (i.e. including project folder structure, CamelCase, etc) are too different from Ignition. I think it may be best if we build a small library and ign-rendering links to it.

However where would that library live? as part of https://github.com/ignitionrobotics like sdformat? as part of ign-rendering/terra?

Rapidjson

It seems Ogre so far has been compiled without JSON support. Terra needs it.

Fortunately rapidjson is a header-only API so it's all about setting up include_directories in CMake, but Ignition has a specific way of doing things with CMake scripts that I prefer someone else performs (right now I'm hacking it)

Minor challenges

• Terra assumes it's rendered directly to screen by the main camera. This is a minor inconvenience where we have to update each Terra instance for each different camera (e.g. for each sensor) that will render it.
• Terra is Y up. I've modified Terra to be Z up in the past. I'll try to think of ways to make this tweakable from upstream out of the box. This is annoying more than anything (because subtle bugs are hard to spot and may be hidden for a while).
• Terra needs heightmap data to be power of 2 (e.g. 512x512), while old Terrain needs it to be power of 2 + 1 (e.g. 513x513)

Progress

Changes started in https://github.com/darksylinc/ign-rendering/tree/matias-ogre22-terra

[chapulina]

ImageHeightmap is in 8 bpp!

We could update that class to also accept 16bpp.

I guess users can plug their own custom version of HeightmapData?

An alternative way to get heightmaps in is through the Dem class (https://github.com/ignitionrobotics/ign-gazebo/issues/235). I didn't add support to that while porting the Ogre 1 code for expediency, but we should eventually support it.

HeightmapData::FillHeightMap has _subSampling to increase the resolution. What's the rationale behind it?

I'm not completely sure, but it may be important for physics, which also uses that class to import heightmaps.

This is a hover car, it has no tires smile

:rocket:

naming conventions ... are too different from Ignition.

We can include it in the codebase as "vendored" code, which we usually don't run linters on (like GTest, Remotery, etc).

However where would that library live? as part of https://github.com/ignitionrobotics like sdformat? as part of ign-rendering/terra?

I think it can be part of ign-rendering. We don't need to expose it to downstream users if it will only be used internally.

I think another potential place for it could be our Ogre 2 fork?

It seems Ogre so far has been compiled without JSON support. Terra needs it.

I don't think Ogre uses ign-cmake, but maybe FindJSONCPP.cmake helps?

Terra needs heightmap data to be power of 2 (e.g. 512x512), while old Terrain needs it to be power of 2 + 1 (e.g. 513x513)

Maybe all we need is to update some documentation? Like this one: https://github.com/ignitionrobotics/ign-common/blob/465bb7780e552b278de559fafaffe3d061a99782/graphics/include/ignition/common/Dem.hh#L74-L81

[iche033]

I'm also confused because in point 1 it's clear you don't use anything that'd give negative heights since ImageHeightmap is in 8 bpp (aka R8_UNORM)

We mainly use ImageHeightmap to create simple terrains. For more accurate terrain simulation, we would import DEMs from online GIS sites. Sometimes we find that the DEM data contain negative heights.

HeightmapData::FillHeightMap has _subSampling to increase the resolution. What's the rationale behind it?

I'm not completely sure, but it may be important for physics, which also uses that class to import heightmaps.

We also had a heightmap editor once in gazebo-classic and increasing the resolution allowed us to make finer modification to the terrain

OgreHeightmap split the terrain into multiple smaller terrains if the resolution was higher than 4096x4096 due to some LOD bug apparently. Terra has no such problem; however all GPUs have a hard cap on max texture size of 16384x16384 (which in VRAM would consume 1GB for heightmap + 1GB for normal data + 1GB for shadow data) is that a problem? IMHO at such large sizes, dynamic streaming (e.g. "infinite terrain") at 8192x8192 makes more sense.

We've used 16K resolution before on more beefy machines with ogre 1.x. So it should be fine. For that particular project the terrain size is actually not very large, 300x300m, it just needed really high fidelity for physics simulation, and we used ogre 1.x's LOD so we can get reasonable framerate. That's one extreme case. I think dynamic streaming would be beneficial for more projects actually but we should still keep the ability for users to use 16K if they choose to.

I think it can be part of ign-rendering. We don't need to expose it to downstream users if it will only be used internally.

yeah let's put it in ign-rendering.

It seems Ogre so far has been compiled without JSON support. Terra needs it.

I don't think Ogre uses ign-cmake, but maybe FindJSONCPP.cmake helps?

ok so we'll need to add rapidjson dependency to ign-rendering, e.g. through a new cmake module in ign-cmake like FindRapidJSON.cmake. How about when packaging and releasing ogre 2.x debs, do we need to build it with rapidjson?

Terra assumes it's rendered directly to screen by the main camera. This is a minor inconvenience where we have to update each Terra instance for each different camera (e.g. for each sensor) that will render it.

ah ok, sounds like there'll be some performance hit here.

Terra is Y up. I've modified Terra to be Z up in the past. I'll try to think of ways to make this tweakable from upstream out of the box. This is annoying more than anything (because subtle bugs are hard to spot and may be hidden for a while).

:+1:

[darksylinc]

An alternative way to get heightmaps in is through the Dem class (ignitionrobotics/ign-gazebo#235). I didn't add support to that while porting the Ogre 1 code for expediency, but we should eventually support it.

Gotcha. That's all I needed to know.

However where would that library live? as part of https://github.com/ignitionrobotics like sdformat? as part of ign-rendering/terra?

I think it can be part of ign-rendering. We don't need to expose it to downstream users if it will only be used internally.

I think another potential place for it could be our Ogre 2 fork?

Ideally the changes in the fork get to upstream to reduce the need for a fork at all :)

Terra needs heightmap data to be power of 2 (e.g. 512x512), while old Terrain needs it to be power of 2 + 1 (e.g. 513x513)

Maybe all we need is to update some documentation? Like this one: https://github.com/ignitionrobotics/ign-common/blob/465bb7780e552b278de559fafaffe3d061a99782/graphics/include/ignition/common/Dem.hh#L74-L81

We'll see what we can do. In theory we could just ignore the last row and column. Or perhaps that introduces problems. Once we get there we'll evaluate. I was pointing it out.

We've used 16K resolution before on more beefy machines with ogre 1.x. So it should be fine. For that particular project the terrain size is actually not very large, 300x300m, it just needed really high fidelity for physics simulation, and we used ogre 1.x's LOD so we can get reasonable framerate. That's one extreme case. I think dynamic streaming would be beneficial for more projects actually but we should still keep the ability for users to use 16K if they choose to.

1.83 cm per pixel, impressive.

Yeah Terra can do that.

ah ok, sounds like there'll be some performance hit here.

Nah, no perf hit. I was just pointing it out; it is a minor inconvenience because all the rendering cameras (e.g. Ogre2GpuRays, Thermal, etc) need to call a function that iterates through all Terras and updates them with the current Camera.

Might also be fixed with a Workspace listener. I'm undecided on the best approach yet.

[darksylinc]

Some progress to get everyone excited.

Alignment / size is not exact to Ogre1 yet. Left is Ogre2, Right is Ogre1

'heightmap_bowl.png' is an edge case scenario for Terra (extremely artificial, non-natural terrain; looking the terrain from outside rather than the inside; and being looked from the outside without having more terrain in its surroundings), which is why the skirts and LODs are so obvious;

[darksylinc]

I'm close to finishing it and it looks they way I'd expect to given the input parameters (btw we support much more to make it more beautiful...) but I don't understand why it looks so different from reference in ogre1 (actually reference looks weird...)

What's missing:

• [x] Apply same blending mode to roughness and metalness params as diffuse
• [x] Fix edge case of skirts which looks really bad but easy to fix
• [ ] Non-deterministic bug where the last N pixels of the heightmap are garbage. Looks like uninitialized variable but I don't know where it's coming from

Regarding the last one (the non-deterministic bug) some runs it looks like this:

Edit:

Oh I understand why it looks so different from reference now. HeightmapTexture::Size is in meters. I thought it was a scale parameter in UV space.

[darksylinc]

I managed to look it like reference:

The only weird thing is that in order to do that, I had to disable a feature. It appears ogre1 is ignoring the alpha channel of the source textures; while ogre2 was using them.

I'm not sure what's the intended scenario but considering the source textures do have alpha channel, it looks it the idea was for it be used.

[darksylinc]

Progress with the non-deterministic bug: It's not mine. It's gazebo's.

Sometimes ImageHeightmap::FillHeightMap will return garbage in the last 3 pixels of the row 0.

I've printed the last 10 pixels of row 0; it should be all 0s: 0.000000, 0.000000, 0.000000, 0.000000, 0.000000, 0.000000, 0.000000, 0.000000, 0.533333, 0.988235,

I suspect two possibilities:

• ImageHeightmap::FillHeightMap was never tested against against a power of 2 output (it's always been pow of 2 + 1)
• Disk access race condition: I've seen something weird: I replace city_terrain.jpg (and heightmap_bowl.png) with my pow-2 version, but after a single run, sometimes it's randomly been overwritten back to its pow of 2 + 1 (WTF?). Likewise sometimes when I restore it back to its pow2+1 version; it randomly reverts back to pow2 version (WTFx2). Is there some write access code that could cause it?

Update: Disk access cannot be (the weirdness is there; but that's not the bug). I've added immutable flag on the file sudo chattr +i city_terrain.jpg but the bug still appeared

Update 2: Sigh

==10550==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x631000150da0 at pc 0x7f6319289979 bp 0x7ffe2c73a280 sp 0x7ffe2c73a270
READ of size 1 at 0x631000150da0 thread T0
    #0 0x7f6319289978 in std::enable_if<std::__and_<std::__not_<std::__is_tuple_like<unsigned char> >, std::is_move_constructible<unsigned char>, std::is_move_assignable<unsigned char> >::value, void>::type std::swap<unsigned char>(unsigned char&, unsigned char&) /usr/include/c++/8/bits/move.h:194
    #1 0x7f6319288266 in ignition::common::Image::Implementation::SwapRedBlue(unsigned int const&, unsigned int const&) /home/matias/Projects/ign/src/ign-common/graphics/src/Image.cc:567
    #2 0x7f6319285e69 in ignition::common::Image::Data(unsigned char**, unsigned int&) /home/matias/Projects/ign/src/ign-common/graphics/src/Image.cc:256
    #3 0x7f631928c16b in ignition::common::ImageHeightmap::FillHeightMap(int, unsigned int, ignition::math::v6::Vector3<double> const&, ignition::math::v6::Vector3<double> const&, bool, std::vector<float, std::allocator<float> >&) /home/matias/Projects/ign/src/ign-common/graphics/src/ImageHeightmap.cc:62
    #4 0x7f630f06f486 in ignition::rendering::v6::Ogre2Heightmap::Init() /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2Heightmap.cc:116
    #5 0x7f630f0f9aa5 in ignition::rendering::v6::Ogre2Scene::InitObject(std::shared_ptr<ignition::rendering::v6::Ogre2Object>, unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2Scene.cc:977
    #6 0x7f630f0f8e17 in ignition::rendering::v6::Ogre2Scene::CreateHeightmapImpl(unsigned int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, ignition::rendering::v6::HeightmapDescriptor const&) /home/matias/Projects/ign/src/ign-rendering/ogre2/src/Ogre2Scene.cc:873
    #7 0x7f631b12a5ec in ignition::rendering::v6::BaseScene::CreateHeightmap(ignition::rendering::v6::HeightmapDescriptor const&) /home/matias/Projects/ign/src/ign-rendering/src/base/BaseScene.cc:1090
    #8 0x55740c646cfe in buildScene(std::shared_ptr<ignition::rendering::v6::Scene>) /home/matias/Projects/ign/src/ign-rendering/examples/heightmap/Main.cc:144
    #9 0x55740c647cde in createCamera(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) /home/matias/Projects/ign/src/ign-rendering/examples/heightmap/Main.cc:194
    #10 0x55740c647fd2 in main /home/matias/Projects/ign/src/ign-rendering/examples/heightmap/Main.cc:217
    #11 0x7f6317da0bf6 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21bf6)
    #12 0x55740c645e29 in _start (/home/matias/Projects/ign/src/ign-rendering/examples/heightmap/build/Debug/heightmap+0x45e29)

0x631000150da0 is located 0 bytes to the right of 66976-byte region [0x631000140800,0x631000150da0)
allocated by thread T0 here:
    #0 0x7f631a09df90 in __interceptor_malloc (/usr/lib/x86_64-linux-gnu/libasan.so.5+0xedf90)
    #1 0x7f631617a422  (/usr/lib/x86_64-linux-gnu/libfreeimage.so.3+0x1c422)

SUMMARY: AddressSanitizer: heap-buffer-overflow /usr/include/c++/8/bits/move.h:194 in std::enable_if<std::__and_<std::__not_<std::__is_tuple_like<unsigned char> >, std::is_move_constructible<unsigned char>, std::is_move_assignable<unsigned char> >::value, void>::type std::swap<unsigned char>(unsigned char&, unsigned char&)
Shadow bytes around the buggy address:
  0x0c6280022160: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c6280022170: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c6280022180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c6280022190: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c62800221a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c62800221b0: 00 00 00 00[fa]fa fa fa fa fa fa fa fa fa fa fa
  0x0c62800221c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c62800221d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c62800221e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c62800221f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c6280022200: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==10550==ABORTING

Update 3: Image::Data not only corrupts, it also leaks. Update 4: It corrupts because it's trying to swap a greyscale image. There's nothing to swap.

[darksylinc]

OK to recap what I'm now missing

• [x] File PR on ignition/common about the Heightmap corruption and leak (Done: https://github.com/ignitionrobotics/ign-common/pull/240)
• [x] Ask whether we should honor alpha channel's content
• [x] Integrate PbsListener so other objects can receive shadows
• [ ] Ask if this CMake script is ok

[iche033]

Ask whether we should honor alpha channel's content

chances are that we just didn't use the alpha channel with ogre 1.x. We copied shader material generation code from ogre 1.x and tweaked it to fix some issues with glsl shaders some time ago. I haven't paid much attention to the blending logic but a quick look seems to suggest that the alpha channel is used to store specular map? https://github.com/ignitionrobotics/ign-rendering/blob/ign-rendering5/ogre/src/OgreHeightmap.cc#L2275-L2291

The latest ogre 1.x doc also talks about merging the diffuse and spec textures into one texture: https://ogrecave.github.io/ogre/api/latest/tut__terrain_sky_fog.html

I think we should respect the alpha channel in ogre 2.x implementation.

Ask if this CMake script is ok

At first glance, I think it may be better to have the terra folder live inside ogre2/src directory as it's very specific to ogre 2. It's fine to not use the ignition-cmake macros to build it for now. As an example, we put an external project, Remotery, in the ign-common/profiler/src directory.

@mjcarroll, do you have any recommendation on the best way to include some ogre2-related external source files (so we can also make windows and bazel happy)?

[iche033]

Update 4: It corrupts because it's trying to swap a greyscale image. There's nothing to swap.

ahh.. :( looks like there needs to be a check for image format before trying to swap..

[darksylinc]

Yes. But please note it's also leaking because a copy /clone of the freeimage handle is made which is never freed (the handle is lost)

[chapulina]

Done in #386 :tada: