A couple of more bug fixes to be included in the 1.5.7 release.

src/draco/core/macros.h

@@ -34,12 +34,6 @@
 #include <iostream>
 namespace draco {
 
-#ifndef DISALLOW_COPY_AND_ASSIGN
-#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
-  TypeName(const TypeName &) = delete;     \
-  void operator=(const TypeName &) = delete;
-#endif  // DISALLOW_COPY_AND_ASSIGN
-
 #ifndef FALLTHROUGH_INTENDED
 #if defined(__clang__) && defined(__has_warning)
 #if __has_feature(cxx_attributes) && __has_warning("-Wimplicit-fallthrough")
@@ -67,6 +61,12 @@ namespace draco {
 
 }  // namespace draco
 
+#ifndef DISALLOW_COPY_AND_ASSIGN
+#define DISALLOW_COPY_AND_ASSIGN(TypeName) \
+  TypeName(const TypeName &) = delete;     \
+  void operator=(const TypeName &) = delete;
+#endif  // DISALLOW_COPY_AND_ASSIGN
+
 #ifdef DRACO_DEBUG
 #define DRACO_DCHECK(x) (assert(x));
 #define DRACO_DCHECK_EQ(a, b) assert((a) == (b));

src/draco/io/gltf_decoder.cc

@@ -313,7 +313,7 @@ Status CopyDataFromBufferView(const tinygltf::Model &model, int buffer_view_id,
   const uint8_t *const data_start = buffer.data.data() + buffer_view.byteOffset;
 
   data->resize(buffer_view.byteLength);
-  memcpy(&(*data)[0], data_start, buffer_view.byteLength);
+  memcpy(data->data(), data_start, buffer_view.byteLength);
   return OkStatus();
 }
 

src/draco/io/gltf_decoder_test.cc

@@ -1268,6 +1268,31 @@ TEST(GltfDecoderTest, DecodeMeshWithMeshFeaturesWithStructuralMetadata) {
   GltfTestHelper::CheckBoxMetaStructuralMetadata(*mesh, use_case);
 }
 
+TEST(GltfDecoderTest, DecodeMeshWithStructuralMetadataWithEmptyStringBuffer) {
+  // Checks that the decoder correctly handles 0-length buffers. An example case
+  // where this could happen is an EXT_structural_metadata extension with a
+  // buffer containing an empty string.
+  const auto path =
+      GetTestFileFullPath("ZeroLengthBufferView/ZeroLengthBufferView.gltf");
+  GltfTestHelper::UseCase use_case;
+  use_case.has_mesh_features = true;
+  use_case.has_structural_metadata = true;
+
+  draco::GltfDecoder decoder;
+  DRACO_ASSIGN_OR_ASSERT(auto mesh, decoder.DecodeFromFile(path));
+  ASSERT_NE(mesh, nullptr);
+  ASSERT_EQ(mesh->GetStructuralMetadata().NumPropertyTables(), 1);
+  ASSERT_EQ(mesh->GetStructuralMetadata().GetPropertyTable(0).GetCount(), 1);
+  ASSERT_EQ(mesh->GetStructuralMetadata().GetPropertyTable(0).NumProperties(),
+            1);
+  ASSERT_EQ(mesh->GetStructuralMetadata()
+                .GetPropertyTable(0)
+                .GetProperty(0)
+                .GetData()
+                .data.size(),
+            0);
+}
+
 TEST(GltfDecoderTest, DecodeMeshWithMeshFeaturesWithDracoCompression) {
   // Checks decoding of a simple glTF with mesh features compressed with Draco
   // as draco::Mesh.

src/draco/io/gltf_encoder.cc

@@ -387,10 +387,8 @@ class GltfAsset {
       std::unordered_map<int, int> *feature_id_name_indices);
 
   // Iterate through the materials that are associated with |mesh| and add them
-  // to the asset. Returns true if |mesh| does not contain any materials or all
-  // the materials are supported. Returns false if |mesh| contains materials
-  // that are not supported.
-  bool AddMaterials(const Mesh &mesh);
+  // to the asset.
+  void AddMaterials(const Mesh &mesh);
 
   // Checks whether a given Draco |attribute| has data of expected |data_type|
   // and whether the data has one of expected |num_components|. Returns true
@@ -428,10 +426,8 @@ class GltfAsset {
   Status AddSceneNode(const Scene &scene, SceneNodeIndex scene_node_index);
 
   // Iterate through the materials that are associated with |scene| and add them
-  // to the asset. Returns true if |scene| does not contain any materials or all
-  // the materials are supported. Returns false if |scene| contains materials
-  // that are not supported.
-  bool AddMaterials(const Scene &scene);
+  // to the asset.
+  void AddMaterials(const Scene &scene);
 
   // Iterate through the animations that are associated with |scene| and add
   // them to the asset. Returns OkStatus() if |scene| does not contain any
@@ -736,9 +732,7 @@ bool GltfAsset::AddDracoMesh(const Mesh &mesh) {
   if (scene_index < 0) {
     return false;
   }
-  if (!AddMaterials(mesh)) {
-    return false;
-  }
+  AddMaterials(mesh);
 
   GltfMesh gltf_mesh;
   meshes_.push_back(gltf_mesh);
@@ -1286,10 +1280,13 @@ int GltfAsset::AddDracoNormals(const Mesh &mesh, int num_encoded_points) {
 int GltfAsset::AddDracoColors(const Mesh &mesh, int num_encoded_points) {
   const PointAttribute *const att =
       mesh.GetNamedAttribute(GeometryAttribute::COLOR);
-  // TODO(b/200302561): Add support for DT_UINT16 with COLOR.
-  if (!CheckDracoAttribute(att, {DT_UINT8, DT_FLOAT32}, {3, 4})) {
+  if (!CheckDracoAttribute(att, {DT_UINT8, DT_UINT16, DT_FLOAT32}, {3, 4})) {
     return -1;
   }
+  if (att->data_type() == DT_UINT16) {
+    return AddAttribute<uint16_t>(*att, mesh.num_points(), num_encoded_points,
+                                  mesh.IsCompressionEnabled());
+  }
   if (att->data_type() == DT_FLOAT32) {
     return AddAttribute<float>(*att, mesh.num_points(), num_encoded_points,
                                mesh.IsCompressionEnabled());
@@ -1460,12 +1457,10 @@ std::vector<std::pair<std::string, int>> GltfAsset::AddDracoGenerics(
   return attrs;
 }
 
-bool GltfAsset::AddMaterials(const Mesh &mesh) {
-  if (mesh.GetMaterialLibrary().NumMaterials() == 0) {
-    return true;
+void GltfAsset::AddMaterials(const Mesh &mesh) {
+  if (mesh.GetMaterialLibrary().NumMaterials()) {
+    material_library_.Copy(mesh.GetMaterialLibrary());
   }
-  material_library_.Copy(mesh.GetMaterialLibrary());
-  return true;
 }
 
 bool GltfAsset::CheckDracoAttribute(const PointAttribute *attribute,
@@ -1589,9 +1584,7 @@ Status GltfAsset::AddScene(const Scene &scene) {
   if (scene_index < 0) {
     return Status(Status::DRACO_ERROR, "Error creating a new scene.");
   }
-  if (!AddMaterials(scene)) {
-    return Status(Status::DRACO_ERROR, "Error adding materials to the scene.");
-  }
+  AddMaterials(scene);
   AddStructuralMetadata(scene);
 
   // Initialize base mesh transforms that may be needed when the base meshes are
@@ -1695,12 +1688,10 @@ Status GltfAsset::AddSceneNode(const Scene &scene,
   return OkStatus();
 }
 
-bool GltfAsset::AddMaterials(const Scene &scene) {
-  if (scene.GetMaterialLibrary().NumMaterials() == 0) {
-    return true;
+void GltfAsset::AddMaterials(const Scene &scene) {
+  if (scene.GetMaterialLibrary().NumMaterials()) {
+    material_library_.Copy(scene.GetMaterialLibrary());
   }
-  material_library_.Copy(scene.GetMaterialLibrary());
-  return true;
 }
 
 Status GltfAsset::AddAnimations(const Scene &scene) {

src/draco/io/obj_encoder.cc

@@ -407,7 +407,8 @@ bool ObjEncoder::EncodeFaceCorner(PointIndex vert_index) {
 }
 
 void ObjEncoder::EncodeFloat(float val) {
-  snprintf(num_buffer_, sizeof(num_buffer_), "%f", val);
+  // Use %F instead of %f to make the floating point non-locale aware.
+  snprintf(num_buffer_, sizeof(num_buffer_), "%F", val);
   buffer()->Encode(num_buffer_, strlen(num_buffer_));
 }
 

src/draco/io/stl_encoder_test.cc

@@ -32,8 +32,15 @@ class StlEncoderTest : public ::testing::Test {
     ASSERT_EQ(mesh0->num_faces(), mesh1->num_faces());
     ASSERT_EQ(mesh0->num_attributes(), mesh1->num_attributes());
     for (size_t att_id = 0; att_id < mesh0->num_attributes(); ++att_id) {
-      ASSERT_EQ(mesh0->attribute(att_id)->size(),
-                mesh1->attribute(att_id)->size());
+      ASSERT_EQ(mesh0->attribute(att_id)->attribute_type(),
+                mesh1->attribute(att_id)->attribute_type());
+      // Normals are recomputed during STL encoding and they may not
+      // correspond to the source ones.
+      if (mesh0->attribute(att_id)->attribute_type() !=
+          GeometryAttribute::NORMAL) {
+        ASSERT_EQ(mesh0->attribute(att_id)->size(),
+                  mesh1->attribute(att_id)->size());
+      }
     }
   }
 

src/draco/mesh/mesh_splitter.cc

@@ -36,6 +36,7 @@ class MeshSplitterInternal {
   struct WorkData : public MeshSplitter::WorkData {
     // TriangleSoupMeshBuilder or PointCloudBuilder.
     std::vector<BuilderT> builders;
+    std::vector<int> *att_id_map;
   };
 
   // Computes number of elements (faces or points) for each sub-mesh.
@@ -112,7 +113,8 @@ StatusOr<MeshSplitter::MeshVector> MeshSplitter::SplitMeshInternal(
   // Create the sub-meshes.
   work_data.builders.resize(num_out_meshes);
   // Map between attribute ids of the input and output meshes.
-  work_data.att_id_map.resize(mesh.num_attributes(), -1);
+  att_id_map_.resize(mesh.num_attributes(), -1);
+  work_data.att_id_map = &att_id_map_;
   const int ignored_att_id =
       (!preserve_split_attribute ? split_attribute_id : -1);
   for (int mi = 0; mi < num_out_meshes; ++mi) {
@@ -185,10 +187,10 @@ void MeshSplitterInternal<BuilderT>::InitializeBuilder(
       continue;
     }
     const GeometryAttribute *const src_att = mesh.attribute(ai);
-    work_data->att_id_map[ai] = work_data->builders[b_index].AddAttribute(
+    (*work_data->att_id_map)[ai] = work_data->builders[b_index].AddAttribute(
         src_att->attribute_type(), src_att->num_components(),
         src_att->data_type(), src_att->normalized());
-    work_data->builders[b_index].SetAttributeName(work_data->att_id_map[ai],
+    work_data->builders[b_index].SetAttributeName(work_data->att_id_map->at(ai),
                                                   src_att->name());
   }
 }
@@ -228,7 +230,7 @@ void AddElementToBuilder(
   const auto &face = mesh.face(source_i);
   for (int ai = 0; ai < mesh.num_attributes(); ++ai) {
     const PointAttribute *const src_att = mesh.attribute(ai);
-    const int target_att_id = work_data->att_id_map[ai];
+    const int target_att_id = work_data->att_id_map->at(ai);
     if (target_att_id == -1) {
       continue;
     }
@@ -245,7 +247,7 @@ void AddElementToBuilder(
     MeshSplitterInternal<PointCloudBuilder>::WorkData *work_data) {
   for (int ai = 0; ai < mesh.num_attributes(); ++ai) {
     const PointAttribute *const src_att = mesh.attribute(ai);
-    const int target_att_id = work_data->att_id_map[ai];
+    const int target_att_id = work_data->att_id_map->at(ai);
     if (target_att_id == -1) {
       continue;
     }
@@ -391,7 +393,7 @@ StatusOr<MeshSplitter::MeshVector> MeshSplitter::FinalizeMeshes(
 
     // Copy over attribute unique ids.
     for (int att_id = 0; att_id < mesh.num_attributes(); ++att_id) {
-      const int mapped_att_id = work_data.att_id_map[att_id];
+      const int mapped_att_id = att_id_map_[att_id];
       if (mapped_att_id == -1) {
         continue;
       }
@@ -431,6 +433,14 @@ StatusOr<MeshSplitter::MeshVector> MeshSplitter::FinalizeMeshes(
         // Create a copy of source mesh features.
         std::unique_ptr<MeshFeatures> mf(new MeshFeatures());
         mf->Copy(mesh.GetMeshFeatures(mfi));
+
+        // Update mesh features attribute index if used.
+        if (mf->GetAttributeIndex() != -1) {
+          const int new_mf_attribute_index =
+              att_id_map_[mf->GetAttributeIndex()];
+          mf->SetAttributeIndex(new_mf_attribute_index);
+        }
+
         const MeshFeaturesIndex new_mfi =
             out_meshes[mi]->AddMeshFeatures(std::move(mf));
         if (work_data.split_by_materials && !preserve_materials_) {
@@ -548,7 +558,8 @@ StatusOr<MeshSplitter::MeshVector> MeshSplitter::SplitMeshToComponents(
   typename MeshSplitterInternal<TriangleSoupMeshBuilder>::WorkData work_data;
   work_data.builders.resize(num_out_meshes);
   work_data.num_sub_mesh_elements.resize(num_out_meshes, 0);
-  work_data.att_id_map.resize(mesh.num_attributes(), -1);
+  att_id_map_.resize(mesh.num_attributes(), -1);
+  work_data.att_id_map = &att_id_map_;
   for (int mi = 0; mi < num_out_meshes; ++mi) {
     const int num_faces = connected_components.NumConnectedComponentFaces(mi);
     work_data.num_sub_mesh_elements[mi] = num_faces;
@@ -572,5 +583,9 @@ StatusOr<MeshSplitter::MeshVector> MeshSplitter::SplitMeshToComponents(
   return FinalizeMeshes(mesh, work_data, std::move(out_meshes));
 }
 
+int MeshSplitter::GetSplitMeshAttributeIndex(int source_mesh_att_index) const {
+  return att_id_map_[source_mesh_att_index];
+}
+
 }  // namespace draco
 #endif  // DRACO_TRANSCODER_SUPPORTED

src/draco/mesh/mesh_splitter.h

@@ -98,10 +98,13 @@ class MeshSplitter {
   StatusOr<MeshVector> SplitMeshToComponents(
       const Mesh &mesh, const MeshConnectedComponents &connected_components);
 
+  // Returns attribute index on each split mesh that corresponds to the
+  // |source_mesh_att_index| of the source Mesh.
+  // Must be called after SplitMesh() or SplitMeshToComponents().
+  int GetSplitMeshAttributeIndex(int source_mesh_att_index) const;
+
  private:
   struct WorkData {
-    // Map between attribute ids of the input and output meshes.
-    std::vector<int> att_id_map;
     std::vector<int> num_sub_mesh_elements;
     bool split_by_materials = false;
   };
@@ -120,6 +123,9 @@ class MeshSplitter {
   bool preserve_structural_metadata_;
   bool deduplicate_vertices_;
 
+  // Map between attribute ids of the input and output meshes.
+  std::vector<int> att_id_map_;
+
   template <typename BuilderT>
   friend class MeshSplitterInternal;
 };

src/draco/scene/scene_utils.cc

@@ -317,6 +317,17 @@ StatusOr<std::unique_ptr<Scene>> SceneUtils::MeshToScene(
       // Copy over mesh features that were associated with the |material_index|.
       Mesh &scene_mesh = scene->GetMesh(mesh_index);
       Mesh::CopyMeshFeaturesForMaterial(*mesh, &scene_mesh, material_index);
+
+      // Update mesh features attribute indices if needed.
+      for (MeshFeaturesIndex mfi(0); mfi < scene_mesh.NumMeshFeatures();
+           ++mfi) {
+        auto &mesh_features = scene_mesh.GetMeshFeatures(mfi);
+        if (mesh_features.GetAttributeIndex() != -1) {
+          mesh_features.SetAttributeIndex(splitter.GetSplitMeshAttributeIndex(
+              mesh_features.GetAttributeIndex()));
+        }
+      }
+
       UpdateMeshFeaturesTexturesOnMesh(old_texture_to_index_map,
                                        &scene->GetNonMaterialTextureLibrary(),
                                        &scene_mesh);

src/draco/scene/scene_utils_test.cc

@@ -403,6 +403,39 @@ TEST(SceneUtilsTest, TestMeshToSceneMultipleMeshFeatures) {
   }
 }
 
+TEST(SceneUtilsTest, TestMeshToSceneMeshFeaturesWithAttributes) {
+  // Tests that converting a mesh into scene properly updates mesh features
+  // attribute indices.
+  auto mesh =
+      draco::ReadMeshFromTestFile("CesiumMilkTruck/glTF/CesiumMilkTruck.gltf");
+  ASSERT_NE(mesh, nullptr);
+
+  // Add a new dummy mesh features and mesh features attribute to the mesh.
+  std::unique_ptr<draco::PointAttribute> mf_att(new draco::PointAttribute());
+  mf_att->Init(draco::GeometryAttribute::GENERIC, 1, draco::DT_FLOAT32, false,
+               mesh->num_points());
+  const int mf_att_id = mesh->AddAttribute(std::move(mf_att));
+  std::unique_ptr<draco::MeshFeatures> mf(new draco::MeshFeatures());
+  mf->SetAttributeIndex(mf_att_id);
+  mesh->AddMeshFeatures(std::move(mf));
+
+  // Convert the mesh into a scene.
+  DRACO_ASSIGN_OR_ASSERT(const std::unique_ptr<draco::Scene> scene_from_mesh,
+                         draco::SceneUtils::MeshToScene(std::move(mesh)));
+  ASSERT_NE(scene_from_mesh, nullptr);
+
+  // Ensure the attribute indices on the meshes from scene are decremented by
+  // one because the material attribute was removed.
+  ASSERT_EQ(scene_from_mesh->NumMeshes(), 4);
+  for (draco::MeshIndex mi(0); mi < scene_from_mesh->NumMeshes(); ++mi) {
+    for (draco::MeshFeaturesIndex mfi(0);
+         mfi < scene_from_mesh->GetMesh(mi).NumMeshFeatures(); ++mfi) {
+      const auto &mf = scene_from_mesh->GetMesh(mi).GetMeshFeatures(mfi);
+      ASSERT_EQ(mf.GetAttributeIndex(), mf_att_id - 1);
+    }
+  }
+}
+
 TEST(SceneUtilsTest, TestMeshToSceneStructuralMetadata) {
   const std::string filename = "cube_att.obj";
   std::unique_ptr<draco::Mesh> mesh = draco::ReadMeshFromTestFile(filename);