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

Implement open3d::t::geometry::TriangleMesh::SelectByIndex #6415

Merged
merged 3 commits into from
Nov 13, 2023

Conversation

nsaiapova
Copy link
Collaborator

@nsaiapova nsaiapova commented Oct 6, 2023

The method uses tensor API. It takes a list of indices and returns a new mesh built with the selected vertices and triangles formed by these vertices.

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes #
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) Resolves #

Motivation and Context

The method is available with the open3d::geometry::TriangleMesh but not in open3d::t::geometry::TriangleMesh.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • [?] Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly. <---- here I am not sure about the Python doc, as I could not generate it locally.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

The implementation is inspired by
open3d::geometry::TriangleMesh::SelectByIndex.
and by
open3d::t::geometry::TriangleMesh::SelectFacesByMask.

We first compute a mask of vertices to be selected. If the input index exceeds the maximum number of vertices, we throw an exception. Based on the vertex mask we build a mapping index vector using inclusive prefix sum algorithm. We then update the selected vertex indices in the triangles CPU copy and build the triangles mask. I put that to a templated helper static function to avoid the code duplication for Int32 and Int64 vertex indices types.

With the computed masks we can select the vertices and triangles back on the mesh's device and copy the attributes.

FYI @ssheorey


This change is Reviewable

@update-docs
Copy link

update-docs bot commented Oct 6, 2023

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


// really copy triangles to CPU as we will modify the indices
core::Tensor tris_cpu =
GetTriangleIndices().To(core::Device(), true).Contiguous();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The true argument here makes the "true" copy, which the SelectFacesByMask does not do. In my case, if I don't have it, the triangles do not match the original mesh if the device is CPU.

Copy link
Member

Choose a reason for hiding this comment

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

(...,/*copy=*/true) is a good way to indicate the name of the argument you are supplying.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

which the SelectFacesByMask does not do

Not a problem at SelectFacesByMask, as it already operates on a copy made by GetIndex(mask).
After extracting the common parts with SelectFacesByMask, I also stopped needing the true copy.

@nsaiapova nsaiapova force-pushed the wip/tmesh-select-by-index branch from e3d47da to 1b1c507 Compare October 6, 2023 21:42
@ssheorey ssheorey self-requested a review October 7, 2023 04:46
cpp/open3d/t/geometry/TriangleMesh.cpp Outdated Show resolved Hide resolved
cpp/tests/t/geometry/TriangleMesh.cpp Outdated Show resolved Hide resolved
cpp/open3d/t/geometry/TriangleMesh.cpp Outdated Show resolved Hide resolved
@nsaiapova nsaiapova force-pushed the wip/tmesh-select-by-index branch from 54aa4b3 to 790fc34 Compare October 13, 2023 15:07
@nsaiapova
Copy link
Collaborator Author

nsaiapova commented Oct 13, 2023

Comment updated on 17.10.2023
In the last fixup 790fc34 I pushed more tests and a few fixes for issues found by these tests, such as:

  1. segfault on a negative index;
  2. exception for an empty mesh while trying access vertices/triangles, although here I might be too strict to not accept meshes w/o triangles;
  3. int32 indices - int64 tringle indices case, as there was an error with accessing int64 indices via GetDataPtr<int32_t>. To fix that I introduced the second templated parameter for indices type itself.

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.

Looks great! Some minor comments about corner cases and a refactoring suggestion.

cpp/open3d/t/geometry/TriangleMesh.cpp Show resolved Hide resolved

// really copy triangles to CPU as we will modify the indices
core::Tensor tris_cpu =
GetTriangleIndices().To(core::Device(), true).Contiguous();
Copy link
Member

Choose a reason for hiding this comment

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

(...,/*copy=*/true) is a good way to indicate the name of the argument you are supplying.

Comment on lines 1112 to 1121
core::AssertTensorShape(indices, {indices.GetLength()});
if (!HasTriangleIndices()) {
utility::LogError("[SelectByIndex] mesh has no triangle indices.");
}
if (!HasVertexPositions()) {
utility::LogError("[SelectByIndex] mesh has no vertex positions.");
}
Copy link
Member

Choose a reason for hiding this comment

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

We can return an empty mesh for corner case such as when indices are empty.

cpp/open3d/t/geometry/TriangleMesh.cpp Show resolved Hide resolved
This is a helper to call a templated function with an integer argument,
based on Dtype.  As a second argument, it takes a suffix, used to build
a unique type name.  This way, we can use it to call a function with
more than one integer argument.

Example:

DISPATCH_INT_DTYPE_PREFIX_TO_TEMPLATE(core::Dtype::Int32, int32, [&]() {
    DISPATCH_INT_DTYPE_PREFIX_TO_TEMPLATE(core::Dtype::UInt64, uint64, [&]() {
    scalar_int32_t a;
    scalar_uint64_t b;
    // ...
});
@nsaiapova nsaiapova force-pushed the wip/tmesh-select-by-index branch 2 times, most recently from f3fef15 to af0bdd1 Compare November 8, 2023 10:48
@nsaiapova nsaiapova requested a review from ssheorey November 8, 2023 11:22
The method takes a list of indices and returns a new mesh built with
the selected vertices and triangles formed by these vertices.
The indices type can be any integral type.  The algorithm is implemented
on CPU only.

The implementation is inspired by
  open3d::geometry::TriangleMesh::SelectByIndex.
and by
  open3d::t::geometry::TriangleMesh::SelectFacesByMask.

We first compute a mask of vertices to be selected.  If the input index
exceeds the maximum number of vertices or is negative, we ignore the
index and print a warning.

If the mesh has triangles, we build  tringle mask and select needed
triangles.

The next step is to update triangle indices to a new ones.  It is similar
to SelectFacesByMask, so I introduced a static helper to do that.  Based on
the vertex mask we build a mapping index vector using inclusive prefix sum
algorithm and use it as a map between old and new indices.

We select the vertices by mask and build the resulting mesh from the selected
vertices and triangles.

Copying the mesh attributes is again similar to SelectFacesByMask, so I put
it to a separate static function.
* Add error handling on empty mesh
* Use DISPATCH_INT_DTYPE_PREFIX_TO_TEMPLATE instead of a conditional
  branch
* Use UpdateTriangleIndicesByVertexMask helper to update triangle
  indices
* Use CopyAttributesByMask helper to copy the mesh attributes
* Add tests
@nsaiapova nsaiapova force-pushed the wip/tmesh-select-by-index branch from af0bdd1 to defc9df Compare November 8, 2023 12:36
@nsaiapova
Copy link
Collaborator Author

@ssheorey I am not exactly sure why the CI fails, this link does not give me the clarity: https://github.com/isl-org/Open3D/actions/runs/6798298171/job/18482179687?pr=6415. Could you please help me to find out whether it is caused by my PR?

cpp/open3d/t/geometry/TriangleMesh.cpp Show resolved Hide resolved
cpp/open3d/t/geometry/TriangleMesh.cpp Show resolved Hide resolved
@ssheorey ssheorey merged commit 1585e96 into isl-org:master Nov 13, 2023
36 checks passed
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.

2 participants