Add RemoveDuplicatedTriangles feature using tensor API #6409
Conversation
|
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
| .Item<int64_t>(); | ||
| int64_t vertice_2 = | ||
| triangle_to_check.GetItem({core::TensorKey::Index(2)}) | ||
| .Item<int64_t>(); |
There was a problem hiding this comment.
We need to ensure this works for different dtypes for vertices (specifically int32_t and int64_t). This will output junk values for int32_t vertices.
minor:
operator[]is shorter:triangle_to_check[0].Item<dtype>()- Would using
std::rotateto reorder elements in the tuple be cleaner than the multipleifs? - Can you comment on parallelization here? Is it possible and would it provide a useful speedup?
There was a problem hiding this comment.
std::rotate can be used but it will need multiple ifs too to know which vertices to rotate. Example {2,1,0} will need 2 rotations to get {0,1,2}. What can be used is instantiate a std::vector<int64_t> of 3 elements, sort it with std::sort and next initialize the tuple for the unordered map.
Also elements of triangle_vec are int64_t like that it can get vertices in int32_t or int64_t.
std::vector<int64_t> triangle_vec;
if (triangles.GetDtype() == core::Int32)
{
auto vertice_0 = triangle_to_check[0].Item<int32_t>();
auto vertice_1 = triangle_to_check[1].Item<int32_t>();
auto vertice_2 = triangle_to_check[2].Item<int32_t>();
triangle_vec = {vertice_0, vertice_1, vertice_2};
}
else if (triangles.GetDtype() == core::Int64)
{
auto vertice_0 = triangle_to_check[0].Item<int64_t>();
auto vertice_1 = triangle_to_check[1].Item<int64_t>();
auto vertice_2 = triangle_to_check[2].Item<int64_t>();
triangle_vec = {vertice_0, vertice_1, vertice_2};
}
// We first need to find the minimum index. Because triangle (0-1-2)
// and triangle (2-0-1) are the same.
std::sort(triangle_vec.begin(), triangle_vec.end());
triangle_in_tuple =
std::make_tuple(triangle_vec[0], triangle_vec[1], triangle_vec[2]);
The parallelization is possible but we need to be sure that the access to the unordered map is in a critical section to avoid multiple accesses adding the duplicate triangle in the unordered map.
| if (triangle_to_old_index.find(triangle_in_tuple) == | ||
| triangle_to_old_index.end()) { | ||
| triangle_to_old_index[triangle_in_tuple] = i; | ||
| index_to_keep.push_back(i); |
There was a problem hiding this comment.
Would using a boolean mask of triangles to keep, instead of a vector of indices be faster? Tensors support indexing with a Boolean mask, similar to numpy. See SelectFacesByMask().
There was a problem hiding this comment.
Yes it is possible to use SelectFacesByMask() but this function doesn't select Normals Triangles faces ?
So if I need to loop over the normals triangles I can keep the actual loop instead of have both SleectFacesByMask() for triangles and the loop for normals triangles.
There was a problem hiding this comment.
I succeed to manage it in 3aa93dd
| core::Tensor triangles_normals, triangles_normals_without_dup; | ||
|
|
||
| // fill only is there are normals triangle to avoid errors | ||
| if (has_tri_normal) { |
There was a problem hiding this comment.
Can you support other triangle attributes? (e.g. triangle colors, etc.) An important feature of the Tensor API data structures is support for generic triangle and vertex attributes.
There was a problem hiding this comment.
Yes the triangle colors are manage in this : 3aa93dd
There was a problem hiding this comment.
We want to support arbitrary user defined attributes. Examples are labels, covariance, opacity, etc. See SelectFacesByMask for how to support arbitrary attributes.
… triangle is less than 2
…ith a mask. Manage also the removing of duplicate normals triangles and colors.
|
|
||
| // We first need to find the minimum index. Because triangle (0-1-2) | ||
| // and triangle (2-0-1) are the same. | ||
| std::sort(triangle_vec.begin(), triangle_vec.end()); |
There was a problem hiding this comment.
Triangles / faces are directional, i.e. triangle {0,1,2} is different from triangle {2,1,0} since their surface normals will face in opposite directions. Instead of sort, we can use std::rotate(triangle_vec.begin(), ptr_to_min, triangle_vec.end()) This ensures direction is preserved.
There was a problem hiding this comment.
Also, repeated memory allocation as we create a new vector for each face should be avoided.
| core::Tensor triangles_normals, triangles_normals_without_dup; | ||
|
|
||
| // fill only is there are normals triangle to avoid errors | ||
| if (has_tri_normal) { |
There was a problem hiding this comment.
We want to support arbitrary user defined attributes. Examples are labels, covariance, opacity, etc. See SelectFacesByMask for how to support arbitrary attributes.
Type
Motivation and Context
This PR is an implementation of the RemoveDuplicatedTriangles but using the tensor API to be able to run it either on CPU or GPU (only CPU for now in this PR).
This was also an opportunity to get familiar with the Open3D code.
Checklist:
python util/check_style.py --applyto apply Open3D code styleto my code.
updated accordingly.
results (e.g. screenshots or numbers) here.
Description
Here is the note of my PR for the RemoveDuplicatedTriangles feture using Tensor API.
The first problem of this feature is how to identify duplicated triangles.
I keep the same solution than the Eigen based API RemoveDuplicatedTriangles that is to use a unordered_map that has a complexity of search in O(1) due to the use of a hash function.
This unordered_map is implemented with a tuple as key instead of a tensor directly is because the hash function using a tensor is not implemented.
So we have to "convert" a vertice tensor of 3 points to a tuple to be able to keep track of the triangle.
The second problem is how to remove the duplicated triangles.
I choose to iterate on the index of the unique triangles and to copy them in a new tensor
I did this like that because we don't know the size of the new tensor of triangles until we have looped on all triangles.
I have tried to use :
tensor.reshape(), but it reshapes the tensor in a tensor with the same number of elements.
tensor.append(), but we need it doesn't increase the size of the vector where we append elements.
In the worst case where we have only one duplicated triangles we will need to iterate one time on the number of triangles and an other time on number of triangles - 1,
so the complexity is in O(n*(n-1)) and in the best case O(n) due to the test if before the second loop.
I have also treated the case if there are some normales triangles. I suppose that the number of triangles and normales triangles are the same and they are at a corresponding index.
This change is