From aef4ecada533c40d41ae4e885eaa39bfe74601cb Mon Sep 17 00:00:00 2001 From: Jeremy Cytryn Date: Wed, 6 May 2020 23:41:05 -0700 Subject: [PATCH 1/3] Fail gltf/gltf2 export whenever invalid / incomplete JSON is generated This can happen currently for example if NaNs are introduced in accessor bounds as rapidjson cannot write NaN/inf floats (see subsequent commit for fix there) and will halt writing to buffer at this point. Fix here ensures that whenever anything like this happens we throw an exception so this ends up as a registered export failure case, rather than silently exporting the incomplete JSON --- code/AssetLib/glTF/glTFAssetWriter.inl | 12 ++++++++---- code/AssetLib/glTF2/glTF2AssetWriter.inl | 10 ++++++---- .../BoxWithInfinites.glb | Bin 0 -> 1900 bytes test/unit/utglTF2ImportExport.cpp | 10 ++++++++++ 4 files changed, 24 insertions(+), 8 deletions(-) create mode 100644 test/models/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites.glb diff --git a/code/AssetLib/glTF/glTFAssetWriter.inl b/code/AssetLib/glTF/glTFAssetWriter.inl index 5e4416ee9..d8d2556fa 100644 --- a/code/AssetLib/glTF/glTFAssetWriter.inl +++ b/code/AssetLib/glTF/glTFAssetWriter.inl @@ -59,7 +59,7 @@ namespace glTF { namespace { template - inline + inline Value& MakeValue(Value& val, T(&r)[N], MemoryPoolAllocator<>& al) { val.SetArray(); val.Reserve(N, al); @@ -70,7 +70,7 @@ namespace glTF { } template - inline + inline Value& MakeValue(Value& val, const std::vector & r, MemoryPoolAllocator<>& al) { val.SetArray(); val.Reserve(static_cast(r.size()), al); @@ -530,7 +530,9 @@ namespace glTF { StringBuffer docBuffer; PrettyWriter writer(docBuffer); - mDoc.Accept(writer); + if (!mDoc.Accept(writer)) { + throw DeadlyExportError("Failed to write scene data!"); + } if (jsonOutFile->Write(docBuffer.GetString(), docBuffer.GetSize(), 1) != 1) { throw DeadlyExportError("Failed to write scene data!"); @@ -569,7 +571,9 @@ namespace glTF { StringBuffer docBuffer; Writer writer(docBuffer); - mDoc.Accept(writer); + if (!mDoc.Accept(writer)) { + throw DeadlyExportError("Failed to write scene data!"); + } if (outfile->Write(docBuffer.GetString(), docBuffer.GetSize(), 1) != 1) { throw DeadlyExportError("Failed to write scene data!"); diff --git a/code/AssetLib/glTF2/glTF2AssetWriter.inl b/code/AssetLib/glTF2/glTF2AssetWriter.inl index 798f38c1c..361af40cd 100644 --- a/code/AssetLib/glTF2/glTF2AssetWriter.inl +++ b/code/AssetLib/glTF2/glTF2AssetWriter.inl @@ -613,7 +613,9 @@ namespace glTF2 { StringBuffer docBuffer; PrettyWriter writer(docBuffer); - mDoc.Accept(writer); + if (!mDoc.Accept(writer)) { + throw DeadlyExportError("Failed to write scene data!"); + } if (jsonOutFile->Write(docBuffer.GetString(), docBuffer.GetSize(), 1) != 1) { throw DeadlyExportError("Failed to write scene data!"); @@ -664,7 +666,9 @@ namespace glTF2 { StringBuffer docBuffer; Writer writer(docBuffer); - mDoc.Accept(writer); + if (!mDoc.Accept(writer)) { + throw DeadlyExportError("Failed to write scene data!"); + } uint32_t jsonChunkLength = (docBuffer.GetSize() + 3) & ~3; // Round up to next multiple of 4 auto paddingLength = jsonChunkLength - docBuffer.GetSize(); @@ -816,5 +820,3 @@ namespace glTF2 { } } - - diff --git a/test/models/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites.glb b/test/models/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites.glb new file mode 100644 index 0000000000000000000000000000000000000000..ae83f1f06578eb6b6dc74988dc1c52b9dd6477ab GIT binary patch literal 1900 zcmb7ES#R1v5T^HiU$1DNTFFJWfdqI#BuXSIjug`N0Y#ND3#{Y=txcLJLgb(2FZ3U! z-z;lllC-3l@y?8Azi(#t79$)Z%!wi%v&soOarPtT3xAS5EauEy-YFF}HjC~jHZz#x zx;%+5g!QMxA(!6;|HUe%!TQHBZx4rx8m1cpHQ+(Ke>~x=Qnc(1a!vzeW)}<= zD>wVTvz%L z`vxR`uV@D3MSm8(3LGCzU|ZqQ4-t>+!B7Mu<`Gh%wl#P#ipQ+7X`2@lsj=xsU)SXH zWV=*Cp^hBU+UE=94P8MumSO;M%f9I26)ZJ5jb;^SjZ$d&f6OlAHCIx|4RWE@J298S z@m5;Y;RAGK(w9vhJDw-pPBU?o0lW-#sxZ1_W-_<96jB0tBQjGbmk&13Do+ShM8$ZQZ!`yTJ2r2|$O zB6&_r=JRNb<)mYwm%0fL6(WT%VxTWR`U(xLO=?#wA%cOx$-)|eG{$=Mr?fdx?_RrG z->JN@lC{eN<~$Qqn$@>Y4Mm|IZk{=TC}vT==P?m_NDaILG6?1zv?kjIcGdL;WP z?J1YEwqjF%$hd|9lav;z_TP F{RglTzqJ4W literal 0 HcmV?d00001 diff --git a/test/unit/utglTF2ImportExport.cpp b/test/unit/utglTF2ImportExport.cpp index 55cd2ef6a..99481101e 100644 --- a/test/unit/utglTF2ImportExport.cpp +++ b/test/unit/utglTF2ImportExport.cpp @@ -436,6 +436,16 @@ TEST_F(utglTF2ImportExport, error_string_preserved) { ASSERT_NE(error.find("BoxTextured0.bin"), std::string::npos) << "Error string should contain an error about missing .bin file"; } +TEST_F(utglTF2ImportExport, export_bad_accessor_bounds) { + Assimp::Importer importer; + Assimp::Exporter exporter; + const aiScene* scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites.glb", aiProcess_ValidateDataStructure); + ASSERT_NE(scene, nullptr); + + EXPECT_EQ(aiReturn_FAILURE, exporter.Export(scene, "glb2", ASSIMP_TEST_MODELS_DIR "/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites_out.glb")); + EXPECT_EQ(aiReturn_FAILURE, exporter.Export(scene, "gltf2", ASSIMP_TEST_MODELS_DIR "/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites_out.gltf")); +} + #endif // ASSIMP_BUILD_NO_EXPORT TEST_F(utglTF2ImportExport, sceneMetadata) { From c5a9fbd47f309cf33e2f8fdef03c952b3e212d63 Mon Sep 17 00:00:00 2001 From: Jeremy Cytryn Date: Wed, 6 May 2020 23:59:48 -0700 Subject: [PATCH 2/3] Gracefully handle NaNs and Infs in gltf2 accessor bound computation --- code/AssetLib/glTF2/glTF2Exporter.cpp | 13 ++++++++++--- test/unit/utglTF2ImportExport.cpp | 4 ++-- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/code/AssetLib/glTF2/glTF2Exporter.cpp b/code/AssetLib/glTF2/glTF2Exporter.cpp index da6d9ab2e..7ed4f2670 100644 --- a/code/AssetLib/glTF2/glTF2Exporter.cpp +++ b/code/AssetLib/glTF2/glTF2Exporter.cpp @@ -1,4 +1,4 @@ -/* +/* Open Asset Import Library (assimp) ---------------------------------------------------------------------- @@ -172,6 +172,13 @@ void SetAccessorRange(Ref acc, void* data, size_t count, for (unsigned int j = 0 ; j < numCompsOut ; j++) { double valueTmp = buffer_ptr[j]; + // Gracefully tolerate rogue NaN's in buffer data + // Any NaNs/Infs introduced in accessor bounds will end up in + // document and prevent rapidjson from writing out valid JSON + if (!std::isfinite(valueTmp)) { + continue; + } + if (valueTmp < acc->min[j]) { acc->min[j] = valueTmp; } @@ -348,7 +355,7 @@ void glTF2Exporter::GetMatTex(const aiMaterial* mat, Ref& texture, aiTe if (path[0] == '*') { // embedded aiTexture* curTex = mScene->mTextures[atoi(&path[1])]; - + texture->source->name = curTex->mFilename.C_Str(); // The asset has its own buffer, see Image::SetData @@ -762,7 +769,7 @@ void glTF2Exporter::ExportMeshes() for (int i = 0; i < AI_MAX_NUMBER_OF_TEXTURECOORDS; ++i) { if (!aim->HasTextureCoords(i)) continue; - + // Flip UV y coords if (aim -> mNumUVComponents[i] > 1) { for (unsigned int j = 0; j < aim->mNumVertices; ++j) { diff --git a/test/unit/utglTF2ImportExport.cpp b/test/unit/utglTF2ImportExport.cpp index 99481101e..4ba7c69d4 100644 --- a/test/unit/utglTF2ImportExport.cpp +++ b/test/unit/utglTF2ImportExport.cpp @@ -442,8 +442,8 @@ TEST_F(utglTF2ImportExport, export_bad_accessor_bounds) { const aiScene* scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites.glb", aiProcess_ValidateDataStructure); ASSERT_NE(scene, nullptr); - EXPECT_EQ(aiReturn_FAILURE, exporter.Export(scene, "glb2", ASSIMP_TEST_MODELS_DIR "/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites_out.glb")); - EXPECT_EQ(aiReturn_FAILURE, exporter.Export(scene, "gltf2", ASSIMP_TEST_MODELS_DIR "/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites_out.gltf")); + EXPECT_EQ(aiReturn_SUCCESS, exporter.Export(scene, "glb2", ASSIMP_TEST_MODELS_DIR "/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites_out.glb")); + EXPECT_EQ(aiReturn_SUCCESS, exporter.Export(scene, "gltf2", ASSIMP_TEST_MODELS_DIR "/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites_out.gltf")); } #endif // ASSIMP_BUILD_NO_EXPORT From c3a21666dadd01f7a2f532d3dc0ecaf2045aa81b Mon Sep 17 00:00:00 2001 From: Jeremy Cytryn Date: Thu, 7 May 2020 01:21:54 -0700 Subject: [PATCH 3/3] Make gltf2 export normal normalization safe This avoids introducing NaNs e.g. when the input mesh has 0-length normals --- code/AssetLib/glTF/glTFExporter.cpp | 2 +- code/AssetLib/glTF2/glTF2Exporter.cpp | 2 +- .../BoxBadNormals-glTF-Binary/BoxBadNormals.glb | Bin 0 -> 1924 bytes test/unit/utglTF2ImportExport.cpp | 15 +++++++++++++++ 4 files changed, 17 insertions(+), 2 deletions(-) create mode 100644 test/models/glTF2/BoxBadNormals-glTF-Binary/BoxBadNormals.glb diff --git a/code/AssetLib/glTF/glTFExporter.cpp b/code/AssetLib/glTF/glTFExporter.cpp index e6c14e7dd..b85affc08 100644 --- a/code/AssetLib/glTF/glTFExporter.cpp +++ b/code/AssetLib/glTF/glTFExporter.cpp @@ -348,7 +348,7 @@ void glTFExporter::GetMatColorOrTex(const aiMaterial* mat, glTF::TexProperty& pr if (path[0] == '*') { // embedded aiTexture* curTex = mScene->mTextures[atoi(&path[1])]; - + prop.texture->source->name = curTex->mFilename.C_Str(); uint8_t *data = reinterpret_cast(curTex->pcData); diff --git a/code/AssetLib/glTF2/glTF2Exporter.cpp b/code/AssetLib/glTF2/glTF2Exporter.cpp index 7ed4f2670..566f95e80 100644 --- a/code/AssetLib/glTF2/glTF2Exporter.cpp +++ b/code/AssetLib/glTF2/glTF2Exporter.cpp @@ -758,7 +758,7 @@ void glTF2Exporter::ExportMeshes() // Normalize all normals as the validator can emit a warning otherwise if ( nullptr != aim->mNormals) { for ( auto i = 0u; i < aim->mNumVertices; ++i ) { - aim->mNormals[ i ].Normalize(); + aim->mNormals[ i ].NormalizeSafe(); } } diff --git a/test/models/glTF2/BoxBadNormals-glTF-Binary/BoxBadNormals.glb b/test/models/glTF2/BoxBadNormals-glTF-Binary/BoxBadNormals.glb new file mode 100644 index 0000000000000000000000000000000000000000..c36727d12ce675125a3f48e1ebeb68fd1a3992b2 GIT binary patch literal 1924 zcmb_d+iu!G5OweO`|TC&Q!6>hHX#XqsYsMaRDe@R`w~T!fdwr2g4QNYgpl&kkLb@- z{ee{KC-uy-OH2}}M2gK}E;~E3GdsH;gnK(T4a4|x&oF-8HjGbguQ_3Un(~af6L!c& z8YD4unNzfw$p$>;!p{-`jW^sks=~zLvnYiT5m|ZLZMoLNI5EVC8w1 zB_yn}Y;RTT)z&;ur!_?~<+f0reOrWaPc|6F5%w2b^Gh?cIejhH| zgZyM?V*|_a#ejOEv|3qH$+R;O^bpW?&Psd1mz;cU-pcE!Ox~NlN)3@)TZ!_F{EUmh z4;RYP;yqMl)DaDy`C%AzTgiAZjM0|V=^a1iTS=IR9ltC48!feA$;Y-Uj#Y6griEAT zwQ`_M(X&GP(mH!;G9Cpu2!~wj_Q`>T_4(mw=r8H z#dDboPltRVj|Mc9hzOz}gZJ5BB)y>+I(NAfLEH;)@NjMrG{b&%#uG?6OCwd;*R#O@$Udv#w_WU_L{iSF8pFg2E>RE;-(`D?m z%?ahBo@Mx}*v|ajz!duK-BOffeEy6(z+C{JLrF=#4?F-K0*@3W`7!VWcnUmIl;r2Y V3*aU2N>P$u18;!0z&k}r{s%56wT%D( literal 0 HcmV?d00001 diff --git a/test/unit/utglTF2ImportExport.cpp b/test/unit/utglTF2ImportExport.cpp index 4ba7c69d4..f0f18d503 100644 --- a/test/unit/utglTF2ImportExport.cpp +++ b/test/unit/utglTF2ImportExport.cpp @@ -446,6 +446,21 @@ TEST_F(utglTF2ImportExport, export_bad_accessor_bounds) { EXPECT_EQ(aiReturn_SUCCESS, exporter.Export(scene, "gltf2", ASSIMP_TEST_MODELS_DIR "/glTF2/BoxWithInfinites-glTF-Binary/BoxWithInfinites_out.gltf")); } +TEST_F(utglTF2ImportExport, export_normalized_normals) { + Assimp::Importer importer; + Assimp::Exporter exporter; + const aiScene* scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/glTF2/BoxBadNormals-glTF-Binary/BoxBadNormals.glb", aiProcess_ValidateDataStructure); + ASSERT_NE(scene, nullptr); + EXPECT_EQ(aiReturn_SUCCESS, exporter.Export(scene, "glb2", ASSIMP_TEST_MODELS_DIR "/glTF2/BoxBadNormals-glTF-Binary/BoxBadNormals_out.glb")); + + // load in again and ensure normal-length normals but no Nan's or Inf's introduced + scene = importer.ReadFile(ASSIMP_TEST_MODELS_DIR "/glTF2/BoxBadNormals-glTF-Binary/BoxBadNormals_out.glb", aiProcess_ValidateDataStructure); + for ( auto i = 0u; i < scene->mMeshes[0]->mNumVertices; ++i ) { + const auto length = scene->mMeshes[0]->mNormals[i].Length(); + EXPECT_TRUE(abs(length) < 1e-6 || abs(length - 1) < 1e-6); + } +} + #endif // ASSIMP_BUILD_NO_EXPORT TEST_F(utglTF2ImportExport, sceneMetadata) {