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

Surface_mesh_approximation: Deal with boundary edges #7574

Merged
merged 8 commits into from
Nov 16, 2023
Merged

Surface_mesh_approximation: Deal with boundary edges #7574

merged 8 commits into from
Nov 16, 2023

Conversation

afabri
Copy link
Member

@afabri afabri commented Jul 5, 2023

Summary of Changes

Add a named parameter to distinguish between the approximation error for boundary and non-boundary edges of the input mesh.

Release Management

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

@afabri afabri added this to the 6.0-beta milestone Jul 5, 2023
sloriot
sloriot reviewed Jul 5, 2023
View reviewed changes
Surface_mesh_approximation/include/CGAL/Variational_shape_approximation.h Outdated
chord_max = citr;
dist_max = dist;
}
FT dist_max(0.0);
Copy link
Member

Choose a reason for hiding this comment

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

unwanted indentation changes I guess

@MaelRL MaelRL added Enhancement Not yet approved The feature or pull-request has not yet been approved. Ready to be tested labels Jul 6, 2023
@sloriot sloriot added Batch_1 First Batch of PRs under testing Tested and removed Under Testing Batch_1 First Batch of PRs under testing Ready to be tested labels Jul 11, 2023
@sloriot
Copy link
Member

sloriot commented Jul 12, 2023

Successfully tested in CGAL-6.0-Ic-20

@github-actions github-actions bot removed the Tested label Jul 17, 2023
@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.

@sloriot
Copy link
Member

sloriot commented Jul 20, 2023

New warnings in Surface_mesh_approximation tests in CGAL-6.0-Ic-25

@sloriot
Copy link
Member

sloriot commented Aug 4, 2023

Successfully tested in CGAL-6.0-Ic-33

@lrineau
Copy link
Member

lrineau commented Aug 23, 2023

@MaelRL Do you remember why you had set Not yet approved The feature or pull-request has not yet been approved. ?

@afabri
Copy link
Member Author

afabri commented Aug 23, 2023

@MaelRL Do you remember why you had set Not yet approved The feature or pull-request has not yet been approved. ?

Because we still have to decide if the API shall also allow to constrain other than boundary vertices.

@github-actions github-actions bot removed the Tested label Sep 25, 2023
@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.

afabri
afabri commented Sep 25, 2023
View reviewed changes
Surface_mesh_approximation/include/CGAL/Variational_shape_approximation.h Outdated
@@ -827,9 +840,11 @@ class Variational_shape_approximation {
using parameters::choose_parameter;

const FT subdivision_ratio = choose_parameter(get_parameter(np, internal_np::subdivision_ratio), FT(5.0));
const FT boundary_subdivision_ratio = choose_parameter(get_parameter(np, internal_np::boundary_subdivision_ratio), FT(5.0));
Copy link
Member Author

Choose a reason for hiding this comment

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

In order to be backward compatible we should set boundary_subdivision_ratioto the value of subdivision_ratio as default.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sloriot. Is this a good idea? This somehow assumes an order on named parameters.

Copy link
Member

Choose a reason for hiding this comment

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

This is correct, the variable subdivision_ratio being initialized before the call to the next get param.

@sloriot sloriot added the Batch_2 Second Batch of PRs under testing label Oct 16, 2023
@sloriot sloriot added Batch_1 First Batch of PRs under testing Under Testing and removed Batch_2 Second Batch of PRs under testing Batch_1 First Batch of PRs under testing labels Oct 31, 2023
@sloriot
Copy link
Member

sloriot commented Nov 15, 2023

#7574

@lrineau
Copy link
Member

lrineau commented Nov 15, 2023

@MaelRL Do you remember why you had set Not yet approved The feature or pull-request has not yet been approved. ?

Because we still have to decide if the API shall also allow to constrain other than boundary vertices.

@MaelRL: this PR is marked Tested , but also Not yet approved The feature or pull-request has not yet been approved. . I will not merge it, for now.

@MaelRL MaelRL removed the Not yet approved The feature or pull-request has not yet been approved. label Nov 16, 2023
@lrineau lrineau merged commit 472fa14 into CGAL:master Nov 16, 2023
9 checks passed
@lrineau lrineau deleted the Shape_approximation-borders-GF branch November 16, 2023 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sloriot sloriot sloriot left review comments

@MaelRL MaelRL MaelRL left review comments

Assignees

@sloriot sloriot

@MaelRL MaelRL

Labels
Enhancement Merged_in_6.0 Pkg::Surface_mesh_approximation Tested
Projects
None yet
Milestone
6.0-beta
Development

Successfully merging this pull request may close these issues.
4 participants
@afabri @sloriot @lrineau @MaelRL