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.

Sign up for GitHub

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

Jump to bottom

Mesh_3 edge_distance criterion : an upper bound for the distances of the feature edge to the input feature #7532

Merged
merged 57 commits into from
May 27, 2024
Merged

Mesh_3 edge_distance criterion : an upper bound for the distances of the feature edge to the input feature #7532

merged 57 commits into from
May 27, 2024

Conversation

ange-clement
Copy link
Member

@ange-clement ange-clement commented Jun 19, 2023 •
edited
Loading

Summary of Changes

Fixes issue #5110.
Adds a edge_distance criteria to the mesh generation process.
This criteria is an upper bound for the distance from the edge to the 1D feature.

Release Management

@janetournois janetournois added this to the 6.0-beta milestone Jun 19, 2023
@janetournois
Copy link
Member

This PR will fix issue #5110

@lrineau lrineau linked an issue Jun 19, 2023 that may be closed by this pull request
Mesh_3: add an approximation criterion to the protecting balls algorithm #5110
Closed
@janetournois
Copy link
Member

janetournois commented Jun 19, 2023 •
edited
Loading

I'm adding a first todo-list

  • compute the distance between a feature edge midpoint and the corresponding feature, and test it
  • make sure this new criterion does not conflict with edge_min_size
  • transform edge_distance into a sizing field, similarly to facet_distance
  • add an example or modify an existing one
  • add a test or modify an existing one
  • write documentation of the new named parameter in Mesh_criteria_3
  • write documentation in Mesh_edge_criteria_3
  • add edge_distance option to the meshing dialog in the demo

@ange-clement
Copy link
Member Author

image
Results on anchor.off without then with edge distance criterion.
Curved features have smaller balls than straight ones.

Ange Clement added 6 commits June 21, 2023 11:36
edge_distance now works on images domains too
732fe69
Clean whitespace in Protect_edges_sizing_field.h
d8f9ea3
Merge remote-tracking branch 'janetournois/Polyhedron_demo-fix_defaul…
ce83dec 
…t_sigma-jtournois' into Mesh-3-edge-distance-criterium-aclement
Added criteria to meshing dialog in the demo + merge from 'Polyhedron…
d2808da 
… demo - fix Mesh_3 dialog initialization'
Added a undocumented test
8998f45
Added testing for edge_distance : - Modified test_mesh_criteria_creat…
447eace 
…ion.cpp to test the creation with or without a SizingField - Modified test_meshing_determinism.cpp to test determinism of mesh creation - Added test_max_edge_distance.cpp that estimate number of vertices and facets and test that it is greater than without the parameter
@janetournois janetournois marked this pull request as ready for review June 23, 2023 07:48
@janetournois

This comment was marked as outdated.

Sign in to view
@ange-clement ange-clement changed the title framework for edge_distance criterium Mesh_3 edge_distance criterium : an upper bound for the distances of the feature edge to the input feature Jun 23, 2023
@github-actions

This comment was marked as outdated.

Sign in to view
@ange-clement

This comment was marked as outdated.

Sign in to view
@lrineau

This comment was marked as outdated.

Sign in to view
@github-actions

This comment was marked as outdated.

Sign in to view
@janetournois
Copy link
Member

/force-build:v0

@github-actions
Copy link

The documentation is built. It will be available, after a few minutes, here: https://cgal.github.io/7532/v0/Manual/index.html

@janetournois
Copy link
Member

I can't reproduce the runtime errors, even using the same random seed. @ange-clement can you reproduce the error?

@afabri
Copy link
Member

afabri commented May 7, 2024 •
edited
Loading

I could reproduce it after 5 runs. As it is in Release mode but as there is an assertion I removed the NDEBUG.

In fact I get another but similar exception than in the testsuite, namely:

Assertion failed: min_vertices_expected <= c3t3.triangulation().number_of_vertices(), file C:\GitHub\cgal\Mesh_3\test\Mesh_3\test_meshing_utilities.h, line 218

@janetournois
the nb of vertices in parallel mode has a pretty large variability
47b74e4 
(from 745 to 893 in my experiments)
so testing their number wrt a reference value does not make much sense

This commit changes to test not to test meshing without comparing nbv and nbf
in parallel mode
@afabri
Copy link
Member

afabri commented May 12, 2024

What variations do you observe. Do you now test that it is between 0 and maxint?

@janetournois
Copy link
Member

The range for the number of vertices (in my experiments) is too wide (+/-20% around average value) so imho it does not make much sense to choose arbitrary bounds to test them.
The test function does more than checking the nb of vertices and facets, and it's used this way elsewhere, so let's do the same.

@sloriot
Copy link
Member

sloriot commented May 15, 2024

Successfully tested in CGAL-6.0-Ic-242

lrineau
lrineau reviewed May 16, 2024
View reviewed changes
Copy link
Member

@lrineau lrineau left a comment

Choose a reason for hiding this comment

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

I have reviewed this pull-request. I would like to say I approve the documentation (except maybe a doubt about a wording, see the comment). But I am not sure about the implementation. I would like to do a peer-review with Ange.

Mesh_3/test/Mesh_3/test_max_edge_distance.cpp Show resolved Hide resolved
Mesh_3/include/CGAL/Mesh_edge_criteria_3.h Show resolved Hide resolved
@lrineau lrineau added the pre-approved For pre-approved small features. After 15 days the feature will be accepted. label May 17, 2024
@github-actions github-actions bot removed the Tested label May 22, 2024
@github-actions GitHub Actions
Copy link

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

@lrineau lrineau removed the Not yet approved The feature or pull-request has not yet been approved. label May 22, 2024
@lrineau lrineau added the Tested label May 22, 2024
@lrineau
Copy link
Member

lrineau commented May 22, 2024

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

We have only modified the CHANGES.md file, in 1bc9fed. This PR can be considered as tested.

And approved.

@sloriot sloriot added Accepted small feature and removed pre-approved For pre-approved small features. After 15 days the feature will be accepted. labels May 27, 2024
@sloriot sloriot merged commit d06a57a into CGAL:master May 27, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janetournois janetournois janetournois approved these changes

@albert-github albert-github albert-github left review comments

@afabri afabri afabri left review comments

@sloriot sloriot sloriot left review comments

@lrineau lrineau lrineau approved these changes

Assignees

@janetournois janetournois

@ange-clement ange-clement

Labels
Accepted small feature Enhancement Merged_in_6.0 Pkg::Mesh_3 Small feature Tested
Projects
None yet
Milestone
6.0-beta
Development

Successfully merging this pull request may close these issues.

Mesh_3: add an approximation criterion to the protecting balls algorithm
7 participants
@ange-clement @janetournois @lrineau @afabri @sloriot @albert-github @MaelRL