Add mergeVertices option and disable by default
#276
Conversation
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
Outdated
| @@ -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. | |||
There was a problem hiding this comment.
Maybe add this requires options.preserve. Same change in lib/parseArguments.
Or maybe we set var shouldPreserve = defaultValue(options.preserve, (defaultValue(options.mergeVertices, false));, but this could cause various other optimizations.
Or a 3rd option is to change
if (!shouldPreserve) {
if (options.mergeVertices) {
mergeDuplicateVertices(gltfWithExtras);
}
MergeDuplicateProperties.mergeAll(gltfWithExtras);
//....................
}
to
if (!shouldPreserve || options.mergeVertices) {
mergeDuplicateVertices(gltfWithExtras);
}
if (!shouldPreserve) {
MergeDuplicateProperties.mergeAll(gltfWithExtras);
//....................
}
PS. I also noticed that options.preserve is not documented here (it is documented in `lib/parseArguments').
There was a problem hiding this comment.
I added doc for preserve and mention it in the doc for merge vertices. I left the logic as-is. This new option should hopefully go soon enough anyway, since it's just to work around #161
|
This is ready unless anyone else has any comments. |
|
LGTM. @lilleyse do you want to look over once and merge? |
|
I added some unit tests to cover the new code path. @lilleyse if you can merge this once it goes green, I'd like to do a new release today (I can do the publish, since I have my linux laptop with me). |
lib/Pipeline.js
Outdated
| @@ -83,6 +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 {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. | |||
There was a problem hiding this comment.
Small typo, should be [options.mergeVertices=false]
lib/Pipeline.js
Outdated
| @@ -112,7 +114,9 @@ Pipeline.processJSONWithExtras = function(gltfWithExtras, options) { | |||
| generateNormals(gltfWithExtras, options); | |||
| } | |||
| if (!shouldPreserve) { | |||
| mergeDuplicateVertices(gltfWithExtras); | |||
| if (options.mergeVertices) { | |||
There was a problem hiding this comment.
mergeVertices should be set to a default value before reaching here.
specs/lib/PipelineSpec.js
Outdated
| spyOn(mergeDuplicateVertices, '_implementation').and.callThrough(); | ||
| var promise = readGltf(gltfPath) | ||
| .then(function (gltf) { | ||
| return processJSONWithExtras(gltf, {mergeVertices: true}).thenReturn(gltf); |
There was a problem hiding this comment.
No need for the thenReturn(gltf), it already resolves to a gltf.
|
Thanks @lilleyse, your review comments have been addressed. |
|
👍 |
|
I'll add to |
This works around #161 by adding a
mergeVerticesand disabling it by default. OncemergeDuplicateVerticesis refactored to use less memory and be orders of magnitude faster, we can take out the option and just always do it.CC @lilleyse