Skip to content

Add do_snap parameter to PMP::autorefine_triangle_soup #8744

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

Open
wants to merge 110 commits into
base: master
Choose a base branch
from

Conversation

LeoValque
Copy link
Contributor

@LeoValque LeoValque commented Feb 17, 2025

Summary of Changes

The PR adds the do_snap parameter to autorefine_triangle_soup(). When set to true, the coordinates are rounded to fit in double with additional subdivisions, preventing any self-intersections from occurring.

Todo

  • Add to change log

Release Management

  • Affected package(s): PMP
  • Issue(s) solved (if any):
  • Feature/Small Feature (if any): link
  • Link to compiled documentation (obligatory for small feature) wrong link name to be changed
  • License and copyright ownership: GeometryFactory

@sloriot
Copy link
Member

sloriot commented Feb 17, 2025

What do you think of apply_iterative_snap_rounding() ? cc @MaelRL

@LeoValque
Copy link
Contributor Author

do you suggest to create a new function apply_iterative_snap_rounding() or simply to rename the parameter do_snap to apply_iterative_snap_rounding ?

@sloriot
Copy link
Member

sloriot commented Feb 17, 2025

do you suggest to create a new function apply_iterative_snap_rounding() or simply to rename the parameter do_snap to apply_iterative_snap_rounding ?

renaming the named parameter, provided Mael thinks it is a better name too.

@afabri
Copy link
Member

afabri commented Feb 17, 2025

What do you think of apply_iterative_snap_rounding() ? cc @MaelRL

I do not like an apply_ prefix. Did you have a look at the existing function for 2D

@sloriot
Copy link
Member

sloriot commented Feb 17, 2025

What do you think of apply_iterative_snap_rounding() ? cc @MaelRL

I do not like an apply_ prefix. Did you have a look at the existing function for 2D

note that's a named parameter, not a free function

@sloriot
Copy link
Member

sloriot commented Feb 17, 2025

What do you think of apply_iterative_snap_rounding() ? cc @MaelRL

I do not like an apply_ prefix. Did you have a look at the existing function for 2D

note that's a named parameter, not a free function

there is already the named parameter : apply_per_connected_component()

@MaelRL
Copy link
Member

MaelRL commented Feb 18, 2025

What do you think of apply_iterative_snap_rounding() ? cc @MaelRL

OK, but I also meant the function snap_polygon_soup(): even if it's internal, best to still avoid naming similarities.

@MaelRL MaelRL added Not yet approved The feature or pull-request has not yet been approved. Small feature labels Feb 18, 2025
@MaelRL MaelRL added this to the 6.1-beta milestone Feb 18, 2025
@sloriot
Copy link
Member

sloriot commented Feb 18, 2025

What do you think of apply_iterative_snap_rounding() ? cc @MaelRL

OK, but I also meant the function snap_polygon_soup(): even if it's internal, best to still avoid naming similarities.

for that one it could be internal::polygon_soup_snap_rounding()

@sloriot
Copy link
Member

sloriot commented Feb 19, 2025

/Users/magritte/cgal_root/CGAL-6.1-Ic-91/include/CGAL/Polygon_mesh_processing/internal/snap_polygon_soup.h:138:13: warning: unused variable 'pm' [-Wunused-variable]
/Users/magritte/cgal_root/CGAL-6.1-Ic-91/include/CGAL/Polygon_mesh_processing/internal/snap_polygon_soup.h:136:9: warning: unused type alias 'GT' [-Wunused-local-typedef]
/mnt/testsuite/include/CGAL/Polygon_mesh_processing/internal/snap_polygon_soup.h:223:5: warning: this 'else' clause does not guard... [-Wmisleading-indentation]

and PMP examples fully red https://cgal.geometryfactory.com/CGAL/testsuite/summary-6.1-Ic-91.html?package=Polygon_mesh_processing_Examples

@@ -23,5 +23,7 @@ class PMPAutorefinementVisitor{
/// called for each subtriangle created from a triangle with intersection, `tgt_id` is the position in the triangle container after calling
/// `autorefine_triangle_soup()` of the subtriangle, while `src_id` was the position of the original support triangle before calling the function.
void new_subtriangle(std::size_t tgt_id, std::size_t src_id);
/// called for each input triangle absent in the output because it was degenerate. `src_id` is the position of that triangle in the input range. Additionally, if `apply_iterative_snap_rounding()` is set to `true`, some extra triangles might become degenerate and will be absent in the output. Those triangles are also reported by this function.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sloriot If I understood correctly, the recommendation to the user is to have their visitor inherit from Default_visitor. If this is done, it is not a breaking change.

namespace autorefine_impl
{

// Certified ceil function for exact number types
Copy link
Member

Choose a reason for hiding this comment

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

@sloriot is that something of general interest and should go in number_utils or similar?

…nternal/triangle_soup_snap_rounding.h

Co-authored-by: Andreas Fabri <[email protected]>
@github-actions github-actions bot removed the Tested label May 19, 2025
Copy link

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

@afabri afabri added pre-approved For pre-approved small features. After 15 days the feature will be accepted. and removed Not yet approved The feature or pull-request has not yet been approved. labels May 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Check size Pkg::PMP pre-approved For pre-approved small features. After 15 days the feature will be accepted. Small feature Under Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants