Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add some legacy warppers for Tensor PointCloud #5330

Merged
merged 13 commits into from
Aug 12, 2022

Conversation

yuecideng
Copy link
Collaborator

@yuecideng yuecideng commented Jul 19, 2022

This PR adds wrapper for FarthestPointDownSample, HiddenPointRemoval and SegmentPlane to tensor PointCloud.


This change is Reviewable

@update-docs
Copy link

update-docs bot commented Jul 19, 2022

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

@@ -871,6 +895,27 @@ TEST_P(PointCloudPermuteDevices, RemoveNonFinitePoints) {
{true, true, true, true, true, true}, device)));
}

TEST_P(PointCloudPermuteDevices, HiddenPointRemoval) {
core::Device device = GetParam();
if (!device.IsCPU()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For non-CPU point cloud, we shall first copy to host, process, and copy back.

Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all wrappers over legacy functions, mention in both C++ and Python docs that only CPU device is supported and CUDA data will be copied to the CPU for processing. Also mention which attributes are preserved by the operation.

In pybind code, document return values and add a brief example. See cluster dbscan as an example.

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @reyanshsolis and @yuecideng)


cpp/open3d/t/geometry/PointCloud.h line 402 at r3 (raw file):

    /// \param camera_location All points not visible from that location will be
    /// removed.
    /// \param radius The radius of the spherical projection.

Add docs for return value (\return)


cpp/open3d/t/geometry/PointCloud.cpp line 858 at r3 (raw file):

            TriangleMesh::FromLegacy(*lmesh, GetPointPositions().GetDtype(),
                                     core::Int64, GetDevice()),
            core::Tensor(indices, {(int64_t)indices.size()}, core::Int64,

Use core::Tensor(std::move(indices)).To(GetDevice()) (Tensor ctor that takes ownership of vector data). This will prevent a copy if data is on the CPU.

Since we know the indices of the retained points, copy over all point attributes for the retained points.


cpp/open3d/t/geometry/PointCloud.cpp line 892 at r3 (raw file):

    return std::make_tuple(
            core::eigen_converter::EigenMatrixToTensor(plane).Flatten(),
            core::Tensor(indices, {(int64_t)indices.size()}, core::Int64,

Use core::Tensor(std::move(indices)).To(GetDevice()) (Tensor ctor that takes ownership of vector data). This will prevent a copy if data is on the CPU.

Copy link
Collaborator Author

@yuecideng yuecideng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to update the docs in pybind file. Please see whether it is acceptable.

Reviewable status: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @reyanshsolis, @ssheorey, and @yuecideng)


cpp/open3d/t/geometry/PointCloud.h line 402 at r3 (raw file):

Previously, ssheorey (Sameer Sheorey) wrote…

Add docs for return value (\return)

Done.


cpp/open3d/t/geometry/PointCloud.cpp line 858 at r3 (raw file):

Previously, ssheorey (Sameer Sheorey) wrote…

Use core::Tensor(std::move(indices)).To(GetDevice()) (Tensor ctor that takes ownership of vector data). This will prevent a copy if data is on the CPU.

Since we know the indices of the retained points, copy over all point attributes for the retained points.

Done.


cpp/open3d/t/geometry/PointCloud.cpp line 892 at r3 (raw file):

Previously, ssheorey (Sameer Sheorey) wrote…

Use core::Tensor(std::move(indices)).To(GetDevice()) (Tensor ctor that takes ownership of vector data). This will prevent a copy if data is on the CPU.

Done.

@yxlao yxlao merged commit 07601e7 into master Aug 12, 2022
@yxlao yxlao deleted the yueci/wrap-legacy-t-pointcloud branch August 12, 2022 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants