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

Fix iterator invalidation bug #5001

Closed
wants to merge 2 commits into from
Closed

Fix iterator invalidation bug #5001

wants to merge 2 commits into from

Conversation

oboes
Copy link
Contributor

@oboes oboes commented Sep 17, 2020

Summary of Changes

There is an iterator invalidation bug when looping over degenerate_face_set and modifying it at the same time (see this page on cppreference for details on iterator invalidation).

On some meshes it can causes segmentation faults. If required I can upload a .off file containing a mesh for which it's the case (but I don't have a minimal example, only a 156K .off file).

I fix that by simply iterating over a copy of degenerate_face_set rather than the original one (while still modifying the original one). It might not be the most computationally efficient way to do it (it involves creating a copy of the set), but it does fix the bug in one line of code.

Release Management

  • Affected package(s): Polygon Mesh Processing

@MaelRL MaelRL self-requested a review September 17, 2020 10:33
@MaelRL MaelRL self-assigned this Sep 17, 2020
@oboes oboes requested a review from lrineau September 17, 2020 11:07
…epair_degeneracies.h

Co-authored-by: Laurent Rineau <Laurent.Rineau@cgal.org>
@MaelRL MaelRL added the Not yet approved The feature or pull-request has not yet been approved. label Sep 18, 2020
@MaelRL MaelRL added Replaced and removed Not yet approved The feature or pull-request has not yet been approved. labels Sep 22, 2020
@MaelRL
Copy link
Member

MaelRL commented Sep 22, 2020

Thank you for the bug report.

I have rewritten the full loop in #5012 to be less intricate (but without copying anything), which should fix your issue. Could you please test it on your data set? I would also be interested if you could please share it, degeneracy removal is still quite WIP, and I do not have non-trivial data for this particular configuration.

@MaelRL MaelRL closed this Sep 22, 2020
@MaelRL MaelRL removed their request for review September 22, 2020 15:14
@oboes
Copy link
Contributor Author

oboes commented Sep 24, 2020

I tried and it does work now. Thanks!

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.

3 participants