From 60b5d4e0ea534871e155e2a1b47658917a9253fb Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Tue, 2 May 2017 16:48:40 -0400 Subject: [PATCH 1/5] Add `mergeVertices` option and disable by default This works around #161 by adding a `mergeVertices` and disabling it by default. Once `mergeDuplicateVertices` is refectored to use less memory and be orders of magnitude faster, we can take out the option and just always do it. --- lib/Pipeline.js | 5 ++++- lib/parseArguments.js | 9 +++++++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/lib/Pipeline.js b/lib/Pipeline.js index ae18e517..7e26f4f1 100644 --- a/lib/Pipeline.js +++ b/lib/Pipeline.js @@ -83,6 +83,7 @@ Pipeline.processJSON = function(gltf, options) { * @param {Object} [options.compressTextureCoordinates=false] Flag to run compressTextureCoordinates stage. * @param {Object} [options.kmcOptions=undefined] Options to pass to the generateModelMaterialsCommon stage, if undefined, stage is not run. * @param {Object} [options.quantize] Flag to run quantizeAttributes stage. + * @param {Boolean} [options.mergeVertices = false} Flag to merge duplicate vertices, which can produce a smaller model size but greatly increases conversion time. * @param {Object|Object[]} [options.textureCompressionOptions=undefined] Options to pass to the compressTextures stage. If an array of options is given, the textures will be compressed in multiple formats. If undefined, stage is not run. * @returns {Promise} A promise that resolves to the processed glTF asset. */ @@ -112,7 +113,9 @@ Pipeline.processJSONWithExtras = function(gltfWithExtras, options) { generateNormals(gltfWithExtras, options); } if (!shouldPreserve) { - mergeDuplicateVertices(gltfWithExtras); + if (options.mergeVertices) { + mergeDuplicateVertices(gltfWithExtras); + } MergeDuplicateProperties.mergeAll(gltfWithExtras); RemoveUnusedProperties.removeAll(gltfWithExtras); removeDuplicatePrimitives(gltfWithExtras); diff --git a/lib/parseArguments.js b/lib/parseArguments.js index 4bdaa22a..3588ef9e 100644 --- a/lib/parseArguments.js +++ b/lib/parseArguments.js @@ -72,6 +72,10 @@ function parseArguments(args) { describe: 'Compress the testure coordinates of this glTF asset.', type: 'boolean' }, + 'mergeVertices': { + describe: 'Merges duplicate vertices, which can produce a smaller model size but greatly increases conversion time.', + type: 'boolean' + }, 'removeNormals': { alias: 'r', describe: 'Strips off existing normals, allowing them to be regenerated.', @@ -261,6 +265,7 @@ function parseArguments(args) { preserve: argv.p, quantize: argv.q, removeNormals: argv.r, - textureCompressionOptions: textureCompressionOptions + textureCompressionOptions: textureCompressionOptions, + mergeVertices: argv.mergeVertices }; -} \ No newline at end of file +} From cecce51183c0b58fc81262f4bf22aaa933166036 Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Fri, 5 May 2017 19:40:09 -0400 Subject: [PATCH 2/5] Changes after review. --- lib/Pipeline.js | 3 ++- lib/parseArguments.js | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/Pipeline.js b/lib/Pipeline.js index 7e26f4f1..923809e2 100644 --- a/lib/Pipeline.js +++ b/lib/Pipeline.js @@ -83,7 +83,8 @@ Pipeline.processJSON = function(gltf, options) { * @param {Object} [options.compressTextureCoordinates=false] Flag to run compressTextureCoordinates stage. * @param {Object} [options.kmcOptions=undefined] Options to pass to the generateModelMaterialsCommon stage, if undefined, stage is not run. * @param {Object} [options.quantize] Flag to run quantizeAttributes stage. - * @param {Boolean} [options.mergeVertices = false} Flag to merge duplicate vertices, which can produce a smaller model size but greatly increases conversion time. + * @param {Object} [options.preserve=false] Flag to turn optimization pipeline stages on/off to preserve the original glTF hierarchy. + * @param {Boolean} [options.mergeVertices=false} Flag to merge duplicate vertices, which can produce a smaller model size but greatly increases conversion time. This setting only applies when options.preserve is false. * @param {Object|Object[]} [options.textureCompressionOptions=undefined] Options to pass to the compressTextures stage. If an array of options is given, the textures will be compressed in multiple formats. If undefined, stage is not run. * @returns {Promise} A promise that resolves to the processed glTF asset. */ diff --git a/lib/parseArguments.js b/lib/parseArguments.js index 3588ef9e..79ed8521 100644 --- a/lib/parseArguments.js +++ b/lib/parseArguments.js @@ -73,7 +73,7 @@ function parseArguments(args) { type: 'boolean' }, 'mergeVertices': { - describe: 'Merges duplicate vertices, which can produce a smaller model size but greatly increases conversion time.', + describe: 'Merges duplicate vertices, which can produce a smaller model size but greatly increases conversion time. This setting only applies when preserve is false.', type: 'boolean' }, 'removeNormals': { From e137165722af60668fb0a25304da0f5671a51cff Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Tue, 9 May 2017 08:41:57 -0400 Subject: [PATCH 3/5] Add unit tests. --- lib/mergeDuplicateVertices.js | 6 +++++- specs/lib/PipelineSpec.js | 26 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/lib/mergeDuplicateVertices.js b/lib/mergeDuplicateVertices.js index d86cc65c..325132fb 100644 --- a/lib/mergeDuplicateVertices.js +++ b/lib/mergeDuplicateVertices.js @@ -25,6 +25,10 @@ module.exports = mergeDuplicateVertices; * @see removeUnusedVertices */ function mergeDuplicateVertices(gltf) { + return mergeDuplicateVertices._implementation(gltf); +} + +mergeDuplicateVertices._implementation = function (gltf) { var meshes = gltf.meshes; var indexAccessors = {}; for (var meshId in meshes) { @@ -37,7 +41,7 @@ function mergeDuplicateVertices(gltf) { mergeDuplicateVerticesFromMapping(gltf, indexAccessors); removeUnusedVertices(gltf); return gltf; -} +}; function mergeDuplicateVerticesFromMapping(gltf, indexAccessors) { var accessors = gltf.accessors; diff --git a/specs/lib/PipelineSpec.js b/specs/lib/PipelineSpec.js index 2925fc01..c76ca247 100644 --- a/specs/lib/PipelineSpec.js +++ b/specs/lib/PipelineSpec.js @@ -4,6 +4,7 @@ var fsExtra = require('fs-extra'); var path = require('path'); var Promise = require('bluebird'); +var mergeDuplicateVertices = require('../../lib/mergeDuplicateVertices'); var Pipeline = require('../../lib/Pipeline'); var readGltf = require('../../lib/readGltf'); @@ -11,6 +12,7 @@ var processFile = Pipeline.processFile; var processFileToDisk = Pipeline.processFileToDisk; var processJSON = Pipeline.processJSON; var processJSONToDisk = Pipeline.processJSONToDisk; +var processJSONWithExtras = Pipeline.processJSONWithExtras; var fsExtraReadFile = Promise.promisify(fsExtra.readFile); @@ -159,4 +161,28 @@ describe('Pipeline', function() { expect(initialUri).not.toEqual(finalUri); }), done).toResolve(); }); + + it('processJSONWithExtras does not merge duplicate vertices by default', function (done) { + spyOn(mergeDuplicateVertices, '_implementation').and.callThrough(); + var promise = readGltf(gltfPath) + .then(function (gltf) { + return processJSONWithExtras(gltf); + }) + .then(function () { + expect(mergeDuplicateVertices._implementation).not.toHaveBeenCalled(); + }); + expect(promise, done).toResolve(); + }); + + it('processJSONWithExtras can merge duplicate vertices.', function (done) { + spyOn(mergeDuplicateVertices, '_implementation').and.callThrough(); + var promise = readGltf(gltfPath) + .then(function (gltf) { + return processJSONWithExtras(gltf, {mergeVertices: true}).thenReturn(gltf); + }) + .then(function (gltf) { + expect(mergeDuplicateVertices._implementation).toHaveBeenCalledWith(gltf); + }); + expect(promise, done).toResolve(); + }); }); From d4ba10c7fb5068c7d4a343bbc8d4f0048d7ed3bd Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Tue, 9 May 2017 10:40:39 -0400 Subject: [PATCH 4/5] Changes after review. --- lib/Pipeline.js | 4 +++- specs/lib/PipelineSpec.js | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/Pipeline.js b/lib/Pipeline.js index 923809e2..c50018a4 100644 --- a/lib/Pipeline.js +++ b/lib/Pipeline.js @@ -113,8 +113,10 @@ Pipeline.processJSONWithExtras = function(gltfWithExtras, options) { if (smoothNormals || faceNormals) { generateNormals(gltfWithExtras, options); } + + var mergeVertices = defaultValue(options.mergeVertices, false); if (!shouldPreserve) { - if (options.mergeVertices) { + if (mergeVertices) { mergeDuplicateVertices(gltfWithExtras); } MergeDuplicateProperties.mergeAll(gltfWithExtras); diff --git a/specs/lib/PipelineSpec.js b/specs/lib/PipelineSpec.js index c76ca247..db15efb0 100644 --- a/specs/lib/PipelineSpec.js +++ b/specs/lib/PipelineSpec.js @@ -178,7 +178,7 @@ describe('Pipeline', function() { spyOn(mergeDuplicateVertices, '_implementation').and.callThrough(); var promise = readGltf(gltfPath) .then(function (gltf) { - return processJSONWithExtras(gltf, {mergeVertices: true}).thenReturn(gltf); + return processJSONWithExtras(gltf, {mergeVertices: true}); }) .then(function (gltf) { expect(mergeDuplicateVertices._implementation).toHaveBeenCalledWith(gltf); From 665a021dc22886f1d7a58342068915487685d4a0 Mon Sep 17 00:00:00 2001 From: Matthew Amato Date: Tue, 9 May 2017 10:41:05 -0400 Subject: [PATCH 5/5] Fix jsDoc typo. --- lib/Pipeline.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Pipeline.js b/lib/Pipeline.js index c50018a4..3bb5b941 100644 --- a/lib/Pipeline.js +++ b/lib/Pipeline.js @@ -84,7 +84,7 @@ Pipeline.processJSON = function(gltf, options) { * @param {Object} [options.kmcOptions=undefined] Options to pass to the generateModelMaterialsCommon stage, if undefined, stage is not run. * @param {Object} [options.quantize] Flag to run quantizeAttributes stage. * @param {Object} [options.preserve=false] Flag to turn optimization pipeline stages on/off to preserve the original glTF hierarchy. - * @param {Boolean} [options.mergeVertices=false} Flag to merge duplicate vertices, which can produce a smaller model size but greatly increases conversion time. This setting only applies when options.preserve is false. + * @param {Boolean} [options.mergeVertices=false] Flag to merge duplicate vertices, which can produce a smaller model size but greatly increases conversion time. This setting only applies when options.preserve is false. * @param {Object|Object[]} [options.textureCompressionOptions=undefined] Options to pass to the compressTextures stage. If an array of options is given, the textures will be compressed in multiple formats. If undefined, stage is not run. * @returns {Promise} A promise that resolves to the processed glTF asset. */