From 2917c88d57695cc1cd4d581a40569d03cfe08d63 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 09:32:42 +0200 Subject: [PATCH 01/14] fix(Relationships): rebuild component index from vertex ids on load The uuid -> graph vertex map was read from the file and trusted: an inconsistent map silently returned the relations of another component, and an out of range index was read out of bounds while removing isolated vertices. The map is now rebuilt in O(V) from the component ids stored on the graph vertices (still read for format compatibility). Vertices with an undefined or duplicated id are removed with their relations, with a warning. Co-Authored-By: Claude Opus 5.5 --- .../mixin/core/detail/relationships_impl.hpp | 12 +- .../model/mixin/core/detail/uuid_to_index.hpp | 10 + .../mixin/core/detail/relationships_impl.cpp | 78 ++++++- src/geode/model/mixin/core/relationships.cpp | 4 +- tests/model/CMakeLists.txt | 7 + tests/model/test-model-corrupted-files.cpp | 216 ++++++++++++++++++ 6 files changed, 315 insertions(+), 12 deletions(-) create mode 100644 tests/model/test-model-corrupted-files.cpp diff --git a/include/geode/model/mixin/core/detail/relationships_impl.hpp b/include/geode/model/mixin/core/detail/relationships_impl.hpp index 24e78ae26..ed2222db7 100644 --- a/include/geode/model/mixin/core/detail/relationships_impl.hpp +++ b/include/geode/model/mixin/core/detail/relationships_impl.hpp @@ -98,7 +98,13 @@ namespace geode [[nodiscard]] const Graph& graph() const; - void delete_isolated_vertices(); + /*! + * Rebuild the uuid to graph vertex map from the ids stored on the + * graph vertices, after loading a file. + * Vertices without a valid or with a duplicated id are removed with + * their relations, as well as isolated vertices. + */ + void repair_after_load(); private: friend class bitsery::Access; @@ -115,7 +121,7 @@ namespace geode archive.object( impl.uuid2index_ ); archive.ext( impl.ids_, bitsery::ext::StdSmartPtr{} ); - impl.delete_isolated_vertices(); + impl.repair_after_load(); }, []( Archive& archive, RelationshipsImpl& impl ) { archive.ext( @@ -123,7 +129,7 @@ namespace geode archive.object( impl.uuid2index_ ); archive.ext( impl.ids_, bitsery::ext::StdSmartPtr{} ); - impl.delete_isolated_vertices(); + impl.repair_after_load(); } } } ); } diff --git a/include/geode/model/mixin/core/detail/uuid_to_index.hpp b/include/geode/model/mixin/core/detail/uuid_to_index.hpp index 7433cfdd4..1271b6f9e 100644 --- a/include/geode/model/mixin/core/detail/uuid_to_index.hpp +++ b/include/geode/model/mixin/core/detail/uuid_to_index.hpp @@ -74,6 +74,16 @@ namespace geode uuid2index_[id] = index; } + void clear() + { + uuid2index_.clear(); + } + + void reserve( index_t capacity ) + { + uuid2index_.reserve( capacity ); + } + void update( absl::Span< const index_t > old2new ) { for( auto& it : uuid2index_ ) diff --git a/src/geode/model/mixin/core/detail/relationships_impl.cpp b/src/geode/model/mixin/core/detail/relationships_impl.cpp index 709b31fb1..d8bceb260 100644 --- a/src/geode/model/mixin/core/detail/relationships_impl.cpp +++ b/src/geode/model/mixin/core/detail/relationships_impl.cpp @@ -25,7 +25,10 @@ #include +#include + #include +#include #include #include @@ -288,18 +291,79 @@ namespace geode return register_component( component_id ); } - void RelationshipsImpl::delete_isolated_vertices() + void RelationshipsImpl::repair_after_load() { - auto builder = GraphBuilder::create( *graph_ ); - const auto old2new = builder->delete_isolated_vertices(); - for( const auto v : Indices{ old2new } ) + OpenGeodeModelException::check_exception( graph_ != nullptr, + nullptr, OpenGeodeException::TYPE::data, + "[Relationships::load] Missing relationships graph" ); + if( !ids_ ) { - if( old2new[v] == NO_ID ) + initialize_attributes(); + } + const auto nb_vertices = graph_->nb_vertices(); + const ComponentType undefined_type{ "undefined" }; + std::vector< bool > vertices_to_delete( nb_vertices, false ); + absl::flat_hash_set< uuid > valid_ids; + valid_ids.reserve( nb_vertices ); + index_t nb_undefined{ 0 }; + index_t nb_duplicated{ 0 }; + index_t nb_wrong_mappings{ 0 }; + for( const auto vertex : Range{ nb_vertices } ) + { + const auto& component_id = ids_->value( vertex ); + if( component_id.type == undefined_type ) { - uuid2index_.erase( v ); + vertices_to_delete[vertex] = true; + nb_undefined++; + continue; + } + if( !valid_ids.emplace( component_id.id ).second ) + { + vertices_to_delete[vertex] = true; + nb_duplicated++; + continue; + } + if( uuid2index_.index( component_id.id ) != vertex ) + { + nb_wrong_mappings++; } } - uuid2index_.update( old2new ); + if( nb_wrong_mappings > 0 + || uuid2index_.nb_mappings() != valid_ids.size() ) + { + Logger::warning( "[Relationships::load] Inconsistent component " + "index, rebuilding it from component ids" ); + } + if( nb_undefined > 0 || nb_duplicated > 0 ) + { + Logger::warning( "[Relationships::load] Removing ", + nb_undefined, " component(s) with undefined id and ", + nb_duplicated, + " duplicated component(s) with their relations" ); + std::vector< bool > edges_to_delete( + graph_->nb_edges(), false ); + for( const auto vertex : Range{ nb_vertices } ) + { + if( !vertices_to_delete[vertex] ) + { + continue; + } + for( const auto& edge : + graph_->edges_around_vertex( vertex ) ) + { + edges_to_delete[edge.edge_id] = true; + } + } + GraphBuilder::create( *graph_ )->delete_edges( + edges_to_delete ); + } + GraphBuilder::create( *graph_ )->delete_isolated_vertices(); + uuid2index_.clear(); + uuid2index_.reserve( graph_->nb_vertices() ); + for( const auto vertex : Range{ graph_->nb_vertices() } ) + { + uuid2index_.set_new_mapping( ids_->value( vertex ).id, vertex ); + } } } // namespace detail } // namespace geode diff --git a/src/geode/model/mixin/core/relationships.cpp b/src/geode/model/mixin/core/relationships.cpp index 8f522bb53..6b95aed6e 100644 --- a/src/geode/model/mixin/core/relationships.cpp +++ b/src/geode/model/mixin/core/relationships.cpp @@ -216,7 +216,7 @@ namespace geode std::move( graph ) ); impl.initialize_attributes(); impl.initialize_relation_attribute(); - impl.delete_isolated_vertices(); + impl.repair_after_load(); }, []( Archive& archive, Impl& impl ) { impl.graph_ = std::make_unique< OpenGeodeGraph >( @@ -228,7 +228,7 @@ namespace geode bitsery::ext::StdSmartPtr{} ); archive.ext( impl.ids_, bitsery::ext::StdSmartPtr{} ); - impl.delete_isolated_vertices(); + impl.repair_after_load(); }, []( Archive& archive, Impl& impl ) { archive.ext( diff --git a/tests/model/CMakeLists.txt b/tests/model/CMakeLists.txt index 9209debae..424d5f57a 100644 --- a/tests/model/CMakeLists.txt +++ b/tests/model/CMakeLists.txt @@ -101,6 +101,13 @@ add_geode_test( ${PROJECT_NAME}::geometry ${PROJECT_NAME}::model ) +add_geode_test( + SOURCE "test-model-corrupted-files.cpp" + DEPENDENCIES + ${PROJECT_NAME}::basic + ${PROJECT_NAME}::mesh + ${PROJECT_NAME}::model +) add_geode_test( SOURCE "test-model-mapping.cpp" DEPENDENCIES diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp new file mode 100644 index 000000000..7ea6f107d --- /dev/null +++ b/tests/model/test-model-corrupted-files.cpp @@ -0,0 +1,216 @@ +/* + * 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 +#include +#include +#include + +#include +#include +#include + +#include +#include +#include + +#include + +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +namespace +{ + /* Files of a model archive: file name -> file content */ + using ArchiveFiles = absl::btree_map< std::string, std::string >; + + std::string read_file( const std::filesystem::path& path ) + { + std::ifstream file{ path, std::ifstream::binary }; + return { std::istreambuf_iterator< char >{ file }, + std::istreambuf_iterator< char >{} }; + } + + ArchiveFiles extract_archive( std::string_view filename ) + { + const geode::UnzipFile unzip{ filename, geode::uuid{}.string() }; + unzip.extract_all(); + ArchiveFiles files; + for( const auto& file : std::filesystem::directory_iterator( + std::filesystem::u8path( unzip.directory() ) ) ) + { + files.emplace( + file.path().filename().u8string(), read_file( file.path() ) ); + } + return files; + } + + void write_archive( const ArchiveFiles& files, std::string_view filename ) + { + geode::ZipFile zip{ filename, geode::uuid{}.string() }; + for( const auto& [name, content] : files ) + { + const auto path = absl::StrCat( zip.directory(), "/", name ); + { + std::ofstream file{ std::filesystem::u8path( path ), + std::ofstream::binary }; + file << content; + } + zip.archive_file( path ); + } + zip.finalize(); + } + + /* Serialized uuid: Growable version 2 followed by the 16 bytes */ + std::string serialized_uuid( const geode::uuid& id ) + { + std::string bytes{ static_cast< char >( 2 ) }; + const auto hex = id.string(); + for( std::size_t i = 0; i < hex.size(); i++ ) + { + if( hex[i] == '-' ) + { + continue; + } + bytes.push_back( static_cast< char >( + std::stoi( hex.substr( i, 2 ), {}, 16 ) ) ); + i++; + } + return bytes; + } + + std::string serialized_u32( std::uint32_t value ) + { + std::string bytes; + for( const auto i : geode::LRange{ 4 } ) + { + bytes.push_back( static_cast< char >( value >> ( 8 * i ) ) ); + } + return bytes; + } + + void replace_unique( std::string& content, + const std::string& from, + const std::string& to, + std::string_view what ) + { + const auto position = content.find( from ); + geode::OpenGeodeModelException::test( + position != std::string::npos, "[Test] ", what, " not found" ); + geode::OpenGeodeModelException::test( + content.find( from, position + 1 ) == std::string::npos, "[Test] ", + what, " is not unique" ); + content.replace( position, from.size(), to ); + } + + geode::BRep saved_brep( std::string_view filename ) + { + auto brep = geode::load_brep( + absl::StrCat( geode::DATA_PATH, "box_brep.og_brep" ) ); + geode::save_brep( brep, filename ); + return brep; + } + + /* + * The relationships file stores a uuid -> graph vertex map duplicating + * the ids stored on the graph vertices. A corrupted map must not change + * the loaded relations. + */ + void test_inconsistent_relationships_map() + { + const auto brep = saved_brep( "relationships.og_brep" ); + const auto& id0 = brep.component_with_relation( 0 ).id; + geode::index_t other{ geode::NO_ID }; + for( const auto v : + geode::Range{ 1, brep.nb_components_with_relations() } ) + { + if( brep.nb_relations( brep.component_with_relation( v ).id ) + != brep.nb_relations( id0 ) ) + { + other = v; + break; + } + } + geode::OpenGeodeModelException::test( + other != geode::NO_ID, "[Test] No component to swap with" ); + const auto& id1 = brep.component_with_relation( other ).id; + auto files = extract_archive( "relationships.og_brep" ); + auto& content = files.at( "relationships" ); + replace_unique( content, + absl::StrCat( serialized_uuid( id0 ), serialized_u32( 0 ) ), + absl::StrCat( serialized_uuid( id0 ), serialized_u32( other ) ), + "First uuid mapping" ); + replace_unique( content, + absl::StrCat( serialized_uuid( id1 ), serialized_u32( other ) ), + absl::StrCat( serialized_uuid( id1 ), serialized_u32( 0 ) ), + "Second uuid mapping" ); + write_archive( files, "corrupted_relationships.og_brep" ); + + const auto loaded = + geode::load_brep( "corrupted_relationships.og_brep" ); + geode::OpenGeodeModelException::test( + loaded.nb_components_with_relations() + == brep.nb_components_with_relations(), + "[Test] Wrong number of components with relations" ); + for( const auto v : + geode::Range{ brep.nb_components_with_relations() } ) + { + const auto& id = brep.component_with_relation( v ).id; + geode::OpenGeodeModelException::test( + loaded.nb_relations( id ) == brep.nb_relations( id ), + "[Test] Wrong number of relations for ", id.string() ); + absl::flat_hash_set< geode::uuid > related; + for( const auto& component : brep.relations( id ) ) + { + related.insert( component.id ); + } + for( const auto& component : loaded.relations( id ) ) + { + geode::OpenGeodeModelException::test( + related.contains( component.id ), + "[Test] Wrong relation for ", id.string() ); + } + } + } + +} // namespace + +void test() +{ + geode::OpenGeodeModelLibrary::initialize(); + test_inconsistent_relationships_map(); +} + +OPENGEODE_TEST( "model-corrupted-files" ) From 44942f177e400d41ab328181d49f5a27840c7026 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 09:38:30 +0200 Subject: [PATCH 02/14] fix(VertexIdentifier): repair unique vertices of loaded models Loading threw when a component mesh had no (or several) "unique vertices" attribute, and unique vertices kept component vertices of components missing from the model or with an out of range vertex index. A missing attribute is now recreated and restored from the unique vertices, with a warning. After loading a BRep or a Section, component vertices of unknown components, with an invalid vertex or linked to another unique vertex are removed, with a warning. Co-Authored-By: Claude Opus 5.5 --- .../builder/vertex_identifier_builder.hpp | 7 + .../model/mixin/core/vertex_identifier.hpp | 7 + .../io/geode/geode_brep_input.hpp | 1 + .../io/geode/geode_section_input.hpp | 1 + .../builder/vertex_identifier_builder.cpp | 6 + .../model/mixin/core/vertex_identifier.cpp | 123 ++++++++++++++++-- tests/model/test-model-corrupted-files.cpp | 112 ++++++++++++++++ 7 files changed, 245 insertions(+), 12 deletions(-) diff --git a/include/geode/model/mixin/builder/vertex_identifier_builder.hpp b/include/geode/model/mixin/builder/vertex_identifier_builder.hpp index 9c3f6ff89..d88540af8 100644 --- a/include/geode/model/mixin/builder/vertex_identifier_builder.hpp +++ b/include/geode/model/mixin/builder/vertex_identifier_builder.hpp @@ -117,6 +117,13 @@ namespace geode */ std::vector< index_t > delete_isolated_vertices(); + /*! + * Remove component vertices referencing an unregistered component or + * an invalid vertex, and restore missing links from component vertices + * to their unique vertex. Used after loading a model. + */ + void repair_component_vertices(); + private: VertexIdentifier& vertex_identifier_; }; diff --git a/include/geode/model/mixin/core/vertex_identifier.hpp b/include/geode/model/mixin/core/vertex_identifier.hpp index 4a34a330f..226b9d5f5 100644 --- a/include/geode/model/mixin/core/vertex_identifier.hpp +++ b/include/geode/model/mixin/core/vertex_identifier.hpp @@ -214,6 +214,13 @@ namespace geode */ std::vector< index_t > delete_isolated_vertices( BuilderKey /*key*/ ); + /*! + * Remove component vertices referencing an unregistered component or + * an invalid vertex, and restore missing links from component vertices + * to their unique vertex. Used after loading a model. + */ + void repair_component_vertices( BuilderKey /*key*/ ); + protected: VertexIdentifier( VertexIdentifier&& other ) noexcept; VertexIdentifier& operator=( VertexIdentifier&& other ) noexcept; diff --git a/include/geode/model/representation/io/geode/geode_brep_input.hpp b/include/geode/model/representation/io/geode/geode_brep_input.hpp index 322a82f13..baf7cde80 100644 --- a/include/geode/model/representation/io/geode/geode_brep_input.hpp +++ b/include/geode/model/representation/io/geode/geode_brep_input.hpp @@ -115,6 +115,7 @@ namespace geode Logger::set_level( level ); detail::register_all_components( brep ); detail::filter_unsupported_components( brep ); + builder.repair_component_vertices(); } } // namespace detail } // namespace geode diff --git a/include/geode/model/representation/io/geode/geode_section_input.hpp b/include/geode/model/representation/io/geode/geode_section_input.hpp index 394ad7e0a..f87a35220 100644 --- a/include/geode/model/representation/io/geode/geode_section_input.hpp +++ b/include/geode/model/representation/io/geode/geode_section_input.hpp @@ -106,6 +106,7 @@ namespace geode Logger::set_level( level ); detail::register_all_components( section ); detail::filter_unsupported_components( section ); + builder.repair_component_vertices(); } } // namespace detail } // namespace geode diff --git a/src/geode/model/mixin/builder/vertex_identifier_builder.cpp b/src/geode/model/mixin/builder/vertex_identifier_builder.cpp index 5596a4cc3..586430755 100644 --- a/src/geode/model/mixin/builder/vertex_identifier_builder.cpp +++ b/src/geode/model/mixin/builder/vertex_identifier_builder.cpp @@ -77,4 +77,10 @@ namespace geode return vertex_identifier_.delete_isolated_vertices( VertexIdentifier::BuilderKey{} ); } + + void VertexIdentifierBuilder::repair_component_vertices() + { + vertex_identifier_.repair_component_vertices( + VertexIdentifier::BuilderKey{} ); + } } // namespace geode diff --git a/src/geode/model/mixin/core/vertex_identifier.cpp b/src/geode/model/mixin/core/vertex_identifier.cpp index 1aec09bf0..43f02d11b 100644 --- a/src/geode/model/mixin/core/vertex_identifier.cpp +++ b/src/geode/model/mixin/core/vertex_identifier.cpp @@ -394,6 +394,83 @@ namespace geode return old2new; } + void repair_component_vertices() + { + index_t nb_unknown_components{ 0 }; + index_t nb_invalid_vertices{ 0 }; + index_t nb_inconsistent_vertices{ 0 }; + index_t nb_restored_vertices{ 0 }; + for( const auto unique_vertex : Range{ nb_unique_vertices() } ) + { + const auto& vertices = + component_vertices_->value( unique_vertex ); + std::vector< bool > to_keep( vertices.size(), true ); + bool update{ false }; + for( const auto v : Indices{ vertices } ) + { + const auto& vertex = vertices[v]; + const auto it = + vertex2unique_vertex_.find( vertex.component_id.id ); + if( it == vertex2unique_vertex_.end() ) + { + to_keep[v] = false; + update = true; + nb_unknown_components++; + continue; + } + auto& attribute = *it->second; + if( vertex.vertex >= attribute.size() ) + { + to_keep[v] = false; + update = true; + nb_invalid_vertices++; + continue; + } + const auto value = attribute.value( vertex.vertex ); + if( value == NO_ID ) + { + attribute.set_value( vertex.vertex, unique_vertex ); + nb_restored_vertices++; + } + else if( value != unique_vertex ) + { + to_keep[v] = false; + update = true; + nb_inconsistent_vertices++; + } + } + if( update ) + { + component_vertices_->set_value( unique_vertex, + extract_vector_elements( to_keep, vertices ) ); + } + } + if( nb_unknown_components > 0 ) + { + Logger::warning( "[VertexIdentifier::load] Removed ", + nb_unknown_components, + " component vertices of unknown components" ); + } + if( nb_invalid_vertices > 0 ) + { + Logger::warning( "[VertexIdentifier::load] Removed ", + nb_invalid_vertices, + " component vertices with invalid vertex index" ); + } + if( nb_inconsistent_vertices > 0 ) + { + Logger::warning( "[VertexIdentifier::load] Removed ", + nb_inconsistent_vertices, + " component vertices linked to another unique vertex" ); + } + if( nb_restored_vertices > 0 ) + { + Logger::warning( "[VertexIdentifier::load] Restored ", + nb_restored_vertices, + " component vertex links to their unique vertex" ); + } + } + void save( std::string_view directory ) const { const auto filename = absl::StrCat( directory, "/vertices" ); @@ -526,16 +603,31 @@ namespace geode const auto unique_vertices_ids = vertex_attribute_manager.attribute_ids_matching_name( UNIQUE_VERTICES_NAME ); - OpenGeodeModelException::check_exception( - unique_vertices_ids.has_value(), nullptr, - OpenGeodeException::TYPE::data, - "[VertexIdentifier::load_component] Unique vertices " - "attribute not found." ); - OpenGeodeModelException::check_exception( - unique_vertices_ids.value().size() == 1, nullptr, - OpenGeodeException::TYPE::data, - "[VertexIdentifier::load_component] Unique vertices " - "attribute is not unique." ); + if( !unique_vertices_ids.has_value() ) + { + Logger::warning( + "[VertexIdentifier::load_component] Unique vertices " + "attribute not found, it will be restored from the " + "unique vertices" ); + AttributeProperties attribute_properties; + attribute_properties.assignable = false; + attribute_properties.interpolable = false; + attribute_properties.transferable = false; + AttributeValues< index_t > unique_vertex_attribute_values; + unique_vertex_attribute_values.default_value = NO_ID; + unique_vertex_attribute_values.no_value = NO_ID; + vertex_attribute_manager + .template create_attribute< VariableAttribute, index_t >( + UNIQUE_VERTICES_NAME, unique_vertex_id_, + unique_vertex_attribute_values, attribute_properties ); + return; + } + if( unique_vertices_ids.value().size() != 1 ) + { + Logger::warning( + "[VertexIdentifier::load_component] Unique vertices " + "attribute is not unique, using the first one" ); + } const auto old_attribute = vertex_attribute_manager .template find_attribute< VariableAttribute, index_t >( @@ -554,8 +646,10 @@ namespace geode new_attribute->set_value( vertex, old_attribute->value( vertex ) ); } - vertex_attribute_manager.delete_attribute( - unique_vertices_ids.value().front() ); + for( const auto& old_id : unique_vertices_ids.value() ) + { + vertex_attribute_manager.delete_attribute( old_id ); + } } private: @@ -696,6 +790,11 @@ namespace geode return impl_->delete_isolated_vertices(); } + void VertexIdentifier::repair_component_vertices( BuilderKey /*key*/ ) + { + impl_->repair_component_vertices(); + } + template void opengeode_model_api VertexIdentifier::load_mesh_component( const Corner2D&, BuilderKey /*key*/ ); template void opengeode_model_api VertexIdentifier::load_mesh_component( diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index 7ea6f107d..ddc57a4dd 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -205,12 +205,124 @@ namespace } } + bool has_mesh_component( const geode::BRep& brep, const geode::uuid& id ) + { + return brep.has_corner( id ) || brep.has_line( id ) + || brep.has_surface( id ) || brep.has_block( id ); + } + + std::string mesh_file_name( + const ArchiveFiles& files, std::string_view prefix ) + { + for( const auto& file : files ) + { + if( absl::StartsWith( file.first, prefix ) + && file.first.size() > prefix.size() + 36 ) + { + return file.first; + } + } + throw geode::OpenGeodeModelException{ nullptr, + geode::OpenGeodeException::TYPE::internal, "[Test] No ", prefix, + " mesh file" }; + } + + void check_same_unique_vertices( + const geode::BRep& brep, const geode::BRep& loaded ) + { + geode::OpenGeodeModelException::test( + loaded.nb_unique_vertices() == brep.nb_unique_vertices(), + "[Test] Wrong number of unique vertices" ); + for( const auto unique_vertex : + geode::Range{ brep.nb_unique_vertices() } ) + { + const auto& vertices = + brep.component_mesh_vertices( unique_vertex ); + geode::OpenGeodeModelException::test( + loaded.component_mesh_vertices( unique_vertex ).size() + == vertices.size(), + "[Test] Wrong number of component vertices for unique vertex ", + unique_vertex ); + for( const auto& vertex : vertices ) + { + geode::OpenGeodeModelException::test( + loaded.unique_vertex( vertex ) == unique_vertex, + "[Test] Wrong unique vertex for component vertex" ); + } + } + } + + /* + * Mesh components have a "unique vertices" attribute linking their + * vertices to the model unique vertices. When this attribute is missing, + * it is restored from the unique vertices. + */ + void test_missing_unique_vertices_attribute() + { + const auto brep = saved_brep( "unique_vertices.og_brep" ); + auto files = extract_archive( "unique_vertices.og_brep" ); + replace_unique( files.at( "vertices" ), + serialized_uuid( brep.unique_vertex_attribute_id() ), + serialized_uuid( geode::uuid{} ), "Unique vertex attribute id" ); + for( const auto prefix : { "Corner", "Line", "Surface", "Block" } ) + { + replace_unique( files.at( mesh_file_name( files, prefix ) ), + "unique vertices", "unique_vertices", + "Unique vertices attribute name" ); + } + write_archive( files, "corrupted_unique_vertices.og_brep" ); + + const auto loaded = + geode::load_brep( "corrupted_unique_vertices.og_brep" ); + check_same_unique_vertices( brep, loaded ); + } + + /* + * Unique vertices referencing a component that does not exist in the + * model are removed. + */ + void test_unknown_component_vertices() + { + saved_brep( "with_corner.og_brep" ); + auto without_corner = geode::load_brep( "with_corner.og_brep" ); + const auto& corner = *without_corner.corners().begin(); + const auto corner_id = corner.id(); + geode::BRepBuilder{ without_corner }.remove_corner( corner ); + geode::save_brep( without_corner, "without_corner.og_brep" ); + auto files = extract_archive( "without_corner.og_brep" ); + files.at( "vertices" ) = + extract_archive( "with_corner.og_brep" ).at( "vertices" ); + write_archive( files, "unknown_corner_vertices.og_brep" ); + + const auto loaded = + geode::load_brep( "unknown_corner_vertices.og_brep" ); + geode::OpenGeodeModelException::test( !loaded.has_corner( corner_id ), + "[Test] Removed corner should not be loaded" ); + for( const auto unique_vertex : + geode::Range{ loaded.nb_unique_vertices() } ) + { + for( const auto& vertex : + loaded.component_mesh_vertices( unique_vertex ) ) + { + geode::OpenGeodeModelException::test( + has_mesh_component( loaded, vertex.component_id.id ), + "[Test] Unique vertex ", unique_vertex, + " references an unknown component" ); + geode::OpenGeodeModelException::test( + loaded.unique_vertex( vertex ) == unique_vertex, + "[Test] Wrong unique vertex for component vertex" ); + } + } + } + } // namespace void test() { geode::OpenGeodeModelLibrary::initialize(); test_inconsistent_relationships_map(); + test_missing_unique_vertices_attribute(); + test_unknown_component_vertices(); } OPENGEODE_TEST( "model-corrupted-files" ) From 35f1e56cafbae36989a2ead5253c7ca1d2ea0fba Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 09:41:43 +0200 Subject: [PATCH 03/14] fix(ComponentsStorage): repair loaded components A missing components file was silently ignored although it is always saved: it is now reported, except for collections which may be missing from files written before they existed. After deserialization, null components are removed and components stored under another uuid than their own are stored back under their id, with a warning. Co-Authored-By: Claude Opus 5.5 --- .../mixin/core/detail/components_storage.hpp | 62 ++++++++- .../model/mixin/core/block_collections.cpp | 2 +- .../model/mixin/core/corner_collections.cpp | 2 +- .../model/mixin/core/line_collections.cpp | 2 +- .../model/mixin/core/surface_collections.cpp | 2 +- tests/model/test-model-corrupted-files.cpp | 119 ++++++++++++++++++ 6 files changed, 184 insertions(+), 5 deletions(-) diff --git a/include/geode/model/mixin/core/detail/components_storage.hpp b/include/geode/model/mixin/core/detail/components_storage.hpp index 72a488bef..ba267a7ca 100644 --- a/include/geode/model/mixin/core/detail/components_storage.hpp +++ b/include/geode/model/mixin/core/detail/components_storage.hpp @@ -35,6 +35,7 @@ #include #include +#include #include @@ -120,10 +121,22 @@ namespace geode::detail components_.erase( components_.find( component_id ) ); } - void load_components( std::string_view filename ) + /*! + * Load the components stored in the given file. + * @param[in] required If false, a missing file is expected (e.g. for + * files written before this component type existed) and silently + * ignored. Otherwise a warning is emitted. + */ + void load_components( std::string_view filename, bool required = true ) { if( !std::filesystem::exists( to_string( filename ) ) ) { + if( required ) + { + Logger::warning( "[ComponentsStorage::load_components] " + "Missing file: ", + filename, ", no component loaded from it" ); + } return; } std::ifstream file{ to_string( filename ), std::ifstream::binary }; @@ -141,6 +154,7 @@ namespace geode::detail "[ComponentsStorage::load_components] Error while reading " "file: ", filename ); + repair_loaded_components( filename ); } [[nodiscard]] absl::flat_hash_map< std::string, std::string > @@ -181,6 +195,52 @@ namespace geode::detail } } } ); } + void repair_loaded_components( std::string_view filename ) + { + bool has_wrong_component{ false }; + for( const auto& [component_id, component] : components_ ) + { + if( !component || component->id() != component_id ) + { + has_wrong_component = true; + break; + } + } + if( !has_wrong_component ) + { + return; + } + ComponentsStore repaired; + repaired.reserve( components_.size() ); + for( auto& [component_id, component] : components_ ) + { + if( !component ) + { + Logger::warning( "[ComponentsStorage::load_components] " + "Removing null component ", + component_id.string(), " from file: ", filename ); + continue; + } + auto id = component->id(); + if( id != component_id ) + { + Logger::warning( "[ComponentsStorage::load_components] " + "Component stored with id ", + component_id.string(), " has id ", id.string(), + " in file: ", filename ); + } + const auto [_, inserted] = + repaired.emplace( id, std::move( component ) ); + if( !inserted ) + { + Logger::warning( "[ComponentsStorage::load_components] " + "Removing duplicated component ", + id.string(), " from file: ", filename ); + } + } + components_ = std::move( repaired ); + } + void check_matching_component_id( const uuid& component_id ) const { OpenGeodeModelException::check_exception( diff --git a/src/geode/model/mixin/core/block_collections.cpp b/src/geode/model/mixin/core/block_collections.cpp index 32e4f836c..f086771b5 100644 --- a/src/geode/model/mixin/core/block_collections.cpp +++ b/src/geode/model/mixin/core/block_collections.cpp @@ -100,7 +100,7 @@ namespace geode std::string_view directory, BlockCollectionsBuilderKey /*key*/ ) { impl_->load_components( - absl::StrCat( directory, "/block_collections" ) ); + absl::StrCat( directory, "/block_collections" ), false ); } template < index_t dimension > diff --git a/src/geode/model/mixin/core/corner_collections.cpp b/src/geode/model/mixin/core/corner_collections.cpp index 1c44de15f..44d3d6839 100644 --- a/src/geode/model/mixin/core/corner_collections.cpp +++ b/src/geode/model/mixin/core/corner_collections.cpp @@ -101,7 +101,7 @@ namespace geode std::string_view directory, CornerCollectionsBuilderKey /*key*/ ) { impl_->load_components( - absl::StrCat( directory, "/corner_collections" ) ); + absl::StrCat( directory, "/corner_collections" ), false ); } template < index_t dimension > diff --git a/src/geode/model/mixin/core/line_collections.cpp b/src/geode/model/mixin/core/line_collections.cpp index 86e340d95..c2b8ed2c4 100644 --- a/src/geode/model/mixin/core/line_collections.cpp +++ b/src/geode/model/mixin/core/line_collections.cpp @@ -100,7 +100,7 @@ namespace geode std::string_view directory, LineCollectionsBuilderKey /*key*/ ) { impl_->load_components( - absl::StrCat( directory, "/line_collections" ) ); + absl::StrCat( directory, "/line_collections" ), false ); } template < index_t dimension > diff --git a/src/geode/model/mixin/core/surface_collections.cpp b/src/geode/model/mixin/core/surface_collections.cpp index a9e9a3fe5..1a2e18ca1 100644 --- a/src/geode/model/mixin/core/surface_collections.cpp +++ b/src/geode/model/mixin/core/surface_collections.cpp @@ -102,7 +102,7 @@ namespace geode std::string_view directory, SurfaceCollectionsBuilderKey /*key*/ ) { impl_->load_components( - absl::StrCat( directory, "/surface_collections" ) ); + absl::StrCat( directory, "/surface_collections" ), false ); } template < index_t dimension > diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index ddc57a4dd..97efd0f3d 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -35,6 +35,8 @@ #include #include +#include +#include #include #include @@ -53,6 +55,67 @@ namespace { + /* Records warnings to check that repairs are reported */ + class WarningRecorder : public geode::LoggerClient + { + public: + static void start() + { + static const auto registered = [] { + geode::LoggerManager::register_client( + std::make_unique< WarningRecorder >() ); + return true; + }(); + geode_unused( registered ); + std::lock_guard< std::mutex > lock{ mutex() }; + warnings().clear(); + } + + static bool has_warning( std::string_view text ) + { + std::lock_guard< std::mutex > lock{ mutex() }; + for( const auto& warning : warnings() ) + { + if( absl::StrContains( warning, text ) ) + { + return true; + } + } + return false; + } + + void trace( const std::string& /*message*/ ) override {} + void debug( const std::string& /*message*/ ) override {} + void info( const std::string& /*message*/ ) override {} + void warning( const std::string& message ) override + { + std::lock_guard< std::mutex > lock{ mutex() }; + warnings().push_back( message ); + } + void error( const std::string& /*message*/ ) override {} + void critical( const std::string& /*message*/ ) override {} + + private: + static std::mutex& mutex() + { + static std::mutex mutex; + return mutex; + } + + static std::vector< std::string >& warnings() + { + static std::vector< std::string > warnings; + return warnings; + } + }; + + void check_warning( std::string_view text ) + { + geode::OpenGeodeModelException::test( + WarningRecorder::has_warning( text ), + "[Test] Missing warning: ", text ); + } + /* Files of a model archive: file name -> file content */ using ArchiveFiles = absl::btree_map< std::string, std::string >; @@ -315,6 +378,60 @@ namespace } } + /* + * The components file of each component type is always saved. A missing + * file is reported. + */ + void test_missing_components_file() + { + saved_brep( "missing_components.og_brep" ); + auto files = extract_archive( "missing_components.og_brep" ); + files.erase( "lines" ); + write_archive( files, "missing_components_file.og_brep" ); + + WarningRecorder::start(); + const auto loaded = + geode::load_brep( "missing_components_file.og_brep" ); + check_warning( "Missing file" ); + geode::OpenGeodeModelException::test( + loaded.nb_lines() == 0, "[Test] No line should be loaded" ); + } + + /* + * Components are stored in a map: uuid -> component. A component stored + * under another uuid than its own is stored back under its own uuid. + */ + void test_wrong_component_key() + { + const auto brep = saved_brep( "component_key.og_brep" ); + const auto& corner_id = ( *brep.corners().begin() ).id(); + auto files = extract_archive( "component_key.og_brep" ); + auto& content = files.at( "corners" ); + const auto key = serialized_uuid( corner_id ); + const auto position = content.find( key ); + geode::OpenGeodeModelException::test( + position != std::string::npos, "[Test] Corner key not found" ); + content.replace( + position, key.size(), serialized_uuid( geode::uuid{} ) ); + geode::OpenGeodeModelException::test( + content.find( key ) != std::string::npos, + "[Test] Corner id not found" ); + write_archive( files, "wrong_component_key.og_brep" ); + + WarningRecorder::start(); + const auto loaded = geode::load_brep( "wrong_component_key.og_brep" ); + check_warning( "has id" ); + geode::OpenGeodeModelException::test( + loaded.nb_corners() == brep.nb_corners(), + "[Test] Wrong number of corners" ); + geode::OpenGeodeModelException::test( loaded.has_corner( corner_id ), + "[Test] Corner should be stored under its id" ); + geode::OpenGeodeModelException::test( + loaded.corner( corner_id ).mesh().nb_vertices() + == brep.corner( corner_id ).mesh().nb_vertices(), + "[Test] Corner mesh should be loaded" ); + } + } // namespace void test() @@ -323,6 +440,8 @@ void test() test_inconsistent_relationships_map(); test_missing_unique_vertices_attribute(); test_unknown_component_vertices(); + test_missing_components_file(); + test_wrong_component_key(); } OPENGEODE_TEST( "model-corrupted-files" ) From 2a02b9050a238621d8f8e3f01d7df0de96945c95 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 09:44:45 +0200 Subject: [PATCH 04/14] fix(ModelIO): remove relations of components missing from loaded models Only relations of unsupported component types were removed after loading a BRep or a Section. Relations of a component of a supported type that does not exist in the model are now removed too, with a warning. Co-Authored-By: Claude Opus 5.5 --- .../representation/builder/detail/filter.hpp | 25 +++++++++++ tests/model/test-model-corrupted-files.cpp | 44 +++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/include/geode/model/representation/builder/detail/filter.hpp b/include/geode/model/representation/builder/detail/filter.hpp index 46c0b021e..e1f702737 100644 --- a/include/geode/model/representation/builder/detail/filter.hpp +++ b/include/geode/model/representation/builder/detail/filter.hpp @@ -23,7 +23,9 @@ #pragma once +#include #include +#include #include #include @@ -64,6 +66,19 @@ namespace geode } }; + template < typename > + struct ComponentsExistenceChecker; + + template < typename... Components > + struct ComponentsExistenceChecker< std::tuple< Components... > > + { + template < typename Model > + bool apply( const Model& model, const uuid& component_id ) const + { + return ( + model.Components::has_component( component_id ) || ... ); + } + }; } // namespace internal namespace detail @@ -74,6 +89,9 @@ namespace geode typename Model::Builder builder{ model }; const internal::ComponentsTypesChecker< typename Model::Components > checker; + const internal::ComponentsExistenceChecker< + typename Model::Components > + existence_checker; std::vector< uuid > components_to_remove; for( const auto vertex : Range{ model.nb_components_with_relations() } ) @@ -84,6 +102,13 @@ namespace geode { components_to_remove.push_back( component_id.id ); } + else if( !existence_checker.apply( model, component_id.id ) ) + { + Logger::warning( "[filter_unsupported_components] " + "Removing relations of missing ", + component_id.string() ); + components_to_remove.push_back( component_id.id ); + } } for( const auto& component : components_to_remove ) { diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index 97efd0f3d..2f5992d5a 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -432,6 +432,49 @@ namespace "[Test] Corner mesh should be loaded" ); } + /* + * Relations referencing a component that does not exist in the model are + * removed. + */ + void test_unknown_component_relations() + { + saved_brep( "relations_with_corner.og_brep" ); + auto without_corner = + geode::load_brep( "relations_with_corner.og_brep" ); + const auto& corner = *without_corner.corners().begin(); + const auto corner_id = corner.id(); + geode::BRepBuilder{ without_corner }.remove_corner( corner ); + geode::save_brep( without_corner, "relations_without_corner.og_brep" ); + auto files = extract_archive( "relations_without_corner.og_brep" ); + files.at( "relationships" ) = + extract_archive( "relations_with_corner.og_brep" ) + .at( "relationships" ); + write_archive( files, "unknown_corner_relations.og_brep" ); + + WarningRecorder::start(); + const auto loaded = + geode::load_brep( "unknown_corner_relations.og_brep" ); + check_warning( "Removing relations of missing" ); + geode::OpenGeodeModelException::test( + loaded.nb_relations( corner_id ) == 0, + "[Test] Removed corner should not have relations" ); + for( const auto vertex : + geode::Range{ loaded.nb_components_with_relations() } ) + { + const auto& component = loaded.component_with_relation( vertex ); + geode::OpenGeodeModelException::test( component.id != corner_id, + "[Test] Removed corner should not be in relationships" ); + } + for( const auto& line : loaded.lines() ) + { + for( const auto& boundary : loaded.boundaries( line ) ) + { + geode::OpenGeodeModelException::test( + boundary.id() != corner_id, + "[Test] Removed corner should not be a boundary" ); + } + } + } } // namespace void test() @@ -442,6 +485,7 @@ void test() test_unknown_component_vertices(); test_missing_components_file(); test_wrong_component_key(); + test_unknown_component_relations(); } OPENGEODE_TEST( "model-corrupted-files" ) From ddf51723a5c36a0aa63265805a9799cf8f7b69dd Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 09:52:25 +0200 Subject: [PATCH 05/14] fix(ModelIO): load components with a missing mesh file or unknown mesh type A missing mesh file made the load fail with a bare std::out_of_range, and a mesh implementation not registered in the MeshFactory made it throw. A component whose mesh file is missing now gets an empty mesh, and an unknown implementation is replaced by the default implementation of the mesh type matching the file extension, with a warning. Co-Authored-By: Claude Opus 5.5 --- .../mixin/core/detail/components_storage.hpp | 40 ++++++++ src/geode/model/mixin/core/blocks.cpp | 50 ++++++++-- src/geode/model/mixin/core/corners.cpp | 15 ++- src/geode/model/mixin/core/lines.cpp | 16 +++- src/geode/model/mixin/core/surfaces.cpp | 35 ++++++- tests/model/test-model-corrupted-files.cpp | 93 +++++++++++++++++++ 6 files changed, 231 insertions(+), 18 deletions(-) diff --git a/include/geode/model/mixin/core/detail/components_storage.hpp b/include/geode/model/mixin/core/detail/components_storage.hpp index ba267a7ca..f79de5c99 100644 --- a/include/geode/model/mixin/core/detail/components_storage.hpp +++ b/include/geode/model/mixin/core/detail/components_storage.hpp @@ -40,6 +40,7 @@ #include #include +#include #include #include @@ -47,6 +48,45 @@ namespace geode::detail { + /*! + * Return the given mesh implementation if it is registered, otherwise + * the default implementation of the given mesh type (with a warning). + */ + [[nodiscard]] inline MeshImpl loadable_mesh_impl( const MeshImpl& impl, + const MeshType& fallback_type, + std::string_view file ) + { + if( MeshFactory::has_creator( impl ) ) + { + return impl; + } + const auto& fallback = MeshFactory::default_impl( fallback_type ); + Logger::warning( "[ComponentsStorage] Unknown mesh implementation ", + impl.get(), " for file ", file, ", loading it as ", + fallback.get() ); + return fallback; + } + + /*! + * Create an empty mesh for a component whose mesh file is missing, using + * the component mesh implementation if it is registered, otherwise the + * default implementation of the given mesh type. + */ + template < typename Component > + [[nodiscard]] std::unique_ptr< typename Component::Mesh > + empty_mesh_of_missing_file( + const Component& component, const MeshType& fallback_type ) + { + Logger::warning( "[ComponentsStorage] Missing mesh file of ", + component.component_id().string(), ", an empty mesh is used" ); + if( MeshFactory::has_creator( component.mesh_type() ) ) + { + return Component::Mesh::create( component.mesh_type() ); + } + return Component::Mesh::create( + MeshFactory::default_impl( fallback_type ) ); + } + template < typename Component > class ComponentsStorage { diff --git a/src/geode/model/mixin/core/blocks.cpp b/src/geode/model/mixin/core/blocks.cpp index 1c0d1472f..ef614cdd8 100644 --- a/src/geode/model/mixin/core/blocks.cpp +++ b/src/geode/model/mixin/core/blocks.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -44,6 +45,27 @@ #include #include +namespace +{ + template < geode::index_t dimension > + geode::MeshType solid_type_from_file( std::string_view file ) + { + const auto extension = + geode::to_string( geode::extension_from_filename( file ) ); + if( geode::TetrahedralSolidInputFactory< dimension >::has_creator( + extension ) ) + { + return geode::TetrahedralSolid< dimension >::type_name_static(); + } + if( geode::HybridSolidInputFactory< dimension >::has_creator( + extension ) ) + { + return geode::HybridSolid< dimension >::type_name_static(); + } + return geode::PolyhedralSolid< dimension >::type_name_static(); + } +} // namespace + namespace geode { template < index_t dimension > @@ -154,25 +176,35 @@ namespace geode for( auto& block : modifiable_blocks( builder_key ) ) { tasks[count++] = async::spawn( [&block, &mapping] { - const auto file = mapping.at( block.id().string() ); - if( MeshFactory::type( block.mesh_type() ) - == TetrahedralSolid< dimension >::type_name_static() ) + const auto file = mapping.find( block.id().string() ); + if( file == mapping.end() ) + { + block.set_mesh( + detail::empty_mesh_of_missing_file( block, + PolyhedralSolid< dimension >::type_name_static() ), + typename Block< dimension >::BlocksKey{} ); + return; + } + const auto impl = detail::loadable_mesh_impl( block.mesh_type(), + solid_type_from_file< dimension >( file->second ), + file->second ); + const auto& type = MeshFactory::type( impl ); + if( type == TetrahedralSolid< dimension >::type_name_static() ) { block.set_mesh( load_tetrahedral_solid< dimension >( - block.mesh_type(), file ), + impl, file->second ), typename Block< dimension >::BlocksKey{} ); } - else if( MeshFactory::type( block.mesh_type() ) - == HybridSolid< dimension >::type_name_static() ) + else if( type == HybridSolid< dimension >::type_name_static() ) { - block.set_mesh( load_hybrid_solid< dimension >( - block.mesh_type(), file ), + block.set_mesh( + load_hybrid_solid< dimension >( impl, file->second ), typename Block< dimension >::BlocksKey{} ); } else { block.set_mesh( load_polyhedral_solid< dimension >( - block.mesh_type(), file ), + impl, file->second ), typename Block< dimension >::BlocksKey{} ); } } ); diff --git a/src/geode/model/mixin/core/corners.cpp b/src/geode/model/mixin/core/corners.cpp index 246f8603b..79ae0a9a4 100644 --- a/src/geode/model/mixin/core/corners.cpp +++ b/src/geode/model/mixin/core/corners.cpp @@ -124,9 +124,20 @@ namespace geode for( auto& corner : modifiable_corners( key ) ) { tasks[count++] = async::spawn( [&corner, &mapping] { - const auto file = mapping.at( corner.id().string() ); + const auto& type = PointSet< dimension >::type_name_static(); + const auto file = mapping.find( corner.id().string() ); + if( file == mapping.end() ) + { + corner.set_mesh( + detail::empty_mesh_of_missing_file( corner, type ), + typename Corner< dimension >::CornersKey{} ); + return; + } corner.set_mesh( - load_point_set< dimension >( corner.mesh_type(), file ), + load_point_set< dimension >( + detail::loadable_mesh_impl( + corner.mesh_type(), type, file->second ), + file->second ), typename Corner< dimension >::CornersKey{} ); } ); } diff --git a/src/geode/model/mixin/core/lines.cpp b/src/geode/model/mixin/core/lines.cpp index 6a7643b43..a5c782e57 100644 --- a/src/geode/model/mixin/core/lines.cpp +++ b/src/geode/model/mixin/core/lines.cpp @@ -122,9 +122,19 @@ namespace geode for( auto& line : modifiable_lines( key ) ) { tasks[count++] = async::spawn( [&line, &mapping] { - const auto file = mapping.at( line.id().string() ); - line.set_mesh( - load_edged_curve< dimension >( line.mesh_type(), file ), + const auto& type = EdgedCurve< dimension >::type_name_static(); + const auto file = mapping.find( line.id().string() ); + if( file == mapping.end() ) + { + line.set_mesh( + detail::empty_mesh_of_missing_file( line, type ), + typename Line< dimension >::LinesKey{} ); + return; + } + line.set_mesh( load_edged_curve< dimension >( + detail::loadable_mesh_impl( + line.mesh_type(), type, file->second ), + file->second ), typename Line< dimension >::LinesKey{} ); } ); } diff --git a/src/geode/model/mixin/core/surfaces.cpp b/src/geode/model/mixin/core/surfaces.cpp index 87f3f46f4..ad8ea0800 100644 --- a/src/geode/model/mixin/core/surfaces.cpp +++ b/src/geode/model/mixin/core/surfaces.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -41,6 +42,20 @@ #include #include +namespace +{ + template < geode::index_t dimension > + geode::MeshType surface_type_from_file( std::string_view file ) + { + if( geode::TriangulatedSurfaceInputFactory< dimension >::has_creator( + geode::to_string( geode::extension_from_filename( file ) ) ) ) + { + return geode::TriangulatedSurface< dimension >::type_name_static(); + } + return geode::PolygonalSurface< dimension >::type_name_static(); + } +} // namespace + namespace geode { template < index_t dimension > @@ -146,18 +161,30 @@ namespace geode for( auto& surface : modifiable_surfaces( key ) ) { tasks[count++] = async::spawn( [&surface, &mapping] { - const auto file = mapping.at( surface.id().string() ); - if( MeshFactory::type( surface.mesh_type() ) + const auto file = mapping.find( surface.id().string() ); + if( file == mapping.end() ) + { + surface.set_mesh( + detail::empty_mesh_of_missing_file( surface, + PolygonalSurface< dimension >::type_name_static() ), + typename Surface< dimension >::SurfacesKey{} ); + return; + } + const auto impl = + detail::loadable_mesh_impl( surface.mesh_type(), + surface_type_from_file< dimension >( file->second ), + file->second ); + if( MeshFactory::type( impl ) == TriangulatedSurface< dimension >::type_name_static() ) { surface.set_mesh( load_triangulated_surface< dimension >( - surface.mesh_type(), file ), + impl, file->second ), typename Surface< dimension >::SurfacesKey{} ); } else { surface.set_mesh( load_polygonal_surface< dimension >( - surface.mesh_type(), file ), + impl, file->second ), typename Surface< dimension >::SurfacesKey{} ); } } ); diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index 2f5992d5a..c357bf447 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include @@ -41,6 +42,7 @@ #include #include +#include #include #include @@ -475,6 +477,95 @@ namespace } } } + + void replace_all( std::string& content, + std::string_view from, + std::string_view to, + std::string_view what ) + { + geode::OpenGeodeModelException::test( + content.find( from ) != std::string::npos, "[Test] ", what, + " not found" ); + content = absl::StrReplaceAll( content, { { from, to } } ); + } + + /* + * A component whose mesh file is missing gets an empty mesh. + */ + void test_missing_mesh_file() + { + const auto brep = saved_brep( "missing_mesh.og_brep" ); + auto files = extract_archive( "missing_mesh.og_brep" ); + const auto surface_file = mesh_file_name( files, "Surface" ); + const auto line_file = mesh_file_name( files, "Line" ); + files.erase( surface_file ); + files.erase( line_file ); + write_archive( files, "missing_mesh_file.og_brep" ); + + WarningRecorder::start(); + const auto loaded = geode::load_brep( "missing_mesh_file.og_brep" ); + check_warning( "Missing mesh file" ); + geode::OpenGeodeModelException::test( + loaded.nb_surfaces() == brep.nb_surfaces(), + "[Test] Wrong number of surfaces" ); + geode::index_t nb_empty_surfaces{ 0 }; + for( const auto& surface : loaded.surfaces() ) + { + const auto& mesh = surface.mesh(); + geode::OpenGeodeModelException::test( + surface.mesh_type() == mesh.impl_name(), + "[Test] Wrong surface mesh type" ); + if( mesh.nb_vertices() == 0 ) + { + nb_empty_surfaces++; + continue; + } + geode::OpenGeodeModelException::test( + mesh.nb_polygons() + == brep.surface( surface.id() ).mesh().nb_polygons(), + "[Test] Wrong surface mesh" ); + } + geode::OpenGeodeModelException::test( + nb_empty_surfaces == 1, "[Test] One surface should be empty" ); + for( const auto unique_vertex : + geode::Range{ loaded.nb_unique_vertices() } ) + { + for( const auto& vertex : + loaded.component_mesh_vertices( unique_vertex ) ) + { + geode::OpenGeodeModelException::test( + loaded.unique_vertex( vertex ) == unique_vertex, + "[Test] Wrong unique vertex for component vertex" ); + } + } + } + + /* + * A component whose mesh implementation is not registered is loaded with + * the default implementation of the mesh type matching its file. + */ + void test_unknown_mesh_impl() + { + const auto brep = saved_brep( "unknown_impl.og_brep" ); + auto files = extract_archive( "unknown_impl.og_brep" ); + replace_all( files.at( "surfaces" ), "OpenGeodeTriangulatedSurface3D", + "UnknownImplTriangulatedSurface", "Surface mesh type" ); + write_archive( files, "unknown_mesh_impl.og_brep" ); + + WarningRecorder::start(); + const auto loaded = geode::load_brep( "unknown_mesh_impl.og_brep" ); + check_warning( "Unknown mesh implementation" ); + for( const auto& surface : loaded.surfaces() ) + { + geode::OpenGeodeModelException::test( + surface.mesh_type().get() == "OpenGeodeTriangulatedSurface3D", + "[Test] Wrong surface mesh type" ); + geode::OpenGeodeModelException::test( + surface.mesh().nb_polygons() + == brep.surface( surface.id() ).mesh().nb_polygons(), + "[Test] Wrong surface mesh" ); + } + } } // namespace void test() @@ -486,6 +577,8 @@ void test() test_missing_components_file(); test_wrong_component_key(); test_unknown_component_relations(); + test_missing_mesh_file(); + test_unknown_mesh_impl(); } OPENGEODE_TEST( "model-corrupted-files" ) From 547b4be6bc23d870294d232251afa711cda3161e Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 09:59:23 +0200 Subject: [PATCH 06/14] fix(ModelIO): save and load RegularGrid surface and block meshes Saving a model whose surface (2D) or block mesh is a RegularGrid threw "Cannot find the explicit SurfaceMesh/SolidMesh type", and loading would have read it as a polygonal surface or polyhedral solid. RegularGrid meshes are now saved and loaded with the RegularGrid IO. Co-Authored-By: Claude Opus 5.5 --- src/geode/model/mixin/core/blocks.cpp | 20 ++++++++ src/geode/model/mixin/core/surfaces.cpp | 55 +++++++++++++++++++- tests/model/CMakeLists.txt | 1 + tests/model/test-model-corrupted-files.cpp | 58 ++++++++++++++++++++++ 4 files changed, 132 insertions(+), 2 deletions(-) diff --git a/src/geode/model/mixin/core/blocks.cpp b/src/geode/model/mixin/core/blocks.cpp index ef614cdd8..b2511f44d 100644 --- a/src/geode/model/mixin/core/blocks.cpp +++ b/src/geode/model/mixin/core/blocks.cpp @@ -34,11 +34,14 @@ #include #include #include +#include #include #include #include #include #include +#include +#include #include #include @@ -62,6 +65,11 @@ namespace { return geode::HybridSolid< dimension >::type_name_static(); } + if( geode::RegularGridInputFactory< dimension >::has_creator( + extension ) ) + { + return geode::RegularGrid< dimension >::type_name_static(); + } return geode::PolyhedralSolid< dimension >::type_name_static(); } } // namespace @@ -150,6 +158,12 @@ namespace geode { save_polyhedral_solid( *poly, file ); } + else if( const auto* grid = + dynamic_cast< const RegularGrid< dimension >* >( + &mesh ) ) + { + save_regular_grid( *grid, file ); + } else { throw OpenGeodeModelException{ nullptr, @@ -201,6 +215,12 @@ namespace geode load_hybrid_solid< dimension >( impl, file->second ), typename Block< dimension >::BlocksKey{} ); } + else if( type == RegularGrid< dimension >::type_name_static() ) + { + block.set_mesh( + load_regular_grid< dimension >( impl, file->second ), + typename Block< dimension >::BlocksKey{} ); + } else { block.set_mesh( load_polyhedral_solid< dimension >( diff --git a/src/geode/model/mixin/core/surfaces.cpp b/src/geode/model/mixin/core/surfaces.cpp index ad8ea0800..dde656ae1 100644 --- a/src/geode/model/mixin/core/surfaces.cpp +++ b/src/geode/model/mixin/core/surfaces.cpp @@ -33,9 +33,12 @@ #include #include +#include #include #include #include +#include +#include #include #include @@ -52,8 +55,49 @@ namespace { return geode::TriangulatedSurface< dimension >::type_name_static(); } + if constexpr( dimension == 2 ) + { + if( geode::RegularGridInputFactory< 2 >::has_creator( + geode::to_string( + geode::extension_from_filename( file ) ) ) ) + { + return geode::RegularGrid< 2 >::type_name_static(); + } + } return geode::PolygonalSurface< dimension >::type_name_static(); } + + template < geode::index_t dimension > + bool save_grid_surface( + const geode::SurfaceMesh< dimension >& mesh, std::string_view file ) + { + if constexpr( dimension == 2 ) + { + if( const auto* grid = + dynamic_cast< const geode::RegularGrid< 2 >* >( &mesh ) ) + { + geode::save_regular_grid( *grid, file ); + return true; + } + } + return false; + } + + template < geode::index_t dimension > + std::unique_ptr< geode::SurfaceMesh< dimension > > load_grid_surface( + const geode::MeshType& type, + const geode::MeshImpl& impl, + std::string_view file ) + { + if constexpr( dimension == 2 ) + { + if( type == geode::RegularGrid< 2 >::type_name_static() ) + { + return geode::load_regular_grid< 2 >( impl, file ); + } + } + return nullptr; + } } // namespace namespace geode @@ -135,7 +179,7 @@ namespace geode { save_polygonal_surface( *polygonal, file ); } - else + else if( !save_grid_surface< dimension >( mesh, file ) ) { throw OpenGeodeModelException{ nullptr, OpenGeodeException::TYPE::data, @@ -174,13 +218,20 @@ namespace geode detail::loadable_mesh_impl( surface.mesh_type(), surface_type_from_file< dimension >( file->second ), file->second ); - if( MeshFactory::type( impl ) + const auto& type = MeshFactory::type( impl ); + if( type == TriangulatedSurface< dimension >::type_name_static() ) { surface.set_mesh( load_triangulated_surface< dimension >( impl, file->second ), typename Surface< dimension >::SurfacesKey{} ); } + else if( auto grid = load_grid_surface< dimension >( + type, impl, file->second ) ) + { + surface.set_mesh( std::move( grid ), + typename Surface< dimension >::SurfacesKey{} ); + } else { surface.set_mesh( load_polygonal_surface< dimension >( diff --git a/tests/model/CMakeLists.txt b/tests/model/CMakeLists.txt index 424d5f57a..ad26dedf6 100644 --- a/tests/model/CMakeLists.txt +++ b/tests/model/CMakeLists.txt @@ -105,6 +105,7 @@ add_geode_test( SOURCE "test-model-corrupted-files.cpp" DEPENDENCIES ${PROJECT_NAME}::basic + ${PROJECT_NAME}::geometry ${PROJECT_NAME}::mesh ${PROJECT_NAME}::model ) diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index c357bf447..727edd48d 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -41,7 +41,15 @@ #include #include +#include +#include + +#include +#include +#include #include +#include +#include #include #include @@ -49,9 +57,13 @@ #include #include #include +#include #include +#include #include #include +#include +#include #include @@ -566,6 +578,51 @@ namespace "[Test] Wrong surface mesh" ); } } + + /* + * Surfaces and blocks may have a RegularGrid mesh. + */ + void test_regular_grid_meshes() + { + geode::Section section; + geode::SectionBuilder section_builder{ section }; + const auto& surface_id = section_builder.add_surface(); + auto surface_grid = geode::RegularGrid2D::create(); + geode::RegularGridBuilder2D::create( *surface_grid ) + ->initialize_grid( geode::Point2D{ { 0., 0. } }, { 3, 4 }, 1 ); + section_builder.update_surface_mesh( + section.surface( surface_id ), std::move( surface_grid ) ); + geode::save_section( section, "grid.og_sctn" ); + const auto loaded_section = geode::load_section( "grid.og_sctn" ); + const auto& loaded_surface = loaded_section.surface( surface_id ); + geode::OpenGeodeModelException::test( + geode::MeshFactory::type( loaded_surface.mesh_type() ) + == geode::RegularGrid2D::type_name_static(), + "[Test] Surface mesh should be a RegularGrid" ); + geode::OpenGeodeModelException::test( + loaded_surface.mesh().nb_polygons() == 12, + "[Test] Wrong number of grid surface cells" ); + + geode::BRep brep; + geode::BRepBuilder brep_builder{ brep }; + const auto& block_id = brep_builder.add_block(); + auto block_grid = geode::RegularGrid3D::create(); + geode::RegularGridBuilder3D::create( *block_grid ) + ->initialize_grid( + geode::Point3D{ { 0., 0., 0. } }, { 2, 3, 4 }, 1 ); + brep_builder.update_block_mesh( + brep.block( block_id ), std::move( block_grid ) ); + geode::save_brep( brep, "grid.og_brep" ); + const auto loaded_brep = geode::load_brep( "grid.og_brep" ); + const auto& loaded_block = loaded_brep.block( block_id ); + geode::OpenGeodeModelException::test( + geode::MeshFactory::type( loaded_block.mesh_type() ) + == geode::RegularGrid3D::type_name_static(), + "[Test] Block mesh should be a RegularGrid" ); + geode::OpenGeodeModelException::test( + loaded_block.mesh().nb_polyhedra() == 24, + "[Test] Wrong number of grid block cells" ); + } } // namespace void test() @@ -579,6 +636,7 @@ void test() test_unknown_component_relations(); test_missing_mesh_file(); test_unknown_mesh_impl(); + test_regular_grid_meshes(); } OPENGEODE_TEST( "model-corrupted-files" ) From b2f35b56c0062b5163940748d6ca684228e834a4 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 10:04:49 +0200 Subject: [PATCH 07/14] fix(ModelIO): check output streams and use UTF-8 paths for model files Saving the components, relationships, unique vertices and physical properties files never checked the output stream: a file that could not be written was silently skipped. The stream is now checked after opening and after flushing, like Identifier::save. These files are also opened with UTF-8 paths (std::filesystem::u8path), consistently with the archive directory, and the mesh files mapping uses UTF-8 file names. Co-Authored-By: Claude Opus 5.5 --- .../mixin/core/detail/components_storage.hpp | 24 +++++--- .../model/mixin/core/physical_properties.cpp | 14 +++-- src/geode/model/mixin/core/relationships.cpp | 12 +++- .../model/mixin/core/vertex_identifier.cpp | 12 +++- tests/model/test-model-corrupted-files.cpp | 59 +++++++++++++++++++ 5 files changed, 103 insertions(+), 18 deletions(-) diff --git a/include/geode/model/mixin/core/detail/components_storage.hpp b/include/geode/model/mixin/core/detail/components_storage.hpp index f79de5c99..ab103ad8c 100644 --- a/include/geode/model/mixin/core/detail/components_storage.hpp +++ b/include/geode/model/mixin/core/detail/components_storage.hpp @@ -141,7 +141,13 @@ namespace geode::detail void save_components( std::string_view filename ) const { - std::ofstream file{ to_string( filename ), std::ofstream::binary }; + std::ofstream file{ std::filesystem::u8path( + to_string( filename ) ), + std::ofstream::binary }; + OpenGeodeModelException::check_exception( file.is_open(), nullptr, + OpenGeodeException::TYPE::internal, + "[ComponentsStorage::save_components] Cannot open file: ", + filename ); TContext context{}; BitseryExtensions::register_serialize_pcontext( std::get< 0 >( context ) ); @@ -149,7 +155,7 @@ namespace geode::detail archive.object( *this ); archive.adapter().flush(); OpenGeodeModelException::check_exception( - std::get< 1 >( context ).isValid(), nullptr, + file.good() && std::get< 1 >( context ).isValid(), nullptr, OpenGeodeException::TYPE::internal, "[ComponentsStorage::save_components] Error while writing " "file: ", @@ -169,7 +175,8 @@ namespace geode::detail */ void load_components( std::string_view filename, bool required = true ) { - if( !std::filesystem::exists( to_string( filename ) ) ) + const auto path = std::filesystem::u8path( to_string( filename ) ); + if( !std::filesystem::exists( path ) ) { if( required ) { @@ -179,7 +186,7 @@ namespace geode::detail } return; } - std::ifstream file{ to_string( filename ), std::ifstream::binary }; + std::ifstream file{ path, std::ifstream::binary }; TContext context{}; BitseryExtensions::register_deserialize_pcontext( std::get< 0 >( context ) ); @@ -201,15 +208,16 @@ namespace geode::detail file_mapping( std::string_view directory ) const { absl::flat_hash_map< std::string, std::string > mapping; - for( const auto& file : - std::filesystem::directory_iterator( to_string( directory ) ) ) + for( const auto& file : std::filesystem::directory_iterator( + std::filesystem::u8path( to_string( directory ) ) ) ) { auto path = file.path(); - auto filename = path.replace_extension( "" ).string(); + auto filename = path.replace_extension( "" ).u8string(); if( filename.size() > 36 ) { auto uuid = filename.substr( filename.size() - 36 ); - mapping.emplace( std::move( uuid ), file.path().string() ); + mapping.emplace( + std::move( uuid ), file.path().u8string() ); } } return mapping; diff --git a/src/geode/model/mixin/core/physical_properties.cpp b/src/geode/model/mixin/core/physical_properties.cpp index d8daa4e57..4a1b5b6b8 100644 --- a/src/geode/model/mixin/core/physical_properties.cpp +++ b/src/geode/model/mixin/core/physical_properties.cpp @@ -98,7 +98,11 @@ namespace geode { const auto filename = absl::StrCat( directory, "/physical_properties" ); - std::ofstream file{ filename, std::ofstream::binary }; + std::ofstream file{ std::filesystem::u8path( filename ), + std::ofstream::binary }; + OpenGeodeModelException::check_exception( file.is_open(), nullptr, + OpenGeodeException::TYPE::internal, + "[PhysicalProperties::save] Cannot open file: ", filename ); TContext context{}; BitseryExtensions::register_serialize_pcontext( std::get< 0 >( context ) ); @@ -106,7 +110,7 @@ namespace geode archive.object( *this ); archive.adapter().flush(); OpenGeodeModelException::check_exception( - std::get< 1 >( context ).isValid(), nullptr, + file.good() && std::get< 1 >( context ).isValid(), nullptr, OpenGeodeException::TYPE::internal, "[PhysicalProperties::save] Error while writing file: ", filename ); @@ -116,11 +120,13 @@ namespace geode { const auto filename = absl::StrCat( directory, "/physical_properties" ); - if( !std::filesystem::exists( filename ) ) + if( !std::filesystem::exists( + std::filesystem::u8path( filename ) ) ) { return; } - std::ifstream file{ filename, std::ifstream::binary }; + std::ifstream file{ std::filesystem::u8path( filename ), + std::ifstream::binary }; TContext context{}; BitseryExtensions::register_deserialize_pcontext( std::get< 0 >( context ) ); diff --git a/src/geode/model/mixin/core/relationships.cpp b/src/geode/model/mixin/core/relationships.cpp index 6b95aed6e..395acc5f4 100644 --- a/src/geode/model/mixin/core/relationships.cpp +++ b/src/geode/model/mixin/core/relationships.cpp @@ -23,6 +23,7 @@ #include +#include #include #include @@ -167,7 +168,11 @@ namespace geode void save( std::string_view directory ) const { const auto filename = absl::StrCat( directory, "/relationships" ); - std::ofstream file{ filename, std::ofstream::binary }; + std::ofstream file{ std::filesystem::u8path( filename ), + std::ofstream::binary }; + OpenGeodeModelException::check_exception( file.is_open(), nullptr, + OpenGeodeException::TYPE::internal, + "[Relationships::save] Cannot open file: ", filename ); TContext context{}; BitseryExtensions::register_serialize_pcontext( std::get< 0 >( context ) ); @@ -175,7 +180,7 @@ namespace geode archive.object( *this ); archive.adapter().flush(); OpenGeodeModelException::check_exception( - std::get< 1 >( context ).isValid(), nullptr, + file.good() && std::get< 1 >( context ).isValid(), nullptr, OpenGeodeException::TYPE::internal, "[Relationships::save] Error while writing file: ", filename ); } @@ -183,7 +188,8 @@ namespace geode void load( std::string_view directory ) { const auto filename = absl::StrCat( directory, "/relationships" ); - std::ifstream file{ filename, std::ifstream::binary }; + std::ifstream file{ std::filesystem::u8path( filename ), + std::ifstream::binary }; TContext context{}; BitseryExtensions::register_deserialize_pcontext( std::get< 0 >( context ) ); diff --git a/src/geode/model/mixin/core/vertex_identifier.cpp b/src/geode/model/mixin/core/vertex_identifier.cpp index 43f02d11b..40d0a6c57 100644 --- a/src/geode/model/mixin/core/vertex_identifier.cpp +++ b/src/geode/model/mixin/core/vertex_identifier.cpp @@ -23,6 +23,7 @@ #include +#include #include #include @@ -474,7 +475,11 @@ namespace geode void save( std::string_view directory ) const { const auto filename = absl::StrCat( directory, "/vertices" ); - std::ofstream file{ filename, std::ofstream::binary }; + std::ofstream file{ std::filesystem::u8path( filename ), + std::ofstream::binary }; + OpenGeodeModelException::check_exception( file.is_open(), nullptr, + OpenGeodeException::TYPE::internal, + "[VertexIdentifier::save] Cannot open file: ", filename ); TContext context{}; BitseryExtensions::register_serialize_pcontext( std::get< 0 >( context ) ); @@ -482,7 +487,7 @@ namespace geode archive.object( *this ); archive.adapter().flush(); OpenGeodeModelException::check_exception( - std::get< 1 >( context ).isValid(), nullptr, + file.good() && std::get< 1 >( context ).isValid(), nullptr, OpenGeodeException::TYPE::internal, "[VertexIdentifier::save] Error while writing file: ", filename ); @@ -491,7 +496,8 @@ namespace geode void load( std::string_view directory ) { const auto filename = absl::StrCat( directory, "/vertices" ); - std::ifstream file{ filename, std::ifstream::binary }; + std::ifstream file{ std::filesystem::u8path( filename ), + std::ifstream::binary }; TContext context{}; BitseryExtensions::register_deserialize_pcontext( std::get< 0 >( context ) ); diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index 727edd48d..53ef5bf35 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -623,6 +623,63 @@ namespace loaded_block.mesh().nb_polyhedra() == 24, "[Test] Wrong number of grid block cells" ); } + + template < typename Saver > + void check_save_failure( std::string_view what, const Saver& saver ) + { + try + { + saver(); + } + catch( const geode::OpenGeodeException& exception ) + { + geode::Logger::info( "Saving ", what, + " in a missing directory throws as expected: ", + exception.what() ); + return; + } + throw geode::OpenGeodeModelException{ nullptr, + geode::OpenGeodeException::TYPE::internal, "[Test] Saving ", what, + " in a missing directory should throw" }; + } + + /* + * Saving model files in a directory that cannot be written throws. + */ + void test_save_in_missing_directory() + { + const auto brep = geode::load_brep( + absl::StrCat( geode::DATA_PATH, "box_brep.og_brep" ) ); + const std::string directory{ "missing_directory/missing" }; + check_save_failure( "relationships", [&] { + brep.save_relationships( directory ); + } ); + check_save_failure( "unique vertices", [&] { + brep.save_unique_vertices( directory ); + } ); + check_save_failure( "physical properties", [&] { + brep.save_physical_properties( directory ); + } ); + check_save_failure( "model boundaries", [&] { + brep.save_model_boundaries( directory ); + } ); + } + + /* + * Model files are saved and loaded with UTF-8 paths. + */ + void test_utf8_path() + { + const auto brep = geode::load_brep( + absl::StrCat( geode::DATA_PATH, "box_brep.og_brep" ) ); + const std::string filename{ "mod\xc3\xa8le_\xc3\xa9t\xc3\xa9.og_brep" }; + geode::save_brep( brep, filename ); + const auto loaded = geode::load_brep( filename ); + geode::OpenGeodeModelException::test( + loaded.nb_surfaces() == brep.nb_surfaces(), + "[Test] Wrong number of surfaces with UTF-8 path" ); + check_same_unique_vertices( brep, loaded ); + } } // namespace void test() @@ -637,6 +694,8 @@ void test() test_missing_mesh_file(); test_unknown_mesh_impl(); test_regular_grid_meshes(); + test_save_in_missing_directory(); + test_utf8_path(); } OPENGEODE_TEST( "model-corrupted-files" ) From 9579a3fb03e39e683474fa7cf08244dfbd2e5407 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 10:09:56 +0200 Subject: [PATCH 08/14] fix(Component): bound legacy component name size when loading The first Component::Impl version read its name with no size limit (max_size()). It now uses MAX_SERIALIZED_NAME_SIZE like other names, so a corrupted size is rejected instead of allocating and reading gigabytes. Co-Authored-By: Claude Opus 5.5 --- src/geode/model/mixin/core/component.cpp | 3 +- tests/model/test-model-corrupted-files.cpp | 41 ++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/geode/model/mixin/core/component.cpp b/src/geode/model/mixin/core/component.cpp index 6e3b31cf3..886b95f49 100644 --- a/src/geode/model/mixin/core/component.cpp +++ b/src/geode/model/mixin/core/component.cpp @@ -28,6 +28,7 @@ #include #include +#include #include #include @@ -66,7 +67,7 @@ namespace geode Growable< Archive, Impl >{ { []( Archive& local_archive, Impl& impl ) { local_archive.text1b( impl.backward_compatible_name_, - impl.backward_compatible_name_.max_size() ); + MAX_SERIALIZED_NAME_SIZE ); local_archive.object( impl.backward_compatible_id_ ); }, []( Archive& local_archive, Impl& impl ) { diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index 53ef5bf35..40ca801ea 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -35,6 +35,7 @@ #include #include +#include #include #include #include @@ -680,6 +681,45 @@ namespace "[Test] Wrong number of surfaces with UTF-8 path" ); check_same_unique_vertices( brep, loaded ); } + + /* + * Names of old components are bounded by MAX_SERIALIZED_NAME_SIZE. + */ + void test_too_long_component_name() + { + auto files = extract_archive( + absl::StrCat( geode::DATA_PATH, "layers.og_brep" ) ); + auto& content = files.at( "corners" ); + const std::string name{ "\x07unknown" }; + const auto position = content.find( name ); + geode::OpenGeodeModelException::test( + position != std::string::npos, "[Test] Component name not found" ); + // bitsery 4-byte size encoding: two high bytes, then a little endian + // 16-bit word + constexpr auto TOO_LONG = geode::MAX_SERIALIZED_NAME_SIZE + 1; + std::string too_long_name{ static_cast< char >( + 0xC0u | ( TOO_LONG >> 24 ) ), + static_cast< char >( TOO_LONG >> 16 ), + static_cast< char >( TOO_LONG ), + static_cast< char >( TOO_LONG >> 8 ) }; + too_long_name.append( TOO_LONG, 'a' ); + content.replace( position, name.size(), too_long_name ); + write_archive( files, "too_long_component_name.og_brep" ); + try + { + geode_unused( + geode::load_brep( "too_long_component_name.og_brep" ) ); + } + catch( const geode::OpenGeodeException& exception ) + { + geode::Logger::info( "Too long component name throws as expected: ", + exception.what() ); + return; + } + throw geode::OpenGeodeModelException{ nullptr, + geode::OpenGeodeException::TYPE::internal, + "[Test] Too long component name should not be loaded" }; + } } // namespace void test() @@ -696,6 +736,7 @@ void test() test_regular_grid_meshes(); test_save_in_missing_directory(); test_utf8_path(); + test_too_long_component_name(); } OPENGEODE_TEST( "model-corrupted-files" ) From 9e565ceb86c3698c2cfe4089af68fcd8faca5510 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 10:19:03 +0200 Subject: [PATCH 09/14] fix(ModelIO): restore the Logger level when a model load or save fails The native BRep and Section input and output lower the Logger level to warning and restore it afterwards, but an exception skipped the restore and left the level stuck. A new detail::ScopedLoggerLevel restores it on scope exit. The level is still global: concurrent loads may race on it. Co-Authored-By: Claude Opus 5.5 --- include/geode/basic/logger.hpp | 32 +++++++++++++++++++ .../io/geode/geode_brep_input.hpp | 6 ++-- .../io/geode/geode_section_input.hpp | 6 ++-- .../io/geode/geode_brep_output.cpp | 4 +-- .../io/geode/geode_section_output.cpp | 4 +-- tests/model/test-model-corrupted-files.cpp | 30 +++++++++++++++++ 6 files changed, 70 insertions(+), 12 deletions(-) diff --git a/include/geode/basic/logger.hpp b/include/geode/basic/logger.hpp index 11dd77e5a..5b5da140a 100644 --- a/include/geode/basic/logger.hpp +++ b/include/geode/basic/logger.hpp @@ -142,6 +142,38 @@ namespace geode private: IMPLEMENTATION_MEMBER( impl_ ); }; + + namespace detail + { + /*! + * Set the Logger level for the lifetime of this object and restore + * the previous level on destruction, even if an exception is thrown. + * The Logger level is global: scopes running concurrently in several + * threads may restore each other's level. + */ + class ScopedLoggerLevel + { + public: + explicit ScopedLoggerLevel( Logger::LEVEL level ) + : previous_level_{ Logger::level() } + { + Logger::set_level( level ); + } + + ~ScopedLoggerLevel() + { + Logger::set_level( previous_level_ ); + } + + ScopedLoggerLevel( const ScopedLoggerLevel & ) = delete; + ScopedLoggerLevel &operator=( const ScopedLoggerLevel & ) = delete; + ScopedLoggerLevel( ScopedLoggerLevel && ) = delete; + ScopedLoggerLevel &operator=( ScopedLoggerLevel && ) = delete; + + private: + Logger::LEVEL previous_level_; + }; + } // namespace detail } // namespace geode #include diff --git a/include/geode/model/representation/io/geode/geode_brep_input.hpp b/include/geode/model/representation/io/geode/geode_brep_input.hpp index baf7cde80..1ba41c160 100644 --- a/include/geode/model/representation/io/geode/geode_brep_input.hpp +++ b/include/geode/model/representation/io/geode/geode_brep_input.hpp @@ -70,8 +70,9 @@ namespace geode void load_brep_files( Model& brep, std::string_view directory ) { BRepBuilder builder{ brep }; - const auto level = Logger::level(); - Logger::set_level( Logger::LEVEL::warning ); + const detail::ScopedLoggerLevel scoped_level{ + Logger::LEVEL::warning + }; async::parallel_invoke( [&builder, &directory] { builder.load_identifier( directory ); @@ -112,7 +113,6 @@ namespace geode [&builder, &directory] { builder.load_unique_vertices( directory ); } ); - Logger::set_level( level ); detail::register_all_components( brep ); detail::filter_unsupported_components( brep ); builder.repair_component_vertices(); diff --git a/include/geode/model/representation/io/geode/geode_section_input.hpp b/include/geode/model/representation/io/geode/geode_section_input.hpp index f87a35220..85d12dd9f 100644 --- a/include/geode/model/representation/io/geode/geode_section_input.hpp +++ b/include/geode/model/representation/io/geode/geode_section_input.hpp @@ -70,8 +70,9 @@ namespace geode void load_section_files( Model& section, std::string_view directory ) { SectionBuilder builder{ section }; - const auto level = Logger::level(); - Logger::set_level( Logger::LEVEL::warning ); + const detail::ScopedLoggerLevel scoped_level{ + Logger::LEVEL::warning + }; async::parallel_invoke( [&builder, &directory] { builder.load_identifier( directory ); @@ -103,7 +104,6 @@ namespace geode [&builder, &directory] { builder.load_unique_vertices( directory ); } ); - Logger::set_level( level ); detail::register_all_components( section ); detail::filter_unsupported_components( section ); builder.repair_component_vertices(); diff --git a/src/geode/model/representation/io/geode/geode_brep_output.cpp b/src/geode/model/representation/io/geode/geode_brep_output.cpp index f281a60dd..699136166 100644 --- a/src/geode/model/representation/io/geode/geode_brep_output.cpp +++ b/src/geode/model/representation/io/geode/geode_brep_output.cpp @@ -49,8 +49,7 @@ namespace geode void OpenGeodeBRepOutput::save_brep_files( const BRep& brep, std::string_view directory ) const { - const auto level = Logger::level(); - Logger::set_level( Logger::LEVEL::warning ); + const detail::ScopedLoggerLevel scoped_level{ Logger::LEVEL::warning }; async::parallel_invoke( [&directory, &brep] { brep.save_identifier( directory ); @@ -91,7 +90,6 @@ namespace geode [&directory, &brep] { brep.save_block_collections( directory ); } ); - Logger::set_level( level ); } std::vector< std::string > OpenGeodeBRepOutput::write( diff --git a/src/geode/model/representation/io/geode/geode_section_output.cpp b/src/geode/model/representation/io/geode/geode_section_output.cpp index 62a87da42..bc8d90d19 100644 --- a/src/geode/model/representation/io/geode/geode_section_output.cpp +++ b/src/geode/model/representation/io/geode/geode_section_output.cpp @@ -39,8 +39,7 @@ namespace geode void OpenGeodeSectionOutput::save_section_files( const Section& section, std::string_view directory ) const { - const auto level = Logger::level(); - Logger::set_level( Logger::LEVEL::warning ); + const detail::ScopedLoggerLevel scoped_level{ Logger::LEVEL::warning }; async::parallel_invoke( [&directory, §ion] { section.save_identifier( directory ); @@ -62,7 +61,6 @@ namespace geode section.save_line_collections( directory ); section.save_surface_collections( directory ); } ); - Logger::set_level( level ); } void OpenGeodeSectionOutput::archive_section_files( diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index 40ca801ea..c6513a4fc 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -720,6 +720,35 @@ namespace geode::OpenGeodeException::TYPE::internal, "[Test] Too long component name should not be loaded" }; } + + /* + * The Logger level changed while loading a model is restored when the + * loading fails. + */ + void test_logger_level_restored() + { + saved_brep( "logger_level.og_brep" ); + auto files = extract_archive( "logger_level.og_brep" ); + auto& content = files.at( "relationships" ); + content.resize( content.size() / 2 ); + write_archive( files, "truncated_relationships.og_brep" ); + geode::Logger::set_level( geode::Logger::LEVEL::info ); + try + { + geode_unused( + geode::load_brep( "truncated_relationships.og_brep" ) ); + } + catch( const geode::OpenGeodeException& /*unused*/ ) + { + geode::OpenGeodeModelException::test( + geode::Logger::level() == geode::Logger::LEVEL::info, + "[Test] Logger level should be restored" ); + return; + } + throw geode::OpenGeodeModelException{ nullptr, + geode::OpenGeodeException::TYPE::internal, + "[Test] Truncated relationships should not be loaded" }; + } } // namespace void test() @@ -737,6 +766,7 @@ void test() test_save_in_missing_directory(); test_utf8_path(); test_too_long_component_name(); + test_logger_level_restored(); } OPENGEODE_TEST( "model-corrupted-files" ) From 288aecf260435ac899cd2b96d701ad8210789877 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 10:23:33 +0200 Subject: [PATCH 10/14] fix(ModelIO): turn std exceptions into OpenGeodeException when loading load_brep, load_section and load_brep_time_series only caught OpenGeodeException: std exceptions raised by an input escaped. They now also catch std::exception and rethrow an OpenGeodeModelException with the usual "Cannot load ... from file" message. Co-Authored-By: Claude Opus 5.5 --- .../model/representation/io/brep_input.cpp | 7 ++ .../io/brep_time_series_input.cpp | 7 ++ .../model/representation/io/section_input.cpp | 7 ++ tests/model/test-model-corrupted-files.cpp | 111 ++++++++++++++++++ 4 files changed, 132 insertions(+) diff --git a/src/geode/model/representation/io/brep_input.cpp b/src/geode/model/representation/io/brep_input.cpp index 5c901ff3b..31e6dd849 100644 --- a/src/geode/model/representation/io/brep_input.cpp +++ b/src/geode/model/representation/io/brep_input.cpp @@ -70,6 +70,13 @@ namespace geode OpenGeodeException::TYPE::data, "Cannot load BRep from file: ", filename }; } + catch( const std::exception& e ) + { + Logger::error( e.what() ); + throw OpenGeodeModelException{ nullptr, + OpenGeodeException::TYPE::internal, + "Cannot load BRep from file: ", filename }; + } } AdditionalFiles brep_additional_files( std::string_view filename ) diff --git a/src/geode/model/representation/io/brep_time_series_input.cpp b/src/geode/model/representation/io/brep_time_series_input.cpp index 713711cf1..6ad3db506 100644 --- a/src/geode/model/representation/io/brep_time_series_input.cpp +++ b/src/geode/model/representation/io/brep_time_series_input.cpp @@ -94,6 +94,13 @@ namespace geode OpenGeodeException::TYPE::data, "Cannot load BRep time series from file: ", filename }; } + catch( const std::exception& e ) + { + Logger::error( e.what() ); + throw OpenGeodeModelException{ nullptr, + OpenGeodeException::TYPE::internal, + "Cannot load BRep time series from file: ", filename }; + } } AdditionalFiles brep_time_series_additional_files( diff --git a/src/geode/model/representation/io/section_input.cpp b/src/geode/model/representation/io/section_input.cpp index 8d09bd7f3..b907f4791 100644 --- a/src/geode/model/representation/io/section_input.cpp +++ b/src/geode/model/representation/io/section_input.cpp @@ -69,6 +69,13 @@ namespace geode OpenGeodeException::TYPE::internal, "Cannot load Section from file: ", filename }; } + catch( const std::exception& e ) + { + Logger::error( e.what() ); + throw OpenGeodeModelException{ nullptr, + OpenGeodeException::TYPE::internal, + "Cannot load Section from file: ", filename }; + } } AdditionalFiles section_additional_files( std::string_view filename ) diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index c6513a4fc..9467c9410 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include @@ -63,6 +64,7 @@ #include #include #include +#include #include #include @@ -749,6 +751,114 @@ namespace geode::OpenGeodeException::TYPE::internal, "[Test] Truncated relationships should not be loaded" }; } + + template < typename Base > + class ThrowingInput final : public Base + { + public: + explicit ThrowingInput( std::string_view filename ) : Base{ filename } + { + } + + typename Base::InputData read() override + { + throw std::runtime_error{ "[Test] Not an OpenGeodeException" }; + } + + geode::AdditionalFiles additional_files() const override + { + return {}; + } + + geode::index_t object_priority() const override + { + return 0; + } + + geode::Percentage is_loadable() const override + { + return geode::Percentage{ 1 }; + } + }; + + class ThrowingTimeSeriesInput final : public geode::BRepTimeSeriesInput + { + public: + explicit ThrowingTimeSeriesInput( std::string_view filename ) + : geode::BRepTimeSeriesInput{ filename } + { + } + + void read( geode::BRep& /*brep*/ ) override + { + throw std::runtime_error{ "[Test] Not an OpenGeodeException" }; + } + + geode::AdditionalFiles additional_files() const override + { + return {}; + } + + geode::Percentage is_loadable() const override + { + return geode::Percentage{ 1 }; + } + }; + + template < typename Function > + void check_opengeode_exception( + std::string_view what, const Function& function ) + { + try + { + function(); + } + catch( const geode::OpenGeodeException& exception ) + { + geode::Logger::info( + what, " throws as expected: ", exception.what() ); + return; + } + catch( const std::exception& exception ) + { + throw geode::OpenGeodeModelException{ nullptr, + geode::OpenGeodeException::TYPE::internal, "[Test] ", what, + " throws a std exception: ", exception.what() }; + } + throw geode::OpenGeodeModelException{ nullptr, + geode::OpenGeodeException::TYPE::internal, "[Test] ", what, + " should throw" }; + } + + /* + * Std exceptions raised while loading a model are turned into + * OpenGeodeException. + */ + void test_std_exceptions() + { + const std::string extension{ "throwing_input" }; + const auto filename = absl::StrCat( "std_exception.", extension ); + { + std::ofstream file{ filename }; + file << "content"; + } + geode::BRepInputFactory::register_creator< + ThrowingInput< geode::BRepInput > >( extension ); + geode::SectionInputFactory::register_creator< + ThrowingInput< geode::SectionInput > >( extension ); + geode::BRepTimeSeriesInputFactory::register_creator< + ThrowingTimeSeriesInput >( extension ); + check_opengeode_exception( "Loading BRep", [&filename] { + geode_unused( geode::load_brep( filename ) ); + } ); + check_opengeode_exception( "Loading Section", [&filename] { + geode_unused( geode::load_section( filename ) ); + } ); + check_opengeode_exception( "Loading BRep time series", [&filename] { + geode::BRep brep; + geode::load_brep_time_series( brep, filename ); + } ); + } } // namespace void test() @@ -767,6 +877,7 @@ void test() test_utf8_path(); test_too_long_component_name(); test_logger_level_restored(); + test_std_exceptions(); } OPENGEODE_TEST( "model-corrupted-files" ) From 7f4e620489f195d439f92aea6f2e479ab1ceaff5 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 10:39:11 +0200 Subject: [PATCH 11/14] fix(ModelIO): check component existence without a qualified fold expression Clang and MSVC reject a fold expression over qualified member calls (model.Components::has_component). The check now goes through a helper function template. Co-Authored-By: Claude Opus 5.5 --- .../model/representation/builder/detail/filter.hpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/include/geode/model/representation/builder/detail/filter.hpp b/include/geode/model/representation/builder/detail/filter.hpp index e1f702737..19efd3127 100644 --- a/include/geode/model/representation/builder/detail/filter.hpp +++ b/include/geode/model/representation/builder/detail/filter.hpp @@ -66,6 +66,13 @@ namespace geode } }; + template < typename Mixin, typename Model > + [[nodiscard]] bool has_mixin_component( + const Model& model, const uuid& component_id ) + { + return model.Mixin::has_component( component_id ); + } + template < typename > struct ComponentsExistenceChecker; @@ -76,7 +83,8 @@ namespace geode bool apply( const Model& model, const uuid& component_id ) const { return ( - model.Components::has_component( component_id ) || ... ); + has_mixin_component< Components >( model, component_id ) + || ... ); } }; } // namespace internal From e4eb903441b70d01a63e711bbb446600b0a4370c Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 10:59:25 +0200 Subject: [PATCH 12/14] fix(IO): use UTF-8 paths in the generic input and output helpers geode_object_input_impl checked the file existence and geode_object_output_impl created the parent directories with narrow paths. On Windows, a non-ASCII (UTF-8) file name was reported as missing when loading and directories got a wrong name when saving. Both now use std::filesystem::u8path, consistently with ZipFile and Identifier. Test: saving and loading a BRep with a non-ASCII archive name and directory, which failed on Windows. Co-Authored-By: Claude Opus 5.5 --- include/geode/basic/detail/geode_input_impl.hpp | 7 ++++--- include/geode/basic/detail/geode_output_impl.hpp | 2 +- tests/model/test-model-corrupted-files.cpp | 7 +++++++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/include/geode/basic/detail/geode_input_impl.hpp b/include/geode/basic/detail/geode_input_impl.hpp index f0908bd41..524371368 100644 --- a/include/geode/basic/detail/geode_input_impl.hpp +++ b/include/geode/basic/detail/geode_input_impl.hpp @@ -62,9 +62,10 @@ namespace geode::detail const Timer timer; auto input = geode_object_input_reader< Factory >( filename ); OpenGeodeBasicException::check_exception( - std::filesystem::exists( input->filename() ), nullptr, - OpenGeodeException::TYPE::data, "[", type, "] File \"", filename, - "\" does not exist" ); + std::filesystem::exists( + std::filesystem::u8path( to_string( input->filename() ) ) ), + nullptr, OpenGeodeException::TYPE::data, "[", type, "] File \"", + filename, "\" does not exist" ); auto object = input->read( std::forward< Args >( args )... ); Logger::info( type, " loaded from ", filename, " in ", timer.duration() ); diff --git a/include/geode/basic/detail/geode_output_impl.hpp b/include/geode/basic/detail/geode_output_impl.hpp index 0c08da1fe..67b07b15b 100644 --- a/include/geode/basic/detail/geode_output_impl.hpp +++ b/include/geode/basic/detail/geode_output_impl.hpp @@ -61,7 +61,7 @@ namespace geode::detail const Timer timer; auto output = geode_object_output_writer< Factory >( filename ); const auto directories = filepath_without_filename( - std::filesystem::path{ expand_predefined_folders( filename ) } ); + std::filesystem::u8path( expand_predefined_folders( filename ) ) ); if( !directories.empty() ) { std::filesystem::create_directories( directories ); diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index 9467c9410..c2db4a324 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -682,6 +682,13 @@ namespace loaded.nb_surfaces() == brep.nb_surfaces(), "[Test] Wrong number of surfaces with UTF-8 path" ); check_same_unique_vertices( brep, loaded ); + + const std::string in_directory{ + "r\xc3\xa9pertoire_\xc3\xa9/mod\xc3\xa8le.og_brep" + }; + geode::save_brep( brep, in_directory ); + const auto loaded_in_directory = geode::load_brep( in_directory ); + check_same_unique_vertices( brep, loaded_in_directory ); } /* From 4fff9753f7e33daab95a60b9d9cc21101c212d34 Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 11:20:37 +0200 Subject: [PATCH 13/14] fix(IO): only test non-ASCII archive names, not directories Model mesh files are written in a temporary directory next to the archive by mesh outputs that still open narrow paths, so a non-ASCII directory fails on Windows. That is a mesh IO limitation outside this change; the UTF-8 test now only covers non-ASCII archive names. Co-Authored-By: Claude Opus 5.5 --- tests/model/test-model-corrupted-files.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/tests/model/test-model-corrupted-files.cpp b/tests/model/test-model-corrupted-files.cpp index c2db4a324..701ae9e05 100644 --- a/tests/model/test-model-corrupted-files.cpp +++ b/tests/model/test-model-corrupted-files.cpp @@ -682,13 +682,8 @@ namespace loaded.nb_surfaces() == brep.nb_surfaces(), "[Test] Wrong number of surfaces with UTF-8 path" ); check_same_unique_vertices( brep, loaded ); - - const std::string in_directory{ - "r\xc3\xa9pertoire_\xc3\xa9/mod\xc3\xa8le.og_brep" - }; - geode::save_brep( brep, in_directory ); - const auto loaded_in_directory = geode::load_brep( in_directory ); - check_same_unique_vertices( brep, loaded_in_directory ); + // Non-ASCII directories are not tested: mesh files, written in a + // temporary directory next to the archive, still use narrow paths. } /* From c749cd2b0a7451bbd644a830ae538059430afd8e Mon Sep 17 00:00:00 2001 From: Arnaud Botella Date: Tue, 20 Oct 2026 14:30:26 +0200 Subject: [PATCH 14/14] perf(VertexIdentifier): scan component vertices in parallel before repairing them repair_component_vertices ran on every model load, serially over all unique vertices, with one allocation per unique vertex. A read-only parallel scan now flags the unique vertices to repair, and only those are repaired sequentially, so valid models skip all allocations and writes. The repair result is unchanged: it only fills NO_ID links, which never belong to unflagged unique vertices. Co-Authored-By: Claude Opus 5.5 --- .../model/mixin/core/vertex_identifier.cpp | 187 +++++++++++------- 1 file changed, 119 insertions(+), 68 deletions(-) diff --git a/src/geode/model/mixin/core/vertex_identifier.cpp b/src/geode/model/mixin/core/vertex_identifier.cpp index 40d0a6c57..6b82e81a8 100644 --- a/src/geode/model/mixin/core/vertex_identifier.cpp +++ b/src/geode/model/mixin/core/vertex_identifier.cpp @@ -397,79 +397,33 @@ namespace geode void repair_component_vertices() { - index_t nb_unknown_components{ 0 }; - index_t nb_invalid_vertices{ 0 }; - index_t nb_inconsistent_vertices{ 0 }; - index_t nb_restored_vertices{ 0 }; - for( const auto unique_vertex : Range{ nb_unique_vertices() } ) - { - const auto& vertices = - component_vertices_->value( unique_vertex ); - std::vector< bool > to_keep( vertices.size(), true ); - bool update{ false }; - for( const auto v : Indices{ vertices } ) - { - const auto& vertex = vertices[v]; - const auto it = - vertex2unique_vertex_.find( vertex.component_id.id ); - if( it == vertex2unique_vertex_.end() ) - { - to_keep[v] = false; - update = true; - nb_unknown_components++; - continue; - } - auto& attribute = *it->second; - if( vertex.vertex >= attribute.size() ) - { - to_keep[v] = false; - update = true; - nb_invalid_vertices++; - continue; - } - const auto value = attribute.value( vertex.vertex ); - if( value == NO_ID ) - { - attribute.set_value( vertex.vertex, unique_vertex ); - nb_restored_vertices++; - } - else if( value != unique_vertex ) + // Read-only parallel scan, nothing to repair in a valid model + std::vector< uint8_t > to_repair( nb_unique_vertices(), 0 ); + async::parallel_for( + async::irange( index_t{ 0 }, nb_unique_vertices() ), + [this, &to_repair]( index_t unique_vertex ) { + for( const auto& vertex : + component_vertices_->value( unique_vertex ) ) { - to_keep[v] = false; - update = true; - nb_inconsistent_vertices++; + if( !is_linked_to_unique_vertex( + vertex, unique_vertex ) ) + { + to_repair[unique_vertex] = 1; + return; + } } - } - if( update ) + } ); + // Sequential repair: a component vertex may be listed in several + // unique vertices, the first one keeps it + ComponentVerticesRepair repair; + for( const auto unique_vertex : Range{ nb_unique_vertices() } ) + { + if( to_repair[unique_vertex] != 0 ) { - component_vertices_->set_value( unique_vertex, - extract_vector_elements( to_keep, vertices ) ); + repair_unique_vertex( unique_vertex, repair ); } } - if( nb_unknown_components > 0 ) - { - Logger::warning( "[VertexIdentifier::load] Removed ", - nb_unknown_components, - " component vertices of unknown components" ); - } - if( nb_invalid_vertices > 0 ) - { - Logger::warning( "[VertexIdentifier::load] Removed ", - nb_invalid_vertices, - " component vertices with invalid vertex index" ); - } - if( nb_inconsistent_vertices > 0 ) - { - Logger::warning( "[VertexIdentifier::load] Removed ", - nb_inconsistent_vertices, - " component vertices linked to another unique vertex" ); - } - if( nb_restored_vertices > 0 ) - { - Logger::warning( "[VertexIdentifier::load] Restored ", - nb_restored_vertices, - " component vertex links to their unique vertex" ); - } + repair.log(); } void save( std::string_view directory ) const @@ -659,6 +613,103 @@ namespace geode } private: + struct ComponentVerticesRepair + { + void log() const + { + if( nb_unknown_components > 0 ) + { + Logger::warning( "[VertexIdentifier::load] Removed ", + nb_unknown_components, + " component vertices of unknown components" ); + } + if( nb_invalid_vertices > 0 ) + { + Logger::warning( "[VertexIdentifier::load] Removed ", + nb_invalid_vertices, + " component vertices with invalid vertex index" ); + } + if( nb_inconsistent_vertices > 0 ) + { + Logger::warning( "[VertexIdentifier::load] Removed ", + nb_inconsistent_vertices, + " component vertices linked to another unique " + "vertex" ); + } + if( nb_restored_vertices > 0 ) + { + Logger::warning( "[VertexIdentifier::load] Restored ", + nb_restored_vertices, + " component vertex links to their unique vertex" ); + } + } + + index_t nb_unknown_components{ 0 }; + index_t nb_invalid_vertices{ 0 }; + index_t nb_inconsistent_vertices{ 0 }; + index_t nb_restored_vertices{ 0 }; + }; + + [[nodiscard]] bool is_linked_to_unique_vertex( + const ComponentMeshVertex& vertex, index_t unique_vertex ) const + { + const auto it = + vertex2unique_vertex_.find( vertex.component_id.id ); + if( it == vertex2unique_vertex_.end() ) + { + return false; + } + const auto& attribute = *it->second; + return vertex.vertex < attribute.size() + && attribute.value( vertex.vertex ) == unique_vertex; + } + + void repair_unique_vertex( + index_t unique_vertex, ComponentVerticesRepair& repair ) + { + const auto& vertices = component_vertices_->value( unique_vertex ); + std::vector< bool > to_keep( vertices.size(), true ); + bool update{ false }; + for( const auto v : Indices{ vertices } ) + { + const auto& vertex = vertices[v]; + const auto it = + vertex2unique_vertex_.find( vertex.component_id.id ); + if( it == vertex2unique_vertex_.end() ) + { + to_keep[v] = false; + update = true; + repair.nb_unknown_components++; + continue; + } + auto& attribute = *it->second; + if( vertex.vertex >= attribute.size() ) + { + to_keep[v] = false; + update = true; + repair.nb_invalid_vertices++; + continue; + } + const auto value = attribute.value( vertex.vertex ); + if( value == NO_ID ) + { + attribute.set_value( vertex.vertex, unique_vertex ); + repair.nb_restored_vertices++; + } + else if( value != unique_vertex ) + { + to_keep[v] = false; + update = true; + repair.nb_inconsistent_vertices++; + } + } + if( update ) + { + component_vertices_->set_value( unique_vertex, + extract_vector_elements( to_keep, vertices ) ); + } + } + OpenGeodeVertexSet unique_vertices_; std::shared_ptr< VariableAttribute< std::vector< ComponentMeshVertex > > >