Add point cloud plane segmentation

[kongsgard]

Implementation of plane segmentation for PointCloud using the RANSAC algorithm.
It will find the plane with the greatest number of points in the cloud. Therefore, if the cloud has several planes, the model can be rerun multiple times until all planes are found by removing the inliers each time.

Adresses issue #591.

Example segmentation for fragment.pcd with 50 iterations is given below:

---

This change is 

[kongsgard]

This could be extended with segmentation of cylinders, spheres, etc. later.
Should the code be in Geometry, or would it be appropriate with a subdirectory Segmentation, as is done for e.g. Registration?

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions

---

src/Open3D/Geometry/PointCloud.h, line 226 at r1 (raw file):

            double distance_threshold = 0.01,
            int ransac_n = 3,
            const registration::RANSACConvergenceCriteria &criteria =

Unsure whether it's best to keep RANSACConvergenceCriteria here, or simply add e.g. int num_iterations.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 121 at r1 (raw file):

            inliers[i] = exchange(inliers[rng() % inliers.size()], indices[i]);
        }
        plane_model = TriangleMesh::ComputeTrianglePlane(

This function could be useful in several instances. Perhaps it could be moved to a more generic class?

[yxlao]

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @kongsgard)

---

src/Open3D/Geometry/PointCloud.h, line 222 at r1 (raw file):

    /// each iteration.
    /// \param criteria The maximum number of iterations. See \ref
    /// RANSACConvergenceCriteria.

Add \return to describe the return value.

---

src/Open3D/Geometry/PointCloud.h, line 226 at r1 (raw file):

Previously, kongsgard (Sondre Kongsgård) wrote…

Unsure whether it's best to keep RANSACConvergenceCriteria here, or simply add e.g. int num_iterations.

LGTM in the current way

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 102 at r1 (raw file):

    for (int itr = 0;
         itr < criteria.max_iteration_ && itr < criteria.max_validation_;

What is the difference between max_iteration and max_validation here, and why in Registration we need both and here we essentially only use one of them?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 104 at r1 (raw file):

         itr < criteria.max_iteration_ && itr < criteria.max_validation_;
         itr++) {
        if (prev_inliers_num == inliers.size()) {

Why do we need to check prev_inliers_num == inliers.size(), shouldn't we always random sample at reach iteration?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 106 at r1 (raw file):

        if (prev_inliers_num == inliers.size()) {
            for (int i = 0; i < ransac_n; ++i) {
                indices[i] = exchange(indices[rng() % num_points], indices[i]);

a = exchange(b, a); seems to be equivalent to std::swap(a, b), is it true?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 111 at r1 (raw file):

            inliers.clear();
            for (size_t idx = 0; idx < ransac_n; ++idx) {
                inliers.emplace_back(indices[idx]);

maybe std::random_shuffle will make this part cleaner, can you check?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 118 at r1 (raw file):

        // Fit model to num_model_parameters randomly selected points among the
        // inliers.
        for (int i = 0; i < num_model_parameters; ++i) {

Looks like you're sampling num_model_parameters points out of ransac_n points, where the ransac_n points are sampled from the original point cloud.

Why not sample num_model_parameters points directly from the original point cloud?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 121 at r1 (raw file):

Previously, kongsgard (Sondre Kongsgård) wrote…

This function could be useful in several instances. Perhaps it could be moved to a more generic class?

Yeah, we could consider that. We can revisit this later.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 141 at r1 (raw file):

        auto this_result =
                EvaluateRANSACBasedOnDistance(inliers, points_, error);

Will be cleaner if we move the inliers and error computation inside EvaluateRANSACBasedOnDistance?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 146 at r1 (raw file):

             this_result.inlier_rmse_ < result.inlier_rmse_)) {
            result = this_result;
            best_plane_model = plane_model;

Seems like we're always fitting the plane with 3 points. While in https://en.wikipedia.org/wiki/Random_sample_consensus, they fit the final model with selected with all selected inliers.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 168 at r1 (raw file):

}  // namespace geometry
}  // namespace open3d

add \n at the end of the file.

---

src/Python/open3d_pybind/geometry/pointcloud.cpp, line 175 at r1 (raw file):

                 "labels, -1 indicates noise according to the algorithm.",
                 "eps"_a, "min_points"_a, "print_progress"_a = false)
            .def("segment_plane", &geometry::PointCloud::SegmentPlane,

Add parameters, as well as their default values. See the "cluster_dbscan" binding for example.

It will look something like this:

.def("segment_plane", &geometry::PointCloud::SegmentPlane,
     "Segments a plane in the point cloud using the RANSAC "
     "algorithm.", "distance_threshold"_a, "ransac_n"_a, other_params...)

---

src/Python/open3d_pybind/geometry/pointcloud.cpp, line 177 at r1 (raw file):

            .def("segment_plane", &geometry::PointCloud::SegmentPlane,
                 "Segments a plane in the point cloud using the RANSAC "
                 "algorithm.")

For new functionalities, we usually add a python example in examples/Python/Basic or examples/Python/Advanced. Looks like you ready have example code that processes fragment.pcd. Could you add the example?

---

src/Python/open3d_pybind/geometry/pointcloud.cpp, line 300 at r1 (raw file):

             {"print_progress",
              "If true the progress is visualized in the console."}});
    docstring::ClassMethodDocInject(m, "PointCloud", "segment_plane");

Add a dictionary mapping "parameter name" to "docstring", see cluster_dbscan above as an example.

[griegler]

In my opinion the plane segmentation is fine in Geometry. However, I think it would be a better choice to move the RANSAC related stuff to its own directory.

Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @kongsgard and @yxlao)

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 43 at r1 (raw file):

