From ce5580cd772bffc9774ba21b9be22591fd925929 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 14 Oct 2026 17:46:19 +0200 Subject: [PATCH 1/3] fix(InspectionIssues): InspectionIssuesMap::nb_issues counts issues, not components nb_issues() returned the number of components having issues, so a surface with 10k unlinked vertices counted as 1 issue. It now sums the issues of every component; the former value is available as nb_components_with_issues(). Co-Authored-By: Claude Opus 5.5 --- .../python/src/inspection/information.hpp | 2 + .../python/tests/inspection/test-py-brep.py | 10 +- .../inspector/inspection/information.hpp | 20 ++- tests/inspection/CMakeLists.txt | 8 ++ tests/inspection/test-brep.cpp | 26 ++-- tests/inspection/test-information.cpp | 122 ++++++++++++++++++ 6 files changed, 171 insertions(+), 17 deletions(-) create mode 100644 tests/inspection/test-information.cpp diff --git a/bindings/python/src/inspection/information.hpp b/bindings/python/src/inspection/information.hpp index 17e7fc8d..6e712f80 100644 --- a/bindings/python/src/inspection/information.hpp +++ b/bindings/python/src/inspection/information.hpp @@ -48,6 +48,8 @@ namespace geode .def( "set_description", &InspectionIssuesMap::set_description ) .def( "description", &InspectionIssuesMap::description ) .def( "nb_issues", &InspectionIssuesMap::nb_issues ) + .def( "nb_components_with_issues", + &InspectionIssuesMap::nb_components_with_issues ) .def( "issues_map", &InspectionIssuesMap::issues_map ) .def( "string", &InspectionIssuesMap::string ); } diff --git a/bindings/python/tests/inspection/test-py-brep.py b/bindings/python/tests/inspection/test-py-brep.py index 25728e2f..584784f6 100644 --- a/bindings/python/tests/inspection/test-py-brep.py +++ b/bindings/python/tests/inspection/test-py-brep.py @@ -221,9 +221,9 @@ def check_a1(verbose): else: print("model_A1 topology is invalid.") nb_model_issues = launch_topological_validity_checks(result.topology, verbose) - if nb_model_issues != 5191: + if nb_model_issues != 6182: raise ValueError( - "[Test] model model_A1 should have 5191 unique vertices with topological problems." + "[Test] model model_A1 should have 6182 unique vertices with topological problems." ) nb_component_meshes_issues = launch_component_meshes_validity_checks( result.meshes, verbose @@ -242,9 +242,9 @@ def inspect_model_A1(model_brep, verbose): else: print("model_A1_valid topology is invalid.") nb_model_issues = launch_topological_validity_checks(result.topology, verbose) - if nb_model_issues != 5191: + if nb_model_issues != 6182: raise ValueError( - "[Test] model model_A1_valid should have 5191 topological problems." + "[Test] model model_A1_valid should have 6182 topological problems." ) nb_component_meshes_issues = launch_component_meshes_validity_checks( result.meshes, verbose @@ -268,7 +268,7 @@ def inspect_model_mss(model_brep, verbose): else: print("model mss topology is invalid.") nb_model_issues = launch_topological_validity_checks(result.topology, verbose) - if nb_model_issues != 50: + if nb_model_issues != 65: raise ValueError( "[Test] model mss.og_strm should have 50 topological problems, not " + str(nb_model_issues) diff --git a/include/geode/inspector/inspection/information.hpp b/include/geode/inspector/inspection/information.hpp index 549514b1..f9e39619 100644 --- a/include/geode/inspector/inspection/information.hpp +++ b/include/geode/inspector/inspection/information.hpp @@ -119,7 +119,23 @@ namespace geode return description_; } + /*! + * Total number of issues, summed over every component of the map + */ [[nodiscard]] index_t nb_issues() const + { + index_t nb{ 0 }; + for( const auto& issues : issues_map_ ) + { + nb += issues.second.nb_issues(); + } + return nb; + } + + /*! + * Number of components having at least one issue + */ + [[nodiscard]] index_t nb_components_with_issues() const { return issues_map_.size(); } @@ -140,8 +156,8 @@ namespace geode { return absl::StrCat( "no issues with ", description_, "\n" ); } - auto message = absl::StrCat( - issues_map_.size(), " issues with ", description_ ); + auto message = + absl::StrCat( nb_issues(), " issues with ", description_ ); for( const auto& issues : issues_map_ ) { absl::StrAppend( &message, "\n\t", issues.second.string() ); diff --git a/tests/inspection/CMakeLists.txt b/tests/inspection/CMakeLists.txt index f51c1a42..b9e4187c 100755 --- a/tests/inspection/CMakeLists.txt +++ b/tests/inspection/CMakeLists.txt @@ -44,6 +44,14 @@ add_geode_test( OpenGeode::mesh ${PROJECT_NAME}::inspection ) + +add_geode_test( + SOURCE "test-information.cpp" + DEPENDENCIES + OpenGeode::basic + ${PROJECT_NAME}::inspection +) + add_geode_test( SOURCE "test-edgedcurve-colocation.cpp" DEPENDENCIES diff --git a/tests/inspection/test-brep.cpp b/tests/inspection/test-brep.cpp index 70e73c49..f33f44b9 100644 --- a/tests/inspection/test-brep.cpp +++ b/tests/inspection/test-brep.cpp @@ -80,8 +80,11 @@ geode::index_t lines_topological_validity( result .unique_vertices_linked_to_several_lines_but_not_linked_to_a_corner .nb_issues(); - nb_issues += - result.line_edges_with_wrong_component_edges_around.nb_issues(); + for( const auto& issue : + result.line_edges_with_wrong_component_edges_around.issues_map() ) + { + nb_issues += issue.second.nb_issues(); + } geode::Logger::info( "BRep Lines Topology check: ", nb_issues, " issues." ); if( string ) { @@ -108,8 +111,11 @@ geode::index_t surfaces_topological_validity( nb_issues += result.unique_vertices_linked_to_a_surface_with_invalid_embbedings .nb_issues(); - nb_issues += - result.surface_polygons_with_wrong_component_facets_around.nb_issues(); + for( const auto& issue : result + .surface_polygons_with_wrong_component_facets_around.issues_map() ) + { + nb_issues += issue.second.nb_issues(); + } geode::Logger::info( "BRep Surfaces Topology check: ", nb_issues, " issues." ); if( string ) @@ -326,8 +332,8 @@ void check_model_a1( bool string ) const auto nb_topological_issues = launch_topological_validity_checks( result.topology, string ); geode::OpenGeodeInspectorInspectionException::test( - nb_topological_issues == 5191, "model_A1 has ", nb_topological_issues, - " topological problems instead of 5191." ); + nb_topological_issues == 6182, "model_A1 has ", nb_topological_issues, + " topological problems instead of 6182." ); const auto nb_component_meshes_issues = launch_component_meshes_validity_checks( result.meshes, string ); @@ -349,8 +355,8 @@ void check_model_a1_valid( bool string ) const auto nb_topological_issues = launch_topological_validity_checks( result.topology, string ); geode::OpenGeodeInspectorInspectionException::test( - nb_topological_issues == 5191, "model_A1_valid has ", - nb_topological_issues, " topological problems instead of 5191." ); + nb_topological_issues == 6182, "model_A1_valid has ", + nb_topological_issues, " topological problems instead of 6182." ); const auto nb_component_meshes_issues = launch_component_meshes_validity_checks( result.meshes, string ); @@ -372,8 +378,8 @@ void check_model_mss( bool string ) const auto nb_topological_issues = launch_topological_validity_checks( result.topology, string ); geode::OpenGeodeInspectorInspectionException::test( - nb_topological_issues == 50, "mss has ", nb_topological_issues, - " topological problems instead of 50." ); + nb_topological_issues == 65, "mss has ", nb_topological_issues, + " topological problems instead of 65." ); const auto nb_component_meshes_issues = launch_component_meshes_validity_checks( result.meshes, string ); diff --git a/tests/inspection/test-information.cpp b/tests/inspection/test-information.cpp new file mode 100644 index 00000000..bcdb3405 --- /dev/null +++ b/tests/inspection/test-information.cpp @@ -0,0 +1,122 @@ +/* + * Copyright (c) 2019 - 2026 Geode-solutions + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + */ + +#include +#include +#include +#include + +#include + +namespace +{ + geode::InspectionIssues< geode::index_t > make_issues( + geode::index_t nb_issues ) + { + geode::InspectionIssues< geode::index_t > issues{ "component issues" }; + for( const auto issue : geode::Range{ nb_issues } ) + { + issues.add_issue( issue, absl::StrCat( "issue ", issue ) ); + } + return issues; + } +} // namespace + +void check_issues() +{ + geode::InspectionIssues< geode::index_t > empty{ "nothing" }; + geode::OpenGeodeInspectorInspectionException::test( + empty.string() == "no issues with nothing\n", + "[Test] Wrong empty InspectionIssues string: ", empty.string() ); + + const auto issues = make_issues( 2 ); + geode::OpenGeodeInspectorInspectionException::test( + issues.nb_issues() == 2, "[Test] Wrong InspectionIssues count" ); + const std::string expected{ + "2 issues with component issues\n\tissue 0\n\tissue 1\n" + }; + geode::OpenGeodeInspectorInspectionException::test( + issues.string() == expected, + "[Test] Wrong InspectionIssues string: ", issues.string() ); +} + +void check_issues_map() +{ + geode::InspectionIssuesMap< geode::index_t > map{ "map issues" }; + geode::OpenGeodeInspectorInspectionException::test( + map.nb_issues() == 0 && map.nb_components_with_issues() == 0, + "[Test] Empty InspectionIssuesMap should have no issues" ); + geode::OpenGeodeInspectorInspectionException::test( + map.string() == "no issues with map issues\n", + "[Test] Wrong empty InspectionIssuesMap string: ", map.string() ); + + const geode::uuid first; + const geode::uuid second; + const geode::uuid third; + map.add_issues_to_map( first, make_issues( 3 ) ); + map.add_issues_to_map( second, make_issues( 0 ) ); + map.add_issues_to_map( third, make_issues( 1 ) ); + geode::OpenGeodeInspectorInspectionException::test( + map.nb_components_with_issues() == 2, + "[Test] InspectionIssuesMap should have 2 components with issues, " + "not ", + map.nb_components_with_issues() ); + geode::OpenGeodeInspectorInspectionException::test( map.nb_issues() == 4, + "[Test] InspectionIssuesMap should have 4 issues, not ", + map.nb_issues() ); + geode::index_t nb_issues_from_map{ 0 }; + for( const auto& issues : map.issues_map() ) + { + nb_issues_from_map += issues.second.nb_issues(); + } + geode::OpenGeodeInspectorInspectionException::test( + map.nb_issues() == nb_issues_from_map, + "[Test] InspectionIssuesMap count differs from its content" ); + + const std::string expected{ "4 issues with map issues" + "\n\t3 issues with component issues" + "\n\tissue 0\n\tissue 1\n\tissue 2\n" + "\n\t1 issues with component issues" + "\n\tissue 0\n" + "\n" }; + geode::OpenGeodeInspectorInspectionException::test( + map.string() == expected, + "[Test] Wrong InspectionIssuesMap string: ", map.string() ); +} + +int main() +{ + try + { + geode::OpenGeodeInspectorInspectionLibrary::initialize(); + check_issues(); + check_issues_map(); + + geode::Logger::info( "TEST SUCCESS" ); + return 0; + } + catch( ... ) + { + return geode::geode_lippincott(); + } +} From af961157be65cb455a4695a973ef56e9fa57906b Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 14 Oct 2026 17:47:25 +0200 Subject: [PATCH 2/3] perf(InspectionIssues): build issue reports in place InspectionIssuesMap::string() built a temporary string per component then copied it; issues are now appended in place into a reserved buffer. Co-Authored-By: Claude Opus 5.5 --- .../inspector/inspection/information.hpp | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/include/geode/inspector/inspection/information.hpp b/include/geode/inspector/inspection/information.hpp index f9e39619..a59f371e 100644 --- a/include/geode/inspector/inspection/information.hpp +++ b/include/geode/inspector/inspection/information.hpp @@ -69,19 +69,33 @@ namespace geode } [[nodiscard]] std::string string() const + { + std::string message; + append_string( message ); + return message; + } + + void append_string( std::string& message ) const { if( issues_.empty() ) { - return absl::StrCat( "no issues with ", description_, "\n" ); + absl::StrAppend( + &message, "no issues with ", description_, "\n" ); + return; } - auto message = - absl::StrCat( issues_.size(), " issues with ", description_ ); + auto size = message.size() + description_.size() + 32; + for( const auto& issue : messages_ ) + { + size += issue.size() + 2; + } + message.reserve( size ); + absl::StrAppend( + &message, issues_.size(), " issues with ", description_ ); for( const auto& issue : messages_ ) { absl::StrAppend( &message, "\n\t", issue ); } - absl::StrAppend( &message, "\n" ); - return message; + message.append( "\n" ); } [[nodiscard]] const std::vector< IssueType >& issues() const @@ -160,7 +174,8 @@ namespace geode absl::StrCat( nb_issues(), " issues with ", description_ ); for( const auto& issues : issues_map_ ) { - absl::StrAppend( &message, "\n\t", issues.second.string() ); + message.append( "\n\t" ); + issues.second.append_string( message ); } absl::StrAppend( &message, "\n" ); return message; From c2c6c2485b194d7ff6857a6fb2ac875c519281b4 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Wed, 14 Oct 2026 17:48:14 +0200 Subject: [PATCH 3/3] fix(CLI): remove nonexistent --noXXX options from usage messages The inspector tools advertised --noXXX flags to disable criteria, but only --input exists. Co-Authored-By: Claude Opus 5.5 --- src/bin/geode-inspector-brep.cpp | 4 +--- src/bin/geode-inspector-edgedcurve.cpp | 8 +++----- src/bin/geode-inspector-section.cpp | 4 +--- src/bin/geode-inspector-solid.cpp | 4 +--- src/bin/geode-inspector-surface.cpp | 8 +++----- 5 files changed, 9 insertions(+), 19 deletions(-) diff --git a/src/bin/geode-inspector-brep.cpp b/src/bin/geode-inspector-brep.cpp index 265171ea..c1ea2667 100644 --- a/src/bin/geode-inspector-brep.cpp +++ b/src/bin/geode-inspector-brep.cpp @@ -54,9 +54,7 @@ int main( int argc, char* argv[] ) { absl::SetProgramUsageMessage( absl::StrCat( "BRep inspector from Geode-solutions.\n", - "Sample usage:\n", argv[0], " --input my_brep.og_brep\n", - "Default behavior tests all available criteria, to disable one " - "use --noXXX, e.g. --nocomponent_linking" ) ); + "Sample usage:\n", argv[0], " --input my_brep.og_brep\n" ) ); absl::ParseCommandLine( argc, argv ); geode::OpenGeodeIOModelLibrary::initialize(); diff --git a/src/bin/geode-inspector-edgedcurve.cpp b/src/bin/geode-inspector-edgedcurve.cpp index dcaa30b7..4002e7c5 100644 --- a/src/bin/geode-inspector-edgedcurve.cpp +++ b/src/bin/geode-inspector-edgedcurve.cpp @@ -56,11 +56,9 @@ int main( int argc, char* argv[] ) { try { - absl::SetProgramUsageMessage( - absl::StrCat( "EdgedCurve inspector from Geode-solutions.\n", - "Sample usage:\n", argv[0], " --input my_edgedcurve.og_edc3d\n", - "Default behavior test all available criteria, to disable one " - "use --noXXX, e.g. --nocolocation" ) ); + absl::SetProgramUsageMessage( absl::StrCat( + "EdgedCurve inspector from Geode-solutions.\n", "Sample usage:\n", + argv[0], " --input my_edgedcurve.og_edc3d\n" ) ); absl::ParseCommandLine( argc, argv ); geode::OpenGeodeIOMeshLibrary::initialize(); diff --git a/src/bin/geode-inspector-section.cpp b/src/bin/geode-inspector-section.cpp index 6672340b..5545af14 100644 --- a/src/bin/geode-inspector-section.cpp +++ b/src/bin/geode-inspector-section.cpp @@ -54,9 +54,7 @@ int main( int argc, char* argv[] ) { absl::SetProgramUsageMessage( absl::StrCat( "Section inspector from Geode-solutions.\n", - "Sample usage:\n", argv[0], " --input my_section.og_sctn\n", - "Default behavior tests all available criteria, to disable one " - "use --noXXX, e.g. --nocomponent_linking" ) ); + "Sample usage:\n", argv[0], " --input my_section.og_sctn\n" ) ); absl::ParseCommandLine( argc, argv ); geode::OpenGeodeIOModelLibrary::initialize(); diff --git a/src/bin/geode-inspector-solid.cpp b/src/bin/geode-inspector-solid.cpp index 0b7ba2e8..7b26f788 100644 --- a/src/bin/geode-inspector-solid.cpp +++ b/src/bin/geode-inspector-solid.cpp @@ -63,9 +63,7 @@ int main( int argc, char* argv[] ) { absl::SetProgramUsageMessage( absl::StrCat( "Solid inspector from Geode-solutions.\n", - "Sample usage:\n", argv[0], " --input my_solid.og_tsf3d\n", - "Default behavior test all available criteria, to disable one " - "use --noXXX, e.g. --noadjacency" ) ); + "Sample usage:\n", argv[0], " --input my_solid.og_tsf3d\n" ) ); absl::ParseCommandLine( argc, argv ); geode::OpenGeodeIOMeshLibrary::initialize(); diff --git a/src/bin/geode-inspector-surface.cpp b/src/bin/geode-inspector-surface.cpp index f148f1b6..d6053d78 100644 --- a/src/bin/geode-inspector-surface.cpp +++ b/src/bin/geode-inspector-surface.cpp @@ -59,11 +59,9 @@ int main( int argc, char* argv[] ) { try { - absl::SetProgramUsageMessage( - absl::StrCat( "Surface inspector from Geode-solutions.\n", - "Sample usage:\n", argv[0], " --input my_surface.og_tsf3d\n", - "Default behavior test all available criteria, to disable one " - "use --noXXX, e.g. --noadjacency" ) ); + absl::SetProgramUsageMessage( absl::StrCat( + "Surface inspector from Geode-solutions.\n", "Sample usage:\n", + argv[0], " --input my_surface.og_tsf3d\n" ) ); absl::ParseCommandLine( argc, argv ); geode::OpenGeodeIOMeshLibrary::initialize();