Add another variant to `getPointCloudRenderingProperties()` #2142

frozar · 2017-12-11T14:36:01Z

In this PR, I proposed another method getPointCloudRenderingProperties() to handle the color property. The prototype is different, that's why I had to add a new method: typically to retrieve r, g, b properties.

And in fact, this method handle only the COLOR property.

SergioRAgostinho · 2017-12-11T23:25:29Z

Proposal:

property is actually of type RenderingProperties
The return value is not three independent variable but a std::vector. why? Because different properties have a different number of elements to be returned. We can template on the type of vector to ensure proper casting depending on the user needs.
The other existing overload is merged into this one.

Opinions @frozar and @taketwo .

frozar · 2017-12-12T08:58:21Z

Right now, this feature doesn't work (sorry)... The management of adding point cloud to the scene is a bit complicated. The VTK property I retrieve is not the right one. I'm working on it.

The return value is not three independent variable but a std::vector.

Which function/method does return a std::vector?

In the idea, why not merging the existing overload :)

taketwo · 2017-12-12T09:52:12Z

I think the proposed method is consistent with the existing interface. There is a setter with three double parameters, and now there is a getter with three output parameters.

What Sergio suggests might be a better approach in general and perhaps using vectors is how I would implement all property setters/getters if I had to start from scratch. But as it is now, will be incoherent and feel foreign in the existing API.

…dle color.

frozar · 2017-12-12T14:24:56Z

Things are clear now. The proposition done in this PR is correct: it works well 👍

Details

The interface to add point cloud with rendering properties is subtle. I custom point cloud color and there are two way to deal with that:

pcl/visualization/include/pcl/visualization/impl/pcl_visualizer.hpp

Lines 129 to 133 in 6d646d1

    
           template <typename PointT> bool 
        
           pcl::visualization::PCLVisualizer::addPointCloud ( 
        
             const typename pcl::PointCloud<PointT>::ConstPtr &cloud, 
        
             const PointCloudColorHandler<PointT> &color_handler, 
        
             const std::string &id, int viewport)

and

pcl/visualization/src/pcl_visualizer.cpp

Lines 1375 to 1377 in 6d646d1

    
           bool 
        
           pcl::visualization::PCLVisualizer::setPointCloudRenderingProperties ( 
        
               int property, double val1, double val2, double val3, const std::string &id, int)

I was in trouble because I used the first method with a chosen PointCloudColorHandler<PointT> &color_handler and I tried to retrieve the color with the getPointCloudRenderingProperties() that I propose in this PR.

After a while, I released that these two methods are not equivalent. When one uses a PointCloudColorHandler<PointT> &color_handler to set the color, the property of the VTK actor that represents a point cloud is not directly updated. At the end of addPointCloud(), the method fromHandlersToScreen() manages the display of a point cloud. When you look inside this method (relevant part of it):

pcl/visualization/include/pcl/visualization/impl/pcl_visualizer.hpp

Lines 1353 to 1373 in 6d646d1

    
           vtkSmartPointer<vtkDataArray> scalars; 
        
           if (color_handler.getColor (scalars)) 
        
           { 
        
             polydata->GetPointData ()->SetScalars (scalars); 
        
             scalars->GetRange (minmax); 
        
             has_colors = true; 
        
           } 
        
           // Create an Actor 
        
           vtkSmartPointer<vtkLODActor> actor; 
        
           createActorFromVTKDataSet (polydata, actor); 
        
           if (has_colors) 
        
             actor->GetMapper ()->SetScalarRange (minmax); 
        
           // Add it to all renderers 
        
           addActorToRenderer (actor, viewport); 
        
           // Save the pointer/ID pair to the global actor map 
        
           CloudActor& cloud_actor = (*cloud_actor_map_)[id]; 
        
           cloud_actor.actor = actor; 
        
           cloud_actor.cells = initcells;

At L. 1354, the custom color is put in a vtkDataArray scalars and then (L. 1356) this scalars is use to set polydata. At L. 1363, the vtkLODActor actor is initialised with polydata through the method createActorFromVTKDataSet(). But in this method, the color property of the argument actor is never set. If it were, you should see something like actor->GetProperty ()->SetColor (). The color is "deeply" set in the polydata object. So the color property of actor has the default value. When I tried to retrieve it with getPointCloudRenderingProperties, I always get 1.0, 1.0, 1.0 so not the expected result.

