[OCTREE] Fix pcl_octree_viewer #1973

frozar · 2017-08-27T13:29:01Z

Fixes issue #1840

To avoid the display of spurious point, check the detph of an octree node before to print it.
Be able to read PLY file.
Fix the display of the octree cube.

taketwo · 2017-08-27T14:33:23Z

Hi, thanks for contributing. If I understand correctly, you propose three different changes here. It is okay to keep them in a single pull request, but would be better to split into three commits with appropriate messages. Further, I'd ask you to follow the PCL style guide. (In particular, spacing in function calls and positioning of curly brackets.) Please update this pull request.

frozar · 2017-08-28T08:30:48Z

I split my contribution in 3 commits as you advise. I try to follow the PCL coding style but if there still is some coding style issue, please put some comment on the concern lines and I'll fix them.

I must say the code I have written to fix the spurious points display is pretty odd: if the iterator over an octree was correct, I should not have to check the depth of the iterator (tree_it.getCurrentOctreeDepth () != depth). Don't you think that there is an issue about the octree iterator?

taketwo · 2017-08-29T08:51:37Z

From what I see, the argument for begin() function is called max_depth_arg, so this is expected that you get nodes with depth less or equal to what is passed to the function.

taketwo · 2017-08-29T08:55:24Z

Regarding the file loading commit. I've just remembered that we have a relatively new load function that automatically deals with all supported filetypes.

frozar · 2017-08-29T12:11:35Z

