Serious glTF importer bugs

[mosra]

@Squareys @NussknackerXXL

I'm quite disappointed by state of the glTF importer code, so the tone of the following might not be all 💮 and 🌞. Sorry in advance.

As reported by https://twitter.com/steeve/status/997138966754230272, the glTF importer plugin at its current state doesn't seem to work at all for any model in https://github.com/KhronosGroup/glTF-Sample-Models. I wonder what it was tested on?

I managed to fix it to display at least the mesh data correctly for Box and and AnimatedBox (patch below, not commited anywhere as it needs corresponding tests), but it still fails really bad on all other models. For the Cesium Milk Truck only the wheels are displayed and with weird texture coordinates, for example. Here's a patch that fixes at least something:

diff --git a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp
index 690ddc0..cf8f7b2 100644
--- a/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp
+++ b/src/MagnumPlugins/TinyGltfImporter/TinyGltfImporter.cpp
@@ -317,9 +317,9 @@ Containers::Optional<MeshData3D> TinyGltfImporter::doMesh3D(const UnsignedInt id
                 return Containers::NullOpt;
             }
 
-            const size_t numPositions = bufferView.byteLength/sizeof(Vector3);
+            const size_t numPositions = accessor.count;
             positions.reserve(numPositions);
-            std::copy_n(reinterpret_cast<const Vector3*>(buffer.data.data() + bufferView.byteOffset), numPositions, std::back_inserter(positions));
+            std::copy_n(reinterpret_cast<const Vector3*>(buffer.data.data() + bufferView.byteOffset + accessor.byteOffset), numPositions, std::back_inserter(positions));
 
         } else if(attribute.first == "NORMAL") {
             if(accessor.type != TINYGLTF_TYPE_VEC3) {
@@ -327,9 +327,9 @@ Containers::Optional<MeshData3D> TinyGltfImporter::doMesh3D(const UnsignedInt id
                 return Containers::NullOpt;
             }
 
-            const size_t numNormals = bufferView.byteLength/sizeof(Vector3);
+            const size_t numNormals = accessor.count;
             normals.reserve(numNormals);
-            std::copy_n(reinterpret_cast<const Vector3*>(buffer.data.data() + bufferView.byteOffset), numNormals, std::back_inserter(normals));
+            std::copy_n(reinterpret_cast<const Vector3*>(buffer.data.data() + bufferView.byteOffset + accessor.byteOffset), numNormals, std::back_inserter(normals));
 
         /* Texture coordinate attribute ends with _0, _1 ... */
         } else if(Utility::String::beginsWith(attribute.first, "TEXCOORD")) {
@@ -341,9 +341,9 @@ Containers::Optional<MeshData3D> TinyGltfImporter::doMesh3D(const UnsignedInt id
             textureLayers.emplace_back();
             std::vector<Vector2>& textureCoordinates = textureLayers.back();
 
-            const size_t numTextureCoordinates = bufferView.byteLength/sizeof(Vector2);
+            const size_t numTextureCoordinates = accessor.count;
             textureCoordinates.reserve(numTextureCoordinates);
-            std::copy_n(reinterpret_cast<const Vector2*>(buffer.data.data() + bufferView.byteOffset), numTextureCoordinates, std::back_inserter(textureCoordinates));
+            std::copy_n(reinterpret_cast<const Vector2*>(buffer.data.data() + bufferView.byteOffset + accessor.byteOffset), numTextureCoordinates, std::back_inserter(textureCoordinates));
 
         /* Color attribute ends with _0, _1 ... */
         } else if(Utility::String::beginsWith(attribute.first, "COLOR")) {
@@ -383,13 +383,13 @@ Containers::Optional<MeshData3D> TinyGltfImporter::doMesh3D(const UnsignedInt id
     }
 
     std::vector<UnsignedInt> indices;
-    const UnsignedByte* start = idxBuffer.data.data() + idxBufferView.byteOffset;
+    const UnsignedByte* start = idxBuffer.data.data() + idxBufferView.byteOffset + idxAccessor.byteOffset;
     if(idxAccessor.componentType == TINYGLTF_COMPONENT_TYPE_UNSIGNED_BYTE) {
-        std::copy_n(start, idxBufferView.byteLength, std::back_inserter(indices));
+        std::copy_n(start, idxAccessor.count, std::back_inserter(indices));
     } else if(idxAccessor.componentType == TINYGLTF_COMPONENT_TYPE_UNSIGNED_SHORT) {
-        std::copy_n(reinterpret_cast<const UnsignedShort*>(start), idxBufferView.byteLength/sizeof(UnsignedShort), std::back_inserter(indices));
+        std::copy_n(reinterpret_cast<const UnsignedShort*>(start), idxAccessor.count, std::back_inserter(indices));
     } else if(idxAccessor.componentType == TINYGLTF_COMPONENT_TYPE_UNSIGNED_INT) {
-        std::copy_n(reinterpret_cast<const UnsignedInt*>(start), idxBufferView.byteLength/sizeof(UnsignedInt), std::back_inserter(indices));
+        std::copy_n(reinterpret_cast<const UnsignedInt*>(start), idxAccessor.count, std::back_inserter(indices));
     } else CORRADE_ASSERT_UNREACHABLE(); /* LCOV_EXCL_LINE */
 
     return MeshData3D(meshPrimitive, std::move(indices), {std::move(positions)}, {std::move(normals)}, textureLayers, colorLayers, &mesh);

So, what's needed:

• look at this tutorial and understand how buffers and views and accessors work. The code had it totally wrong and the test data did not cover any of that at all.
• As far as I know, the official glTF repositories provide test data for all the corner cases. Use them, please.
• Only the 8-bit indices are tested, never the 16-bit or 32-bit indices (and they have the same problem re views and accessors). These need to be fixed and tested too, the patch does that a bit, but probably not fully.
• Assimp is able to import at least some material properties (color and roughness from PBR materials), so maybe a similar thing could be done here (until we have PBR import). Otherwise we get more reports about everything being plain gray.
• In Tinygltf importer fixes #41 I asked for testing and fixing proper handling of file opening failure. It was not done. Opening a nonexistent file still blows up somewhere later in the code. (I admit I should have triple checked before merging, sorry.)

This is quite important, as the latest release was marketed as having glTF support. This is far from that. I hope you have time for this, otherwise I need to drop whatever I am doing now and take over this plugin, as a lot of things will rely on it in the near future.

[steeve]

We did a very dirty workaround because apparently webgl textures have top-left origins, while opengl has bottom-left, so, to fix the texture coordinates:

            for (auto &tc : textureCoordinates) {
                tc = Vector2(tc.x(), -tc.y());
            }

[Squareys]

@mosra Outch, sorry about that! The code in question was my fault, it came in with the recent fixes. I remember we originally had the mesh loading right (as in using the accessors correctly), so I broke that with those changes. I should not have relied on the test cases alone (did admittedly not actually try loading a mesh and looking at that with the viewer).

I have time over the weekend and will fix this (There is a WebXR jam kinda thing coming up anyway for which I'll need it, so I need this fixed myself, too). You have better things to do 😉

[mosra]

@Squareys thanks!

Not sure about the texture coordinates, though: in my experience there was no difference in WebGL compared to desktop GL (that would cause all text-related webgl examples to look flipped, I think, but they aren't). We're doing some image flipping on import to match GL, so maybe there's some issue with it (being flipped twice or not at all).

[steeve]

I think I've read that on KhronosGroup/glTF#674
Try the TextureCoordinates glTF sample to make sure.

[steeve]

While you're at it @Squareys, can you check for this?

Trade::TinyGltfImporter::mesh3D(): unsupported mesh vertex attribute TANGENT

A lot of meshes found on the web use that, for the record.

[Squareys]

@steeve Sure, I put that there, what about it? Magnum does not support tangents, so it ignores them and produces this warning. That should not be an issue, I hope... although it would be nice to be able to import those, of course.

(@mosra Should I add support for tangents in MeshData3D while I'm at it? Skin weights also?)

[mosra]

For the record: I'm now working on redesigning MeshData to support tangents, vertex weights, custom attributes and other things. It will materialize as part of mosra/magnum#240.

[steeve]

hey guys, where are we with this ?
cheers !

[steeve]

my bad i didn't see the PR ! that's great !

[mosra]

@steeve I did some investigation regarding texture coordinate Y flipping today in order to deconfuse myself.

The theory:

• OpenGL conventions are that the first data row is at Y coordinate 0, which is at the bottom.

• WebGL spec says the following for texImage2D() and texSubImage2D() that takes TexImageSource as parameter:

  The first pixel transferred from the source to the WebGL implementation corresponds to the upper left corner of the source. This behavior is modified by the the UNPACK_FLIP_Y_WEBGL pixel storage parameter.

This seeming difference (which is apparently only in case of TexImageSource, to my understanding) leads to extremely long threads full of extremely confused comments on many Khronos glTF repos.

The practice:

• Both OpenGL and WebGL behave exactly the same in my testing (and it also always behaved like that). If I enable UNPACK_FLIP_Y_WEBGL, then I get flipped images on web compared to desktop. Apparently the spec above is just for TexImageSource and since Emscripten is passing a raw ArrayBuffer there, the convention is upper left as in OpenGL and so I didn't ever experience any differences.
• In order to accomodate for OpenGL (and WebGL) conventions, I'm flipping all images on load.
• glTF, for some unknown reason, chose to go against the common OpenGL conventions and specifies the origin as upper left. All tools now accomodate for that and flip Y texture coordinate. As far as I understood from the extremely long threads, this changed in summer 2017 (glTF 2.0 was meant to follow GL specs, but then they reverted back to what 1.0 had to avoid breaking existing implementations and models and ... or some such thing?)
• Just to verify everything again for the 100th time, I tried to export a simple textured model from Blender as both OBJ and glTF. OBJ matches the expectations and origin is bottom left. glTF has the coordinates flipped.

So the glTF importer will be flipping Y (the same as you do) in order to have the same behavior as everything else in the engine. And that'll be part of #43 as well.

[mosra]

#43 is merged now, with a bunch of things on top like trivial import of multi-primitive meshes and mesh name import. I'm now in the middle of implementing real support for multi-primitive meshes, current status is this:

After that I'll focus on additional mesh and material properties.

[mosra]

For the record, two years and one apocalypse later, with mosra/magnum#371 and 1cc6b4e TinyGltfImporter supports tangent attributes as well as a ton of other stuff.