From 485645e0b9aaa2dcfa6e8ad35c05a1760b088364 Mon Sep 17 00:00:00 2001 From: Sean Lilley Date: Tue, 18 Sep 2018 20:34:56 -0400 Subject: [PATCH 1/2] Back port fixes from #153 --- CHANGES.md | 4 +++ lib/createGltf.js | 55 ++++++++++++++++++++----------------- lib/loadObj.js | 50 +++++++++++++++++++-------------- specs/lib/createGltfSpec.js | 36 ++++++++++++------------ specs/lib/loadObjSpec.js | 36 ++++++++++++------------ 5 files changed, 99 insertions(+), 82 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 516a0c4..e443a4a 100644 --- a/CHANGES.md +++ b/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) diff --git a/lib/createGltf.js b/lib/createGltf.js index f7bc747..f183757 100644 --- a/lib/createGltf.js +++ b/lib/createGltf.js @@ -218,10 +218,14 @@ function createGltf(objData, options) { var meshes = nodes[i].meshes; var meshesLength = meshes.length; for (var j = 0; j < meshesLength; ++j) { - // Reserve the 65535 index for primitive restart - var vertexCount = meshes[j].positions.length / 3; - if (vertexCount > 65534) { - return true; + 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 = primitives[k].positions.length / 3; + if (vertexCount > 65534) { + return true; + } } } } @@ -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'; diff --git a/lib/loadObj.js b/lib/loadObj.js index efe97a9..5b6cf8c 100644 --- a/lib/loadObj.js +++ b/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); - mesh.positions.push(py); - mesh.positions.push(pz); + primitive.positions.push(px); + primitive.positions.push(py); + 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); - mesh.normals.push(ny); - mesh.normals.push(nz); + primitive.normals.push(nx); + primitive.normals.push(ny); + 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); - mesh.uvs.push(uy); + primitive.uvs.push(ux); + 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 - return (mesh.primitives.length > 0) && (mesh.positions.length > 0); + // Valid meshes must have at least one primitive + return (mesh.primitives.length > 0); }); } diff --git a/specs/lib/createGltfSpec.js b/specs/lib/createGltfSpec.js index afd0963..87ea212 100644 --- a/specs/lib/createGltfSpec.js +++ b/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].uvs.length = 0; + boxObjData.nodes[0].meshes[0].primitives[0].normals.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 indices = mesh.primitives[0].indices; - var positions = mesh.positions; - var normals = mesh.normals; - var uvs = mesh.uvs; + var primitive = objData.nodes[0].meshes[0].primitives[0]; + var indices = primitive.indices; + var positions = primitive.positions; + var normals = primitive.normals; + 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 indicesLength = mesh.primitives[0].indices.length; - var vertexCount = mesh.positions.length / 3; + var primitive = boxObjData.nodes[0].meshes[0].primitives[0]; + var indicesLength = primitive.indices.length; + var vertexCount = primitive.positions.length / 3; var gltf = createGltf(boxObjData, defaultOptions); - var primitive = gltf.meshes[Object.keys(gltf.meshes)[0]].primitives[0]; - var indicesAccessor = gltf.accessors[primitive.indices]; + var gltfPrimitive = gltf.meshes[Object.keys(gltf.meshes)[0]].primitives[0]; + 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); }); diff --git a/specs/lib/loadObjSpec.js b/specs/lib/loadObjSpec.js index bfe4c44..66c8b36 100644 --- a/specs/lib/loadObjSpec.js +++ b/specs/lib/loadObjSpec.js @@ -91,9 +91,9 @@ describe('loadObj', function() { expect(node.name).toBe('Cube'); expect(mesh.name).toBe('Cube-Mesh'); - expect(mesh.positions.length / 3).toBe(24); - expect(mesh.normals.length / 3).toBe(24); - expect(mesh.uvs.length / 2).toBe(24); + expect(primitive.positions.length / 3).toBe(24); + expect(primitive.normals.length / 3).toBe(24); + expect(primitive.uvs.length / 2).toBe(24); expect(primitive.indices.length).toBe(36); expect(primitive.material).toBe('Material'); }), done).toResolve(); @@ -102,20 +102,20 @@ describe('loadObj', function() { it('loads obj with normals', function(done) { expect(loadObj(objNormalsUrl, defaultOptions) .then(function(data) { - var mesh = getMeshes(data)[0]; - expect(mesh.positions.length / 3).toBe(24); - expect(mesh.normals.length / 3).toBe(24); - expect(mesh.uvs.length / 2).toBe(0); + var primitive = getPrimitives(data)[0]; + expect(primitive.positions.length / 3).toBe(24); + expect(primitive.normals.length / 3).toBe(24); + 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]; - expect(mesh.positions.length / 3).toBe(20); - expect(mesh.normals.length / 3).toBe(0); - expect(mesh.uvs.length / 2).toBe(20); + var primitive = getPrimitives(data)[0]; + expect(primitive.positions.length / 3).toBe(20); + expect(primitive.normals.length / 3).toBe(0); + expect(primitive.uvs.length / 2).toBe(20); }), done).toResolve(); }); @@ -125,8 +125,8 @@ describe('loadObj', function() { loadObj(objNegativeIndicesUrl, defaultOptions) ]) .then(function(results) { - var positionsReference = getMeshes(results[0])[0].positions.toFloatBuffer(); - var positions = getMeshes(results[1])[0].positions.toFloatBuffer(); + var positionsReference = getPrimitives(results[0])[0].positions.toFloatBuffer(); + var positions = getPrimitives(results[1])[0].positions.toFloatBuffer(); expect(positions).toEqual(positionsReference); }), done).toResolve(); }); @@ -134,9 +134,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 +200,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 +363,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)); } From 684914952015718e4cf4dc374a5032fe55c610c0 Mon Sep 17 00:00:00 2001 From: Sean Lilley Date: Tue, 18 Sep 2018 23:00:41 -0400 Subject: [PATCH 2/2] Added test --- .../box-mixed-attributes.mtl | 12 +++++ .../box-mixed-attributes.obj | 46 +++++++++++++++++++ specs/lib/loadObjSpec.js | 9 ++++ 3 files changed, 67 insertions(+) create mode 100644 specs/data/box-mixed-attributes/box-mixed-attributes.mtl create mode 100644 specs/data/box-mixed-attributes/box-mixed-attributes.obj diff --git a/specs/data/box-mixed-attributes/box-mixed-attributes.mtl b/specs/data/box-mixed-attributes/box-mixed-attributes.mtl new file mode 100644 index 0000000..4f8d129 --- /dev/null +++ b/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 diff --git a/specs/data/box-mixed-attributes/box-mixed-attributes.obj b/specs/data/box-mixed-attributes/box-mixed-attributes.obj new file mode 100644 index 0000000..cb9055e --- /dev/null +++ b/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 diff --git a/specs/lib/loadObjSpec.js b/specs/lib/loadObjSpec.js index 66c8b36..43fcd8b 100644 --- a/specs/lib/loadObjSpec.js +++ b/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'; @@ -409,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); });