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 - mesh polyhedra with self intersections #5916

Closed
wants to merge 13 commits into from
Closed

Mesh 3 - mesh polyhedra with self intersections #5916

wants to merge 13 commits into from

Conversation

janetournois
Copy link
Member

@janetournois janetournois commented Aug 18, 2021 •
edited
Loading

Summary of Changes

This PR introduces a new visitor for facet criteria to be able to deal with self-intersecting polyhedral inputs.

In the current version (c4142fa), there are 2 breaking changes :

  • Mesh_domain::Intersection is changed from std::tuple<Point_3, Index, int> to std::tuple<Point_3, Index, int, bool>
  • a new field std::array<bool, 4> self_intersections_table_ along with the associated set_facet_on_self_intersection() and is_facet_on_self_intersection() has been added to Compact_mesh_cell_base

Release Management

  • Affected package(s): Mesh_3
  • Issue(s) solved (if any): fix #0000, fix #0000,...
  • Feature/Small Feature (if any): todo
  • Link to compiled documentation (obligatory for small feature) wrong link name to be changed
  • License and copyright ownership: unchanged

Todo

  • discuss the API breaking changes
  • small feature

@lrineau
Copy link
Member

lrineau commented Aug 19, 2021

Would you mind using std::bitset<4> instead of std::array<bool, 4>? I think that will save memory space.

@janetournois
Copy link
Member Author

Sure!
I would like to avoid as much as possible breaking changes, and to discuss that with you

@lrineau
Copy link
Member

lrineau commented Aug 19, 2021 •
edited
Loading

I have reviewed this PR. The functionality is certainly something we want to have in CGAL. However, as Jane said, that is a breaking change, where two concepts are amended: MeshDomain_3 and MeshCellBase_3.

Even if only polyhedral mesh domains can benefit from the feature, for the moment all the users of Mesh_3 will be impacted by the new additions to the two concepts. Maybe we can find out a way to require the new fields conditionally.

@janetournois
Copy link
Member Author

I agree
We willl discuss that next week!

@sloriot sloriot added this to the 5.5-beta1 milestone Sep 23, 2021
@sloriot sloriot modified the milestones: 5.5-beta, 5.6-beta Apr 12, 2022
@MaelRL MaelRL modified the milestones: 5.6-beta, 5.7-beta Mar 31, 2023
janetournois and others added 9 commits May 19, 2023 09:44
@janetournois
mesh polyhedral domains with self-intersections
774fc14 
the new visitor Facet_criterion_visitor_with_self_intersections
allows to ignore "bad" mesh facets that have their dual intersecting
a polyhedron facet that takes part into a self-intersection of the input model
@janetournois
revert derivation change introduced in 4868748
f402b2c
@janetournois
move the new "self-intersection" boolean to Facet_properties for unif…
bb6c531 
…ormity

# Conflicts:
#	Mesh_3/include/CGAL/Mesh_3/Refine_facets_3.h
@janetournois
modify Labeled_mesh_domain_3 accordingly
c0ec880
@janetournois
fix hybrid domain to deal with self-intersections
e479ebf
@janetournois
add self-intersecting input
035ebe9
@janetournois
little cleaning and fix missing typename
2431867
@janetournois
add missing typename
9e227d3
@janetournois
update filename
887c8d9
@janetournois janetournois force-pushed the Mesh_3-mesh_with_self_intersections-jtournois branch from a02cafa to 887c8d9 Compare May 19, 2023 07:58
@janetournois janetournois marked this pull request as ready for review May 19, 2023 08:16
@janetournois
Copy link
Member Author

@lrineau I have updated this PR. We have to decide whether we keep the breaking changes or find another API solution.

@janetournois
Copy link
Member Author

I have the feeling that now that we have facet_min_size, this PR becomes useless. Both approaches tend to ignore problematic regions.
@lrineau @mael I move this PR to "stalled" for now. Do you think we should close it?

@lrineau
Copy link
Member

lrineau commented Jul 25, 2023

I agree with you that the facet_min_size feature is a more general solution to the issue. We can close this PR.

@janetournois janetournois removed the TODO label Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrineau lrineau lrineau left review comments

Assignees

@lrineau lrineau

Labels
Enhancement Pkg::Mesh_3
Projects
None yet
Milestone
6.0-beta
Development

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