Add compute bounding box member method #3970

truhoang · 2020-04-24T06:25:08Z

As requested by @kunaltyagi in #3946

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

kunaltyagi

LGTM except inline

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

kunaltyagi

LGTM, except the naming of the function (but that's not a blocking concern, just a bikeshedding topic)

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

kunaltyagi

I missed that 👀

kunaltyagi · 2020-05-20T22:14:23Z

@Morwenn could you give this a look over again please?

Also, thoughts on renaming the function to use BBox or AABB instead of longer BoundingBox? The word template isn't needed in either the function name or the parameter name. A better idea for the name?

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

truhoang · 2020-05-21T22:27:41Z

@Morwenn Updated with suggestions

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

kunaltyagi · 2020-06-04T13:56:59Z

Ubuntu Build is failing

…efactor-eigen

SergioRAgostinho · 2020-07-01T09:52:26Z

recognition/include/pcl/recognition/linemod/line_rgbd.h

@@ -282,6 +282,10 @@ namespace pcl
      bool 
      readLTMHeader (int fd, pcl::io::TARHeader &header);

+      /** \brief Compute bounding box from PCD masks **/


Missing parameter description

SergioRAgostinho · 2020-07-01T09:55:16Z

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

+  BoundingBoxXYZ bb;
+  bb.x = bb.y = bb.z = std::numeric_limits<float>::max ();


Move this definition to where you actually use it. Line 187. No need to initialize it's members x, y, and z

SergioRAgostinho · 2020-07-01T09:59:28Z

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

+  Eigen::Vector4f geometric_center = Eigen::Vector4f::Zero ();
+  Eigen::Vector4f min_pos, max_pos;


Use Eigen::Arrays and instead of matrices to express better your intent for coefficient-wise operations.

SergioRAgostinho · 2020-07-01T10:10:14Z

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

+  for (const PointXYZRGBA & p : template_point_cloud.points)
+  {
+    if (!isXYZFinite(p))
+      continue;

-    center_x /= static_cast<float> (counter);
-    center_y /= static_cast<float> (counter);
-    center_z /= static_cast<float> (counter);
+    min_pos = min_pos.cwiseMin (p.getVector4fMap ());
+    max_pos = max_pos.cwiseMax (p.getVector4fMap ());

-    bb.width  = max_x - min_x;
-    bb.height = max_y - min_y;
-    bb.depth  = max_z - min_z;
+    geometric_center += p.getVector4fMap ();

-    bb.x = (min_x + bb.width / 2.0f) - center_x - bb.width / 2.0f;
-    bb.y = (min_y + bb.height / 2.0f) - center_y - bb.height / 2.0f;
-    bb.z = (min_z + bb.depth / 2.0f) - center_z - bb.depth / 2.0f;
+    ++counter;
+  }
+  geometric_center /= static_cast<float> (counter);


Have a look at this to prevent the need for having a foor loop, in case the point cloud is dense.

dim=4
stride=sizeof(PointXYZRGBA)/sizeof(float) (confirm this is an actual integer even if you did floating point division. it should be 5)
offset=0

Then use eigen function for mean and extracting min and max element.

SergioRAgostinho · 2020-07-01T10:13:55Z

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

+    geometric_center += p.getVector4fMap ();

-    bb.x = (min_x + bb.width / 2.0f) - center_x - bb.width / 2.0f;
-    bb.y = (min_y + bb.height / 2.0f) - center_y - bb.height / 2.0f;
-    bb.z = (min_z + bb.depth / 2.0f) - center_z - bb.depth / 2.0f;
+    ++counter;
+  }
+  geometric_center /= static_cast<float> (counter);


This is not a geometric center, this is the center of mass of all points. Did you expect this to be the center of the bounding box? If so, this is a bug.

SergioRAgostinho · 2020-07-01T10:17:35Z

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

+  Eigen::Vector4f bb_dim = max_pos - min_pos;
+  bb.width  = bb_dim[0];
+  bb.height = bb_dim[1];
+  bb.depth  = bb_dim[2];


To compute the geometric center

Eigen::Vector4f geometric_center = 0.5*(max_pos + min_pos);

SergioRAgostinho · 2020-07-01T10:18:08Z

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

-      p.x -= center_x;
-      p.y -= center_y;
-      p.z -= center_z;
+  for (PointXYZRGBA & p : template_point_cloud.points)


