Add draco mesh compression #353

FrankGalligan · 2018-03-14T22:41:08Z

The Draco glTF files created with gltf-pipeline will be created according to the KHR_draco_mesh_compression specification located
here:
https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_draco_mesh_compression/README.md

lilleyse · 2018-03-15T14:18:28Z

Awesome @FrankGalligan and @fanzhanggoogle. One of us should have a chance to review pretty soon.

Is it possible to clean up the git history a bit?

lilleyse · 2018-03-15T14:18:48Z

Fixes #348

shehzan10

Thanks for opening this PR. I did a quick 1st pass overview and things generally look pretty good. Mostly just formatting changes for now.

shehzan10 · 2018-03-15T14:18:42Z

README.md

+|`--draco.quantizeTexcoord`|Quantization bits for texture coordinate attribute when using Draco compression.|No, default `12`|
+|`--draco.quantizeColor`|Quantization bits for color attribute when using Draco compression.|No, default `8`|
+|`--draco.quantizeSkin`|Quantization bits for skinning attribute (joint indices and joint weights) when using Draco compression.|No, default `12`|
+|`--draco.unifiedQuantization`|Quantize positions of all primitives using the same quantization grid defined by the unified bounding box of all primitives. If this option is not set, quantization is applied on each primitive separately which can result in gaps appearing between different primitives.|No, default `false`|


Can we shorten the description of this? Probably too long for an options table. We could also put it just before the table with a *.

shehzan10 · 2018-03-15T14:28:50Z

lib/getComponentReader.js