// std::exchange introduced in C++14
template <class T, class U = T>
T exchange(T &obj, U &&new_value) {

This might be better placed in Utility.h

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 49 at r1 (raw file):

}

class RANSACResult {

please add some doc describing the purpose.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 59 at r1 (raw file):

};

RANSACResult EvaluateRANSACBasedOnDistance(

please add some doc describing the purpose.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 162 at r1 (raw file):

    }

    utility::LogDebug("RANSAC | Inliers: {:d}, Fitness: {:.4f}, RMSE: {:.4f}",

maybe use {:e} for fitness and rmse.

---

src/Python/open3d_pybind/geometry/pointcloud.cpp, line 177 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

For new functionalities, we usually add a python example in examples/Python/Basic or examples/Python/Advanced. Looks like you ready have example code that processes fragment.pcd. Could you add the example?

Also a minimal unit test would be nice. I guess that would require to pass the random seed somehow.

[kongsgard]

Reviewable status: 0 of 6 files reviewed, 20 unresolved discussions (waiting on @griegler, @kongsgard, and @yxlao)

---

src/Open3D/Geometry/PointCloud.h, line 222 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

Add \return to describe the return value.

I added a brief description. Should it follow any standard template?

---

src/Open3D/Geometry/PointCloud.h, line 226 at r2 (raw file):

            const double distance_threshold = 0.01,
            const int ransac_n = 3,
            const int num_iterations = 100) const;

I changed to num_iterations for now, as I had some issues with the Python bindings when using RANSACConvergenceCriteria. How can one include RANSACConvergenceCriteria without duplicating this code?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 43 at r1 (raw file):

Previously, griegler (Gernot) wrote…

This might be better placed in Utility.h

Based on the feedback below, I've changed to std::swap instead. Thus, this function is no longer needed.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 49 at r1 (raw file):

Previously, griegler (Gernot) wrote…

please add some doc describing the purpose.

I added a comment above the class. Does it follow the right format?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 59 at r1 (raw file):

Previously, griegler (Gernot) wrote…

please add some doc describing the purpose.

Done.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 102 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

What is the difference between max_iteration and max_validation here, and why in Registration we need both and here we essentially only use one of them?

There is no difference here, so we could potentially remove max_validation.

If we follow the RANSAC algorithm closely, we should only validate the model if the number of inliers is greater than a given constant. However, I think there are some disadvantages with that in this specific case. The validation step is fast performance-wise since the number of potential inliers have to be calculated anyways, and it would add another parameter that might be difficult to set correctly.

Correct me if I'm wrong, but although max_validation is used in the functions RegistrationRANSACBasedOnCorrespondence and RegistrationRANSACBasedOnFeatureMatching it doesn't seem to have any functionality that max_iteration_ doesn't already cover? Should the validation steps in these functions be done conditionally?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 104 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

Why do we need to check prev_inliers_num == inliers.size(), shouldn't we always random sample at reach iteration?

Yes, that's right. Removed.
I was thinking that sampling out of a smaller subset could give better results if the number of iterations is low, but that probably has no effect in practice.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 106 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

a = exchange(b, a); seems to be equivalent to std::swap(a, b), is it true?

