Add vertex normal estimation to PolygonMesh #3591
Conversation
|
The code is mostly similar, except:
Would it make sense to split this function in multiple parts to reduce code duplication? The interface would not change Will it make sense to provide 1 more overload for the existing functionality with |
Yes, you are right. I'll refactor this to avoid code duplication.
I can add it with ease, so why not? Will do this soon! |
|
Also, please add tests so we can prevent regressions/breaking builds in future. Reorder so we can have the tests and then iterate on the interface (leaving the test commit alone) |
kunaltyagi
added
module: features
needs: more work
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| #include "pcl/features/normal_3d.h" | |||
| #include "pcl/Vertices.h" | |||
| #include "pcl/point_types.h" | |||
There was a problem hiding this comment.
Prefer using '<' over '"' for header includes
| pcl::fromPCLPointCloud2(mesh.cloud, cloud); | ||
|
|
||
| // For each point intialize the normal as zeros. | ||
| for (auto &point : cloud.points) |
There was a problem hiding this comment.
Utilize the other function to avoid repeating code
|
Just to update on this - I've been busy for the past couple of months, but I will provide the changes requested by the reviewers when suitable. However, to whomever this may be useful, the implementation works as it is. |
|
Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this. |
kunaltyagi
added
the
changelog: enhancement
|
Marking this as stale due to 30 days of inactivity. Commenting or adding a new commit to the pull request will revert this. |
This updates #1262.
Instead of overloading the original
computeApproximateNormalsI decided to add two functions. The difference is, the original function did not normalize the face normal before adding it to the vertex and the author flipped the normals towards the viewpoint. I do not know the reason behind these choices, but I disagree with them due to the following rationale.