Decide a collapse's fate before copying its star - #9587
Open
IasonManolas wants to merge 8 commits into
Open
Conversation
…ing it CollapseTriangulation copies the star of the edge into a triangulation of its own and runs the collapse on it, only to reach a last test comparing the dihedral angles before and after. On bear.mesh that test is what rejects the collapse in every single rejected case - 14215 of the 31518 candidates that get that far - so nearly half of those copies are built to answer a question about the geometry alone. The cells the collapse keeps are the star minus the ring of the edge, with both extremities at the collapse point, so the comparison can be made on the triangulation itself, before the copy exists. Dihedral_angle_cosine's operator< cross-multiplies stored squares in double, which is not transitive: two representations of the same cosine, arising from cells reduced in a different order, can compare differently against a third value. This test and the copy's own version of the same comparison reduce over the cells in different orders, so a boolean version of this test would occasionally disagree with the copy on a candidate within rounding of a threshold - measured at about 1 part in 1e16, i.e. one ULP. Comparing on the signed square of the cosine (values in [-1, 1]) with a 1e-9 margin - nine orders above that measured disagreement and far below anything geometrically meaningful - lets the test answer decisively away from a threshold and defer to the copy, unchanged, whenever it is within the margin of one. Measured to be within margin on 0.3-4.0% of a mesh's rejections. Byte-identical on bear, bear_2sub, bear_8sub, bunny00, and on a Thingi10K sample of 13 meshes up to 4.5M tets including 101556.mesh (578,115 v / 4,539,866 tets) - the mesh whose scale first exposed the comparator's non-transitivity in an earlier, margin-less version of this same test.
…oldness Both collapse_keeps_angles_acceptable() and is_cells_set_manifold() are pure predicates over the same cell set, so their order does not change the result - only which one pays for a candidate the other would also have rejected. The angle test walks the star once; the manifoldness test walks the star of each of the edge's two vertices, so it is the more expensive of the two, and the angle test is also the more selective one. Running it first leaves the manifoldness walk unpaid for whatever it rejects.
…ided Once collapse_keeps_angles_acceptable() returns ANGLES_ACCEPTED, its verdict is exactly the one CollapseTriangulation's copy would reach performing the same comparison on its own copy of the star - that is what ANGLES_ACCEPTED means. Building the copy, running the collapse on it and re-deriving that comparison is then answering a question already answered. The copy is still built for the ANGLES_UNDECIDED minority the angle test could not decide on its own margin, where it keeps its original role as the tie-breaker. On bear.mesh the angle test was handed 31518 candidates and rejected 14215 of them outright; with it running first, the copy is built only for the 17303 survivors, of which measurement puts 0.3-4.0% at ANGLES_UNDECIDED depending on the mesh - the rest proceed straight to the real collapse. Byte-identical on bear, bear_2sub, bear_8sub, bunny00, and the same 13-mesh Thingi10K sample used to validate the angle test itself, up to and including 101556.mesh (578,115 v / 4,539,866 tets).
…finite-vertex adjacency The CollapseTriangulation simulation used to catch every collapse whose two merged neighbor cells both have the infinite vertex as their opposite vertex, before the real mutating collapse() ran. Skipping the simulation on the decisive-accept path removed that guard for the majority of candidates: collapse() rewires cell neighbor pointers before its own equivalent check, so a candidate that hits this case now corrupts the triangulation instead of being rejected cleanly. collapse_avoids_infinite_adjacency() replicates the same check read-only on the real triangulation, and is called on exactly the decisive-accept path that lost the simulation's protection.
janetournois
reviewed
Aug 6, 2026
| if(local_tri.collapse() != VALID) | ||
| return Vertex_handle(); | ||
| } | ||
| else if(!collapse_avoids_infinite_adjacency<C3t3>(edge, collapse_type, c3t3)) |
Member
There was a problem hiding this comment.
is this a new condition, or moved from somewhere else?
Member
Author
There was a problem hiding this comment.
As the commit message explains, its a read only replication of the check on the real triangulation.
The check in the original code happened around lines 887-888.
Member
There was a problem hiding this comment.
do you think it is possible to avoid the duplication of this test?
Resolving a cell incident to the collapsed edge - finding the two cells that take its place and this cell's index in each of them - was written out three times: in the CollapseTriangulation simulation, in collapse() itself, and in the read-only check added for the decisive-accept fast path. Introduce Collapse_star_cell to name that triple, and make_collapse_star_cell() as its single point of derivation. The infinite-vertex adjacency test becomes a member of it, so it can no longer be called with a cell and an index that do not match. collapse() now resolves the whole star in a first pass and mutates in a second one, which lets it reject an infinite adjacency itself. collapse_avoids_infinite_adjacency() then has nothing left to do and is removed, taking the third traversal of the star with it. Hoisting the test also fixes the bail-out: it used to run after set_neighbor() had already rewired part of the star, so returning there left the triangulation half-collapsed. Output is byte-identical on bear (13.8k vertices), bunny00 (37.7k) and 101556 (1.2M vertices / 8.3M cells).
Member
|
Successfully tested in CGAL-6.3-Ic-52 |
…before-simulating
<boost/container/flat_set.hpp> is already included two lines below. The second copy only existed because this branch predates that one on main, and it is the last thing making this branch conflict with CGAL#9594.
|
This pull-request was previously marked with the label |
Hoisting the infinite-adjacency test into collapse() gave that function a second outcome: a null vertex handle, meaning the star was refused before anything had been rewired. The three call sites here did not read it. They handed it straight to set_dimension(), which dereferenced it. They had also already moved the endpoints, in the expectation of a collapse that then did not happen. A null check on its own would leave both vertices sitting on the midpoint with no collapse performed, which is worse than the crash because it is silent. So the points are put back, and the single set_dimension() call now sits behind the guard rather than being repeated in each of the three branches. Reaching this needs a star whose two outer neighbours both face the infinite vertex. None of the CDT meshes the branch was tested on produce one, which is why it went unseen. Nine mesh_3 generated meshes do: four of them crashed before this, and all nine now finish with output identical to main.
janetournois
approved these changes
Aug 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
collapse_edge()decides validity partly viaCollapseTriangulation, which copies the edge's star into its own triangulation, collapses it there, and inspects the result: expensive, and unconditional. Instrumenting its return value on bear.mesh showed why that's suspicious: of 31,518 candidates reaching it, 14,215 (45%) came backANGLE_PROBLEM, decided entirely by the copy's last step, a before/after dihedral-angle comparison needing only geometry the real triangulation already has, and no other rejection reason ever fired. Separately, the guards ahead of the copy were never ordered by cost:is_cells_set_manifoldwalks the star of both edge endpoints where the angle comparison walks it once, the same cheap-test-behind-expensive-test defect this project's prior work found elsewhere in this file. Three commits:reject a collapse on its angles before simulating it: addscollapse_keeps_angles_acceptable(), reproducingCollapseTriangulation's angle comparison so a decisive answer agrees with the copy's. Placed where the copy already sat, so this commit only adds an early rejection.test the angles of a collapse before the manifoldness: reorders the new guard ahead ofis_cells_set_manifold(cheaper, more selective).stop simulating a collapse that is already decided: skips the copy when the angle test's own verdict is decisive; still builds it, unchanged, for the minority it cannot decide.Only
collapse_short_edges.his touched, insidecollapse_edge()and the newcollapse_keeps_angles_acceptable().Why the angle test can only sometimes decide on its own
collapse_keeps_angles_acceptable()returnsANGLES_REJECTED,ANGLES_ACCEPTED, orANGLES_UNDECIDED, not a plain bool:Dihedral_angle_cosinestores a cosine as(sign, sq_num, sq_den)and itsoperator<cross-multiplies indouble:l.sq_num * r.sq_den < r.sq_num * l.sq_den. This is not transitive: two(sq_num, sq_den)pairs can represent the same cosine yet compare equal to each other while rounding differently against a third value, if they arose from reducing the same cells in a different order.collapse_keeps_angles_acceptable()reduces its "before" set inflat_set/small_vectororder;CollapseTriangulation's copy reduces the same cells inCell_circulatororder. So on a candidate whose worst angle sits within rounding of a threshold, the two can reach opposite verdicts. Because a collapse mutates the mesh, one differing decision propagates through the rest of the run.Why
1e-9. Measuring this disagreement directly (on a related, margin-less version of this same guard) found it at `1e-9'. Thus a gap below it cannot be trusted, and a gap above it cannot have come from rounding.Why a margin instead of fixing
operator<. The comparator's non-transitivity is a real, pre-existing defect used throughout the package, including flip decisions this MR doesn't touch. Fixing it changes which cell wins a tied maximum everywhere it's compared, including insideCollapseTriangulation's own unmodified code, which is a behavior change againstcgal/main, and this MR's evidence is entirely byte-identity. The margin changes nothing except the cases the comparator was never trustworthy for.The cost.
ANGLES_UNDECIDEDis measured at 0.3-4.0% of a mesh's rejections. For those,collapse_edge()still buildsCollapseTriangulationand asks it, exactly as before, which is why the class isn't deleted here.Testing
Verdict equivalence, instrumented directly against
cgal/main: built plainmainandmain+ commits 1–2 (angle test added and reordered, simulation still runs unconditionally), both countingCollapseTriangulation::collapse()'s return value.cgal/main: simulated / valid / refusedThe
validcount matches exactly.Byte-identical output, all three commits applied to
cgal/main:101556.mesh matters specifically: a margin-less version of this angle test was not byte-identical here (4 of 10 large Thingi10K meshes drifted), which is the reason the margin exists. With it, this mesh is clean too.
Performance.
perf stat -e instructions, Linux, Release, sequential. Instructions are the primary metric (deterministic, environment-independent); wall-clock is 8-run position-balanced ABBA where practical.cgal/maininstrcgal/mainwallRelease Management