[Depreciating RegistrationColoredICP function]. Legacy ColoredICP Refactoring.

[reyanshsolis]

This PR depreciates the function RegistationColoredICP from legacy, to resolve the following issues:

• The behavior of passing TransformationEstimationType::ColoredICP to the function RegistrationICP was not defined, hence segmentation fault.
• Due to the previous design, it was difficult to add MultiScaleICP support to legacy.

Changes:

• Removed RegistrationColoredICP, made required changes to generalize RegistrationICP. Now one may directly use RegistrationICP / registration_icp function for colored icp, by passing TransformationEstimationType::ColoredICP, like they do for PointToPoint and PointToPlane.
• Exposed color_gradients_ attribute for legacy pointcloud and added changes to the required functions.
• Exposed EstimateColorGradients function.
• Added pragma omp parallel for construct in EstimateColorGradients at points level. [Performance may be further improved].
• MultiScaleICP support in legacy. [Exposing color_gradient and implementing multi_scale_icp may have combined performance improvement as previously users had to manually implement it for multi-scale (the paper suggests better converge in colored-icp for multi-scale), which made the function reigstration_colored_icp compute color_gradient for each scale].
• Added colored_icp to benchmarks. Added pybinds and unit test.
• Updated examples and reconstruction_system to use the newly added supports.

TODO:

• Modify cpp and python examples.
• Verify reconstruction system result.

Change 1: registration_icp can be directly used, instead of registration_colored_icp.

Change 2: MultiScale ICP support.

Performance Improvement:

Example: test_data/ColoredICP/flag_115.ply | flag_116.ply
(voxel_size = 0.0125, iterations = 20)

Before: 378 ms    |    After: 240 ms

To verify that colored icp is still working the same:

---

This change is 

[update-docs]

Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[ssheorey]

Line #8.    criteria_list = [

Simplify o3d calls with some imports:

from o3d.pipelines.registration import ICPConvergenceCriteria, TransformationEstimationForColoredICP, registration_multi_scale_icp

from o3d.geometry import KDTreeSearchParamHybrid

Reply via ReviewNB

[ssheorey]

typo: depreciated -> deprecated

Reply via ReviewNB

[ssheorey]

Looks good to me, other than the minor comments / typo in the review.
Pretty comprehensive update!

Verify reconstruction system result. is also done, right?

Reviewable status: 0 of 19 files reviewed, 16 unresolved discussions (waiting on @reyanshsolis and @yxlao)

---

cpp/open3d/geometry/PointCloud.cpp, line 106 at r1 (raw file):

    if ((!HasPoints() || HasNormals()) && cloud.HasNormals()) {
        normals_.resize(new_vert_num);
#pragma omp parallel for schedule(static)

This is likely too fine grained an operation and memory access bound to benefit from multi threading. It may even slow things down.

---

cpp/open3d/geometry/PointCloud.cpp, line 115 at r1 (raw file):

    if ((!HasPoints() || HasColors()) && cloud.HasColors()) {
        colors_.resize(new_vert_num);
#pragma omp parallel for schedule(static)

Same here.

---

cpp/open3d/geometry/PointCloud.cpp, line 124 at r1 (raw file):

    if ((!HasPoints() || HasColorGradients()) && cloud.HasColorGradients()) {
        colors_.resize(new_vert_num);
#pragma omp parallel for schedule(static)

Same...

---

cpp/open3d/geometry/PointCloud.cpp, line 133 at r1 (raw file):

    points_.resize(new_vert_num);
#pragma omp parallel for schedule(static)
    for (int i = 0; i < add_vert_num; i++) {

..here

---

cpp/open3d/geometry/PointCloud.cpp, line 356 at r1 (raw file):

    }

    for (auto accpoint : voxelindex_to_accpoint) {

This doesn't look necessary. It's a good idea to keep changes to only those required to speed up reviews.

---

cpp/open3d/geometry/PointCloud.cpp, line 414 at r1 (raw file):

                *this, static_cast<size_t>(i), cid, approximate_class);
    }

Same...

---

cpp/open3d/geometry/PointCloud.cpp, line 616 at r1 (raw file):

            size_t nn = point_idx.size();
            Eigen::MatrixXd A(nn, 3);
            Eigen::MatrixXd b(nn, 1);

nit: A, b memory can be reused for all points by making them thread_local and defining them before the parallel for.

---

cpp/open3d/pipelines/registration/Registration.h, line 189 at r1 (raw file):

/// \param target The target point cloud.
/// \param voxel_sizes VectorDouble of voxel scales of type double.
/// \param criterias Vector of ICPConvergenceCriteria objects for each scale.

Mention example or default values for ICPConvergenceCriteria, if any.

---

cpp/open3d/pipelines/registration/Registration.h, line 193 at r1 (raw file):

/// points-pair distances of type double, for each iteration. Must be of same
/// length as voxel_sizes and criterias.
/// \param init Initial transformation estimation.

Default values?

---

cpp/open3d/pipelines/registration/Registration.h, line 205 at r1 (raw file):

Depreciated

\deprecated Please use the general RegistrationICP instead.
(doxygen markup)

---

cpp/open3d/pipelines/registration/Registration.cpp, line 195 at r1 (raw file):

        target_copy.EstimateColorGradients(geometry::KDTreeSearchParamHybrid(
                max_correspondence_distances[num_iterations - 1] * 2.0, 30));
        target_ptr = std::make_shared<geometry::PointCloud>(target_copy);

Can skip this if color_gradients_ is already present.
nit: A plain pointer is OK instead of a shared_ptr since we are not passing the data to another function.

---

cpp/open3d/pipelines/registration/Registration.cpp, line 287 at r1 (raw file):

Depreciated

Deprecated (typo)

---

cpp/open3d/pipelines/registration/TransformationEstimation.cpp, line 205 at r1 (raw file):

                         1.0 - nt(1) * nt(1), -nt(1) * nt(2), -nt(0) * nt(2),
                         -nt(1) * nt(2), 1.0 - nt(2) * nt(2))
                                .finished();

nit: Arrange in 3x3 grid for easier reading. Use // clang-format off if it complains.

---

cpp/pybind/pipelines/registration/registration.cpp, line 598 at r1 (raw file):

    m.def("registration_colored_icp", &RegistrationColoredICP,
          py::call_guard<py::gil_scoped_release>(),
          "[Depreciated Function] Colored ICP registration", "source"_a,

deprecated (typo)

---

examples/python/reconstruction_system/colored_icp.py, line 82 at r1 (raw file):

                                                          relative_rmse=1e-6,
                                                          max_iteration=14)
    ]

Simplify open3d function calls with some imports (as in the Jupyter notebook).

[ssheorey]

We should merge PR #3181 before this, since there may be some updates needed to this PR from changes there.

[heethesh]

@reyanshsolis can this be merged now? Color ICP segfaults on 0.13.0