Support 16 bit heightmaps

[vatanaksoytezer]

🦟 Bug fix

Fixes #265

Summary

Assuming PixelFormat function returns a correct value this should allow supporting 8, 16 and 32 bit heightmaps. Having said that I tried to test this with a hand constructed 16bit version of city_terrain.jpg (just multiplied each value by 256 didn't want to spend so much time on that), the PixelFormat function is still returning L_INT8 (which is the same as the 8 bit version) causing the spikes. This needs to be resolved as well. I'll try to have a deeper look on why we aren't getting correct return from this function. That was my bad that I left the extension of 16 bit image as .jpg (should have been png), it's now throwing a very suppressive error messages starting with [Err] [Image.cc:478] Image: Coordinates out of range [0 0] and going through all the pixels.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

Note to maintainers: Remember to use Squash-Merge

[vatanaksoytezer]

Tagging @iche033 for advices.

[codecov]

Codecov Report

Merging #266 (a272643) into ign-common4 (c0f8521) will decrease coverage by 0.09%.
The diff coverage is 61.40%.

@@               Coverage Diff               @@
##           ign-common4     #266      +/-   ##
===============================================
- Coverage        77.11%   77.01%   -0.10%     
===============================================
  Files               75       76       +1     
  Lines            10696    10725      +29     
===============================================
+ Hits              8248     8260      +12     
- Misses            2448     2465      +17     

Impacted Files	Coverage Δ
graphics/src/ImageHeightmap.cc	55.55% <14.28%> (-32.91%)	⬇️
graphics/include/ignition/common/ImageHeightmap.hh	88.88% <88.88%> (ø)
graphics/src/Image.cc	69.14% <0.00%> (+0.39%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0f8521...a272643. Read the comment docs.

[vatanaksoytezer]

Upon further investigation with more than 8 bits (apart from the fixes in this PR) the problem is stemming from inside maxColor function in Image.cc FreeImage_GetColorTypehttps://github.com/ignitionrobotics/ign-common/blob/ign-common4/graphics/src/Image.cc#L438 is returning FIC_RGB which causes all sorts of problems on out of range. Although the FreeImage_GetImageType is correctly returning FIT_UINT16 now, color seems wrong. I have a "not so nice" idea of fixing it by overloading a heightmap argument to MaxColor which would assume it's grayscale. I am open to any other ideas, will send update to this PR.

[vatanaksoytezer]

Another update: Freeimage seems to have no way of reading 16-bit int values from 16-bit images. From the documentation it seems there is FreeImage_GetPixelIndex for 1-4-8 bit imahes (which takes a byte as argument) and FreeImage_GetPixelColor for 16-24-32 bit images which takes an RGBQUADas argument.RGBQUAD` is a combination of 3 bytes, which in this case won't work again.

Here is the definition of RGBQUAD:

typedef struct tagRGBQUAD {
#if FREEIMAGE_COLORORDER == FREEIMAGE_COLORORDER_BGR
  BYTE rgbBlue;
  BYTE rgbGreen;
  BYTE rgbRed;
#else
  BYTE rgbRed;
  BYTE rgbGreen;
  BYTE rgbBlue;
#endif // FREEIMAGE_COLORORDER
  BYTE rgbReserved;
} RGBQUAD;

I can't seem to find a way of using freeimage to access 16-bit greyscale values, we can of course utilize another library for that though.

Tagging @iche033, @chapulina and @nkoenig for any insights you might have.

[vatanaksoytezer]

Upon more playing the following bit seems to be able to read the values in 16 bit integers, it's raw access but still doable:

    for(y = 0; y < this->Height(); y++) {
      short *bits = (short *)FreeImage_GetScanLine(this->dataPtr->bitmap, y);
      for(x = 0; x < this->Width(); x++) {
        ignmsg << bits[x] << " ";
      }
      ignmsg << std::endl;
    }

[mjcarroll]

Upon more playing the following bit seems to be able to read the values in 16 bit integers, it's raw access but still doable:

    for(y = 0; y < this->Height(); y++) {
      short *bits = (short *)FreeImage_GetScanLine(this->dataPtr->bitmap, y);
      for(x = 0; x < this->Width(); x++) {
        ignmsg << bits[x] << " ";
      }
      ignmsg << std::endl;
    }

This looks like the correct approach based on the documentation. I don't think there is any way around the raw access with the way the API is set up.

[vatanaksoytezer]

I found the bigger bottleneck is implementing Data and DataImpl functions on: https://github.com/ignitionrobotics/ign-common/blob/ign-common4/graphics/src/Image.cc#L284 those functions are originally written for unsigned char utilizing FreeImage_ConvertToRawBits which is also unavailable for 16 bits and 16 bit integers. I have implemented these using raw access methods, but currently trying to figure out some segfaults in my implementation I can push if anyone wants to have a look.

[iche033]

I just tried your changes as they are right now and I think it's working with just another minor change. In the code below, it's indexing into data which is of type unsigned char []. For 16 bit image, you can reintepret cast this
https://github.com/ignitionrobotics/ign-common/blob/cbcfc96f061e1d5a8220b5111aeb33440fa6842f/graphics/src/ImageHeightmap.cc#L123-L133

I made the following quick hack for testing and that was sufficient to load a 16bit grayscale png heightmap for me:

diff --git a/graphics/src/ImageHeightmap.cc b/graphics/src/ImageHeightmap.cc
index fec86b5..9410355 100644
--- a/graphics/src/ImageHeightmap.cc
+++ b/graphics/src/ImageHeightmap.cc
@@ -100,6 +100,8 @@ void ImageHeightmap::FillHeightMap(int _subSampling,
     return;
   }
 
+  uint16_t *dataShort = reinterpret_cast<uint16_t *>(data);
+
   // Iterate over all the vertices
   for (unsigned int y = 0; y < _vertSize; ++y)
   {
@@ -121,15 +123,19 @@ void ImageHeightmap::FillHeightMap(int _subSampling,
       double dx = xf - x1;
 
       double px1 = static_cast<int>(
-        data[y1 * pitch + x1 * bpp]) / maxPixelValue;
+        // dataShort[y1 * pitch + x1 * bpp]) / maxPixelValue;
+        dataShort[y1 * pitch/bpp + x1 ]) / maxPixelValue;
       double px2 = static_cast<int>(
-        data[y1 * pitch + x2 * bpp]) / maxPixelValue;
+        // dataShort[y1 * pitch + x2 * bpp]) / maxPixelValue;
+        dataShort[y1 * pitch/bpp + x2]) / maxPixelValue;
       float h1 = (px1 - ((px1 - px2) * dx));
 
       double px3 = static_cast<int>(
-        data[y2 * pitch + x1 * bpp]) / maxPixelValue;
+        // dataShort[y2 * pitch + x1 * bpp]) / maxPixelValue;
+        dataShort[y2 * pitch/bpp + x1]) / maxPixelValue;
       double px4 = static_cast<int>(
-        data[y2 * pitch + x2 * bpp]) / maxPixelValue;
+        // dataShort[y2 * pitch + x2 * bpp]) / maxPixelValue;
+        dataShort[y2 * pitch/bpp + x2]) / maxPixelValue;
       float h2 = (px3 - ((px3 - px4) * dx));
 
       float h = (h1 - ((h1 - h2) * dy)) * _scale.Z();

[vatanaksoytezer]

I made the following quick hack for testing and that was sufficient to load a 16bit grayscale png heightmap for me:

@iche033 thanks for having a look. That didn't work for me though, may you be using cache in this test? (I always make sure I remove the cache with: rm -rf /home/tezer/.ignition/rendering/ogre-paging/example_city/) I am sprayed with Coordinates out of range errors in MaxColor() because the image in registered as FIC_RGB if I use the PR as is + your changes. Also reinterpreting from 8 bit is OK, but aren't we losing accuracy here? Now it's not really 16 bit but 8-bit image represented as 16-bit. If we are OK with that I would prefer to just change the image to 8-bit in the start of FillHeightmap and leave the other functions as is.

[iche033]

That didn't work for me though, may you be using cache in this test?

oh I see why we are getting different results. I'm testing with ogre2 render engine with ign-rendering6. Can you try testing with ign-rendering6 and running the heightmap demo? It should use ogre2 by default.

I do get the same error as you when testing with ogre 1.x. There could be a problem somewhere else when launching with ogre 1.x.

Also reinterpreting from 8 bit is OK, but aren't we losing accuracy here? Now it's not really 16 bit but 8-bit image represented as 16-bit.

My understanding is that the raw data we get back from freeimage (done in Image::Implementation::DataImpl) is 16 bit data but we're just incorrectly storing it in unsigned char array. By doing a reinterpret_cast, and changing the way we are indexing and reading this data from memory - we are now reading 2 bytes at a time and casting that as a uint16_t value, instead of reading 1 byte of unsigned char data.

[iche033]

Here's the png file I'm testing with.

$ file 16bit_grayscale.png 
16bit_grayscale.png: PNG image data, 2048 x 2048, 16-bit grayscale, non-interlaced

[vatanaksoytezer]

oh I see why we are getting different results. I'm testing with ogre2 render engine with ign-rendering6. Can you try testing with ign-rendering6 and running the heightmap demo? It should use ogre2 by default.

Sorry for the delays, it worked with ign-rendering6 and I just pushed a commit with that version. Though common-4 is both used by ign-rendering5 (in Edifice) and ign-rendering6 (in Fortress). Would it be a problem to support just one of them?

[vatanaksoytezer]

@iche033 I think it should be all good now with a nicer implementation, and ready to merge. For some reason the heightmap test was failing with 1 pixel off, I am not sure why it does, but might not be a major issue (last commit should fix that). Appreciate if you can drop a last review.

[ColeOSRF]

I'll let @iche033 take a closer look but it works well for me!

[iche033]

nice, thanks for cleaning up the code and making a generic function.

For some reason the heightmap test was failing with 1 pixel off

The quick workaround I did just made it work for single channel 16 bit grayscale image. Looks like the test was using 3 channel RGB image so that caused it to fail. I've made some changes in the patch below to make it work for both grayscale and rgb images. I also made some other ignition coding / naming style changes:

• replaced get prefix from function name (getHeights -> FillHeights)
• fixed indentation
• added doxygen comments
• add _ prefix to function arg names

patch

diff --git a/graphics/include/ignition/common/ImageHeightmap.hh b/graphics/include/ignition/common/ImageHeightmap.hh
index 4c48bee..44f3bd0 100644
--- a/graphics/include/ignition/common/ImageHeightmap.hh
+++ b/graphics/include/ignition/common/ImageHeightmap.hh
@@ -65,15 +65,32 @@ namespace ignition
       private: ignition::common::Image img;
 
       /// \brief Get Heightmap heights given the image
+      /// \param[in] _data Image data
+      /// \param[in] _pitch Size of a row of image pixels in bytes
+      /// \param[in] _subSampling Subsampling factor
+      /// \param[in] _vertSize Number of points per row.
+      /// \param[in] _size Real dimmensions of the terrain.
+      /// \param[in] _scale Vector3 used to scale the height.
+      /// \param[in] _flipY If true, it inverts the order in which the vector
+      /// is filled.
+      /// \param[out] _heights Vector containing the terrain heights.
       private: template <typename T>
-      void getHeights(T data, const double& maxPixelValue,
-        const int& imgHeight, const int& imgWidth,
-        const unsigned int& pitch, const unsigned int& bpp,
-        const int& _subSampling, unsigned int& _vertSize,
+      void FillHeights(T *_data, int _imgHeight, int _imgWidth,
+        unsigned int _pitch, int _subSampling, unsigned int _vertSize,
         const ignition::math::Vector3d &_size,
         const ignition::math::Vector3d &_scale,
-        const bool& _flipY, std::vector<float> &_heights)
+        bool _flipY, std::vector<float> &_heights)
       {
+        // bytes per pixel
+        unsigned int bpp = _pitch / _imgWidth;
+        // number of channels in a pixel
+        unsigned int channels = bpp / sizeof(T);
+        // number of pixels in a row of image
+        unsigned int pitchInPixels = _pitch / bpp;
+
+        double maxPixelValue =
+            static_cast<double>(std::numeric_limits<T>::max());
+
         // Iterate over all the vertices
         for (unsigned int y = 0; y < _vertSize; ++y)
         {
@@ -81,8 +98,8 @@ namespace ignition
           double yf = y / static_cast<double>(_subSampling);
           int y1 = static_cast<int>(std::floor(yf));
           int y2 = static_cast<int>(std::ceil(yf));
-          if (y2 >= imgHeight)
-            y2 = imgHeight-1;
+          if (y2 >= _imgHeight)
+            y2 = _imgHeight-1;
           double dy = yf - y1;
 
           for (unsigned int x = 0; x < _vertSize; ++x)
@@ -90,22 +107,21 @@ namespace ignition
             double xf = x / static_cast<double>(_subSampling);
             int x1 = static_cast<int>(std::floor(xf));
             int x2 = static_cast<int>(std::ceil(xf));
-            if (x2 >= imgWidth)
-              x2 = imgWidth-1;
+            if (x2 >= _imgWidth)
+              x2 = _imgWidth-1;
             double dx = xf - x1;
 
             double px1 = static_cast<int>(
-              data[y1 * pitch / bpp + x1]) / maxPixelValue;
+              _data[(y1 * pitchInPixels + x1) * channels]) / maxPixelValue;
             double px2 = static_cast<int>(
-              data[y1 * pitch / bpp + x2]) / maxPixelValue;
+              _data[(y1 * pitchInPixels + x2) * channels]) / maxPixelValue;
             float h1 = (px1 - ((px1 - px2) * dx));
 
             double px3 = static_cast<int>(
-              data[y2 * pitch / bpp + x1]) / maxPixelValue;
+              _data[(y2 * pitchInPixels + x1) * channels]) / maxPixelValue;
             double px4 = static_cast<int>(
-              data[y2 * pitch / bpp + x2]) / maxPixelValue;
+              _data[(y2 * pitchInPixels + x2) * channels]) / maxPixelValue;
             float h2 = (px3 - ((px3 - px4) * dx));
-
             float h = (h1 - ((h1 - h2) * dy)) * _scale.Z();
 
             // invert pixel definition so 1=ground, 0=full height,
@@ -121,7 +137,6 @@ namespace ignition
               _heights[(_vertSize - y - 1) * _vertSize + x] = h;
           }
         }
-        delete [] data;
       }
     };
   }
diff --git a/graphics/src/ImageHeightmap.cc b/graphics/src/ImageHeightmap.cc
index 2c7543f..6c86308 100644
--- a/graphics/src/ImageHeightmap.cc
+++ b/graphics/src/ImageHeightmap.cc
@@ -54,9 +54,6 @@ void ImageHeightmap::FillHeightMap(int _subSampling,
   // Bytes per row
   unsigned int pitch = this->img.Pitch();
 
-  // Bytes per pixel
-  unsigned int bpp = pitch / imgWidth;
-
   // Get the image format so we can arrange our heightmap
   // Currently supported: 8-bit and 16-bit.
   auto imgFormat = this->img.PixelFormat();
@@ -65,7 +62,7 @@ void ImageHeightmap::FillHeightMap(int _subSampling,
   unsigned int count;
   this->img.Data(&data, count);
 
-  if(imgFormat == ignition::common::Image::PixelFormatType::L_INT8 ||
+  if (imgFormat == ignition::common::Image::PixelFormatType::L_INT8 ||
     imgFormat == ignition::common::Image::PixelFormatType::RGB_INT8 ||
     imgFormat == ignition::common::Image::PixelFormatType::RGBA_INT8 ||
     imgFormat == ignition::common::Image::PixelFormatType::BAYER_BGGR8 ||
@@ -76,26 +73,26 @@ void ImageHeightmap::FillHeightMap(int _subSampling,
     imgFormat == ignition::common::Image::PixelFormatType::BGR_INT8 ||
     imgFormat == ignition::common::Image::PixelFormatType::BGRA_INT8)
   {
-    getHeights(data, 255.0, imgHeight, imgWidth, pitch, bpp,
-    _subSampling, _vertSize, _size, _scale, _flipY, _heights);
+    this->FillHeights<unsigned char>(data, imgHeight, imgWidth, pitch,
+        _subSampling, _vertSize, _size, _scale, _flipY, _heights);
   }
-  else if(imgFormat == ignition::common::Image::PixelFormatType::BGR_INT16 ||
+  else if (imgFormat == ignition::common::Image::PixelFormatType::BGR_INT16 ||
     imgFormat == ignition::common::Image::PixelFormatType::L_INT16 ||
     imgFormat == ignition::common::Image::PixelFormatType::RGB_FLOAT16 ||
     imgFormat == ignition::common::Image::PixelFormatType::RGB_INT16 ||
     imgFormat == ignition::common::Image::PixelFormatType::R_FLOAT16)
   {
     uint16_t *dataShort = reinterpret_cast<uint16_t *>(data);
-    getHeights(dataShort, 65535.0,  imgHeight, imgWidth, pitch, bpp,
-    _subSampling, _vertSize, _size, _scale, _flipY, _heights);
+    this->FillHeights<uint16_t>(dataShort, imgHeight, imgWidth, pitch,
+        _subSampling, _vertSize, _size, _scale, _flipY, _heights);
   }
   else
   {
     ignerr << "Unsupported image format, "
-    "heightmap will not be loaded" << std::endl;
+      "heightmap will not be loaded" << std::endl;
     return;
   }
-
+  delete [] data;
 }
 
 //////////////////////////////////////////////////
diff --git a/graphics/src/ImageHeightmap_TEST.cc b/graphics/src/ImageHeightmap_TEST.cc
index c15f06c..568b324 100644
--- a/graphics/src/ImageHeightmap_TEST.cc
+++ b/graphics/src/ImageHeightmap_TEST.cc
@@ -94,7 +94,7 @@ TEST_F(ImageHeightmapTest, FillHeightmap)
   // Check the elevation of some control points
   EXPECT_NEAR(0.0, elevations.at(0), ELEVATION_TOL);
   EXPECT_NEAR(10.0, elevations.at(elevations.size() - 1), ELEVATION_TOL);
-  EXPECT_NEAR(5.0, elevations.at((elevations.size() / 2) - 2), ELEVATION_TOL);
+  EXPECT_NEAR(5.0, elevations.at((elevations.size() / 2)), ELEVATION_TOL);
 }
 
 /////////////////////////////////////////////////

Please take a look. If the changes look good to you. Can you apply to your branch? thanks.

[vatanaksoytezer]

@iche033 thanks a ton for the patch. I applied it, and worked nicely for me. I think this should be good for a last review & merge now.

[mjcarroll]

@vatanaksoytezer Looks good to me, I have a suggestion to add const in a few places where we can here: 3bc4c11

[vatanaksoytezer]

@vatanaksoytezer Looks good to me, I have a suggestion to add const in a few places where we can here: 3bc4c11

Sure!

[osrf-triage]

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-24-citadel-edifice-fortress/1241/1