I agree with you for the max_depth_arg. I think the first person who wrote octree_viewer.cpp didn't notice that. Don't you think that it would be valuable to provide an other iterator that does the work? (I prepare a merge request in this way, but I don't if this feature will be merged (I send a mail on the PCL-users mailing list, but no answer yet...)).

I update the code with the load function.

taketwo · 2017-08-29T12:35:27Z

Don't you think that it would be valuable to provide an other iterator that does the work?

Honestly, I don't have a strong opinion here. The octree module has been around for many years now and nobody submitted either a feature request or an implementation of such an iterator. So it seems fine without. That said, if you find multiple places in existing code in PCL that can be rewritten in a more clear and concise way by using the proposed iterator, I would take it as a strong evidence that the iterator is valuable and will be happy to merge it. Of course, other opinions are welcome @SergioRAgostinho @jspricke

I send a mail on the PCL-users mailing list, but no answer yet...

This issue tracker is the right place to propose and discuss new functionality before actually crafting a pull request.

taketwo

Added a bunch of inline comments where style has to be fixed.

taketwo · 2017-08-29T12:36:29Z

tools/octree_viewer.cpp


  /* \brief display octree cubes via vtk-functions
   *
   */
  void showCubes(double voxelSideLen)
  {
-    //get the renderer of the visualizer object
-    vtkRenderer *renderer = viz.getRenderWindow()->GetRenderers()->GetFirstRenderer();
+    vtkSmartPointer<vtkAppendPolyData> appendFilter = vtkSmartPointer<vtkAppendPolyData>::New();


Need space before brackets, i.e. New ();

taketwo · 2017-08-29T12:36:50Z

tools/octree_viewer.cpp

    double s = voxelSideLen / 2.0;
-    for (i = 0; i < displayCloud->points.size(); i++)
+    for (size_t i = 0; i < displayCloud->points.size(); i++)


Need space before brackets, i.e. points.size ();

taketwo · 2017-08-29T12:36:59Z

tools/octree_viewer.cpp

      double x = displayCloud->points[i].x;
      double y = displayCloud->points[i].y;
      double z = displayCloud->points[i].z;

+      vtkSmartPointer<vtkCubeSource> wk_cubeSource = vtkSmartPointer<vtkCubeSource>::New();


taketwo · 2017-08-29T12:37:05Z

tools/octree_viewer.cpp

      double x = displayCloud->points[i].x;
      double y = displayCloud->points[i].y;
      double z = displayCloud->points[i].z;

+      vtkSmartPointer<vtkCubeSource> wk_cubeSource = vtkSmartPointer<vtkCubeSource>::New();
+
+      wk_cubeSource->SetBounds(x - s, x + s, y - s, y + s, z - s, z + s);


Space here and in all line.s below.

taketwo · 2017-08-29T12:37:38Z

tools/octree_viewer.cpp


-    treeActor->GetProperty()->SetColor(1.0, 1.0, 1.0);
-    treeActor->GetProperty()->SetLineWidth(2);
+    multiActor->GetProperty( )->SetColor(0.0, 1.0, 1.0);


No spaces inside brackets

taketwo · 2017-08-29T12:37:53Z

tools/octree_viewer.cpp

-    treeActor->GetProperty()->SetLineWidth(2);
+    multiActor->GetProperty( )->SetColor(0.0, 1.0, 1.0);
+    multiActor->GetProperty( )->SetAmbient(1.0);
+    multiActor->GetProperty( )->SetLineWidth(2);
    if(wireframe)


Space after if

frozar · 2017-08-29T14:02:56Z

Maybe you should check the coding style again: I never wrote code like that... ^^'

taketwo · 2017-08-29T15:05:33Z

tools/octree_viewer.cpp

 #if VTK_MAJOR_VERSION < 6
-      treeWireframe->AddInput(GetCuboid(x - s, x + s, y - s, y + s, z - s, z + s));
+      appendFilter->AddInput(wk_cubeSource->GetOutput ());


Missing space after AddInput.

taketwo · 2017-08-29T15:05:42Z

tools/octree_viewer.cpp

 #else
-      treeWireframe->AddInputData (GetCuboid (x - s, x + s, y - s, y + s, z - s, z + s));
+      appendFilter->AddInputData(wk_cubeSource->GetOutput ());


taketwo · 2017-08-29T15:05:54Z

tools/octree_viewer.cpp

-    mapper->SetInputData (treeWireframe->GetOutput ());
-#endif
-    treeActor->SetMapper(mapper);
+    cleanFilter->SetInputConnection(appendFilter->GetOutputPort ());


Missing space after SetInputConnection

taketwo · 2017-08-29T15:06:20Z

tools/octree_viewer.cpp

-    treeActor->GetProperty()->SetColor(1.0, 1.0, 1.0);
-    treeActor->GetProperty()->SetLineWidth(2);
-    if(wireframe)
+    multiActor->GetProperty ()->SetColor (0.0, 1.0, 1.0);


Any particular reason to change the color from white to cyan?

Not at all. In fact, I was just trying to find the color used to produce the image there, but unsuccessfully...

Ok, then let's leave it white as it was before.

Oh, I didn't see this message...

taketwo · 2017-08-29T15:06:48Z

tools/octree_viewer.cpp

@@ -339,6 +345,9 @@ class OctreeViewer

    for (tree_it = octree.begin(depth); tree_it!=tree_it_end; ++tree_it)
    {
+      if (tree_it.getCurrentOctreeDepth () != depth)
+        continue ;


Extra space before semicolon.

taketwo · 2017-08-29T16:01:41Z

So besides from color everything looks good now. Once that is fixed, please regroup all related changes so that we have 3 topic commits, as planned, and then we are good to go.

…n octree node before to print it.

…put file.

frozar · 2017-08-30T08:37:20Z

I rewrite the history, now it should be fine 👌

taketwo · 2017-08-30T12:28:53Z

Thanks for the effort. From my side everything is fine, but we'll need to wait a bit until one other maintainer takes a look and pushes the merge button.

frozar · 2017-08-30T12:36:59Z

Nice 😄

SergioRAgostinho · 2017-09-01T15:34:50Z

Of course, other opinions are welcome @SergioRAgostinho

+1 for the iterator

taketwo added this to the pcl-1.9.0 milestone Aug 27, 2017

taketwo added the needs: author reply Specify why not closed/merged yet label Aug 27, 2017

frozar force-pushed the octree_viewer branch from ffc566b to bdc3f51 Compare August 28, 2017 08:17

taketwo reviewed Aug 29, 2017

View reviewed changes

frozar added 2 commits August 30, 2017 10:31

[OCTREE] To avoid the display of spurious point, check the detph of a…

f8a2acd

…n octree node before to print it.

[OCTREE] Be able to read PLY file. Use the 'load' function to read in…

3d39470

…put file.

frozar force-pushed the octree_viewer branch from e7200f9 to c037931 Compare August 30, 2017 08:33

[OCTREE] Fix the display of the octree cube.

c63f102

frozar force-pushed the octree_viewer branch from c037931 to c63f102 Compare August 30, 2017 08:43

taketwo approved these changes Aug 30, 2017

View reviewed changes

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

frozar mentioned this pull request Sep 1, 2017

[OCTREE] Compute accurately the centroids of octree in 'pcl_octree_viewer' #1981

Merged

SergioRAgostinho merged commit 5cc08f8 into PointCloudLibrary:master Sep 1, 2017

frozar deleted the octree_viewer branch September 1, 2017 18:06

frozar mentioned this pull request Nov 26, 2017

pcl_octree_viewer displays spurious points when visualizing a PointCloud | Octree Viewer .pcd #1840

Closed

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: tools label Aug 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OCTREE] Fix pcl_octree_viewer #1973

[OCTREE] Fix pcl_octree_viewer #1973

frozar commented Aug 27, 2017

taketwo commented Aug 27, 2017

frozar commented Aug 28, 2017

taketwo commented Aug 29, 2017

taketwo commented Aug 29, 2017

frozar commented Aug 29, 2017

taketwo commented Aug 29, 2017

taketwo left a comment

taketwo Aug 29, 2017

taketwo Aug 29, 2017

taketwo Aug 29, 2017

taketwo Aug 29, 2017

taketwo Aug 29, 2017

taketwo Aug 29, 2017

frozar commented Aug 29, 2017

taketwo Aug 29, 2017

taketwo Aug 29, 2017

taketwo Aug 29, 2017

taketwo Aug 29, 2017

frozar Aug 29, 2017

taketwo Aug 29, 2017

frozar Aug 30, 2017

taketwo Aug 29, 2017

taketwo commented Aug 29, 2017

frozar commented Aug 30, 2017

taketwo commented Aug 30, 2017

frozar commented Aug 30, 2017

SergioRAgostinho commented Sep 1, 2017

[OCTREE] Fix pcl_octree_viewer #1973

[OCTREE] Fix pcl_octree_viewer #1973

Conversation

frozar commented Aug 27, 2017

taketwo commented Aug 27, 2017

frozar commented Aug 28, 2017

taketwo commented Aug 29, 2017

taketwo commented Aug 29, 2017

frozar commented Aug 29, 2017

taketwo commented Aug 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frozar commented Aug 29, 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

taketwo commented Aug 29, 2017

frozar commented Aug 30, 2017

taketwo commented Aug 30, 2017

frozar commented Aug 30, 2017

SergioRAgostinho commented Sep 1, 2017