diff --git a/src/draco/core/macros.h b/src/draco/core/macros.h index a31e7c44..f6eeda0a 100644 --- a/src/draco/core/macros.h +++ b/src/draco/core/macros.h @@ -34,12 +34,6 @@ #include 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)); diff --git a/src/draco/io/gltf_decoder.cc b/src/draco/io/gltf_decoder.cc index 690500eb..ebfd3ec8 100644 --- a/src/draco/io/gltf_decoder.cc +++ 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(); } diff --git a/src/draco/io/gltf_decoder_test.cc b/src/draco/io/gltf_decoder_test.cc index 31648be6..6efc0657 100644 --- a/src/draco/io/gltf_decoder_test.cc +++ b/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. diff --git a/src/draco/io/gltf_encoder.cc b/src/draco/io/gltf_encoder.cc index cc7a1956..0dfff991 100644 --- a/src/draco/io/gltf_encoder.cc +++ b/src/draco/io/gltf_encoder.cc @@ -387,10 +387,8 @@ class GltfAsset { std::unordered_map *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(*att, mesh.num_points(), num_encoded_points, + mesh.IsCompressionEnabled()); + } if (att->data_type() == DT_FLOAT32) { return AddAttribute(*att, mesh.num_points(), num_encoded_points, mesh.IsCompressionEnabled()); @@ -1460,12 +1457,10 @@ std::vector> 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) { diff --git a/src/draco/io/obj_encoder.cc b/src/draco/io/obj_encoder.cc index 1ddfd92b..70156fc4 100644 --- a/src/draco/io/obj_encoder.cc +++ b/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_)); } diff --git a/src/draco/io/stl_encoder_test.cc b/src/draco/io/stl_encoder_test.cc index da6298d6..adca5541 100644 --- a/src/draco/io/stl_encoder_test.cc +++ b/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()); + } } } diff --git a/src/draco/mesh/mesh_splitter.cc b/src/draco/mesh/mesh_splitter.cc index 4ef16b05..ad987045 100644 --- a/src/draco/mesh/mesh_splitter.cc +++ b/src/draco/mesh/mesh_splitter.cc @@ -36,6 +36,7 @@ class MeshSplitterInternal { struct WorkData : public MeshSplitter::WorkData { // TriangleSoupMeshBuilder or PointCloudBuilder. std::vector builders; + std::vector *att_id_map; }; // Computes number of elements (faces or points) for each sub-mesh. @@ -112,7 +113,8 @@ StatusOr 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::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::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::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::FinalizeMeshes( // Create a copy of source mesh features. std::unique_ptr 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::SplitMeshToComponents( typename MeshSplitterInternal::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::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 diff --git a/src/draco/mesh/mesh_splitter.h b/src/draco/mesh/mesh_splitter.h index 948081c0..d69d8b84 100644 --- a/src/draco/mesh/mesh_splitter.h +++ b/src/draco/mesh/mesh_splitter.h @@ -98,10 +98,13 @@ class MeshSplitter { StatusOr 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 att_id_map; std::vector 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 att_id_map_; + template friend class MeshSplitterInternal; }; diff --git a/src/draco/scene/scene_utils.cc b/src/draco/scene/scene_utils.cc index 57c41cc1..e43753e7 100644 --- a/src/draco/scene/scene_utils.cc +++ b/src/draco/scene/scene_utils.cc @@ -317,6 +317,17 @@ StatusOr> 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); diff --git a/src/draco/scene/scene_utils_test.cc b/src/draco/scene/scene_utils_test.cc index 7d39cac2..c3ead1db 100644 --- a/src/draco/scene/scene_utils_test.cc +++ b/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 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 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 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 mesh = draco::ReadMeshFromTestFile(filename); diff --git a/testdata/ZeroLengthBufferView/ZeroLengthBufferView.bin b/testdata/ZeroLengthBufferView/ZeroLengthBufferView.bin new file mode 100644 index 00000000..9d268a99 Binary files /dev/null and b/testdata/ZeroLengthBufferView/ZeroLengthBufferView.bin differ diff --git a/testdata/ZeroLengthBufferView/ZeroLengthBufferView.gltf b/testdata/ZeroLengthBufferView/ZeroLengthBufferView.gltf new file mode 100644 index 00000000..5f5fda60 --- /dev/null +++ b/testdata/ZeroLengthBufferView/ZeroLengthBufferView.gltf @@ -0,0 +1,130 @@ +{ + "asset": { + "generator": "by_hand", + "version": "2.0" + }, + "scenes": [ + { + "nodes": [ + 0 + ] + } + ], + "scene": 0, + "nodes": [ + { + "mesh": 0 + } + ], + "meshes": [ + { + "primitives": [ + { + "attributes": { + "POSITION": 1 + }, + "indices": 0, + "mode": 4, + "material": 0 + } + ] + } + ], + "materials": [ + { + "pbrMetallicRoughness": { + "baseColorFactor": [ + 1, + 1, + 1, + 1 + ], + "metallicFactor": 0 + } + } + ], + "accessors": [ + { + "bufferView": 0, + "componentType": 5121, + "count": 6, + "type": "SCALAR" + }, + { + "bufferView": 1, + "componentType": 5126, + "count": 4, + "max": [ + 1, + 1, + 0 + ], + "min": [ + 0, + 0, + 0 + ], + "type": "VEC3" + } + ], + "extensions": { + "EXT_structural_metadata": { + "schema": { + "classes": { + "class": { + "properties": { + "entity_id": { + "type": "STRING" + } + } + } + } + }, + "propertyTables": [ + { + "class": "class", + "count": 1, + "properties": { + "entity_id": { + "values": 2, + "stringOffsetType": "UINT8", + "stringOffsets": 3 + } + } + } + ], + "propertyAttributes": [] + } + }, + "bufferViews": [ + { + "buffer": 0, + "byteOffset": 0, + "byteLength": 8 + }, + { + "buffer": 0, + "byteOffset": 8, + "byteLength": 48 + }, + { + "buffer": 0, + "byteOffset": 56, + "byteLength": 0 + }, + { + "buffer": 0, + "byteOffset": 56, + "byteLength": 4 + } + ], + "buffers": [ + { + "byteLength": 60, + "uri": "ZeroLengthBufferView.bin" + } + ], + "extensionsUsed": [ + "EXT_structural_metadata" + ] +}