Merge pull request #154 from AnalyticalGraphicsInc/fix-uv-error-1.0

Remove faces that don't match the same attribute layout (1.0)

CHANGES.md

@@ -1,6 +1,10 @@
 Change Log
 ==========
 
+### 1.3.3 2018-??-??
+
+* Fixed handling of objs with mismatching attribute layouts. [#154](https://github.com/AnalyticalGraphicsInc/obj2gltf/pull/154)
+
 ### 1.3.2 2018-06-07
 * Fixed greyscale images loading as alpha instead of luminance. [#144](https://github.com/AnalyticalGraphicsInc/obj2gltf/pull/144)
 

lib/createGltf.js

@@ -218,13 +218,17 @@ function createGltf(objData, options) {
             var meshes = nodes[i].meshes;
             var meshesLength = meshes.length;
             for (var j = 0; j < meshesLength; ++j) {
+                var primitives = meshes[j].primitives;
+                var primitivesLength = primitives.length;
+                for (var k = 0; k < primitivesLength; ++k) {
                     // Reserve the 65535 index for primitive restart
-                var vertexCount = meshes[j].positions.length / 3;
+                    var vertexCount = primitives[k].positions.length / 3;
                     if (vertexCount > 65534) {
                         return true;
                     }
                 }
             }
+        }
         return false;
     }
 
@@ -250,26 +254,6 @@ function createGltf(objData, options) {
             var meshId = mesh.name;
             gltfNodeMeshes.push(meshId);
 
-            var hasPositions = mesh.positions.length > 0;
-            var hasNormals = mesh.normals.length > 0;
-            var hasUVs = mesh.uvs.length > 0;
-
-            var attributes = {};
-            if (hasPositions) {
-                attributes.POSITION = addVertexAttribute(mesh.positions, 3);
-            }
-            if (hasNormals) {
-                attributes.NORMAL = addVertexAttribute(mesh.normals, 3);
-            }
-            if (hasUVs) {
-                attributes.TEXCOORD_0 = addVertexAttribute(mesh.uvs, 2);
-            }
-
-            // Unload resources
-            mesh.positions = undefined;
-            mesh.normals = undefined;
-            mesh.uvs = undefined;
-
             var gltfMeshPrimitives = [];
             gltf.meshes[meshId] = {
                 name : meshId,
@@ -281,10 +265,31 @@ function createGltf(objData, options) {
             var primitivesLength = primitives.length;
             for (var k = 0; k < primitivesLength; ++k) {
                 var primitive = primitives[k];
+
+                var hasPositions = primitive.positions.length > 0;
+                var hasNormals = primitive.normals.length > 0;
+                var hasUVs = primitive.uvs.length > 0;
+
+                var attributes = {};
+                if (hasPositions) {
+                    attributes.POSITION = addVertexAttribute(primitive.positions, 3);
+                }
+                if (hasNormals) {
+                    attributes.NORMAL = addVertexAttribute(primitive.normals, 3);
+                }
+                if (hasUVs) {
+                    attributes.TEXCOORD_0 = addVertexAttribute(primitive.uvs, 2);
+                }
+
                 var indexAccessorId = addIndexArray(primitive.indices, uint32Indices);
-                primitive.indices = undefined; // Unload resources
                 var materialId = primitive.material;
 
+                // Unload resources
+                primitive.positions = undefined;
+                primitive.normals = undefined;
+                primitive.uvs = undefined;
+                primitive.indices = undefined;
+
                 if (!defined(materialId)) {
                     // Create a default material if the primitive does not specify one
                     materialId = 'default';

lib/loadObj.js

@@ -38,14 +38,14 @@ function Node() {
 function Mesh() {
     this.name = undefined;
     this.primitives = [];
-    this.positions = new ArrayStorage(ComponentDatatype.FLOAT);
-    this.normals = new ArrayStorage(ComponentDatatype.FLOAT);
-    this.uvs = new ArrayStorage(ComponentDatatype.FLOAT);
 }
 
 function Primitive() {
     this.material = undefined;
     this.indices = new ArrayStorage(ComponentDatatype.UNSIGNED_INT);
+    this.positions = new ArrayStorage(ComponentDatatype.FLOAT);
+    this.normals = new ArrayStorage(ComponentDatatype.FLOAT);
+    this.uvs = new ArrayStorage(ComponentDatatype.FLOAT);
 }
 
 // OBJ regex patterns are modified from ThreeJS (https://github.com/mrdoob/three.js/blob/master/examples/js/loaders/OBJLoader.js)
@@ -87,7 +87,7 @@ function loadObj(objPath, options) {
     // All nodes seen in the obj
     var nodes = [];
 
-    // Used to build the indices. The vertex cache is unique to each mesh.
+    // Used to build the indices. The vertex cache is unique to each primitive.
     var vertexCache = {};
     var vertexCacheLimit = 1000000;
     var vertexCacheCount = 0;
@@ -123,16 +123,16 @@ function loadObj(objPath, options) {
         mesh.name = getName(name);
         node.meshes.push(mesh);
         addPrimitive();
-
-        // Clear the vertex cache for each new mesh
-        vertexCache = {};
-        vertexCacheCount = 0;
-        vertexCount = 0;
     }
 
     function addPrimitive() {
         primitive = new Primitive();
         mesh.primitives.push(primitive);
+
+        // Clear the vertex cache for each new primitive
+        vertexCache = {};
+        vertexCacheCount = 0;
+        vertexCount = 0;
     }
 
     function useMaterial(name) {
@@ -167,9 +167,9 @@ function loadObj(objPath, options) {
             var px = positions.get(pi + 0);
             var py = positions.get(pi + 1);
             var pz = positions.get(pi + 2);
-            mesh.positions.push(px);
+            primitive.positions.push(px);
-            mesh.positions.push(py);
+            primitive.positions.push(py);
-            mesh.positions.push(pz);
+            primitive.positions.push(pz);
         }
 
         // Normals
@@ -178,9 +178,9 @@ function loadObj(objPath, options) {
             var nx = normals.get(ni + 0);
             var ny = normals.get(ni + 1);
             var nz = normals.get(ni + 2);
-            mesh.normals.push(nx);
+            primitive.normals.push(nx);
-            mesh.normals.push(ny);
+            primitive.normals.push(ny);
-            mesh.normals.push(nz);
+            primitive.normals.push(nz);
         }
 
         // UVs
@@ -188,8 +188,8 @@ function loadObj(objPath, options) {
             var ui = getOffset(u, uvs, 2);
             var ux = uvs.get(ui + 0);
             var uy = uvs.get(ui + 1);
-            mesh.uvs.push(ux);
+            primitive.uvs.push(ux);
-            mesh.uvs.push(uy);
+            primitive.uvs.push(uy);
         }
     }
 
@@ -382,6 +382,16 @@ function loadObj(objPath, options) {
         var isWindingCorrect = true;
         var faceNormal;
 
+        var firstFace = primitive.indices.length === 0;
+        var faceHasUvs = uvs[0].length > 0;
+        var faceHasNormals = normals[0].length > 0;
+        var primitiveHasUvs = primitive.uvs.length > 0;
+        var primitiveHasNormals = primitive.normals.length > 0;
+        if (!firstFace && (faceHasUvs !== primitiveHasUvs || faceHasNormals !== primitiveHasNormals)) {
+            // Discard faces that don't use the same attributes
+            return;
+        }
+
         // If normals are defined, find a face normal to use in winding order sanitization.
         // If no face normal, we have to assume the winding is correct.
         if (normals[0].length > 0) {
@@ -614,10 +624,10 @@ function removeEmptyMeshes(meshes) {
     return meshes.filter(function(mesh) {
         // Remove empty primitives
         mesh.primitives = mesh.primitives.filter(function(primitive) {
-            return primitive.indices.length > 0;
+            return primitive.indices.length > 0 && primitive.positions.length > 0;
         });
-        // Valid meshes must have at least one primitive and contain positions
+        // Valid meshes must have at least one primitive
-        return (mesh.primitives.length > 0) && (mesh.positions.length > 0);
+        return (mesh.primitives.length > 0);
     });
 }
 

specs/data/box-mixed-attributes/box-mixed-attributes.mtl

@@ -0,0 +1,12 @@
+# Blender MTL File: 'None'
+# Material Count: 1
+
+newmtl Material
+Ns 96.078431
+Ka 0.100000 0.000000 0.000000
+Kd 0.640000 0.640000 0.640000
+Ks 0.500000 0.500000 0.500000
+Ke 0.000000 0.000000 0.100000
+Ni 1.000000
+d 1.000000
+illum 2

specs/data/box-mixed-attributes/box-mixed-attributes.obj

@@ -0,0 +1,46 @@
+# Blender v2.78 (sub 0) OBJ File: ''
+# www.blender.org
+mtllib box-mixed-attributes.mtl
+o Cube
+v -1.000000 -1.000000 1.000000
+v -1.000000 1.000000 1.000000
+v -1.000000 -1.000000 -1.000000
+v -1.000000 1.000000 -1.000000
+v 1.000000 -1.000000 1.000000
+v 1.000000 1.000000 1.000000
+v 1.000000 -1.000000 -1.000000
+v 1.000000 1.000000 -1.000000
+vt 0.0000 0.0000
+vt 1.0000 0.0000
+vt 1.0000 1.0000
+vt 0.0000 1.0000
+vt 0.0000 0.0000
+vt 1.0000 0.0000
+vt 1.0000 1.0000
+vt 0.0000 1.0000
+vt 0.0000 0.0000
+vt 1.0000 0.0000
+vt 1.0000 1.0000
+vt 0.0000 1.0000
+vt 0.0000 0.0000
+vt 1.0000 0.0000
+vt 1.0000 1.0000
+vt 0.0000 1.0000
+vt 1.0000 0.0000
+vt 1.0000 1.0000
+vt 0.0000 0.0000
+vt 0.0000 1.0000
+vn -1.0000 0.0000 0.0000
+vn 0.0000 0.0000 -1.0000
+vn 1.0000 0.0000 0.0000
+vn 0.0000 0.0000 1.0000
+vn 0.0000 -1.0000 0.0000
+vn 0.0000 1.0000 0.0000
+usemtl Material
+s off
+f 1/1/1 2/2/1 4/3/1 3/4/1
+f 3 4 8 7
+f 7/9/3 8/10/3 6/11/3 5/12/3
+f 5/13/4 6/14/4 2/15/4 1/16/4
+f 3//5 7//5 5//5 1//5
+f 8/19 4/6 2/15 6/20

specs/lib/createGltfSpec.js

@@ -206,7 +206,7 @@ describe('createGltf', function() {
     });
 
     it('sets constant material when there are no normals', function() {
-        boxObjData.nodes[0].meshes[0].normals.length = 0;
+        boxObjData.nodes[0].meshes[0].primitives[0].normals.length = 0;
 
         var material = new Material();
         material.diffuseTexture = diffuseTextureUrl;
@@ -255,7 +255,7 @@ describe('createGltf', function() {
     it('handles material used with and without normals (1)', function() {
         // Two meshes - one with normals, and one without
         boxObjData.nodes.push(duplicateBoxObjData.nodes[0]);
-        boxObjData.nodes[1].meshes[0].normals.length = 0;
+        boxObjData.nodes[1].meshes[0].primitives[0].normals.length = 0;
 
         var gltf = createGltf(boxObjData, defaultOptions);
         var kmc1 = gltf.materials.Material.extensions.KHR_materials_common;
@@ -268,7 +268,7 @@ describe('createGltf', function() {
     it('handles material used with and without normals (2)', function() {
         // Now test in a different order
         boxObjData.nodes.push(duplicateBoxObjData.nodes[0]);
-        boxObjData.nodes[0].meshes[0].normals.length = 0;
+        boxObjData.nodes[0].meshes[0].primitives[0].normals.length = 0;
 
         var gltf = createGltf(boxObjData, defaultOptions);
         var kmc1 = gltf.materials.Material.extensions.KHR_materials_common;
@@ -279,7 +279,7 @@ describe('createGltf', function() {
     });
 
     it('runs without normals', function() {
-        boxObjData.nodes[0].meshes[0].normals.length = 0;
+        boxObjData.nodes[0].meshes[0].primitives[0].normals.length = 0;
 
         var gltf = createGltf(boxObjData, defaultOptions);
         var attributes = gltf.meshes[Object.keys(gltf.meshes)[0]].primitives[0].attributes;
@@ -289,7 +289,7 @@ describe('createGltf', function() {
     });
 
     it('runs without uvs', function() {
-        boxObjData.nodes[0].meshes[0].uvs.length = 0;
+        boxObjData.nodes[0].meshes[0].primitives[0].uvs.length = 0;
 
         var gltf = createGltf(boxObjData, defaultOptions);
         var attributes = gltf.meshes[Object.keys(gltf.meshes)[0]].primitives[0].attributes;
@@ -299,8 +299,8 @@ describe('createGltf', function() {
     });
 
     it('runs without uvs and normals', function() {
-        boxObjData.nodes[0].meshes[0].normals.length = 0;
+        boxObjData.nodes[0].meshes[0].primitives[0].normals.length = 0;
-        boxObjData.nodes[0].meshes[0].uvs.length = 0;
+        boxObjData.nodes[0].meshes[0].primitives[0].uvs.length = 0;
 
         var gltf = createGltf(boxObjData, defaultOptions);
         var attributes = gltf.meshes[Object.keys(gltf.meshes)[0]].primitives[0].attributes;
@@ -310,11 +310,11 @@ describe('createGltf', function() {
     });
 
     function expandObjData(objData, duplicatesLength) {
-        var mesh = objData.nodes[0].meshes[0];
+        var primitive = objData.nodes[0].meshes[0].primitives[0];
-        var indices = mesh.primitives[0].indices;
+        var indices = primitive.indices;
-        var positions = mesh.positions;
+        var positions = primitive.positions;
-        var normals = mesh.normals;
+        var normals = primitive.normals;
-        var uvs = mesh.uvs;
+        var uvs = primitive.uvs;
 
         var indicesLength = indices.length;
         var vertexCount = positions.length / 3;
@@ -338,18 +338,18 @@ describe('createGltf', function() {
 
     it('detects need to use uint32 indices', function() {
         expandObjData(boxObjData, 2731); // Right above 65536 limit
-        var mesh = boxObjData.nodes[0].meshes[0];
+        var primitive = boxObjData.nodes[0].meshes[0].primitives[0];
-        var indicesLength = mesh.primitives[0].indices.length;
+        var indicesLength = primitive.indices.length;
-        var vertexCount = mesh.positions.length / 3;
+        var vertexCount = primitive.positions.length / 3;
 
         var gltf = createGltf(boxObjData, defaultOptions);
-        var primitive = gltf.meshes[Object.keys(gltf.meshes)[0]].primitives[0];
+        var gltfPrimitive = gltf.meshes[Object.keys(gltf.meshes)[0]].primitives[0];
-        var indicesAccessor = gltf.accessors[primitive.indices];
+        var indicesAccessor = gltf.accessors[gltfPrimitive.indices];
         expect(indicesAccessor.count).toBe(indicesLength);
         expect(indicesAccessor.max[0]).toBe(vertexCount - 1);
         expect(indicesAccessor.componentType).toBe(WebGLConstants.UNSIGNED_INT);
 
-        var positionAccessor = gltf.accessors[primitive.attributes.POSITION];
+        var positionAccessor = gltf.accessors[gltfPrimitive.attributes.POSITION];
         expect(positionAccessor.count).toBe(vertexCount);
     });
 

specs/lib/loadObjSpec.js

@@ -32,6 +32,7 @@ var objMissingTextureUrl = 'specs/data/box-missing-texture/box-missing-texture.o
 var objSubdirectoriesUrl = 'specs/data/box-subdirectories/box-textured.obj';
 var objWindowsPathsUrl = 'specs/data/box-windows-paths/box-windows-paths.obj';
 var objComplexMaterialUrl = 'specs/data/box-complex-material/box-complex-material.obj';
+var objMixedAttributesUrl = 'specs/data/box-mixed-attributes/box-mixed-attributes.obj';
 var objInvalidContentsUrl = 'specs/data/box/box.mtl';
 var objInvalidUrl = 'invalid.obj';
 
@@ -91,9 +92,9 @@ describe('loadObj', function() {
 
                 expect(node.name).toBe('Cube');
                 expect(mesh.name).toBe('Cube-Mesh');
-                expect(mesh.positions.length / 3).toBe(24);
+                expect(primitive.positions.length / 3).toBe(24);
-                expect(mesh.normals.length / 3).toBe(24);
+                expect(primitive.normals.length / 3).toBe(24);
-                expect(mesh.uvs.length / 2).toBe(24);
+                expect(primitive.uvs.length / 2).toBe(24);
                 expect(primitive.indices.length).toBe(36);
                 expect(primitive.material).toBe('Material');
             }), done).toResolve();
@@ -102,20 +103,20 @@ describe('loadObj', function() {
     it('loads obj with normals', function(done) {
         expect(loadObj(objNormalsUrl, defaultOptions)
             .then(function(data) {
-                var mesh = getMeshes(data)[0];
+                var primitive = getPrimitives(data)[0];
-                expect(mesh.positions.length / 3).toBe(24);
+                expect(primitive.positions.length / 3).toBe(24);
-                expect(mesh.normals.length / 3).toBe(24);
+                expect(primitive.normals.length / 3).toBe(24);
-                expect(mesh.uvs.length / 2).toBe(0);
+                expect(primitive.uvs.length / 2).toBe(0);
             }), done).toResolve();
     });
 
     it('loads obj with uvs', function(done) {
         expect(loadObj(objUvsUrl, defaultOptions)
             .then(function(data) {
-                var mesh = getMeshes(data)[0];
+                var primitive = getPrimitives(data)[0];
-                expect(mesh.positions.length / 3).toBe(20);
+                expect(primitive.positions.length / 3).toBe(20);
-                expect(mesh.normals.length / 3).toBe(0);
+                expect(primitive.normals.length / 3).toBe(0);
-                expect(mesh.uvs.length / 2).toBe(20);
+                expect(primitive.uvs.length / 2).toBe(20);
             }), done).toResolve();
     });
 
@@ -125,8 +126,8 @@ describe('loadObj', function() {
             loadObj(objNegativeIndicesUrl, defaultOptions)
         ])
             .then(function(results) {
-                var positionsReference = getMeshes(results[0])[0].positions.toFloatBuffer();
+                var positionsReference = getPrimitives(results[0])[0].positions.toFloatBuffer();
-                var positions = getMeshes(results[1])[0].positions.toFloatBuffer();
+                var positions = getPrimitives(results[1])[0].positions.toFloatBuffer();
                 expect(positions).toEqual(positionsReference);
             }), done).toResolve();
     });
@@ -134,9 +135,8 @@ describe('loadObj', function() {
     it('loads obj with triangle faces', function(done) {
         expect(loadObj(objTrianglesUrl, defaultOptions)
             .then(function(data) {
-                var mesh = getMeshes(data)[0];
                 var primitive = getPrimitives(data)[0];
-                expect(mesh.positions.length / 3).toBe(24);
+                expect(primitive.positions.length / 3).toBe(24);
                 expect(primitive.indices.length).toBe(36);
             }), done).toResolve();
     });
@@ -201,9 +201,8 @@ describe('loadObj', function() {
     it('loads obj with concave face containing 5 vertices', function(done) {
         expect(loadObj(objConcaveUrl, defaultOptions)
             .then(function(data) {
-                var mesh = getMeshes(data)[0];
                 var primitive = getPrimitives(data)[0];
-                expect(mesh.positions.length / 3).toBe(30);
+                expect(primitive.positions.length / 3).toBe(30);
                 expect(primitive.indices.length).toBe(48);
             }), done).toResolve();
     });
@@ -365,12 +364,12 @@ describe('loadObj', function() {
     });
 
     function getFirstPosition(data) {
-        var positions = data.nodes[0].meshes[0].positions;
+        var positions = data.nodes[0].meshes[0].primitives[0].positions;
         return new Cartesian3(positions.get(0), positions.get(1), positions.get(2));
     }
 
     function getFirstNormal(data) {
-        var normals = data.nodes[0].meshes[0].normals;
+        var normals = data.nodes[0].meshes[0].primitives[0].normals;
         return new Cartesian3(normals.get(0), normals.get(1), normals.get(2));
     }
 
@@ -411,6 +410,14 @@ describe('loadObj', function() {
             }), done).toResolve();
     });
 
+    it('discards faces that don\'t use the same attributes as other faces in the primitive', function(done) {
+        expect(loadObj(objMixedAttributesUrl, defaultOptions)
+            .then(function(data) {
+                var primitive = getPrimitives(data)[0];
+                expect(primitive.indices.length).toBe(18); // 3 faces removed
+            }), done).toResolve();
+    });
+
     it('throws when file has invalid contents', function(done) {
         expect(loadObj(objInvalidContentsUrl, defaultOptions), done).toRejectWith(RuntimeError);
     });