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

Shape_detection: using proper triangulated polygonal faces for linear_least_squares #8314

Merged

Conversation

soesau
Copy link
Member

@soesau soesau commented Jun 27, 2024

Summary of Changes

Polygonal faces are now triangulated and face normals are calculated using PMP::compute_face_normal.

The calculated face normals and triangulations are buffered in Least_squares_plane_fit_region. However, Least_squares_plane_fit_sorting is independent and does not benefit from that buffered data.

Release Management

@lrineau
Copy link
Member

lrineau commented Jun 27, 2024

@afabri @sloriot @MaelRL: This bug-fix pull-request adds new dependencies to non-trivial parts of PMP. For the license checking, that means a user of CGAL-5.6.1 might have a new license issue after an update to CGAL-5.6.2. That does not seem right.

@soesau
Copy link
Member Author

soesau commented Jun 28, 2024

Sebastien motivated yesterday having a basic triangulation for exactly cases like this.
Here a simple triangulation of simple polygons is required. Using PMP::triangulate_hole_polyline creates all those mentioned dependencies.

@lrineau lrineau added the Not yet approved The feature or pull-request has not yet been approved. label Jun 28, 2024
@sloriot
Copy link
Member

sloriot commented Jul 2, 2024

runtime error of test_region_growing_on_degenerated_meshfor shape detection in CGAL-6.0-Ic-279

handling of degenerate faces
@sloriot
Copy link
Member

sloriot commented Jul 8, 2024

Successfully tested in CGAL-6.0-Ic-282

@afabri
Copy link
Member

afabri commented Aug 5, 2024

Can we not identify three points which are maximal un-collinear, by computing Eigen values/vectors ?

@@ -383,25 +415,6 @@ namespace Polygon_mesh {
return Point_3(x, y, z);
}

// Compute normal of the face.
template<typename Face>
Vector_3 get_face_normal(const Face& face) const {
Copy link
Member

Choose a reason for hiding this comment

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

I think the issue encountered by @lrineau would be fixed by only fixing this function. We should look for 3 non-collinear vertices to get a normal. I would then skip the proper 2D CDT and keep the centroid triangulation for the plane fitting (only matters for non exactly coplanar points anyway). @afabri what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I also thought that for computing a face normal it is enough to find 3 sufficiently non-collinear points, but Sven told me that for non-convex faces he has to compute some weight and for that he needs a triangulation. We have to check with him.

Copy link
Member Author

Choose a reason for hiding this comment

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

These were two separate problems:

  1. get_face_normal is used for fitting a plane to a single face and for checking whether a face fits to an adjacent region. This was likely the issue encountered by @lrineau.
  2. PCA was not using triangulated faces.

Copy link

This pull-request was previously marked with the label Tested, but has been modified with new commits. That label has been removed.

@sloriot
Copy link
Member

sloriot commented Aug 19, 2024

Successfully tested in CGAL-6.0-Ic-307

@sloriot sloriot merged commit 8730ff5 into CGAL:5.6.x-branch Aug 19, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Merged_in_6.0 Not yet approved The feature or pull-request has not yet been approved. Pkg::Shape_detection Tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants