From 7f8746f56e018b82b915993b47f9384e2703b652 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A4r=20Winzell?= Date: Thu, 9 Nov 2017 20:15:00 -0800 Subject: [PATCH 1/5] Minor README tweaks. --- README.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 0a35c71..60e6c98 100644 --- a/README.md +++ b/README.md @@ -124,7 +124,8 @@ As part of this process, you will be asked to choose which generator to use. **At present, only Visual Studio 2017 is supported.** Older versions of the IDE are unlikely to successfully build the tool. -*(MinGW support is plausible. Contributions welcome.)* +*(MinGW support is plausible. The difficulty is linking statically against the +FBX SDK .lib file. Contributions welcome.)* Note that the `CMAKE_BUILD_TYPE` variable from the Unix Makefile system is entirely ignored here; it is when you open the generated solution that @@ -150,7 +151,7 @@ node, and whenever we find any node that's rotated, translated or scaled, we record that fact in the output. Beyond skeleton-based animation, *Blend Shapes* are also supported; they are -read from the FBX file on a per-mesh basis, and animations can them by varying +read from the FBX file on a per-mesh basis, and clips can use them by varying the weights associated with each one. The baking method has the benefit of being simple and precise. It has the @@ -165,7 +166,8 @@ There are three future enhancements we hope to see for animations: it into a long sequence of linear approximations. - We do not yet ever generate [sparse accessors](https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#sparse-accessors), - but many animations would benefit from this storage optimisation. + but many animations (especially morph targets) would benefit from this + storage optimisation. - Perhaps most useful in practice is the idea of compressing animation curves the same way we use Draco to compress meshes (see below). Like geometry, animations are highly redundant — each new value is highly predictable from From e73c2f46e2ecba204a98906a8975d3b48228c9e4 Mon Sep 17 00:00:00 2001 From: Par Winzell Date: Tue, 14 Nov 2017 13:47:34 -0800 Subject: [PATCH 2/5] Only use vertex alpha if color layer is present. This was previously flagging far too many materials as transparent. --- src/Fbx2Raw.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Fbx2Raw.cpp b/src/Fbx2Raw.cpp index 9535037..62463d8 100644 --- a/src/Fbx2Raw.cpp +++ b/src/Fbx2Raw.cpp @@ -772,7 +772,7 @@ static void ReadMesh(RawModel &raw, FbxScene *pScene, FbxNode *pNode, const std: vertex.polarityUv0 = false; // flag this triangle as transparent if any of its corner vertices substantially deviates from fully opaque - vertexTransparency |= (fabs(fbxColor.mAlpha - 1.0) > 1e-3); + vertexTransparency |= colorLayer.LayerPresent() && (fabs(fbxColor.mAlpha - 1.0) > 1e-3); rawSurface.bounds.AddPoint(vertex.position); From 5eb36702b5eb3e99c1217bab6a0dc1338fa430aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A4r=20Winzell?= Date: Tue, 14 Nov 2017 14:36:44 -0800 Subject: [PATCH 3/5] Make blend shape normals/tangents opt-in. (#40) The FBX SDK absolutely claims that there is a normal layer to each FbxShape, with non-trivial data, even when the corresponding FBX file, upon visual inspection, explicitly contains nothing but zeroes. The only conclusion I can draw is that the SDK is computing normals from geometry, without being asked to, which seems kind of sketchy. These computed normals are often not at all what the artist wanted, they take up a lot of space -- often pointlessly, since if they're computed, we could just as well compute them on the client -- and at least in the case of three.js their inclusion uses up many of the precious 8 morph target slots in the shader. So, they are now opt-in, at least until we can solve the mystery of just what goes on under the hood in the SDK. --- README.md | 11 +++++++++++ src/Raw2Gltf.cpp | 8 ++++---- src/Raw2Gltf.h | 4 ++++ src/main.cpp | 20 ++++++++++++++------ 4 files changed, 33 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 60e6c98..aa2ec16 100644 --- a/README.md +++ b/README.md @@ -48,6 +48,10 @@ Usage: --pbr-specular-glossiness (WIP) Experimentally fill in the KHR_materials_pbrSpecularGlossiness extension. + --blend-shape-normals Include blend shape normals, if reported + present by the FBX SDK. + --blend-shape-tangents Include blend shape tangents, if reported + present by the FBX SDK. -k, --keep-attribute arg Used repeatedly to build a limiting set of vertex attributes to keep. -v, --verbose Enable verbose output. @@ -81,6 +85,13 @@ Some of these switches are not obvious: the conversion process. This is a way to trim the size of the resulting glTF if you know the FBX contains superfluous attributes. The supported arguments are `position`, `normal`, `tangent`, `color`, `uv0`, and `uv1`. +- When **blend shapes** are present, you may use `--blend-shape-normals` and + `--blend-shape-tangents` to include normal and tangent attributes in the glTF + morph targets. They are not included by default because they rarely or never + seem to be correctly present in the actual FBX source, which means the SDK + must be computing them from geometry, unasked? In any case, they are beyond + the control of the artist, and can yield strange crinkly behaviour. Since + they also take up significant space in the output file, we made them opt-in. ## Building it on your own diff --git a/src/Raw2Gltf.cpp b/src/Raw2Gltf.cpp index 2c06466..9ed143e 100644 --- a/src/Raw2Gltf.cpp +++ b/src/Raw2Gltf.cpp @@ -619,10 +619,10 @@ ModelData *Raw2Gltf( auto blendVertex = surfaceModel.GetVertex(jj).blends[channelIx]; shapeBounds.AddPoint(blendVertex.position); positions.push_back(blendVertex.position); - if (channel.hasNormals) { + if (options.useBlendShapeTangents && channel.hasNormals) { normals.push_back(blendVertex.normal); } - if (channel.hasTangents) { + if (options.useBlendShapeTangents && channel.hasTangents) { tangents.push_back(blendVertex.tangent); } } @@ -633,14 +633,14 @@ ModelData *Raw2Gltf( pAcc->max = toStdVec(shapeBounds.max); std::shared_ptr nAcc; - if (channel.hasNormals) { + if (!normals.empty()) { nAcc = gltf->AddAccessorWithView( *gltf->GetAlignedBufferView(buffer, BufferViewData::GL_ARRAY_BUFFER), GLT_VEC3F, normals); } std::shared_ptr tAcc; - if (channel.hasTangents) { + if (!tangents.empty()) { nAcc = gltf->AddAccessorWithView( *gltf->GetAlignedBufferView(buffer, BufferViewData::GL_ARRAY_BUFFER), GLT_VEC4F, tangents); diff --git a/src/Raw2Gltf.h b/src/Raw2Gltf.h index 5de9218..8e6ecf5 100644 --- a/src/Raw2Gltf.h +++ b/src/Raw2Gltf.h @@ -53,6 +53,10 @@ struct GltfOptions bool usePBRMetRough; /** Whether to use KHR_materials_pbrSpecularGlossiness to extend material definitions. */ bool usePBRSpecGloss; + /** Whether to include blend shape normals, if present according to the SDK. */ + bool useBlendShapeNormals; + /** Whether to include blend shape tangents, if present according to the SDK. */ + bool useBlendShapeTangents; }; struct ComponentType { diff --git a/src/main.cpp b/src/main.cpp index 0fcd6bc..faa6c2d 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -44,12 +44,14 @@ int main(int argc, char *argv[]) GltfOptions gltfOptions{ -1, // keepAttribs - false, // outputBinary - false, // embedResources - false, // useDraco - false, // useKHRMatCom - false, // usePBRMetRough - false // usePBRSpecGloss + false, // outputBinary + false, // embedResources + false, // useDraco + false, // useKHRMatCom + false, // usePBRMetRough + false, // usePBRSpecGloss + false, // useBlendShapeNormals + false, // useBlendShapeTangents }; options.positional_help("[]"); @@ -81,6 +83,12 @@ int main(int argc, char *argv[]) ( "pbr-specular-glossiness", "(WIP) Experimentally fill in the KHR_materials_pbrSpecularGlossiness extension.", cxxopts::value(gltfOptions.usePBRSpecGloss)) + ( + "blend-shape-normals", "Include blend shape normals, if reported present by the FBX SDK.", + cxxopts::value(gltfOptions.useBlendShapeNormals)) + ( + "blend-shape-tangents", "Include blend shape tangents, if reported present by the FBX SDK.", + cxxopts::value(gltfOptions.useBlendShapeTangents)) ( "k,keep-attribute", "Used repeatedly to build a limiting set of vertex attributes to keep.", cxxopts::value>()) From bcbfdae6bea3522f0a355f1e97765c9d9627bab7 Mon Sep 17 00:00:00 2001 From: Par Winzell Date: Thu, 16 Nov 2017 14:10:12 -0800 Subject: [PATCH 4/5] Drop misleading warning. It's perfectly fine for materials to have neither diffuse texture nor vertex colours. This dates back to a time when the tool had more limited use cases. To compensate: https://github.com/facebookincubator/FBX2glTF/issues/43 --- src/Raw2Gltf.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/Raw2Gltf.cpp b/src/Raw2Gltf.cpp index 9ed143e..45c98e6 100644 --- a/src/Raw2Gltf.cpp +++ b/src/Raw2Gltf.cpp @@ -474,18 +474,11 @@ ModelData *Raw2Gltf( for (size_t surfaceIndex = 0; surfaceIndex < materialModels.size(); surfaceIndex++) { const RawModel &surfaceModel = materialModels[surfaceIndex]; - assert(surfaceModel.GetSurfaceCount() == 1); + assert(surfaceModel.GetSurfaceCount() == 1); const RawSurface &rawSurface = surfaceModel.GetSurface(0); - const std::string surfaceName = std::to_string(surfaceIndex) + "_" + rawSurface.name; const RawMaterial &rawMaterial = surfaceModel.GetMaterial(surfaceModel.GetTriangle(0).materialIndex); - if (rawMaterial.textures[RAW_TEXTURE_USAGE_DIFFUSE] < 0 && - (surfaceModel.GetVertexAttributes() & RAW_VERTEX_ATTRIBUTE_COLOR) == 0) { - if (verboseOutput) { - fmt::printf("Warning: surface %s has neither texture nor vertex colors.\n", surfaceName.c_str()); - } - } const MaterialData &mData = require(materialsByName, materialHash(rawMaterial)); std::string nodeName = rawSurface.nodeName; From c70ded31d51afa117010e889a9d6dc58a015b396 Mon Sep 17 00:00:00 2001 From: Par Winzell Date: Thu, 16 Nov 2017 18:19:52 -0800 Subject: [PATCH 5/5] Explicit versions of dependencies. As we approach 1.0.0 we can't just download whatever someone's slammed into the master branch of our various open source dependencies. --- CMakeLists.txt | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index b883b1e..4f236a7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -26,7 +26,6 @@ endif() # DRACO ExternalProject_Add(Draco - UPDATE_DISCONNECTED TRUE GIT_REPOSITORY https://github.com/google/draco PREFIX draco INSTALL_DIR @@ -44,9 +43,9 @@ endif() set(mathfu_build_benchmarks OFF CACHE BOOL "") set(mathfu_build_tests OFF CACHE BOOL "") ExternalProject_Add(MathFu - UPDATE_DISCONNECTED TRUE - GIT_REPOSITORY https://github.com/google/mathfu PREFIX mathfu + GIT_REPOSITORY https://github.com/google/mathfu + GIT_TAG v1.1.0 CONFIGURE_COMMAND ${CMAKE_COMMAND} -E echo "Skipping MathFu configure step." BUILD_COMMAND ${CMAKE_COMMAND} -E echo "Skipping MathFu build step." INSTALL_COMMAND ${CMAKE_COMMAND} -E echo "Skipping MathFu install step." @@ -57,9 +56,9 @@ set(MATHFU_INCLUDE_DIRS # JSON ExternalProject_Add(Json - UPDATE_DISCONNECTED TRUE - GIT_REPOSITORY https://github.com/nlohmann/json PREFIX json + GIT_REPOSITORY https://github.com/nlohmann/json + GIT_TAG v2.1.1 CONFIGURE_COMMAND ${CMAKE_COMMAND} -E echo "Skipping JSON configure step." BUILD_COMMAND ${CMAKE_COMMAND} -E echo "Skipping JSON build step." INSTALL_COMMAND ${CMAKE_COMMAND} -E echo "Skipping JSON install step." @@ -68,9 +67,8 @@ set(JSON_INCLUDE_DIR "${CMAKE_BINARY_DIR}/json/src/Json/src") # cppcodec ExternalProject_Add(CPPCodec - UPDATE_DISCONNECTED TRUE - GIT_REPOSITORY https://github.com/tplgy/cppcodec PREFIX cppcodec + GIT_REPOSITORY https://github.com/tplgy/cppcodec CONFIGURE_COMMAND ${CMAKE_COMMAND} -E echo "Skipping CPPCodec configure step." BUILD_COMMAND ${CMAKE_COMMAND} -E echo "Skipping CPPCodec build step." INSTALL_COMMAND ${CMAKE_COMMAND} -E echo "Skipping CPPCodec install step." @@ -79,8 +77,8 @@ set(CPPCODEC_INCLUDE_DIR "${CMAKE_BINARY_DIR}/cppcodec/src/CPPCodec") # CXXOPTS ExternalProject_Add(CxxOpts - UPDATE_DISCONNECTED TRUE GIT_REPOSITORY https://github.com/jarro2783/cxxopts + GIT_TAG v1.4.4 PREFIX cxxopts CONFIGURE_COMMAND ${CMAKE_COMMAND} -E echo "Skipping cxxopts configure step." BUILD_COMMAND ${CMAKE_COMMAND} -E echo "Skipping cxxopts build step." @@ -90,12 +88,12 @@ set(CXXOPTS_INCLUDE_DIR "${CMAKE_BINARY_DIR}/cxxopts/src/CxxOpts/include") # FMT ExternalProject_Add(Fmt - UPDATE_DISCONNECTED TRUE + PREFIX fmt GIT_REPOSITORY https://github.com/fmtlib/fmt + GIT_TAG 4.0.0 CMAKE_CACHE_ARGS "-DFMT_DOC:BOOL=OFF" "-DFMT_INSTALL:BOOL=ON" "-DFMT_TEST:BOOL=OFF" CMAKE_ARGS -DCMAKE_INSTALL_PREFIX= - PREFIX fmt ) set(FMT_INCLUDE_DIR "${CMAKE_BINARY_DIR}/fmt/include") if (WIN32)