for (PointXYZRGBA & p : template_point_cloud)

SergioRAgostinho · 2020-07-01T10:20:35Z

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

-    bb.x = (min_x + bb.width / 2.0f) - center_x - bb.width / 2.0f;
-    bb.y = (min_y + bb.height / 2.0f) - center_y - bb.height / 2.0f;
-    bb.z = (min_z + bb.depth / 2.0f) - center_z - bb.depth / 2.0f;


🤦‍♂️ I'm embarrassed this exists in our code base.

SergioRAgostinho · 2020-07-01T10:21:06Z

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp

+  for (PointXYZRGBA & p : template_point_cloud.points)
+  {
+    if (!isXYZFinite(p))
+      continue;

-      template_point_cloud[j] = p;
-    }
+    p.getVector4fMap () -= geometric_center;


No need to check if the point is finite. if it isn't the subtraction operation will generate more nans on the same points. Meaning you can use a matrix wise row subtraction with the Mapping I mentioned earlier.

stale · 2020-08-01T11:34:23Z

Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this.

truhoang added 2 commits April 23, 2020 23:17

Add compute bounding box member method

ab728a1

Refactor position coordinates with Eigen vectors

0dc36ce

kunaltyagi requested changes Apr 25, 2020

View reviewed changes

kunaltyagi added changelog: enhancement Meta-information for changelog generation module: recognition needs: more work Specify why not closed/merged yet labels Apr 25, 2020

Use eigen interface to access point's coordinates

54452ab

kunaltyagi requested changes Apr 26, 2020

View reviewed changes

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

Change function interface and remove auto

ec1adc6

kunaltyagi reviewed Apr 27, 2020

View reviewed changes

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

kunaltyagi reviewed Apr 27, 2020

View reviewed changes

Change function name

7c9940e

kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels Apr 27, 2020

kunaltyagi reviewed Apr 30, 2020

View reviewed changes

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Apr 30, 2020

kunaltyagi reviewed May 3, 2020

View reviewed changes

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

Use eigen min/max functions

999d30f

truhoang force-pushed the refactor-eigen branch from 0ebefdb to 999d30f Compare May 4, 2020 03:28

kunaltyagi approved these changes May 4, 2020

View reviewed changes

kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels May 4, 2020

Morwenn reviewed May 6, 2020

View reviewed changes

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

Fix Eigen min/ma assignment

593be52

kunaltyagi approved these changes May 7, 2020

View reviewed changes

Morwenn reviewed May 20, 2020

View reviewed changes

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

Morwenn reviewed May 20, 2020

View reviewed changes

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

Morwenn reviewed May 20, 2020

View reviewed changes

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

Clean up for-loops and remove redundancies

7db6764

kunaltyagi requested changes May 29, 2020

View reviewed changes

recognition/include/pcl/recognition/impl/linemod/line_rgbd.hpp Outdated Show resolved Hide resolved

kunaltyagi added needs: more work Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels May 29, 2020

Replace vector3f with vector4f

614a1dd

truhoang added 4 commits June 29, 2020 09:45

Merge branch 'master' into refactor-eigen

9a53321

Empty commit to start CI

e5d5533

Merge branch 'refactor-eigen' of ssh://github.com/truhoang/pcl into r…

18c4f75

…efactor-eigen

Fix eigen vector type bug

fd9b041

SergioRAgostinho reviewed Jul 1, 2020

View reviewed changes

stale bot added the status: stale label Aug 1, 2020

		BoundingBoxXYZ bb;
		bb.x = bb.y = bb.z = std::numeric_limits<float>::max ();

		Eigen::Vector4f geometric_center = Eigen::Vector4f::Zero ();
		Eigen::Vector4f min_pos, max_pos;

Uh oh!

Add compute bounding box member method #3970

Are you sure you want to change the base?

Add compute bounding box member method #3970

Uh oh!

Conversation

truhoang commented Apr 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

kunaltyagi commented May 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

truhoang commented May 21, 2020

Uh oh!

Uh oh!

kunaltyagi commented Jun 4, 2020

Uh oh!

SergioRAgostinho Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho Jul 1, 2020

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Aug 1, 2020

Uh oh!

Uh oh!

kunaltyagi commented May 20, 2020 •

edited

Loading

SergioRAgostinho Jul 1, 2020 •

edited

Loading

SergioRAgostinho Jul 1, 2020 •

edited

Loading