They seem to be more or less equivalent - changed to std::swap(a, b).

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 111 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

maybe std::random_shuffle will make this part cleaner, can you check?

I think that would make the code run much slower, as std::random_shuffle runs in linear time and would necessitate a shuffle of the entire indices vector. With the current implementation, only ransac_n swaps are done.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 118 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

Looks like you're sampling num_model_parameters points out of ransac_n points, where the ransac_n points are sampled from the original point cloud.

Why not sample num_model_parameters points directly from the original point cloud?

Good point. Done.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 141 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

Will be cleaner if we move the inliers and error computation inside EvaluateRANSACBasedOnDistance?

Done.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 146 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

Seems like we're always fitting the plane with 3 points. While in https://en.wikipedia.org/wiki/Random_sample_consensus, they fit the final model with selected with all selected inliers.

I added a new function to find the plane which best matches all the selected inliers for the final model.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 162 at r1 (raw file):

Previously, griegler (Gernot) wrote…

maybe use {:e} for fitness and rmse.

Done.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 168 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

add \n at the end of the file.

Done.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 90 at r2 (raw file):

// Reference:
// https://www.ilikebigbits.com/2015_03_04_plane_from_points.html
Eigen::Vector4d GetPlaneFromPoints(const std::vector<Eigen::Vector3d> &points,

I can add this function to the PointCloud class, if preferable.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 123 at r2 (raw file):

    }

    double norm = abc.norm();

There is some duplicated logic here, e.g. from TriangleMesh::ComputeTrianglePlane. I'm open to suggestions on how to make this more compact.

---

src/Python/open3d_pybind/geometry/pointcloud.cpp, line 175 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

Add parameters, as well as their default values. See the "cluster_dbscan" binding for example.

It will look something like this:

.def("segment_plane", &geometry::PointCloud::SegmentPlane,
     "Segments a plane in the point cloud using the RANSAC "
     "algorithm.", "distance_threshold"_a, "ransac_n"_a, other_params...)

Done.

---

src/Python/open3d_pybind/geometry/pointcloud.cpp, line 177 at r1 (raw file):

Previously, griegler (Gernot) wrote…

Also a minimal unit test would be nice. I guess that would require to pass the random seed somehow.

Added the example.

For the unit test, I'm not sure how we could pass the random seed. Could it be possible to set a preprocessor variable from the unit test?
Alternatively, we could create some simple test data with very few outliers. The probability of finding the correct inliers should then be close to 100% given enough iterations. Would that work?

---

src/Python/open3d_pybind/geometry/pointcloud.cpp, line 300 at r1 (raw file):

Previously, yxlao (Yixing Lao) wrote…

Add a dictionary mapping "parameter name" to "docstring", see cluster_dbscan above as an example.

Done. Is it possible to add a reference to RANSACConvergenceCriteria as is done in the C++ documentation?

[griegler]

Reviewed 4 of 6 files at r2.
Reviewable status: 4 of 6 files reviewed, 19 unresolved discussions (waiting on @griegler, @kongsgard, and @yxlao)

---

examples/Python/Basic/pointcloud_plane_segmentation.py, line 16 at r2 (raw file):

        "Find the plane model and the inliers of the largest planar segment in the point cloud."
    )
    plane_model, inliers = pcd.segment_plane(0.01, 3, 250)

Would be nice to use the keywords for the function arguments.

---

src/Open3D/Geometry/PointCloud.h, line 222 at r1 (raw file):

Previously, kongsgard (Sondre Kongsgård) wrote…

I added a brief description. Should it follow any standard template?

Looks good

---

src/Open3D/Geometry/PointCloud.h, line 226 at r2 (raw file):

Previously, kongsgard (Sondre Kongsgård) wrote…

I changed to num_iterations for now, as I had some issues with the Python bindings when using RANSACConvergenceCriteria. How can one include RANSACConvergenceCriteria without duplicating this code?

@yxlao can you help here?

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 49 at r1 (raw file):

Previously, kongsgard (Sondre Kongsgård) wrote…

I added a comment above the class. Does it follow the right format?

@yxlao oversees the C++ documentation. I am sure he can point you to some guidelines.

---

src/Python/open3d_pybind/geometry/pointcloud.cpp, line 177 at r1 (raw file):

Previously, kongsgard (Sondre Kongsgård) wrote…

Added the example.