If I set the color with setPointCloudRenderingProperties() (it changes the color property of actor and visibly works), I can retrieve it with getPointCloudRenderingProperties(), so I stop using PointCloudColorHandler<PointT> &color_handler.

Conclusion

As there are different ways to specify a color and they are not handle/stored in the same way, to retrieve a point cloud color, currently it is only possible with the setPointCloudRenderingProperties()/getPointCloudRenderingProperties().

To be able to retrieve a color set with a PointCloudColorHandler<PointT> &color_handler, a method should be added to the sub-classes of PointCloudColorHandler with the prototype:

virtual bool
getColor (double &r, double &g, double &b) const

And this color handler should always be accessible through a field of CloudActor.

taketwo · 2017-12-12T14:45:00Z

To be able to retrieve a color set with a PointCloudColorHandler<PointT> &color_handler, a method should be added to the sub-classes of PointCloudColorHandler

But such a getter only makes sense for a subset of all color handlers, namely PointCloudColorHandlerCustom and PointCloudColorHandlerRandom...

frozar · 2017-12-12T15:05:29Z

@taketwo I'm lucky, the PointCloudColorHandler class is an abstract class, and thanks god:

virtual bool
getColor (vtkSmartPointer<vtkDataArray> &scalars) const = 0;

So as getColor() is pure virtual, all derived classes implement it, even if it is irrevelent (I'm not sure that it is the case). So in this situation, even if the getColor(vtkSmartPointer<vtkDataArray> &scalars) method doesn't mean anything in a derived class, you just have to mimic this behaviour for getColor (double &r, double &g, double &b).

Anyway in this approach, the specification maybe a difficulty but the fact that a color handler is not always stored in a CloudActor makes the implementation really complicated (sometime you will be able to retrieve the color, sometime not because the handler is not stored). To deal with it, the first step should be to always store the color handler in CloudActor.

For me, this discussion is beyong the scope of this PR.

taketwo · 2017-12-12T15:08:52Z

So as getColor() is pure virtual, all derived classes implement it, even if it is irrevelent (I'm not sure that it is the case)

getColor() is the single most important function of a color handler class, all of them implement it! With this function VTK gets to know the actual color for each vertex in the point cloud.

frozar · 2017-12-12T15:21:00Z

@taketwo What do you advice for this PR?

taketwo

As per Sergio's comment:

property is actually of type RenderingProperties

Besides from that, I'm fine with merging.

taketwo · 2017-12-12T15:24:04Z

visualization/src/pcl_visualizer.cpp

+    {
+      double rgb[3];
+      actor->GetProperty ()->GetColor (rgb);
+      actor->Modified ();


Do we need this?

No, I delete it.

frozar · 2017-12-12T16:10:00Z

The prototype is fixed.

SergioRAgostinho

How about adding a unit test to validate what you just added and improve our test coverage in 0.0001%? :)

SergioRAgostinho · 2017-12-13T00:21:33Z

visualization/include/pcl/visualization/pcl_visualizer.h

+         * \param[out] val2 the resultant property value
+         * \param[out] val3 the resultant property value
+         * \param[in] id the point cloud object id (default: cloud)
+         * \note The list of properties can be found in \ref pcl::visualization::LookUpTableRepresentationProperties.


Lacks the comment about the \return value.

SergioRAgostinho · 2017-12-13T00:22:36Z

visualization/src/pcl_visualizer.cpp

@@ -1503,6 +1503,39 @@ pcl::visualization::PCLVisualizer::getPointCloudRenderingProperties (int propert
  }
  return (true);
 }
+/////////////////////////////////////////////////////////////////////////////////////////////
+bool
+pcl::visualization::PCLVisualizer::getPointCloudRenderingProperties (RenderingProperties property, double &val1, double &val2, double &val3, const std::string &id)


Split each argument to one line each. This line is excessively long.

SergioRAgostinho · 2017-12-13T00:36:54Z

visualization/src/pcl_visualizer.cpp

+    }
+    default:
+    {
+      pcl::console::print_error ("[getPointCloudRenderingProperties] Unknown property (%d) specified!\n", property);


This error message is misleading. PCL_VISUALIZER_LINE_WIDTH is a known property but would trigger this error message anyway.

"Property (%d) is either unknown or it requires a different number of variables to retrieve its contents."

frozar · 2017-12-13T10:23:17Z

Unit test is always painful for me as I didn't know how to handle them. I made a proposition in my last commit.

To compile and execute the test, I use the following command line:

rm -rf ./* && cmake -DPCL_ONLY_CORE_POINT_TYPES=ON -DPCL_NO_PRECOMPILE=ON -DBUILD_tools=OFF -DBUILD_examples=OFF -DBUILD_apps=OFF -DBUILD_2d=OFF -DBUILD_filters=OFF -DBUILD_outofcore=OFF -DBUILD_people=OFF -DBUILD_recognition=OFF -DBUILD_registration=OFF -DBUILD_sample_consensus=OFF -DBUILD_ml=OFF -DBUILD_stereo=OFF -DBUILD_surface=OFF -DBUILD_geometry=ON -DBUILD_io=ON -DBUILD_kdtree=ON -DBUILD_octree=ON -DBUILD_search=ON -DBUILD_features=ON -DBUILD_visualization=ON -DBUILD_global_tests=ON -DBUILD_tests_common=OFF -DBUILD_tests_geometry=OFF -DBUILD_tests_io=OFF -DBUILD_tests_kdtree=OFF -DBUILD_tests_octree=OFF -DBUILD_tests_search=OFF -DBUILD_tests_surface=OFF -DBUILD_tests_segmentation=OFF -DBUILD_tests_features=OFF -DBUILD_tests_filters=OFF -DBUILD_tests_keypoints=OFF -DBUILD_tests_people=OFF -DBUILD_tests_outofcore=OFF -DBUILD_tests_recognition=OFF -DBUILD_tests_registration=OFF -DBUILD_tests_segmentation=OFF -DBUILD_tests_sample_consensus=OFF -DBUILD_tests_visualization=ON -DGTEST_INCLUDE_DIR=/home/frozar/soft/googletest/googletest/include -DGTEST_SRC_DIR=/home/frozar/soft/googletest/googletest -DCMAKE_BUILD_TYPE=Debug .. && make -j8 && ./test/visualization/test_visualization ../test/bunny.pcd

Compare to the last unit tests added by @SergioRAgostinho (about octree), the test/visualization/test_visualization.cpp is really ugly. Let me know what you think about this one.

frozar · 2017-12-13T13:44:39Z

CI Issue

There is something I don't understand with the continous integration test TASK="test-ext-1". This test should launch the visualization test, but when I check the execution on the last commit, I don't see the status of the test_visualisation program in the end of the log. Even stranger, I cannot find the "Linking CXX executable test_visualization" line during the compilation, and the cmake output say it will build the subsystem "tests_visualization".

For this test, I check the CMake configuration, compilation and execution on my laptop, and the "test_visualization" is effectively executed. I check a previous Travis build and "test_visualization" was also missing. It seems that the TASK="test-ext-1" never launch the "test_visualization".

Does someone have an idea why?

SergioRAgostinho · 2017-12-13T14:09:34Z

I can't look into right now but if you want some pointers check the differences between the cmake files for the other test/modules and the test/visualization one.

taketwo · 2017-12-13T14:49:09Z

Visualization tests are disabled when running in headless mode (i.e. without display), which is the case on Travis:

pcl/test/visualization/CMakeLists.txt

Line 12 in b0e83f2

if(BUILD_features AND NOT UNIX OR (UNIX AND DEFINED ENV{DISPLAY}))

frozar · 2017-12-13T14:52:37Z

@taketwo Well seen!

So what should I do for this PR?

taketwo · 2017-12-14T10:05:06Z

From my side, we can merge.

SergioRAgostinho

Thanks for the trouble of writing the test :)

SergioRAgostinho · 2017-12-14T14:28:37Z

test/visualization/test_visualization.cpp

+////////////////////////////////////////////////////////////////////////////////
+TEST (PCL, PCLVisualizer_getPointCloudRenderingProperties)
+{
+  PCLVisualizer::Ptr visualizer (new PCLVisualizer);


This doesn't need to be a shared pointer.

SergioRAgostinho · 2017-12-14T14:30:39Z

test/visualization/test_visualization.cpp

+  std::string cloud_id = "input_cloud";
+  visualizer->addPointCloud (cloud, cloud_id);
+  ASSERT_TRUE (visualizer->setPointCloudRenderingProperties (PCL_VISUALIZER_COLOR,
+                                                             1.d, 0.d, 0.d, cloud_id));


I'm so surprised these .d suffixes work. I couldn't find them on cppreference. I just hope it's supported by all the compilers.

I'm surprised that you are surprised, I used to see them (maybe it's a Fortran deformatin...). Bah, anyway, it is said that C++ assume an expression like '1.0' as a double (check this section).

So I delete this suffixes.

frozar force-pushed the get_point_cloud_rendering_properties branch from 418bbad to 79f6553 Compare December 11, 2017 15:09

SergioRAgostinho added the status: needs decision label Dec 11, 2017

[VISU] Add another method 'getPointCloudRenderingProperties()' to han…

9d48cb3

…dle color.

frozar force-pushed the get_point_cloud_rendering_properties branch from 79f6553 to 9d48cb3 Compare December 12, 2017 10:35

taketwo reviewed Dec 12, 2017

View reviewed changes

frozar added 2 commits December 12, 2017 17:08

[VISU] Fix prototype.

20e3a87

[VISU] Delete useless line.

ae1f15a

SergioRAgostinho reviewed Dec 13, 2017

View reviewed changes

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed status: needs decision labels Dec 13, 2017

[VISU] Review from Sergio.

dd44e1b

taketwo approved these changes Dec 13, 2017

View reviewed changes

[TEST] Add a unit test for 'getPointCloudRenderingProperties()'.

d9bd688

taketwo added needs: code review Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Dec 14, 2017

SergioRAgostinho reviewed Dec 14, 2017

View reviewed changes

SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Dec 14, 2017

[TEST] Visu: review from Sergio.

7fa84d7

SergioRAgostinho removed the needs: author reply Specify why not closed/merged yet label Dec 15, 2017

SergioRAgostinho merged commit c1e395b into PointCloudLibrary:master Dec 15, 2017

frozar deleted the get_point_cloud_rendering_properties branch December 15, 2017 15:50

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 changed the title ~~[VISU] Add another method 'getPointCloudRenderingProperties()'~~ Add another variant to getPointCloudRenderingProperties() Aug 26, 2018

SergioRAgostinho added the module: visualization label Aug 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add another variant to `getPointCloudRenderingProperties()` #2142

Add another variant to `getPointCloudRenderingProperties()` #2142

frozar commented Dec 11, 2017

SergioRAgostinho commented Dec 11, 2017

frozar commented Dec 12, 2017

taketwo commented Dec 12, 2017

frozar commented Dec 12, 2017 •

edited

Loading

taketwo commented Dec 12, 2017

frozar commented Dec 12, 2017

taketwo commented Dec 12, 2017

frozar commented Dec 12, 2017

taketwo left a comment

taketwo Dec 12, 2017

frozar Dec 12, 2017

frozar commented Dec 12, 2017

SergioRAgostinho left a comment

SergioRAgostinho Dec 13, 2017

frozar Dec 13, 2017

SergioRAgostinho Dec 13, 2017

frozar Dec 13, 2017

SergioRAgostinho Dec 13, 2017

frozar Dec 13, 2017

frozar commented Dec 13, 2017

frozar commented Dec 13, 2017

SergioRAgostinho commented Dec 13, 2017

taketwo commented Dec 13, 2017

frozar commented Dec 13, 2017

taketwo commented Dec 14, 2017

SergioRAgostinho left a comment

SergioRAgostinho Dec 14, 2017

frozar Dec 14, 2017

SergioRAgostinho Dec 14, 2017

frozar Dec 14, 2017

Add another variant to getPointCloudRenderingProperties() #2142

Add another variant to getPointCloudRenderingProperties() #2142

Conversation

frozar commented Dec 11, 2017

SergioRAgostinho commented Dec 11, 2017

frozar commented Dec 12, 2017

taketwo commented Dec 12, 2017

frozar commented Dec 12, 2017 • edited Loading

Details

Conclusion

taketwo commented Dec 12, 2017

frozar commented Dec 12, 2017

taketwo commented Dec 12, 2017

frozar commented Dec 12, 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

frozar commented Dec 12, 2017

SergioRAgostinho 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frozar commented Dec 13, 2017

frozar commented Dec 13, 2017

CI Issue

SergioRAgostinho commented Dec 13, 2017

taketwo commented Dec 13, 2017

frozar commented Dec 13, 2017

taketwo commented Dec 14, 2017

SergioRAgostinho 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

Add another variant to `getPointCloudRenderingProperties()` #2142

Add another variant to `getPointCloudRenderingProperties()` #2142

frozar commented Dec 12, 2017 •

edited

Loading