RemoveDuplicateVertices Implementation for TriangleMesh. #6414
Conversation
Functionality to remove duplicate vertices and update other vertex attributes and triangle indices is implemented. The C++ and python tests are also written to test the code. On branch sganjugu/remdup2 Changes to be committed: modified: cpp/open3d/t/geometry/TriangleMesh.cpp modified: cpp/open3d/t/geometry/TriangleMesh.h modified: cpp/pybind/t/geometry/trianglemesh.cpp modified: cpp/tests/t/geometry/TriangleMesh.cpp modified: python/test/t/geometry/test_trianglemesh.py
|
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
There was a problem hiding this comment.
Hi @intelshashi , this approach and implementation are sound and logical. Since Open3D needs to be performance portable on both CPU and GPU, we have added some optimized Tensor primitives to ease algorithm development. You can see some of these in TriangleMesh::SelectFacesByMask which solves a similar problem - keeping a subset of vertices. That approach ensures that these operations work on both CPU and GPU and are performant on the GPU. I would recommend switching to that approach before we merge this PR. Specifically:
- create a
vertex_maskof vertices to be kept and useTensor::IndexGetto subset vertices and attributes. - Use
utility::InclusivePrefixSumto remap vertex indices.
Previously, I was doing a manual copy and shrinking the vertices. Instead in this checkin all vertex attributes, including the coordinates are shrunk using IndexGet method using a vertex mask. Further, the triangle index remapping is similar to what was done earlier, with some simplications. One thing to note, is that we cannot use utility::InclusivePrefixSum to remap vertex indices because the duplicates can occur in any position and may be duplicates of any earlier vertex. For e.g., suppose there were 9 vertices to start with, and 8th (index 7, starting from 0), was a duplicate of 2nd (index 1, starting from 0). So, the vertex mask would look like this: Vertex indices: [0, 1, 2, 3, 4, 5, 6, 7, 8] Vertex mask: [1, 1, 1, 1, 1, 1, 1, 0, 1] Prefix sum: [0, 1, 2, 3, 4, 5, 6, 7, 7, 8] This gives an incorrect index map for 8th vertex, which is mapped to index 7 instead of 1. On branch sganjugu/remdup2 Your branch is up to date with 'origin/sganjugu/remdup2'. Changes to be committed: modified: ../cpp/open3d/t/geometry/TriangleMesh.cpp modified: ../python/test/t/geometry/test_trianglemesh.py
For e.g., suppose, you had 9 vertices, and 8th (index 7, starting from 0), was a duplicate of 2nd (index 1, starting from 0). |
| (int)(orig_num_vertices - k)); | ||
|
|
||
| return mesh; | ||
|
|
There was a problem hiding this comment.
Nit: this empty line is not needed.
| auto orig_num_vertices = vertices.GetLength(); | ||
| const auto vmask_type = core::Int32; | ||
| using vmask_itype = int32_t; | ||
| core::Tensor vertex_mask = core::Tensor::Zeros({orig_num_vertices}, vmask_type); |
There was a problem hiding this comment.
If the mask is not used to store the remapped indices, I think we can have it as Bool right from the initialization.
| auto triangles = GetTriangleIndices(); | ||
| if (core::Int32 != triangles.GetDtype() && core::Int64 != triangles.GetDtype()) { | ||
| utility::LogError("Only Int32 or Int64 are supported for triangle indices"); | ||
| return *this; |
There was a problem hiding this comment.
I think we don't need to return after utility::LogError. (here and below)
| if(core::Float32 == vertices.GetDtype()) { | ||
| if (core::Int32 == triangles.GetDtype()) { | ||
| return RemoveDuplicateVerticesWorker<float, int>(*this); | ||
| } else { | ||
| return RemoveDuplicateVerticesWorker<float, int64_t>(*this); | ||
| } | ||
| } else { | ||
| if (core::Int32 == triangles.GetDtype()) { | ||
| return RemoveDuplicateVerticesWorker<double, int>(*this); | ||
| } else { | ||
| return RemoveDuplicateVerticesWorker<double, int64_t>(*this); | ||
| } | ||
| } |
There was a problem hiding this comment.
While working on my SelectByIndex method, I discovered a set of macros to do this in more compact way:
https://github.com/isl-org/Open3D/blob/main/cpp/open3d/core/Dispatch.h#L90
I think we could rewrite it as smt like:
| if(core::Float32 == vertices.GetDtype()) { | |
| if (core::Int32 == triangles.GetDtype()) { | |
| return RemoveDuplicateVerticesWorker<float, int>(*this); | |
| } else { | |
| return RemoveDuplicateVerticesWorker<float, int64_t>(*this); | |
| } | |
| } else { | |
| if (core::Int32 == triangles.GetDtype()) { | |
| return RemoveDuplicateVerticesWorker<double, int>(*this); | |
| } else { | |
| return RemoveDuplicateVerticesWorker<double, int64_t>(*this); | |
| } | |
| } | |
| DISPATCH_FLOAT_INT_DTYPE_TO_TEMPLATE(vertices.GetDtype(), triangles.GetDtype()), [&]() { | |
| RemoveDuplicateVerticesWorker<scalar_t, int_t>(*this); | |
| }); |
| TriangleMesh& RemoveDuplicateVerticesWorker(TriangleMesh &mesh) | ||
| { | ||
| //Algorithm based on Eigen implementation | ||
| if(!mesh.HasVertexPositions()) { |
There was a problem hiding this comment.
Could you please run python util/check_style.py --apply on your changes? There are a few accidental space misses.
| //REM_DUP_VERT_STEP 1: | ||
| // Compute map from points to old indices, and use a counter | ||
| // to compute a map from old indices to new unique indices. | ||
| const Coord_t *vertices_ptr = vertices.GetDataPtr<Coord_t>(); |
There was a problem hiding this comment.
I think we need to move vertices tensor to CPU device, too.
| return *this; | ||
| } | ||
| auto vertices = GetVertexPositions(); | ||
| auto triangles = GetTriangleIndices(); |
There was a problem hiding this comment.
There could be meshes w/o triangles tensor. In this case, the call to GetTrianglesIndices throws.
Could you please add safety checks for this case?
Functionality to remove duplicate vertices and update other vertex attributes and triangle indices is implemented. The C++ and python tests are also written to test the code.
On branch sganjugu/remdup2
Changes to be committed:
modified: cpp/open3d/t/geometry/TriangleMesh.cpp
modified: cpp/open3d/t/geometry/TriangleMesh.h
modified: cpp/pybind/t/geometry/trianglemesh.cpp
modified: cpp/tests/t/geometry/TriangleMesh.cpp
modified: python/test/t/geometry/test_trianglemesh.py
Type
Motivation and Context
Checklist:
python util/check_style.py --applyto apply Open3D code styleto my code.
updated accordingly.
results (e.g. screenshots or numbers) here.
Description
This change is