For the unit test, I'm not sure how we could pass the random seed. Could it be possible to set a preprocessor variable from the unit test?
Alternatively, we could create some simple test data with very few outliers. The probability of finding the correct inliers should then be close to 100% given enough iterations. Would that work?

I think a simple test with no outliers would be sufficient for now. We have to work on this problem in another PR.

[kongsgard]

Reviewable status: 2 of 7 files reviewed, 19 unresolved discussions (waiting on @griegler, @kongsgard, and @yxlao)

---

examples/Python/Basic/pointcloud_plane_segmentation.py, line 16 at r2 (raw file):

Previously, griegler (Gernot) wrote…

Would be nice to use the keywords for the function arguments.

Done.

---

src/Python/open3d_pybind/geometry/pointcloud.cpp, line 177 at r1 (raw file):

Previously, griegler (Gernot) wrote…

I think a simple test with no outliers would be sufficient for now. We have to work on this problem in another PR.

Done.

[griegler]

Waiting for @yxlao for response.

Reviewed 3 of 4 files at r3.
Reviewable status: 5 of 7 files reviewed, 20 unresolved discussions (waiting on @griegler, @kongsgard, and @yxlao)

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 49 at r1 (raw file):

Previously, griegler (Gernot) wrote…

@yxlao oversees the C++ documentation. I am sure he can point you to some guidelines.

Here is an example you could follow: https://github.com/intel-isl/Open3D/blob/master/src/Open3D/Camera/PinholeCameraIntrinsic.h

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 150 at r3 (raw file):

    size_t num_points = points_.size();
    std::vector<size_t> indices(num_points);
    std::iota(std::begin(indices), std::end(indices), 0);

I think this requires #include <numeric> to satisfy VS compiler.

---

src/UnitTest/Geometry/PointCloud.cpp, line 1070 at r3 (raw file):

    geometry::PointCloud pc;

    for (int i = 0; i < ref.size(); i++) {

size_t i

[kongsgard]

Reviewable status: 4 of 7 files reviewed, 20 unresolved discussions (waiting on @griegler, @kongsgard, and @yxlao)

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 49 at r1 (raw file):

Previously, griegler (Gernot) wrote…

Here is an example you could follow: https://github.com/intel-isl/Open3D/blob/master/src/Open3D/Camera/PinholeCameraIntrinsic.h

Thanks for the link. Adapted the comments accordingly.

---

src/Open3D/Geometry/PointCloudSegmentation.cpp, line 150 at r3 (raw file):

Previously, griegler (Gernot) wrote…

I think this requires #include <numeric> to satisfy VS compiler.

Added.

---

src/UnitTest/Geometry/PointCloud.cpp, line 1070 at r3 (raw file):

Previously, griegler (Gernot) wrote…

size_t i

Done.

[prass-anyvision]

Could I help, this would be really useful for me? Is the code already usable from the branch?

[griegler]

What I can see we are waiting for @yxlao to approve the changes.

[kongsgard]

Fixed the Python style formatting.
Let me know if there is anything else that should be done before this PR can be merged.

@prass-anyvision You could add a triangular mesh with 3-4 vertices acquired from the plane equation (select x, y arbitrary and solve for z) to draw a plane.
Could you share the point cloud you've used in your example? It might be necessary to increase the number of iterations, depending on the number of points in the point cloud.

[gamahirawan]

Hi, I interested to this tools and tried it directly. However I got error notification at line 16

open3d.open3d.geometry.PointCloud' object has no attribute 'segment_plane' I am not sure is it problem in the code or I didn't run the code properly? Thanks

[prass-anyvision]

Hi, I interested to this tools and tried it directly. However I got error notification at line 16

open3d.open3d.geometry.PointCloud' object has no attribute 'segment_plane' I am not sure is it problem in the code or I didn't run the code properly? Thanks

It has not yet been merged in....

[Patrick19941221]

Reviewed 3 of 6 files at r2, 1 of 4 files at r3, 2 of 2 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 20 unresolved discussions (waiting on @griegler, @kongsgard, and @yxlao)

[xiongzubiao]

Bump. This feature is really useful.

[yxlao]

Reviewed 1 of 4 files at r3.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @griegler, @kongsgard, and @yxlao)

---

src/Open3D/Geometry/PointCloud.h, line 226 at r2 (raw file):

Previously, griegler (Gernot) wrote…

@yxlao can you help here?

RANSACConvergenceCriteria contains max_iteration and max_validation, but we only need max_validation here so it shall be fine to keep it as it is.