Flip chord topology reorder - #9588
Merged
Merged
Conversation
…cets find_best_flip_to_improve_dh circulated the facets around an edge once to enumerate its apices, then re-circulated them again for every apex to test for chords. The second circulation always re-derives the same ring, so collect it once into ring_apices and index the chord test on it directly.
find_best_flip built two unordered_set<Vertex_handle> (heap allocation plus hashing) to hold the handful of always-distinct ring apices around a flip candidate edge, only ever scanned for size() and iterated. A small_vector with a linear membership check holds the same set inline. flip_3_to_2, the only consumer, does not depend on iteration order: its is_facet check is symmetric in its three arguments, and vh2/vh3 are each picked by testing individually against ch0/ch1.
For each boundary-facet apex vi, topology_test ran nb_incident_subdomains(vi) (a full cell-star walk) before is_edge_in_complex(v0,vi) and (v1,vi), each of which walks the star again just to reach a single bimap lookup. vi shares its facet with v0 and v1, so both edges exist in the triangulation by construction: the star walks inside is_edge_in_complex always succeed and exist only to reach c3t3.is_in_complex(edge), which is what c3t3.is_in_complex(v0,vi) computes directly. Reordering the three (now pure) predicates so the two cheap complex-membership lookups run first leaves the conjunction's result unchanged.
IasonManolas
changed the title
Member
|
explanatory comments are definitely fine! Can you please remove your last commit and leave the comments? |
IasonManolas
force-pushed
the
mr/flip-chord-topology-reorder
branch
from
August 3, 2026 11:29
ab12c2d to
f9af469
Compare
janetournois
reviewed
Aug 6, 2026
janetournois
reviewed
Aug 6, 2026
Comment on lines
+1317
to
+1319
| if (c3t3.is_in_complex(v0, vi) | ||
| && c3t3.is_in_complex(v1, vi) | ||
| && nb_incident_subdomains(vi, c3t3) > 1) |
Member
There was a problem hiding this comment.
In theory it is possible to have a corner (0d-in-complex) or feature edge (1d-in-complex) that has only one incident subdomain, like for example a needle in the middle of a sphere.
I agree that is_edge() is not necessary though
Member
Author
There was a problem hiding this comment.
I agree that case is missed, but nb_incident_subdomains(vi, c3t3) > 1 was not introduced by this PR. I would rather not change it here, since dropping the subdomain clause makes topology_test stricter and would reject collapses that are currently accepted, i.e. it changes remeshing output and needs its own quality evaluation.
Member
There was a problem hiding this comment.
let's leave it unchanged for now, and only reorder conditions by cost
Member
|
Successfully tested in CGAL-6.3-Ic-52 |
The reordered guard only ever asks whether a second subdomain index exists around vi, never how many there are, so the star walk it ends with can stop at the first one it finds instead of counting the whole star. Written for CGAL#9590 and moved here: topology_test is its only caller, and CGAL#9590's copy of it conflicted with the reordering this branch applies to the same line.
IasonManolas
added a commit
to IasonManolas/cgal
that referenced
this pull request
Aug 12, 2026
has_several_incident_subdomains() has exactly one caller, the boundary-apex guard in topology_test(), and CGAL#9588 rewrites that same guard so its two complex-membership lookups run before the star walk. Holding the helper here made the two branches conflict on that one line for no reason: it belongs with the reordering that decides when it runs. CGAL#9588 now introduces it and uses it as the last conjunct, which is faster than either branch alone.
|
This pull-request was previously marked with the label |
janetournois
approved these changes
Aug 18, 2026
janetournois
approved these changes
Aug 18, 2026
Member
|
Successfully tested in CGAL-6.3-Ic-58 |
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
5 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
Profiling the flip pass on Thingi10K-scale meshes found three independent costs in the same small region of
flip_edges.h/tetrahedral_remeshing_helpers.h, all coming from redundant circulation or hashing over the same handful of vertices around a candidate flip edge. Three commits:collect flip chord-test ring apices once instead of re-circulating facets:find_best_flip_to_improve_dh's chord test used to re-circulate the facets around the edge once per ring apex just to enumerate the other apices. Those apices are collected once into asmall_vectorduring the existing first pass, and the chord test indexes that ring instead, in the same order, stopping at the first chord found exactly as before.flip apex vertex set: unordered_set -> small_vector:find_best_flipbuilt a heap-allocated, hashedunordered_setto hold a handful of always-distinct ring apices, only ever scanned forsize()and iterated. Asmall_vectorwith a linear membership check holds the same set inline. The only consumer,flip_3_to_2, does not depend on iteration order.topology_test: check complex membership before the subdomain star walk: for each boundary-facet apex,topology_testran a full cell-star walk (nb_incident_subdomains) before two cheap complex-membership checks that, in this call site, always succeed anyway (the apex shares its facet with both edge endpoints, so both edges exist by construction). Reordering the checks and replacing the redundant edge-lookup with a direct vertex-pair lookup removes an unnecessary star walk without changing the result.Testing
Byte-identical output:
Performance.
perf stat -e instructions, Linux, Release, sequential, against plaincgal/main. Wall-clock is 8-run position-balanced ABBA (median of 4) for bear and bunny00, single run for 101556.mesh (10 iterations is intractable at that size, so the run used 3).cgal/maininstrcgal/mainwallRelease Management