fix serialization and more string constants (#72)

* fix serialization and more string constants

* (fix) force string type on ident serialization

* update precommit and fix formatting

* build ci multi-threaded

Author: nathanhhughes

.github/workflows/ci.yaml

@@ -35,7 +35,7 @@ jobs:
             -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
             -DCMAKE_C_COMPILER_LAUNCHER=ccache
       - name: Build
-        run: cmake --build ${{github.workspace}}/build --config Release
+        run: cmake --build ${{github.workspace}}/build --config Release -j
       - name: Test
         working-directory: ${{github.workspace}}/build
         run: ctest -C Release

.pre-commit-config.yaml

@@ -5,7 +5,7 @@ exclude: python/tests/resources|.clang-format
 repos:
   - repo: https://github.com/astral-sh/ruff-pre-commit
     # Ruff version.
-    rev: v0.9.2
+    rev: v0.14.2
     hooks:
       - id: ruff
         args: [--select, I, --fix]
@@ -14,11 +14,11 @@ repos:
       # Run the formatter.
       - id: ruff-format
   - repo: https://github.com/pre-commit/mirrors-clang-format
-    rev: v14.0.6
+    rev: v21.1.2
     hooks:
       - id: clang-format
   - repo: https://github.com/pre-commit/pre-commit-hooks
-    rev: v3.2.0
+    rev: v6.0.0
     hooks:
       - id: trailing-whitespace
       - id: end-of-file-fixer

CMakeLists.txt

@@ -6,7 +6,7 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)
 set(CMAKE_CXX_EXTENSIONS OFF)
 
 if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
-  add_compile_options(-Wall -Wextra)
+  add_compile_options(-Wall -Wextra -Wpedantic)
 endif()
 
 if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)

include/spark_dsg/serialization/versioning.h

