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

frozar · 2017-09-01T09:10:14Z

Use the method getVoxelCentroidsRecursive() to retrieve every centroid under a given BranchNode and compute the correct global centroid.

This pull request improves the accuracy of pcl_octree_viewer. The code itself is useful as example to show how to handle the octree classes.

Nota Bene: This merge request relies on top of the pull request #1973

taketwo · 2017-09-01T09:35:35Z

I do not think that it makes sense to align cubes with centroids. This viewer visualizes the hierarchical nested structure of an octree, illustrates how the space is partitioned into a regular 3D grid.

However, I agree that this will serve as an example of handling octree branches/leaves and also that visualization of centroids may be insightful. Therefore, I propose to just add centroid visualization capability alongside with cubes display.

frozar · 2017-09-01T09:43:28Z

Sorry, I didn't realise that I screw up the cube visualization... I completely agree with you. I prepare a fix.

frozar · 2017-09-01T10:07:51Z

Cubes visualization: fixed.

taketwo · 2017-09-02T15:18:00Z

Hi, thanks for the edits, works mostly fine on my side. However, I noticed that when the viewer is in the "point representation", it is not possible to toggle display of the original point cloud (i.e. "x" key has no effect). Further, when we are in "cube representation", it is possible to show the original cloud, but then it is impossible to rotate the camera anymore because "x" shortcut coincidentally switches on point selection mode. Thus I have a couple of proposals.

First, right now we have three different visualization objects: original point cloud, voxel centroid cloud, and voxel boxes. What if we allow to toggle them independently? Second, let's change the shortcuts, in particular avoid using "x". Finally, I think it makes sense to have the cube visualization on by default, that's the whole point of having this viewer, after all.

frozar · 2017-09-03T14:24:49Z

I completly agree with your previous remarks. All remarks have been taken into accompt . I let you check it.

taketwo · 2017-09-06T19:27:46Z

Hi, sorry for the delay. I tried to run it on my machine, works fine. Only problem I noticed is that the lines for v anb b key are overlapping:

Also, I wonder if we need that s/w switch. PCL Visualizer provides these hotkeys by default (as can be seen in the console output when pressing h for help). So probably s/w in this app is just a duplicate and can be removed.

Could you please do me a favor and rebase this pull request branch onto the current master? This will remove the commits that have been merged already making it easier for me to review only the new changes.

frozar · 2017-09-07T08:54:45Z

Happy to see your answer. You were right, there was some legend overlap. Also I remove the "s/w" line in the legend, but in the code, the bool wireframe is still present.

The reason for that is when you don't save this information, the default behavior of the camera is to display the octree cubes with surfaces visible. When you toggle to the wireframe view at a given depth, is works, OK, but when you change the depth, you get back to the "surface visible" view. I try to show it in this animation:

So to keep the wireframe/surface visible state, you have to save it.

I realises that it's the same with the "+/-" event which change the size of point when point cloud are displayed. I this it's the good time to introduce another parameter, let say point_size_, to keep the point size when one changes the depth of the representation.

What do you think about it?

taketwo · 2017-09-07T09:59:19Z

Thanks for the explanation, makes sense.

I this it's the good time to introduce another parameter, let say point_size_, to keep the point size when one changes the depth of the representation.

Very good point. The size is also reset when toggling visibility of the points, would be nice to preserve this.

frozar · 2017-09-07T10:05:39Z

Nice, it's done in the last commit.

taketwo · 2017-09-07T10:11:16Z

Thanks, works great. So functionality-wise I'd say with PR is complete. Could you please rebase as I requested before so that we can do a quick code review and be ready to merge?

frozar · 2017-09-07T11:13:59Z

This branch is now rebase onto master. I didn't rewrite the history, but I can if you want.

taketwo · 2017-09-07T13:21:11Z

tools/octree_viewer.cpp

