Fix pcd_viewer color handling when invalid fields are present in pcd #2105

hrnr · 2017-11-28T23:59:44Z

Hi all,

I'm still very new to the pcl library (few hours), but I think that I have already fixed a bug. Please correct me if I'm wrong, I'm really a newbie here.

I have a .pcd file with dimensions like this:

Available dimensions: x y z _ rgb _

I have noticed that pcd_viewer does not show this .pcd in rgb colors. After some investigation it seems that it is caused by preceding invalid field before rgb field.

I'm still wondering what are these invalid fields doing in my pointcloud. Is this supposed to be a normal result of serialization to binary .pcd?

Cheers,
Jiri

taketwo · 2017-11-29T07:20:09Z

In order to allow SSE optimizations, point cloud fields are aligned. For example, the xyz field occupies 128 bits instead of 96, which means that in fact it is a xyz_ field with garbage in the end.

Please post your point cloud so that we can give it a try. Also, are you using PCL master?

hrnr · 2017-11-29T13:46:09Z

Yes, I'm using master at 47629c1.

My pointcloud is here: https://drive.google.com/file/d/1svASo3UfT-ozJG6QrJLMA0kr_Io5ApA_/view?usp=sharing

When you load with master pcd_viewer you will get an warning and ColorHandler for random color. This this patch it should load without warning and with ColorHandler using rgb.

Let me know if you want any new test for this. However the bug is in the main() of pcd_viewer.cpp, so I guess it might be quite hard to test.

