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

AW3: improve manifoldness enforcing heuristics #7629

Closed

Conversation

MaelRL
Copy link
Member

@MaelRL MaelRL commented Aug 2, 2023

Summary of Changes

There is, at the end of the alpha wrap process, a post-processing step that re-adds material to enforce geometrical manifoldness.

This step is driven by a heuristics that prioritize cells around non-manifold vertices. The current heuristic prefers:

  • cells that are not incident to artificial vertices (bbox and cave initilization points)
  • cells that are incident to a lot of facets
  • cells that are adding a small volume

Turns out the 2nd criterion is not so great: because it ignores volume, it can add very large tetrahedra. Furthermore, it can cascade into adding absurd amounts of tetrahedra. Its purpose was to reduce the amount of tetrahedra added, hoping that by adding few tetrahedra (regardless of their volume), it would end up adding less volume than adding small tetrahedra. In practice, it seems like it doesn't work that well, so just remove it and let the process be driven entirely by tetrahedron size.

Here is a comparison for a typical case reported in #7625:

image

TODO:

  • do not resort at every step since we don't use the #boundary facet anymore
  • retest manually post bc8351f

Release Management

@lrineau
Copy link
Member

lrineau commented Aug 3, 2023

Is this PR a change that could be backported to 5.5.x or 5.6.x?

@MaelRL
Copy link
Member Author

MaelRL commented Aug 3, 2023

It's on the fence between bug fix and changing the behavior. I picked the latter for the (low) probability that it would be an unwanted change for some users.

@lrineau
Copy link
Member

lrineau commented Aug 3, 2023

It's on the fence between bug fix and changing the behavior. I picked the latter for the (low) probability that it would be an unwanted change for some users.

Then, maybe rebase the branch onto a release branch, but let the PR be based on master. That will ease a possible merge of the changes by a user (by a merge of your branch).

@MaelRL MaelRL force-pushed the AW3-Improve_manifoldness_enforcement-GF branch from 4bc5ebc to c5de266 Compare August 3, 2023 10:24
MaelRL added 4 commits August 3, 2023 12:30
This combinatorial choice seemed like a good idea, but it can have
nasty cascading effects, adding very large tetrahedra. See this
issue: CGAL#7625

In the end, the only thing we care about is small volumes being added.

I keep the artificial vertex for now, but I am not fully convinced
these should be actually kept too.
@MaelRL MaelRL force-pushed the AW3-Improve_manifoldness_enforcement-GF branch from c5de266 to 330ff2e Compare August 3, 2023 10:32
@sloriot sloriot added Tests failing Batch_1 First Batch of PRs under testing and removed Under Testing labels Aug 9, 2023
@MaelRL MaelRL added the TODO label Sep 27, 2023
There was a need for sorting at every iteration when the sorting
used criteria which were changing with every iteration. This
is no longer the case after c7b9317.

Also make it deterministic.
@MaelRL MaelRL removed the TODO label Oct 9, 2023
@sloriot

This comment was marked as outdated.

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

MaelRL commented Nov 29, 2023

Merged with #7805.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Alpha Wrapping Strange Features
4 participants