+    show_centroids_ (false),
+    show_original_points_ (false),
+    point_size_ (1.0)
+    {


This bracket is over-indented.

taketwo · 2017-09-07T13:23:27Z

tools/octree_viewer.cpp

@@ -197,26 +229,30 @@ class OctreeViewer
  /* \brief Helper function that draw info for the user on the viewer
   *
   */
-  void showLegend(bool showCubes)
+  void showLegend()


Missing space

taketwo · 2017-09-07T13:24:34Z

tools/octree_viewer.cpp

+    viz.addText (level, 0, 30, 1.0, 0.0, 0.0, "level_t1");
+
+    viz.removeShape ("level_t2");
+    sprintf (level, "Voxel size: %.4fm [%lu voxels]", sqrt(octree.getVoxelSquaredSideLen(displayedDepth)),


Missing spaces. Also, please use std::sqrt.

taketwo · 2017-09-07T13:24:53Z

tools/octree_viewer.cpp

-        pcl::visualization::PointCloudColorHandlerGenericField<pcl::PointXYZ> color_handler(cloud, "z");
-        viz.addPointCloud(cloud, color_handler, "cloud");
-      }
+      showCubes (sqrt (octree.getVoxelSquaredSideLen (displayedDepth)));


std::sqrt

taketwo · 2017-09-07T13:27:19Z

tools/octree_viewer.cpp

+      cloudVoxel->points.push_back (pt_voxel_center);
+
+      // If the asked depth is the depth of the octree, retrieve the centroid at this LeafNode
+      if ( octree.getTreeDepth() == depth ){


Extra spaces inside braces, missing space after getTreeDepth, opening curly bracket should go on a separate line.

taketwo · 2017-09-07T13:27:56Z

tools/octree_viewer.cpp

+      if ( octree.getTreeDepth() == depth ){
+        pcl::octree::OctreePointCloudVoxelCentroid<pcl::PointXYZ>::LeafNode* container = static_cast<pcl::octree::OctreePointCloudVoxelCentroid<pcl::PointXYZ>::LeafNode*> (tree_it.getCurrentOctreeNode ());
+
+        container->getContainer().getCentroid (pt_centroid);


Missing space

taketwo · 2017-09-07T13:28:07Z

tools/octree_viewer.cpp

+        container->getContainer().getCentroid (pt_centroid);
+      }
+      // Else, compute the centroid of the LeafNode under the current BranchNode
+      else {


{ on a new line

taketwo · 2017-09-07T13:33:50Z

tools/octree_viewer.cpp

+        pcl::PointCloud<pcl::PointXYZ>::VectorType voxelCentroids;
+        octree.getVoxelCentroidsRecursive (static_cast<pcl::octree::OctreePointCloudVoxelCentroid<pcl::PointXYZ>::BranchNode*> (*tree_it), dummy_key, voxelCentroids);
+
+        // Iterate over the leafs to compute the centroid of all of them


We have a dedicated utility class for this: CentroidPoint.

// Add this include #include <pcl/common/centroid.h> // Utility class, instantiate before the loop CentroidPoint<pcl::PointXYZ> centroid; // Inside the loop add points centroid.add (voxelCentroids[j]); // Fetch centroid after the loop centroid.get (pt_centroid);

Note that the line 410 is not necessary.

frozar · 2017-09-08T07:31:57Z

Every comment has been taken into account in the last commit.

taketwo

Besides from one minor style problem everything looks alright for me. When you fix it please also squash the commits and we will be ready to merge.

taketwo · 2017-09-08T07:49:35Z

tools/octree_viewer.cpp

+      cloudVoxel->points.push_back (pt_voxel_center);
+
+      // If the asked depth is the depth of the octree, retrieve the centroid at this LeafNode
+      if (octree.getTreeDepth() == depth){


Still style problems here: missing space and { should go to a new line.

frozar · 2017-09-08T22:21:58Z

Should I squash every commit in a single commit?

frozar · 2017-09-09T08:15:44Z

@SergioRAgostinho : The Travis check failed because a job timeout. Can you handle that please?

taketwo · 2017-09-09T17:23:02Z

I restarted the failed job.
I'd say the point size commit can remain on its own, and the rest can be squashed.

Use the method 'getVoxelCentroidsRecursive' to retrieve every centroids under a given BranchNode and compute the correct global centroid.

SergioRAgostinho · 2017-09-09T17:34:38Z

I restarted it before as well. So it means it didn't complete again :/ it might not get through…

frozar · 2017-09-09T17:36:12Z

The squash of commit is done.

Hope the Travis check will be ok, but let see ^^'

taketwo added this to the pcl-1.9.0 milestone Sep 1, 2017

frozar force-pushed the octree_voxel_centroid branch from d32afd4 to 9fb4686 Compare September 1, 2017 10:06

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

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

taketwo self-assigned this Sep 3, 2017

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

frozar force-pushed the octree_voxel_centroid branch from c361bdc to 4d6e875 Compare September 7, 2017 11:10

taketwo reviewed Sep 7, 2017

View reviewed changes

taketwo removed the needs: code review Specify why not closed/merged yet label Sep 7, 2017

taketwo approved these changes Sep 8, 2017

View reviewed changes

frozar added 2 commits September 9, 2017 19:32

[OCTREE] Compute accurately the centroid of octree in 'octree_viewer'.

aede78e

Use the method 'getVoxelCentroidsRecursive' to retrieve every centroids under a given BranchNode and compute the correct global centroid.

[OCTREE] Save the point size setting between different depth.

6dadd0b

frozar force-pushed the octree_voxel_centroid branch from 6e2e1fc to 6dadd0b Compare September 9, 2017 17:33

taketwo added status: ready to merge and removed needs: author reply Specify why not closed/merged yet labels Sep 11, 2017

SergioRAgostinho merged commit fa9b816 into PointCloudLibrary:master Sep 21, 2017

taketwo removed the status: ready to merge label Sep 23, 2017

frozar deleted the octree_voxel_centroid branch September 27, 2017 18:59

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] Compute accurately the centroids of octree in 'pcl_octree_viewer' #1981

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

frozar commented Sep 1, 2017

taketwo commented Sep 1, 2017 •

edited

Loading

frozar commented Sep 1, 2017

frozar commented Sep 1, 2017

taketwo commented Sep 2, 2017

frozar commented Sep 3, 2017

taketwo commented Sep 6, 2017 •

edited

Loading

frozar commented Sep 7, 2017

taketwo commented Sep 7, 2017

frozar commented Sep 7, 2017

taketwo commented Sep 7, 2017

frozar commented Sep 7, 2017

taketwo Sep 7, 2017

taketwo Sep 7, 2017

taketwo Sep 7, 2017

taketwo Sep 7, 2017

taketwo Sep 7, 2017

taketwo Sep 7, 2017

taketwo Sep 7, 2017

taketwo Sep 7, 2017

frozar commented Sep 8, 2017

taketwo left a comment

taketwo Sep 8, 2017

frozar Sep 8, 2017

frozar commented Sep 8, 2017

frozar commented Sep 9, 2017

taketwo commented Sep 9, 2017

SergioRAgostinho commented Sep 9, 2017 via email

frozar commented Sep 9, 2017

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

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

Conversation

frozar commented Sep 1, 2017

taketwo commented Sep 1, 2017 • edited Loading

frozar commented Sep 1, 2017

frozar commented Sep 1, 2017

taketwo commented Sep 2, 2017

frozar commented Sep 3, 2017

taketwo commented Sep 6, 2017 • edited Loading

frozar commented Sep 7, 2017

taketwo commented Sep 7, 2017

frozar commented Sep 7, 2017

taketwo commented Sep 7, 2017

frozar commented Sep 7, 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

frozar commented Sep 8, 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 Sep 8, 2017

frozar commented Sep 9, 2017

taketwo commented Sep 9, 2017

SergioRAgostinho commented Sep 9, 2017 via email

frozar commented Sep 9, 2017

taketwo commented Sep 1, 2017 •

edited

Loading

taketwo commented Sep 6, 2017 •

edited

Loading