I have checked the faulty CI: Travis timeouted, because it took too long (but I'm pretty sure this change is not increasing build time considerably). Appveyor faulted even before building for its internal reasons (is it even configured?). I think that none of these is related to this PR.

taketwo · 2017-11-29T13:53:45Z

Thanks, I'll have a look.

CI errors are unrelated. On Travis we've been struggling with timeouts in app building task for a while now. Appveyor is currently in the process of being set up (#2083).

taketwo

Indeed a bug. Proposed fix looks good, thanks!

taketwo · 2017-11-29T17:10:13Z

visualization/src/pcl_visualizer.cpp

@@ -2983,7 +2983,7 @@ pcl::visualization::PCLVisualizer::updateColorHandlerIndex (const std::string &i
  int color_handler_size = int (am_it->second.color_handlers.size ());
  if (index >= color_handler_size)
  {
-    pcl::console::print_warn (stderr, "[updateColorHandlerIndex] Invalid index <%d> given! Maximum range is: 0-%lu.\n", index, static_cast<unsigned long> (am_it->second.color_handlers.size ()));
+    pcl::console::print_warn (stderr, "[updateColorHandlerIndex] Invalid index <%d> given! Maximum range is: [0-%d).\n", index, color_handler_size);


Shouldn't it be color_handler_size - 1?

Yes, it was wrong there. I have tried to fix that with right-open interval convention [0-5), which makes it now correct, but it is not maybe that natural. I will rework that.

taketwo · 2017-11-29T17:10:43Z

visualization/tools/pcd_viewer.cpp

      for (size_t f = 0; f < cloud->fields.size (); ++f)
      {
+        if (!isValidFieldName (cloud->fields[f].name)) {


Please put the curly bracket on a separate line

sorry about that. I'm not used to this convention. Do you use some clang-format/linter?

btw do you know there are many trailing whitespace issues?

hrnr · 2017-11-29T23:22:52Z

I have fixed both issues and rebased.

Error comment now should be more clear. I have also fix the check to check also for underflows, not it is guaranteed we can use that as an index.

SergioRAgostinho · 2017-11-30T01:50:18Z

My two cents

diff --git a/visualization/tools/pcd_viewer.cpp b/visualization/tools/pcd_viewer.cpp
index b9d2d5e..7bfdbfb 100644
--- a/visualization/tools/pcd_viewer.cpp
+++ b/visualization/tools/pcd_viewer.cpp
@@ -617,16 +617,16 @@ main (int argc, char** argv)
     {
       int rgb_idx = 0;
       int label_idx = 0;
-      for (size_t f = 0; f < cloud->fields.size (); ++f)
+      for (size_t f = 0, i = 0; f < cloud->fields.size (); ++f)
       {
         if (cloud->fields[f].name == "rgb" || cloud->fields[f].name == "rgba")
         {
-          rgb_idx = f + 1;
+          rgb_idx = ++i;
           color_handler.reset (new pcl::visualization::PointCloudColorHandlerRGBField<pcl::PCLPointCloud2> (cloud));
         }
         else if (cloud->fields[f].name == "label")
         {
-          label_idx = f + 1;
+          label_idx = ++i;
           color_handler.reset (new pcl::visualization::PointCloudColorHandlerLabelField<pcl::PCLPointCloud2> (cloud, !use_optimal_l_colors));
         }
         else

taketwo · 2017-11-30T08:39:23Z

Do you use some clang-format/linter?

Unfortunately we don't have a clang-format config matching PCL style guide.

btw do you know there are many trailing whitespace issues?

Yep. There is also lots of misformatted code in the repo, but the policy is to not touch it (otherwise git blame is confused).

hrnr · 2017-11-30T10:35:25Z

@SergioRAgostinho That patch does not work. Try for yourself with any pcd. As you see it is hard to get it right (after all we have the bug there, so it must be hard, right?). You would need to increment i for all fields, but be careful enough to skip invalid fields.

My solution might not seem very elegant to you (matter of opinion), but it has some important properties.

It is clearly documenting that we need to skip invalid fields. It is important that we somehow document this bug (especially when we don't have test for it), so somebody does not reintroduce it on the next change of this code. Using a variable is better than a comment, because it is much easier to miss a comment (and some people tend to ignore comments).
It keeps the explicit +1 for skipping the random color handler. This is very non-obvious with your approach.
It skips invalid fields as early as possible, so there is a better chance, that they will not cause another bugs for people modifying this code in the future.

I've added a comment about that +1, to make it clear why it is there.

taketwo · 2017-11-30T10:40:48Z

visualization/src/pcl_visualizer.cpp

  {
-    pcl::console::print_warn (stderr, "[updateColorHandlerIndex] Invalid index <%d> given! Maximum range is: 0-%lu.\n", index, static_cast<unsigned long> (am_it->second.color_handlers.size ()));
+    pcl::console::print_warn (stderr, "[updateColorHandlerIndex] Invalid index <%d> given! Index must be less than %d.\n", index, int(color_handler_size));


Actually, why casting here? Just use %zu format.

%zu is officially available only from C++11 forwards. is it okay to use?

AFAIK it's ANSI C99 feature. But your question reminded me of the issues we had with MSVC (#588). So better use %lu.

Yes, exactly it is ANSI C99 feature. But ISO C++03 has not been rebased on C99, so also %zu is missing. This has been done in C++11 (rebase on C99) and there is %zu along with other. Only C++14 is finally based on C11.

However in real world %zu is pretty much fine on all linuxes, and some recent VS. macs should be fine too. If you turn on -pedantic it should be reported as standard violation.

Oh, good to know!

taketwo · 2017-11-30T10:41:00Z

visualization/src/pcl_visualizer.cpp

-  int color_handler_size = int (am_it->second.color_handlers.size ());
-  if (index >= color_handler_size)
+  size_t color_handler_size = am_it->second.color_handlers.size ();
+  if (!(size_t(index) < color_handler_size))


Sorry, missing space again :(

which space needs to be there? size_t (index)? something about brackets?

size_t (index)

SergioRAgostinho · 2017-11-30T10:56:10Z

Second try for completeness

diff --git a/visualization/tools/pcd_viewer.cpp b/visualization/tools/pcd_viewer.cpp
index b9d2d5e..63d4120 100644
--- a/visualization/tools/pcd_viewer.cpp
+++ b/visualization/tools/pcd_viewer.cpp
@@ -617,22 +617,25 @@ main (int argc, char** argv)
     {
       int rgb_idx = 0;
       int label_idx = 0;
-      for (size_t f = 0; f < cloud->fields.size (); ++f)
+      for (size_t f = 0, i = 1; f < cloud->fields.size (); ++f, ++i)
       {
         if (cloud->fields[f].name == "rgb" || cloud->fields[f].name == "rgba")
         {
-          rgb_idx = f + 1;
+          rgb_idx = i;
           color_handler.reset (new pcl::visualization::PointCloudColorHandlerRGBField<pcl::PCLPointCloud2> (cloud));
         }
         else if (cloud->fields[f].name == "label")
         {
-          label_idx = f + 1;
+          label_idx = i;
           color_handler.reset (new pcl::visualization::PointCloudColorHandlerLabelField<pcl::PCLPointCloud2> (cloud, !use_optimal_l_colors));
         }
         else
         {
           if (!isValidFieldName (cloud->fields[f].name))
+          {
+            --i;
             continue;
+          }
           color_handler.reset (new pcl::visualization::PointCloudColorHandlerGenericField<pcl::PCLPointCloud2> (cloud, cloud->fields[f].name));
         }
         // Add the cloud to the renderer

Do you use some clang-format/linter?

I started making something yesterday. It's still not quite there yet.

taketwo · 2017-11-30T10:58:02Z

I started making something yesterday. It's still not quite there yet.

Make sure you use clang-5, it has a number of new settings.

SergioRAgostinho · 2017-11-30T11:01:59Z

Yeah. I noticed it already :/ But should we aim already for 5 even though xenial is shipping 3.8?

taketwo · 2017-11-30T11:09:35Z

Let's move format discussion to #2106.

* clearly state index must be less than size we are printing * catch also underflow when negative index is given

* invalid fields needs to be skipped otherwise ColorHandler indexes will overflow

SergioRAgostinho added the needs: author reply Specify why not closed/merged yet label Nov 29, 2017

taketwo self-assigned this Nov 29, 2017

taketwo requested changes Nov 29, 2017

View reviewed changes

hrnr force-pushed the pcd_viewer_color_handling branch from 0a4aedb to 2a2a169 Compare November 29, 2017 23:20

hrnr force-pushed the pcd_viewer_color_handling branch from 2a2a169 to 30f29c1 Compare November 30, 2017 10:35

taketwo reviewed Nov 30, 2017

View reviewed changes

hrnr added 2 commits November 30, 2017 14:26

visualizer: make warning more comprehensive

83dab52

* clearly state index must be less than size we are printing * catch also underflow when negative index is given

pcd_viewer: fix color handling when there are invalid fields

6fb8dc6

* invalid fields needs to be skipped otherwise ColorHandler indexes will overflow

hrnr force-pushed the pcd_viewer_color_handling branch from 30f29c1 to 6fb8dc6 Compare November 30, 2017 13:26

taketwo approved these changes Nov 30, 2017

View reviewed changes

taketwo removed the needs: author reply Specify why not closed/merged yet label Nov 30, 2017

SergioRAgostinho merged commit eaa403d into PointCloudLibrary:master Nov 30, 2017

SergioRAgostinho mentioned this pull request Aug 6, 2018

Change Log automation #2385

Closed

SergioRAgostinho mentioned this pull request Aug 25, 2018

Change Log generation tool. Automates change log generation. #2396

Merged

SergioRAgostinho added the module: visualization label Aug 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pcd_viewer color handling when invalid fields are present in pcd #2105

Fix pcd_viewer color handling when invalid fields are present in pcd #2105

hrnr commented Nov 28, 2017

taketwo commented Nov 29, 2017

hrnr commented Nov 29, 2017

taketwo commented Nov 29, 2017

taketwo left a comment

taketwo Nov 29, 2017

hrnr Nov 29, 2017

taketwo Nov 29, 2017

hrnr Nov 29, 2017

hrnr commented Nov 29, 2017

SergioRAgostinho commented Nov 30, 2017

taketwo commented Nov 30, 2017

hrnr commented Nov 30, 2017

taketwo Nov 30, 2017

hrnr Nov 30, 2017

taketwo Nov 30, 2017

hrnr Nov 30, 2017

taketwo Nov 30, 2017

taketwo Nov 30, 2017

hrnr Nov 30, 2017

taketwo Nov 30, 2017

hrnr Nov 30, 2017

SergioRAgostinho commented Nov 30, 2017

taketwo commented Nov 30, 2017

SergioRAgostinho commented Nov 30, 2017

taketwo commented Nov 30, 2017

Fix pcd_viewer color handling when invalid fields are present in pcd #2105

Fix pcd_viewer color handling when invalid fields are present in pcd #2105

Conversation

hrnr commented Nov 28, 2017

taketwo commented Nov 29, 2017

hrnr commented Nov 29, 2017

taketwo commented Nov 29, 2017

taketwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hrnr commented Nov 29, 2017

SergioRAgostinho commented Nov 30, 2017

taketwo commented Nov 30, 2017

hrnr commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SergioRAgostinho commented Nov 30, 2017

taketwo commented Nov 30, 2017

SergioRAgostinho commented Nov 30, 2017

taketwo commented Nov 30, 2017