Repository navigation
fix(Colocation): robustness and performance of colocation and degeneration inspections - #203
Conversation
The small edges task of each surface captured the loop-local enable_edges flag by reference and read it after the iteration ended (use after scope). Not covered by a deterministic test: only sanitizers can catch it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Vertices without unique vertex all compared equal (NO_ID) and were filtered out as if they were the same unique vertex. The filter now keeps one point per unique vertex plus every unlinked point, with one unique vertex lookup per point instead of one per pair. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…edges inspect_edges_degeneration(threshold) only checked line and surface edges, while inspect_elements_degeneration also reports block small edges. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Small edges detection computed a square root per edge. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Small edges detection enabled the SurfaceEdges/SolidEdges of every surface and block (one hash map insertion per element edge) and kept them attached to the meshes, only to test edge lengths. Element edges are now scanned directly; edges are built only when a small edge is found, to report its index, and only kept on the components having issues. cva/brep_from_topo 1.81s -> 1.55s, corbi_tet_mesh 0.37s -> 0.36s (inspect_elements_degeneration, identical results). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
small_height_polygons/polyhedra computed the minimum height of each element serially. Heights are now computed in parallel, issues are added in element order afterwards. cva/brep_from_topo 1.55s -> 0.38s, corbi_tet_mesh 0.36s -> 0.10s (inspect_elements_degeneration, identical results). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A vector was allocated for every unique point (one allocation per mesh vertex) before dropping the groups with a single point. Points are now counted per unique point and groups only built when needed, in the same order. cva/brep_from_topo 0.114s -> 0.092s (identical results). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tion The boolean query computed the full colocation mapping. It now looks for a second point within GLOBAL_EPSILON among the two nearest neighbors of each point, in parallel, and stops as soon as one is found. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The line, surface and block colocation loops were three copies of the same code, each formatting the component name and uuid twice per issue. They share one helper which builds the label once, only for components having colocated vertices. Messages are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each line, surface and block built its NNSearch serially. Components are now inspected in parallel tasks; issues are added in the same order. corbi_tet_mesh 0.037s -> 0.013s, fimiston 0.073s -> 0.036s (identical results). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- component lookups: model.component(id) tried every component type before a second typed lookup; the cmv type is now dispatched once - the active unique vertices PointSet is built directly instead of being created for all unique vertices then shrunk with delete_vertices, and the GenericMapping is replaced by index vectors - points and per unique vertex checks are computed in parallel - the work is done on first use instead of in the constructor, which BRepMeshesInspector always calls fimiston 0.19s -> 0.085s, cva/brep_from_topo 0.21s -> 0.087s (inspect_unique_vertices, identical results). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cpp-Linter Report
|
| if( Vector< dimension >{ mesh_.point( edge_vertices[0] ), | ||
| mesh_.point( edge_vertices[1] ) } | ||
| .length2() | ||
| <= threshold * threshold ) |
There was a problem hiding this comment.
is the performance improvement sufficient enough to trade off with the much less readibility of this proposal? if such strategy is deployed very enough why not add a edge_length2(..) method in meshes?
There was a problem hiding this comment.
Not worth it here: the gain is one sqrt per edge, negligible next to the rest of the loop. Reverted to mesh_.edge_length( edge_id ) in 3d6658c. The squared comparison stays only in DegenerationImpl::edge_is_smaller_than_threshold, where it is hidden behind a named helper, so no edge_length2 needed in OpenGeode for now.
| small_edge_indices.push_back( edge_index.value() ); | ||
| } | ||
| } | ||
| absl::c_sort( small_edge_indices ); |
There was a problem hiding this comment.
is it better to use a c_sort on a flat_hash_set, or to use a linked_hash_set?
There was a problem hiding this comment.
The sort is not on the set itself but on the edge indices obtained afterwards via edge_from_vertices, so issues are reported in edge index order (same order as before this PR, and deterministic). A linked_hash_set would keep insertion order, i.e. polygon/polyhedron traversal order, which is a different order and costs more memory per entry. Since the number of small edges is usually tiny, sorting that small vector is cheap; keeping flat_hash_set + sort.
| surface_id, ") colocated vertices" ) ) | ||
| && absl::StrContains( message, | ||
| absl::StrCat( "Surface ", surface_id, " (", surface_id, | ||
| ") has vertices 1 0 colocated at position [0 0 0]" ) ), |
There was a problem hiding this comment.
two sucessive space between "vertices" and "1 0"
There was a problem hiding this comment.
Fixed in 3d6658c: the separator was added both by the literal and by the vertex list. Removed the trailing space in the three colocation messages (component meshes, mesh colocation, unique vertices) and updated the test.
Remove the double space in colocated vertices messages and use EdgedCurve::edge_length for readability. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🎉 This PR is included in version 6.12.4-rc.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 6.12.4 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Summary
Robustness and performance fixes for the colocation and degeneration inspections, from a review of OpenGeode-Inspector. There is one commit per fix. The new tests are in
tests/inspection/test-brep-colocation-degeneration.cpp.Robustness
NO_ID), so they were never reported. The filter now does one unique vertex lookup per point instead of one per pair.BRepComponentMeshesDegeneration::inspect_edges_degeneration(threshold)now includes block edges, asinspect_elements_degenerationalready did.Performance (identical results on all benchmark models)
SurfaceEdges/SolidEdgeswere enabled on every surface and block, and kept attached to them, only to measure edge lengths. Element edges are now scanned directly. Edges are only built when a small edge is found, so that its index can be reported, and they are only kept on the components that have issues.mesh_has_colocated_pointsstops at the first colocation.GenericMapping;inspect_elements_degeneration, cva/brep_from_topoinspect_elements_degeneration, corbi_tet_meshinspect_unique_vertices, fimistoninspect_unique_vertices, cva/brep_from_topoinspect_meshes_point_colocations, fimistoninspect_meshes_point_colocations, corbi_tet_meshThe large models come from the Geode-Conversion, Geode-Explicit and Geode-Background test data.
Not changed
<vs<=against the threshold: OpenGeode mixes them too (is_edge_degenerateduses<=,is_polyhedron_degenerateduses<), and the difference only matters for a value exactly equal to the threshold.🤖 Generated with Claude Code