RemoveDuplicateVertices Implementation for TriangleMesh (continued) #6636
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
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
- Made vertex mask type bool. - Check if mesh has triangle indices before getting them. - Modified some errors to warnings. 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
Instead of using if statements explicitly, used DISPATCH_FLOAT_INT_DTYPE_TO_TEMPLATE to dispatch to RemoveDuplicateVerticesWorker function appropriately. 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
|
This is continuation of the PR: |
nsaiapova
reviewed
Mar 8, 2024
| @@ -30,6 +30,7 @@ | |||
| - Fix geometry picker Error when LineSet objects are presented (PR #6499) | |||
| - Fix mis-configured application .desktop link for the Open3D viewer when installing to a custom path (PR #6599) | |||
| - Fix regression in printing cuda tensor from PR #6444 🐛 | |||
| - Implemented functionality for removing duplicate vertices from TriangleMesh (PR #6414). | |||
There was a problem hiding this comment.
Could you please align this line with the rest of the list (e.g. add two more spaces after -)?
Comment on lines
+1262
to
+1268
| /// (MapA.) From vertex coordinates to their indices. | ||
| // This is implemented using unordered_map. | ||
| /// (MapB.) From the vertex indices before duplicate removal to indices after | ||
| /// removal. | ||
| /// This is implemented using std::vector. | ||
| /// | ||
| /// MapA allows the function to detect duplicates and MapB allows it update |
There was a problem hiding this comment.
Suggested change
| /// (MapA.) From vertex coordinates to their indices. | |
| // This is implemented using unordered_map. | |
| /// (MapB.) From the vertex indices before duplicate removal to indices after | |
| /// removal. | |
| /// This is implemented using std::vector. | |
| /// | |
| /// MapA allows the function to detect duplicates and MapB allows it update | |
| /// * point_to_old_index maps vertex coordinates to their indices. | |
| // This is implemented using unordered_map. | |
| /// * index_old_to_new maps vertex indices before duplicate removal to indices after | |
| /// the removal. This is implemented using std::vector. | |
| /// The first map allows the function to detect duplicates and the second allows it to update |
| std::unordered_map<Point3d, size_t, utility::hash_tuple<Point3d>> | ||
| point_to_old_index; | ||
|
|
||
| auto vertices = mesh.GetVertexPositions().To(mesh.GetDevice()).Contiguous(); |
There was a problem hiding this comment.
IIUC, the vertex tensor is already on the device of the mesh, but we want to move the tensor to CPU, so I think we need to use core::Device() here.
| using vmask_itype = bool; | ||
| core::Tensor vertex_mask = | ||
| core::Tensor::Zeros({orig_num_vertices}, vmask_type); | ||
| using Length_t = decltype(orig_num_vertices); |
There was a problem hiding this comment.
Could we just use int64_t here? It looks like the GetLength type is always int64_t.
Comment on lines
+1295
to
+1296
| const auto vmask_type = core::Bool; | ||
| using vmask_itype = bool; |
There was a problem hiding this comment.
Maybe a personal taste, but I find the original types, bool and core::Bool, more descriptive in this case.
| point_to_old_index[coord] = i; | ||
| index_old_to_new[i] = k; | ||
| ++k; | ||
| vertex_mask_ptr[i] = 1; |
There was a problem hiding this comment.
nit: 1 -> true, since it is a bool value here. (it is 1 in other functions above as we use int there for the prefix_sum approach).
| def gen_box_mesh_dup_vertex(unique_v, unique_i, v_index, insert_pos): | ||
| """ | ||
| Generates a duplicate vertex list and an updated index list from | ||
| the uniques afte inserting a vertex at v_index in unique_v to position |
Comment on lines
+635
to
+636
| unique_i = [[4, 7, 5], [4, 6, 7], [0, 2, 4], [2, 6, 4], [0, 1, | ||
| 2], [1, 3, 2], |
There was a problem hiding this comment.
Does the line break here look as intended?
ssheorey
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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