+ *
+ * @private
+ */
+function getComponentReader(componentType) {


Does splitting this into it's own module warrant adding tests for it?

Actually the 2.0 branch doesn't have any unit tests now... but they will come before it is merged into master.

OK I can add tests when needed.

shehzan10 · 2018-03-15T14:29:45Z

lib/draco_encoder_nodejs.js

@@ -0,0 +1,28 @@
+module.exports = CreateDracoModule;


Camel case for the filename: draco_encoder_nodejs.js -> dracoEncoder.js.

I assume this file is copied as-is, but we should format the spacing/tables/empty lines to maintain consistency throughout the project.

Unless I'm missing something it looks like the JS encoder isn't being used.

True. Removed this file.

shehzan10 · 2018-03-15T14:56:24Z

lib/compressMeshes.js

+var Cesium = require('cesium');
+var hashObject = require('object-hash');
+var ForEach = require('./ForEach');
+var numberOfComponentsForType = require('./numberOfComponentsForType');


Minor pick: Let's order these (including the draco ones below) alphabetically.

shehzan10 · 2018-03-15T14:57:14Z

lib/compressMeshes.js

+
+module.exports = compressMeshes;
+
+


Formatting: Remove extra empty line.

shehzan10 · 2018-03-15T15:11:08Z

lib/compressMeshes.js

+
+
+function TypeToName() {
+}


Formatting: Empty function should have brackets in the same line (like you did for Remove).

shehzan10 · 2018-03-15T15:14:14Z

lib/compressMeshes.js

+
+
+
+


Remove extra empty lines.

shehzan10 · 2018-03-15T15:16:39Z

lib/compressMeshes.js

+
+    return {
+      numComponents : componentsPerAttribute,
+      numVertices : accessor.count,


Naming: Use full words. numComponents -> numberOfComponents. Same for numVertices.

shehzan10 · 2018-03-15T16:28:09Z

lib/compressMeshes.js

+              console.log('Found duplicate!');
+              // Copy compressed primitive.
+              copyCompressedExtensionToPrimitive(
+                  primitive, hashPrimitives[hashValue]);


Formatting: Combine into same line as the function.

shehzan10 · 2018-03-15T16:28:41Z

lib/compressMeshes.js

+            const numFaces = indices.length / 3;
+            const numIndices = indices.length;
+
+            console.log("Num of faces: " + numFaces);


This should probably be removed.

FrankGalligan · 2018-03-15T17:20:38Z

@lilleyse For the history let me see what I can do. I tried to pull your 2.0 branch into my fork and then my "add_draco_mesh_compression" branch on top of that. But this PR does not reflect the history I would have liked.

The files are created according to the KHR_draco_mesh_compression specification located here: https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_draco_mesh_compression/README.md

shehzan10 · 2018-03-15T18:43:19Z

README.md

@@ -105,6 +108,13 @@ processGltf(gltf, options)
 |`--separateTextures`, `-t`|Write out separate textures only.|No, default `false`|
 |`--checkTransparency`|Do a more exhaustive check for texture transparency by looking at the alpha channel of each pixel. By default textures are considered to be opaque.|No, default `false`|
 |`--stats`|Print statistics to console for input and output glTF files.|No, default `false`|
+|`--draco.compressionLevel`|Draco compression level [0-10], most is 10, least is 0.|No, default `7`|


Need to add compressMesh option.here.

shehzan10 · 2018-03-15T18:47:32Z

lib/compressMeshes.js

+            if (encodedLen > 0) {
+              console.log("Encoded size is " + encodedLen);
+            } else {
+              console.log("Error: Encoding failed.");


These console.logs should be removed as well.

FrankGalligan · 2018-03-15T21:24:29Z

OK, the history looks good.

- Shorten unifiedQuantization options table text. - Add compressMesh to options table text. This address a comment in CesiumGS#353

- Errors now throw DeveloperError.

Address comments from CesiumGS#353

Fix some eslint errors

shehzan10 · 2018-03-16T13:54:09Z

CI is failing on ESLint: https://travis-ci.org/AnalyticalGraphicsInc/gltf-pipeline/jobs/354149293#L1718-L1732

- Removed parameters from ForEach that were not being used.

Fix comments gltfpipeline 353

FrankGalligan · 2018-03-16T16:02:00Z

@shehzan10 I fixed the lint errors in compressMeshes.js, but the error in updateVersion.js is in code I did not touch and a little harder to fix as it is the first parameter that is not used.

So I can add some code like
if (!defined(accessorId)) { return; }

Or I could add a new function to ForEach.js like
ForEach.meshPrimitiveAttributeSemantic(primitive, function(semantic) {

Any suggestions?

Fix comments gltfpipeline 353

pjcozzi · 2018-04-03T21:42:46Z

We received a CLA from @FrankGalligan, thanks again!

lilleyse

Thanks for the updates. I have mostly small comments and one bigger comment about removing the uncompressed data.

lilleyse · 2018-04-02T21:06:19Z

bin/gltf-pipeline.js

+            default : false,
+            describe: 'Quantize positions of all primitives using the same quantization grid defined by the unified bounding box of all primitives. If this option is not set, quantization is applied on each primitive separately which can result in gaps appearing between different primitives. Default is false.',
+            type: 'boolean'
+        },


Remove trailing comma.

lilleyse · 2018-04-03T21:21:27Z

lib/compressDracoMeshes.js

+    addExtensionsRequired(gltf, 'KHR_draco_mesh_compression');
+    var hashPrimitives = [];
+    options = defaultValue(options, {});
+    options.dracoOptions= defaultValue(options.dracoOptions, {});


Small tweak: options.dracoOptions =

lilleyse · 2018-04-03T21:24:54Z

lib/compressDracoMeshes.js

+
+            // Add attributes to mesh.
+            var attributeToId = {};
+            var attributes = primitive.attributes;


This variable is unused.

lilleyse · 2018-04-03T21:32:01Z

lib/removeUnusedElements.js

+
+/**
+ * Removes unused elements from gltf.
+ * This function currenlty only works for accessors, buffers, and bufferViews.


currenlty typo

lilleyse · 2018-04-03T21:33:40Z

lib/removeUnusedElements.js

+ * Contains functions for getting a list of element ids in use by the glTF asset.
+ * @constructor
+ */
+function getListOfElementsIdsInUse() {}


Looks slightly clearer if getListOfElementsIdsInUse is capitalized.

So do you want it as 'GetListOfElementsIdsInUse', or 'getListOfElementsIDsInUse', or something else?

Actually it's probably fine as is.

lilleyse · 2018-04-03T21:34:08Z

lib/removeUnusedElements.js

+/**
+ * Contains functions for getting a list of element ids in use by the glTF asset.
+ * @constructor
+ */


For each of the new classes mark as @private so they don't show up in the doc.

lilleyse · 2018-04-03T21:38:58Z

lib/removeUnusedElements.js

+
+TypeToGltfElementName.bufferView = function() {
+    return 'bufferViews';
+};


TypeToGltfElementName can be simplified to

var TypeToGltfElementName = { accessor : 'accessors', buffer : 'buffers', bufferView : 'bufferViews' };

lilleyse · 2018-04-03T21:39:41Z

lib/removeUnusedElements.js

+ * This function currenlty only works for accessors, buffers, and bufferViews.
+ *
+ * @param {Object} gltf A javascript object containing a glTF asset.
+ * @returns {Object} The glTF asset with the added pipeline extras.


Fix the description, looks like a copy-paste error.

lilleyse · 2018-04-03T21:47:59Z

lib/removeUnusedElements.js

+    ForEach.skin(gltf, function(skin) {
+       if (defined(skin.inverseBindMatrices)) {
+           usedAccessorIds[skin.inverseBindMatrices] = true;
+       }


The indentation in this block is slightly off.

lilleyse · 2018-04-03T21:52:28Z

lib/removeUnusedElements.js

@@ -0,0 +1,235 @@
+'use strict';


Does the uncompressed data get spliced out of the original buffer in all cases?

It's possible that the uncompressed attributes and a texture may be stored in a buffer, and because that texture's bufferView references the buffer the buffer won't get removed in removeUnusedElements.

Currently yes. I have another branch that I think should fix this issue here:
https://github.com/FrankGalligan/gltf-pipeline/commits/fix_buffer_packing

I can merge that branch into this branch. I also have some other branches as well that I want to merge. Like a fix for JOINT_ATTRIBUTES not using integer, and a branch that will add support to go from Draco glTF -> glTF.

I was going to make PRs from them after this was landed as I think it would be easier to review, but I can merge them into this this PR if you want.

Maybe open a PR for fix_buffer_packing that targets this branch? The others can wait for later.

@shehzan10

- This fixes an issue raised in CesiumGS#353 by @shehzan10. - Debug_checks were turned on and the check was wrong in an older version of the JavaScript encoder.

Update draco3d version dependency

FrankGalligan · 2018-04-05T23:27:49Z

@shehzan10 I fixed the issue you were having with the encoder. I updated the npm package, so if you update your dependencies this should compress now. There isstill an issue with the writing of the gltf, which I'm looking into.

shehzan10 · 2018-04-06T16:27:10Z

Can confirm that the updates fix the crashing models.

There is still an issue with the writing of the gltf, which I'm looking into.

Writing to disk looks ok, I guess. But, it looks like the test model fails to load for compression level 7 and above and works for 6 and lower.

FrankGalligan · 2018-04-06T22:27:32Z

@shehzan10 Yeah so the decoder has the dcheck as well. It the decoder is using the nodejs modules and they update it should be fine. Otherwise they will have to wait until we push the fixes to github. We are not pushing the changes until the week of 4/16.

-The old code would turn on Draco compression if the file had the text "draco" in it. -This fixes #18

Fix draco parameter handling

shehzan10 · 2018-04-19T20:38:08Z

@FrankGalligan can you update this to draco 1.3 and fix the remaining comments from @lilleyse? I think we should be able to merge then.

- Also fixes some comments in CesiumGS#353

Update draco3d to 1.3.0

FrankGalligan · 2018-04-24T21:34:19Z

@shehzan10 Updated to 1.3. Waiting on one comment form @lilleyse

The bufferView's for inlined images was not being processed.

Fix inline images

lilleyse · 2018-04-25T17:46:48Z

The code changes looks good. We're just going to do some final testing.

lilleyse · 2018-04-26T17:01:00Z

All good here - thanks @FrankGalligan

shehzan10 requested changes Mar 15, 2018

View reviewed changes

FrankGalligan closed this Mar 15, 2018

FrankGalligan force-pushed the add_draco_mesh_compression branch from ea553d3 to 3df32a5 Compare March 15, 2018 17:58

Add support for creating Draco glTF files

0ec62a8

The files are created according to the KHR_draco_mesh_compression specification located here: https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_draco_mesh_compression/README.md

FrankGalligan reopened this Mar 15, 2018

shehzan10 reviewed Mar 15, 2018

View reviewed changes

Update README

1583f42

- Shorten unifiedQuantization options table text. - Add compressMesh to options table text. This address a comment in CesiumGS#353

FrankGalligan mentioned this pull request Mar 15, 2018

Address comments from https://github.com/AnalyticalGraphicsInc/gltf-pipeline/pull/353 FrankGalligan/gltf-pipeline#11

Merged

FrankGalligan and others added 11 commits March 15, 2018 15:19

Removed draco_encoder_nodejs.js

61ada45

Small formatting changes

80eb3ab

Enforce 4 space indent

d72d8ba

Enforce prefix in for loops

02a78f0

Change some variables to use full words

15affb6

Remove unused code

9c07d04

Removed console.log calls

70fffad

- Errors now throw DeveloperError.

Add a comment header

71a3158

Merge pull request #11 from FrankGalligan/fix_comments_gltfpipeline_353

32910be

Address comments from CesiumGS#353

Fix some eslint errors

25427c4

Merge pull request #12 from FrankGalligan/fix_comments_gltfpipeline_353

15c6532

Fix some eslint errors

FrankGalligan added 3 commits March 16, 2018 08:45

Fix eslint errors

06a1626

- Removed parameters from ForEach that were not being used.

Add support to skip compressing non-triangle mesh

debf5eb

Merge pull request #13 from FrankGalligan/fix_comments_gltfpipeline_353

10cf23c

Fix comments gltfpipeline 353

Hook up Draco command line options

37538d2

FrankGalligan added 4 commits April 2, 2018 11:55

Fix eslint errors

98a482a

Handle -d arg again

47e99e2

Add Draco argument range validation

b3790dc

Merge pull request #15 from FrankGalligan/fix_comments_gltfpipeline_353

8ab69e0

Fix comments gltfpipeline 353

lilleyse reviewed Apr 3, 2018

View reviewed changes

Update draco3d version dependency

6e2e876

- This fixes an issue raised in CesiumGS#353 by @shehzan10. - Debug_checks were turned on and the check was wrong in an older version of the JavaScript encoder.

FrankGalligan mentioned this pull request Apr 5, 2018

Update draco3d version dependency FrankGalligan/gltf-pipeline#17

Merged

Merge pull request #17 from FrankGalligan/fix_comments_gltfpipeline_353

55a5057

Update draco3d version dependency

FrankGalligan added 2 commits April 6, 2018 15:51

Fix draco parameter handling

869a3e3

-The old code would turn on Draco compression if the file had the text "draco" in it. -This fixes #18

Merge pull request #19 from FrankGalligan/fix_comments_gltfpipeline_353

a6fcf42

Fix draco parameter handling

emackey mentioned this pull request Apr 18, 2018

Draco extension KhronosGroup/glTF-Blender-Exporter#195

Closed

ondys mentioned this pull request Apr 18, 2018

glTF export? google/draco#369

Closed

Update draco3d to 1.3.0

5911c86

- Also fixes some comments in CesiumGS#353

FrankGalligan mentioned this pull request Apr 24, 2018

Update draco3d to 1.3.0 FrankGalligan/gltf-pipeline#20

Merged

Merge pull request #20 from FrankGalligan/fix_comments_gltfpipeline_353

3203399

Update draco3d to 1.3.0

Fix inline images

be05dda

The bufferView's for inlined images was not being processed.

FrankGalligan mentioned this pull request Apr 25, 2018

Fix inline images FrankGalligan/gltf-pipeline#21

Merged

Merge pull request #21 from FrankGalligan/fix_comments_gltfpipeline_353

ab9f5e2

Fix inline images

lilleyse merged commit 668dcd1 into CesiumGS:2.0 Apr 26, 2018

FrankGalligan deleted the add_draco_mesh_compression branch April 26, 2018 17:44

emackey mentioned this pull request Apr 27, 2018

Merge in Draco branch #327

Closed

pjcozzi mentioned this pull request Jun 17, 2018

Support for KHR_draco_mesh_compression. #348

Closed


		module.exports = compressMeshes;

Add draco mesh compression #353

Add draco mesh compression #353

Conversation

FrankGalligan commented Mar 14, 2018

lilleyse commented Mar 15, 2018

lilleyse commented Mar 15, 2018

shehzan10 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankGalligan commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankGalligan commented Mar 15, 2018

shehzan10 commented Mar 16, 2018

FrankGalligan commented Mar 16, 2018

pjcozzi commented Apr 3, 2018

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrankGalligan commented Apr 5, 2018

shehzan10 commented Apr 6, 2018

FrankGalligan commented Apr 6, 2018

shehzan10 commented Apr 19, 2018

FrankGalligan commented Apr 24, 2018

lilleyse commented Apr 25, 2018

lilleyse commented Apr 26, 2018