@@ -45,7 +45,7 @@ namespace spark_dsg::io {
 
 // Define the current project and version when serializing data with this
 // implementation.
-inline const std::string CURRENT_PROJECT_NAME = "main";
+inline constexpr static const char* CURRENT_PROJECT_NAME = "main";
 
 // Which project names are compatible to load given the current project. <current,
 // {compatible, ...}>. Projects are always assumed to be compatible with themselves.
@@ -89,7 +89,11 @@ struct FileHeader {
   // The file type identifier to make sure we're reading compatible binary data.
   //           ! DO NOT CHANGE THIS VALUE !
   // (Otherwise all older files will be unreadable.)
-  inline static const std::string IDENTIFIER_STRING = "SPARK_DSG";
+  inline constexpr static const char* IDENTIFIER_STRING = "SPARK_DSG";
+  //! @brief Get header key used for json serialization
+  static std::string header_json_key() {
+    return std::string(IDENTIFIER_STRING) + "_header";
+  }
 
   // String identifier for different variants of spark-dsg that may not be compatible.
   std::string project_name = "main";

src/node_attributes.cpp

@@ -193,16 +193,20 @@ void SemanticNodeAttributes::serialization_info() {
   serialization::field("name", name);
   const auto& header = io::GlobalInfo::loadedHeader();
   if (header.version <= io::Version(1, 0, 2)) {
+    io::warnOutdatedHeader(header);
+
     Eigen::Matrix<uint8_t, 3, 1> color_uint8;
     serialization::field("color", color_uint8);
     color = Color(color_uint8[0], color_uint8[1], color_uint8[2]);
-    io::warnOutdatedHeader(header);
   } else {
     serialization::field("color", color);
   }
+
   serialization::field("bounding_box", bounding_box);
   serialization::field("semantic_label", semantic_label);
   if (header.version <= io::Version(1, 0, 4)) {
+    io::warnOutdatedHeader(header);
+
     Eigen::MatrixXd feature;
     serialization::field("semantic_feature", feature);
     semantic_feature = feature.cast<float>();
@@ -458,10 +462,10 @@ void AgentNodeAttributes::serialization_info() {
   NodeAttributes::serialization_info();
 
   const auto& header = io::GlobalInfo::loadedHeader();
-  if (header.version >= io::Version(1, 1, 0)) {
-    serialization::field("timestamp", timestamp);
-  } else {
+  if (header.version < io::Version(1, 1, 0)) {
     io::warnOutdatedHeader(header);
+  } else {
+    serialization::field("timestamp", timestamp);
   }
 
   serialization::field("world_R_body", world_R_body);
@@ -486,7 +490,7 @@ bool AgentNodeAttributes::is_equal(const NodeAttributes& other) const {
          dbow_values == derived->dbow_values;
 }
 
-KhronosObjectAttributes::KhronosObjectAttributes() : mesh(true, false, false){};
+KhronosObjectAttributes::KhronosObjectAttributes() : mesh(true, false, false) {}
 
 NodeAttributes::Ptr KhronosObjectAttributes::clone() const {
   return std::make_unique<KhronosObjectAttributes>(*this);
@@ -519,6 +523,7 @@ void KhronosObjectAttributes::serialization_info() {
   const auto& header = io::GlobalInfo::loadedHeader();
   if (header.version <= io::Version(1, 0, 1)) {
     io::warnOutdatedHeader(header);
+
     std::vector<float> xyz;
     serialization::field("vertices", xyz);
     std::vector<uint8_t> rgb;

src/serialization/binary_conversions.cpp

@@ -44,9 +44,10 @@ namespace spark_dsg {
 void read_binary(const serialization::BinaryDeserializer& s, BoundingBox& box) {
   const auto& header = io::GlobalInfo::loadedHeader();
   if (header.version < io::Version(1, 0, 3)) {
+    io::warnOutdatedHeader(header);
+
     // Legacy bboxes with min/max encoding.
     s.checkFixedArrayLength(5);
-    io::warnOutdatedHeader(header);
   } else {
     // New bboxes with dimensions encoding.
     s.checkFixedArrayLength(4);
@@ -57,6 +58,8 @@ void read_binary(const serialization::BinaryDeserializer& s, BoundingBox& box) {
   box.type = static_cast<BoundingBox::Type>(raw_type);
 
   if (header.version < io::Version(1, 0, 3)) {
+    io::warnOutdatedHeader(header);
+
     Eigen::Vector3f min, max;
     s.read(min);
     s.read(max);

src/serialization/graph_binary_serialization.cpp

@@ -85,6 +85,8 @@ NodeId parseNode(const AttributeFactory<NodeAttributes>& factory,
   PartitionId partition = 0;
   const auto& header = io::GlobalInfo::loadedHeader();
   if (header.version < io::Version(1, 1, 0)) {
+    io::warnOutdatedHeader(header);
+
     std::optional<std::chrono::nanoseconds> stamp;
     deserializer.read(stamp);
     // guess at interlayer IDs based on node symbol prefix
@@ -204,6 +206,8 @@ template <typename Attrs>
 AttributeFactory<Attrs> loadFactory(const io::FileHeader& header,
                                     const BinaryDeserializer& deserializer) {
   if (header.version < io::Version(1, 0, 2)) {
+    io::warnOutdatedHeader(header);
+
     return serialization::AttributeRegistry<Attrs>::current();
   }
 
@@ -216,6 +220,8 @@ bool updateGraph(DynamicSceneGraph& graph, const BinaryDeserializer& deserialize
   const auto& header = io::GlobalInfo::loadedHeader();
 
   if (header.version < io::Version(1, 1, 2)) {
+    io::warnOutdatedHeader(header);
+
     // NOTE(nathan) we intentionally don't try to use the layer IDs to populate anything
     // because they will not include partitions and cause lots of serialization churn
     std::vector<LayerId> layer_ids;
@@ -237,6 +243,8 @@ bool updateGraph(DynamicSceneGraph& graph, const BinaryDeserializer& deserialize
   }
 
   if (header.version < io::Version(1, 0, 2)) {
+    io::warnOutdatedHeader(header);
+
     LayerId mesh_layer_id;
     deserializer.read(mesh_layer_id);
   }
@@ -247,12 +255,16 @@ bool updateGraph(DynamicSceneGraph& graph, const BinaryDeserializer& deserialize
 
   std::map<std::string, LayerKey> layer_names;
   if (header.version < io::Version(1, 1, 0)) {
+    io::warnOutdatedHeader(header);
+
     layer_names = {{DsgLayers::OBJECTS, 2},
                    {DsgLayers::AGENTS, 2},
                    {DsgLayers::PLACES, 3},
                    {DsgLayers::ROOMS, 4},
                    {DsgLayers::BUILDINGS, 5}};
   } else if (header.version < io::Version(1, 1, 1)) {
+    io::warnOutdatedHeader(header);
+
     std::map<std::string, LayerId> names;
     deserializer.read(names);
     layer_names = DynamicSceneGraph::LayerNames(names.begin(), names.end());
@@ -264,7 +276,9 @@ bool updateGraph(DynamicSceneGraph& graph, const BinaryDeserializer& deserialize
     graph.addLayer(key.layer, key.partition, name);
   }
 
-  if (header.version >= io::Version(1, 0, 6)) {
+  if (header.version < io::Version(1, 0, 6)) {
+    io::warnOutdatedHeader(header);
+  } else {
     std::string metadata_json;
     deserializer.read(metadata_json);
     try {
@@ -290,6 +304,8 @@ bool updateGraph(DynamicSceneGraph& graph, const BinaryDeserializer& deserialize
   }
 
   if (header.version < io::Version(1, 1, 0)) {
+    io::warnOutdatedHeader(header);
+
     deserializer.checkDynamicArray();
     while (!deserializer.isDynamicArrayEnd()) {
       stale_nodes.erase(parseNode(

src/serialization/graph_json_serialization.cpp

@@ -71,6 +71,8 @@ void read_node_from_json(const serialization::AttributeFactory<NodeAttributes>&
   PartitionId partition = 0;
   const auto& header = io::GlobalInfo::loadedHeader();
   if (header.version < io::Version(1, 1, 0)) {
+    io::warnOutdatedHeader(header);
+
     if (record.contains("timestamp")) {
       partition = NodeSymbol(node_id).category();
     }
@@ -111,8 +113,7 @@ namespace io::json {
 
 std::string writeGraph(const DynamicSceneGraph& graph, bool include_mesh) {
   nlohmann::json record;
-  record[io::FileHeader::IDENTIFIER_STRING + "_header"] = io::FileHeader::current();
-
+  record[io::FileHeader::header_json_key()] = io::FileHeader::current();
   record["directed"] = false;
   record["multigraph"] = false;
   record["nodes"] = nlohmann::json::array();
@@ -161,7 +162,7 @@ DynamicSceneGraph::Ptr readGraph(const std::string& contents) {
   const auto record = nlohmann::json::parse(contents);
 
   // Parse header.
-  const std::string header_field_name = FileHeader::IDENTIFIER_STRING + "_header";
+  const auto header_field_name = FileHeader::header_json_key();
   const auto header = record.contains(header_field_name)
                           ? record.at(header_field_name).get<io::FileHeader>()
                           : io::FileHeader::legacy();
@@ -171,6 +172,8 @@ DynamicSceneGraph::Ptr readGraph(const std::string& contents) {
 
   DynamicSceneGraph::LayerKeys layer_keys;
   if (header.version < io::Version(1, 1, 2)) {
+    io::warnOutdatedHeader(header);
+
     const auto layer_ids = record.at("layer_ids").get<std::vector<LayerId>>();
     layer_keys = DynamicSceneGraph::LayerKeys(layer_ids.begin(), layer_ids.end());
   } else {
@@ -179,12 +182,16 @@ DynamicSceneGraph::Ptr readGraph(const std::string& contents) {
 
   DynamicSceneGraph::LayerNames layer_names;
   if (header.version < io::Version(1, 1, 0)) {
+    io::warnOutdatedHeader(header);
+
     layer_names = {{DsgLayers::OBJECTS, 2},
                    {DsgLayers::AGENTS, 2},
                    {DsgLayers::PLACES, 3},
                    {DsgLayers::ROOMS, 4},
                    {DsgLayers::BUILDINGS, 5}};
   } else if (header.version < io::Version(1, 1, 1)) {
+    io::warnOutdatedHeader(header);
+
     const auto names = record.at("layer_names").get<std::map<std::string, LayerId>>();
     layer_names = DynamicSceneGraph::LayerNames(names.begin(), names.end());
   } else {

src/serialization/versioning.cpp

@@ -124,7 +124,7 @@ std::string Version::toString() const {
 std::vector<uint8_t> FileHeader::serializeToBinary() const {
   std::vector<uint8_t> buffer;
   serialization::BinarySerializer serializer(&buffer);
-  serializer.write(IDENTIFIER_STRING);
+  serializer.write(std::string(IDENTIFIER_STRING));
   serializer.write(*this);
   return buffer;
 }

tests/serialization/utest_file_io.cpp

@@ -63,7 +63,7 @@ TEST(FileIoTests, VersionSerialization) {
 
   // Check deserialization.
   const auto result = FileHeader::deserializeFromBinary(buffer);
-  EXPECT_TRUE(result.has_value());
+  ASSERT_TRUE(result.has_value());
   EXPECT_EQ(header.project_name, result->project_name);
   EXPECT_EQ(header.version, result->version);