Repository navigation
fix(Topology): robustness and performance of the topology inspection - #200
Merged
Merged
Conversation
…sage Unique vertices non-bijectively linked to a component mesh vertex were reported as "linked to inexistant mesh vertex". Report the unique vertex the mesh vertex is actually linked to instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
inspect_brep/section_topology caught any OpenGeodeException thrown by the corners, lines, surfaces or blocks inspection and silently left that whole result empty, so the model could be reported valid. A unique vertex linked to a missing or unregistered component (which the per vertex checks look up without has_*() guards, and unique_vertex() throws std::out_of_range for) triggered it. Each per unique vertex check now skips such a vertex (it is reported as linked to an inexistant mesh vertex), the validity check treats it as invalid, and other exceptions are no longer swallowed. Such links cannot be built through the public API (removing a component cleans them), only from corrupted files, so this is not covered by a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The invalid embedding messages called brep.surface() on the embedding even when it was a Block, which threw while building the message: the issue was lost (and, before the previous fix, the whole lines result). The message now uses the embedding component type and name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…aces closed_linked_surfaces called front()/pop() on an empty queue when the model boundaries had no items (undefined behavior). An empty list is now handled and empty model boundaries have nothing to check. The new test documents the expected result; without the fix the undefined behavior does not crash here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ction BRepRayTracing::is_point_inside_block throws on ambiguous rays; the exception escaped find_dangling_surfaces and emptied the whole blocks topology result. A few polygons of the surface are now tried, a surface which cannot be located is not reported as dangling, and only active blocks are considered. No deterministic ambiguous configuration is available for a test; results on all test models are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ndaries fuse_brep_surfaces_from_list merged vertices by unique vertex: every vertex without unique vertex (NO_ID) was fused into a single point, which broke the enclosing surface used to check block boundaries closure. Unlinked vertices now keep their own point. Not covered by a dedicated test: it requires a block with several boundary shells and unlinked boundary vertices. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The predicted number of block component mesh vertices was computed with unsigned arithmetic: an invalid topology giving a negative prediction was reported as expecting about 4294967295 block cmvs. It is now computed signed. Results on all test models are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…non-manifold expected_block_cmvs_and_error logged a formatted warning (point to string) for every unique vertex on one side of a topological non-manifold, from the parallel inspection tasks and again during validity checks. The inspection result already reports the vertices whose count is wrong. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…mbda Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
For each polygon of each surface, component_mesh_polygons computed the line, surface and block polygons around it (the surface part going through polygons_around_vertex of every surface sharing the vertices), plus one unique_vertex lookup per polygon vertex, while only the block facets are checked. Only the block facets are now computed, surface unique vertices are fetched once, and the surface/block relationships (is_active, is_boundary, is_internal) are computed once per surface instead of once per polygon. inspect_brep_topology: fimiston 0.46s -> 0.15s, cva/brep_from_topo 0.37s -> 0.30s, corbi_tet_mesh 0.25s -> 0.18s (identical results). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cpp-Linter Report
|
Member
|
On the summary, I think there is indeed a deep analysis of active/inactive homogeneity in the ecosystem |
panquez
approved these changes
Oct 15, 2026
| brep_.unique_vertex( cmv ); | ||
| linked_unique_vertex != uv_id ) | ||
| { | ||
| linked_to_nonbijective_result.add_issue( uv_id, |
Member
There was a problem hiding this comment.
I suggest that a follow-up of this PR is to review and improve message for readability by users
Member
Author
There was a problem hiding this comment.
Agreed, message readability deserves its own pass: I'll keep it for a follow-up PR so this one stays focused on robustness/performance.
| result | ||
| .unique_vertices_linked_to_surface_with_wrong_relationship_to_blocks | ||
| .add_issue( unique_vertex_id, problem_message.value() ); | ||
| if( const auto problem_message = |
Member
There was a problem hiding this comment.
Maybe putting each if-branch call into a dedicated try catch block will be much better to fill result with other non failing criteria
Member
Author
There was a problem hiding this comment.
Done in d024403: each check now runs in its own try/catch through a new internal::add_unique_vertex_issue helper (topology_helpers.hpp), so a throwing check no longer prevents the other checks from filling the result. Covered by test_checks_independent_of_failing_check (fails before, passes after).
| result | ||
| .unique_vertices_linked_to_not_internal_nor_boundary_corner | ||
| .add_issue( unique_vertex_id, problem_message.value() ); | ||
| if( const auto problem_message = |
Member
There was a problem hiding this comment.
same remark here, and check everywhere to homogeneize changes if applied
Member
Author
There was a problem hiding this comment.
Done in d024403, applied homogeneously to all per-unique-vertex loops: BRep corners/lines/surfaces/blocks and Section corners/lines/surfaces.
| { | ||
| return brep_.unique_vertex( cmv ); | ||
| } | ||
| catch( const std::out_of_range& ) |
Member
There was a problem hiding this comment.
only this exception, or catch all kind of exceptions ? What happens if we change the throwable exception types without changing this code
Member
Author
There was a problem hiding this comment.
Good point: it now catches std::exception (d024403, BRep and Section), so changing the thrown type upstream won't break it.
| /// On one side of a topological non-manifold => 2 cases possible, | ||
| /// depending on wether the topological non-manifold is on the | ||
| /// interior or exterior of the block | ||
| geode::Logger::warning( absl::StrCat( |
Member
There was a problem hiding this comment.
I do not agree to remove this comment. The formulae found and implemented is shown to not work in certain non manifold block vertex configurations. Do not remove it. Or solve the expected number of block cmvs formulae, which I assume is quite complicated
Member
Author
There was a problem hiding this comment.
You're right, restored in a98d601: the warning is back on the topological non-manifold case where the formula is unsure.
A check throwing on a wrong link to a component no longer prevents the following checks on the same unique vertex from filling the result. Catch std::exception when looking up the unique vertex of a component mesh vertex, so it does not depend on the exact exception type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… count The expected number of block cmvs formula is known to fail in some non manifold configurations, keep warning the user about it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Member
Author
|
🎉 This PR is included in version 6.12.4-rc.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
2 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
Robustness and performance fixes for the BRep and Section topology inspections, from a review of OpenGeode-Inspector. There is one commit per fix. The tests are in
tests/inspection/test-brep-topology.cppandtests/inspection/test-section-topology.cpp.Robustness
inspect_brep_topologyandinspect_section_topologycaught anyOpenGeodeExceptionfrom the corners, lines, surfaces or blocks inspection and silently left that whole result empty, so the model could be reported valid.brep.surface()on a Block, which threw. The issues were lost; they are now reported with the right component type.front()/pop()on an empty queue, which is undefined behavior.Performance (identical results on all test and benchmark models)
polygons_around_vertexof every surface. Surface unique vertices and the surface/block relationships are now computed once per surface.inspect_brep_topology(best of 3):Not done
polygons_around_vertex,polyhedra_around_vertexandis_vertex_on_border, whose lazy caches race in the current OpenGeode. This should come after fix(Mesh): robustness fixes found while reviewing OpenGeode-Inspector OpenGeode#1354 is released.brep_blocks_are_meshedon rare branches, the edge 0 CME per line vertex, per vertexcomponents_uuidsallocations, the BFS of closed boundaries, andline_on_block_boundary_count. The lines, corners and blocks parts are now a small share of the time.🤖 Generated with Claude Code