Add support for Draco glTF models #6191

ggetz · 2018-02-06T22:22:31Z

Part of #5120
Fixes #6298

Added support for Draco compressed models. Added sample models of the Mi;k Truck and Cesium Man.

TODO:

Include Quantization

cesium-concierge · 2018-02-06T22:22:32Z

@ggetz, thanks for the pull request! Maintainers, we have a signed CLA from @ggetz, so you can review this at any time.

⚠️ I noticed that a file in one of our ThirdParty folders (ThirdParty/, Source/ThirdParty/) has been added or modified. Please verify that it has a section in LICENSE.md and that its license information is up to date with this new version. Once you do, please confirm by commenting on this pull request.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

ggetz · 2018-02-06T22:51:41Z

Test is failing on Travis because of the following:

DeveloperError: IndexDatatype.UNSIGNED_INT requires OES_element_index_uint, which is not supported on this system.  Check context.elementIndexUint.

I use an an unsigned int for the index array, should I be using something else?

pjcozzi · 2018-02-06T23:53:30Z

Exciting!

@ggetz please update this tasklist: #5120 (comment)

@lilleyse bump to me for a quick once over when you think this is ready.

lilleyse

The implementation looks really clean and self-contained - nice job!

Besides just loading the models to see if they worked I tested creating two of the same models to see how the new code interacted with the resource cache. Everything seems to be working fine.

lilleyse · 2018-02-07T18:04:08Z

Source/Scene/Model.js

@@ -158,6 +164,8 @@ define([
        return {};
    }

+    var dracoDecoder = new draco.Decoder();


I wonder if this should be initialized on demand in case this is a heavy object to add to the global state.

lilleyse · 2018-02-07T18:19:10Z

Source/Scene/Model.js

+                var typedArray = arraySlice(rawBuffer.extras._pipeline.source, bufferView.byteOffset, bufferView.byteOffset + bufferView.byteLength);
+
+                buffer = new draco.DecoderBuffer();
+                buffer.Init(new Int8Array(typedArray), bufferView.byteLength);


Does Draco require an Int8Array instead of a Uint8Array? We should be careful because constructing a typed array from another typed array will copy the data, which we should try to avoid.

lilleyse · 2018-02-07T18:23:58Z

Source/Scene/Model.js

+                    throw new RuntimeError('Unsupported draco mesh geometry type.');
+                }
+
+                dracoGeometry = new draco.Mesh();


This one is not getting destroyed later. It looks like the rest of the draco objects are good though.

lilleyse · 2018-02-07T19:08:27Z

Source/Scene/Model.js

@@ -4076,7 +4076,7 @@ define([
            context : context,
            typedArray : typedArray,
            usage : BufferUsage.STATIC_DRAW,
-            indexDatatype : IndexDatatype.UNSIGNED_INT
+            indexDatatype : IndexDatatype.UNSIGNED_SHORT


We should test a model that has more than 65536 vertices. It will likely fail if UNSIGNED_SHORT is hardcoded. Could the index datatype be determined when reading then number of vertices in the compressed data?

Fixed this so it uses UNSIGNED_INT for more vertices. Do you know where I could find a model to test?

ggetz · 2018-02-07T20:22:39Z

@lilleyse Updated. See #6191 (comment)

lilleyse · 2018-02-07T21:36:27Z

The new changes look good and the large model test worked.

@pjcozzi this is ready for you to look at.

pjcozzi · 2018-02-07T21:58:47Z

How is code coverage? Unit tests look light.
Did we test with the sample models? Update Draco gltf models using ratified bitstream KhronosGroup/glTF-Sample-Models#145
How expensive is the decode for geometry-heavy models, e.g., a b3dm in a 3D tileset for a BIM model? Is it expensive enough that we should merge this into a staging branch and then merge everything into master once we are decoding on a web worker?
What is the size of the built release version of Cesium.js before and after this PR? The new draco-decoder-gltf.js is 712 KB. If the size increase is significant, what do we do? Move it to a web worker and be OK with the large download only when Draco is used? Provide a separate build that includes Draco support? I don't think the later is a good idea, but it could lead to other ideas. What do three.js and Babylon do?
- There is also WIP for a pure JS decoder, but we'll likely want the webasm/emscripten one here for performance.

ggetz · 2018-02-07T23:01:19Z

How is code coverage? Unit tests look light.

Coverage is good other than the error handling, I'll add test cases for those.

Did we test with the sample models? KhronosGroup/glTF-Sample-Models#145

Yes, tested a subsection, I can test them all if needed?

How expensive is the decode for geometry-heavy models, e.g., a b3dm in a 3D tileset for a BIM model? Is it expensive enough that we should merge this into a staging branch and then merge everything into master once we are decoding on a web worker?

I'll profile, but it shouldn't be too hard to move to a web worker since it's typed array in, typed array out.

If the size increase is significant, what do we do?

ThreeJS allows you to set the path of the module and load it at runtime, see here

pjcozzi · 2018-02-07T23:47:32Z

Yes, tested a subsection, I can test them all if needed?

Absolutely.

They don't need to be submitted to the repo, just tested manually is OK unless they are needed for coverage.

mramato · 2018-02-08T00:03:07Z

Move it to a web worker and be OK with the large download only when Draco is used?

👍 for this approach. As @ggetz already mention, this should be a trivial change so it ca happen before this goes into master. Should also be faster too because we'll be decompressing multiple meshes in parallel.

EDIT: I was too curious not to check, final gzippped files size of Cesium.js went up 18% compared to master (722 KB to 865 KB). So that alone is reason enough to move it to a work and avoid loading it unless needed.

pjcozzi · 2018-02-08T14:43:58Z

CC @cx20 - this is the PR to watch for Draco support in Cesium.

ggetz · 2018-03-01T21:35:16Z

@lilleyse Updated!

The decoder module is now only included when a web worker is launched to decode a model. The decoding is concurrent, based on hardware limits, and parallel per primitive.

All the example models have been tested.

Is there a way to test the web workers directly? I'm testing the draco decoding one through modelSpec, but not directly.

mramato · 2018-03-01T21:36:50Z

Specs/Scene/ModelSpec.js

+        });
+    });
+
+    fit('error decoding a glTF causes model loading to fail', function() {


Accidentally left fit in here

lilleyse · 2018-03-02T21:14:13Z

Source/Scene/Model.js

+                    var bufferView = gltf.bufferViews[compressionData.bufferView];
+                    var rawBuffer = gltf.buffers[bufferView.buffer];
+                    var data = rawBuffer.extras._pipeline.source;
+                    data = data.slice(0, data.length);


Use arraySlice instead.

lilleyse · 2018-03-02T21:15:11Z

Source/Scene/Model.js

+                    var bufferView = gltf.bufferViews[compressionData.bufferView];
+                    var rawBuffer = gltf.buffers[bufferView.buffer];
+                    var data = rawBuffer.extras._pipeline.source;
+                    data = data.slice(0, data.length);


Is it possible to slice only the section of the buffer that Draco uses?

lilleyse · 2018-03-02T21:33:05Z

Source/Scene/Model.js

-                        parseShaders(this);
-                        parsePrograms(this);
-                        parseTextures(this, context);
+                    if (loadResources.decoding) {


The interaction of gltf-pipeline updgrading, resource parsing, and draco loading is getting a bit confusing. They should be separated in a cleaner way. Also I realize some of the confusion was already here from before.

Restructured, let me know if that's similar to what you had in mind.

lilleyse · 2018-03-02T21:37:28Z

Source/Workers/decodeDraco.js

+        return decodedAttributeData;
+    }
+
+    function decodeDracoPrimitive(parameters, transferableObjects) {


transferableObjects isn't used.

lilleyse · 2018-03-05T14:51:13Z

Source/Scene/Model.js

+        });
+    }
+
+    Model._maxDecodingConcurrency = Math.max(FeatureDetection.hardwareConcurrency - 1, 1);  // Maximum concurrency to use wehn deocding draco models


Typos in comment.

lilleyse · 2018-03-05T15:01:26Z

Source/Scene/Model.js

+            promise = decoderTaskProcessor.scheduleTask(taskData, [taskData.array.buffer]);
+        }
+
+        while (defined(promise)) {


Would it simplify the code to loop over loadResources.primitivesToDecode.length?

With that change the code below may only need to be called in one place.

promise = undefined; taskData = loadResources.primitivesToDecode.peek(); if (defined(taskData)) { promise = decoderTaskProcessor.scheduleTask(taskData, [taskData.array.buffer]); }

I updated this to be less redundant, but I'm hesitant to iterate through all of the primitives, as we don't necessarily iterate through all the primitives in the queue each frame. It depends on how many task can be scheduled as well.

Ah, good point.

lilleyse · 2018-03-05T15:04:39Z

Source/Scene/Model.js

+
+    Model._maxDecodingConcurrency = Math.max(FeatureDetection.hardwareConcurrency - 1, 1);  // Maximum concurrency to use wehn deocding draco models
+    Model._decoderTaskProcessor = undefined;
+    Model._getDecoderTaskProcessor = function () {


You might want to add a comment that this is exposed for testing purposes.

lilleyse · 2018-03-05T15:13:40Z

Source/Workers/decodeDraco.js

+
+                if (attribute.data_type() === 4) {
+                    attributeData = new draco.DracoInt32Array();
+                    vertexArray = new Uint16Array(numPoints * numComponents);


Do we have information about the max value in the DracoInt32Array? Ideally vertexArray should be either a Uint16Array or Uint32Array depending on the max value.

This value isn't available through the draco API. We can only get it if we start iterating through and getting the values. Is it worth stopping and remaking the array if it should be Uint32?

It's probably safe to just use uint16 then. I can't think of an integer attribute that commonly has elements above 65535. Leave a comment though explaining why a Uint16Array is used and the potential problem.

OK, will do. Int attributes were rarely used, I don't think the glTF example models use them at all.

lilleyse · 2018-03-05T15:16:04Z

Source/Workers/decodeDraco.js

+                    dracoDecoder.GetAttributeFloatForAllPoints(dracoGeometry, attribute, attributeData);
+                }
+
+                for (var i = 0; i < vertexArray.length; ++i) {


Cesium style note: factor out vertexArray.length to its own variable.

lilleyse · 2018-03-05T15:24:52Z

Specs/Scene/ModelSpec.js

@@ -123,6 +123,9 @@ defineSuite([
    var animatedMorphCubeUrl = './Data/Models/PBR/AnimatedMorphCube/AnimatedMorphCube.gltf';
    var twoSidedPlaneUrl = './Data/Models/PBR/TwoSidedPlane/TwoSidedPlane.gltf';
    var vertexColorTestUrl = './Data/Models/PBR/VertexColorTest/VertexColorTest.gltf';
+    var dracoCompressedModelUrl = './Data/Models/DracoCompression/CesiumMilkTruck/CesiumMilkTruck.gltf';
+    var dracoCompressedModelWithAnimationUrl = './Data/Models/DracoCompression/CesiumMan/CesiumMan.gltf';
+    var dracoCompressedModelWithErrorUrl = './Data/Models/DracoCompression/ShouldError/CesiumMilkTruck.gltf';


Is ShouldError/CesiumMilkTruck.gltf different than CesiumMilkTruck/CesiumMilkTruck.gltf? If not it can be removed.

lilleyse

For #6191 (comment) and #6191 (comment), the new restructuring looks great.

lilleyse · 2018-03-08T15:56:17Z

Source/Scene/DracoLoader.js

+        ForEach,
+        when
+    ) {
+    'use strict';


Use slightly different formatting here:

when) { 'use strict';

lilleyse · 2018-03-08T15:57:50Z

Source/Scene/DracoLoader.js

+        };
+    }
+
+    function scheduleDecodingTask (decoderTaskProcessor, model, loadResources, context) {


Remove space: function scheduleDecodingTask(

lilleyse · 2018-03-08T15:58:47Z

Source/Scene/DracoLoader.js

+                }
+
+                var bufferView = gltf.bufferViews[compressionData.bufferView];
+                //console.log(gltf.buffers[bufferView.buffer].extras._pipeline);


Remove comment.

lilleyse · 2018-03-08T16:09:44Z

Source/Workers/decodeDraco.js

+
+                if (attribute.data_type() === 4) {
+                    attributeData = new draco.DracoInt32Array();
+                    vertexArray = new Uint16Array(numPoints * numComponents);


It's probably safe to just use uint16 then. I can't think of an integer attribute that commonly has elements above 65535. Leave a comment though explaining why a Uint16Array is used and the potential problem.

lilleyse · 2018-03-08T16:14:38Z

Is [ ] Include Quantization something that is still planned for this PR?

ggetz · 2018-03-08T16:16:25Z

Is [ ] Include Quantization something that is still planned for this PR?

I have another branch with the support that I will open in another PR - I need to confirm the test models and I don't want to slow down this one.

ggetz · 2018-03-08T17:23:57Z

Thanks @lilleyse ! Addressed your remaining comments.

lilleyse · 2018-03-08T19:54:56Z

There are some sample models that aren't working. But they all have the same error so it should be easy to track down:

2CylinderEngine
Buggy
GearboxAssy
ReciprocatingSaw

ggetz · 2018-03-08T22:49:05Z

@lilleyse Fixed a race condition that was causing this error, and tested these models and a few others.

lilleyse · 2018-03-09T14:23:34Z

There is still something weird going on with those models:

It should look like:

ggetz · 2018-03-09T16:32:53Z

@lilleyse Sorry, missed that since the model had partially loaded. Updated and loading correctly now!

mramato · 2018-03-09T17:31:25Z

FYI, do to the build issue @ggetz found (and fixed) regarding including things that were only used on workers (and now that draco is only used on a worker), this PR actually shrinks the size of minified/gzipped Cesium.js by about ~40KB compared to mater! Awesome.

lilleyse · 2018-03-09T18:22:41Z

Source/Scene/Model.js

                offset = (ix.byteOffset / IndexDatatype.getSizeInBytes(ix.componentType));  // glTF has offset in bytes.  Cesium has offsets in indices
            }
            else {
                var positions = accessors[primitive.attributes.POSITION];
                count = positions.count;
                offset = 0;
-            }
+           }


Fix formatting.

lilleyse · 2018-03-09T18:22:48Z

Source/Scene/DracoLoader.js

+        var bufferViewId = addBufferToModelResources(model, indexBuffer);
+        return {
+            bufferViewId: bufferViewId,
+            numberOfIndices : indexBuffer.numberOfIndices


Since numberOfIndices can get gotten from bufferViewId (though indirectly), it may be fine to not store numberOfIndices anywhere.

Up to you though, it doesn't matter much to me.

I prefer getting all the needed data in one place, then just accessing it later. I did update comments to closely mirror the wording in the spec:

When loading each accessor, you must ignore the bufferView and go to the previously decoded Draco geometry in the primitive to get the data of indices and attributes.

Sounds good.

lilleyse · 2018-03-09T19:05:10Z

Thanks @ggetz, can't wait to start using this with our data.

pjcozzi

@ggetz looks pretty clean at quick glance. I would suggest updating the Sandcastle example with a "Load Draco compressed model" even if the code is exactly the same as the others with a comment pointing to https://github.com/google/draco or whatever content pipeline they need until ion does Draco compression.

The reference doc should also be updated to say that Model supports the Draco extension (and any other non-draft extensions that might have slipped through the cracks): https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Model.js#L1073

I won't have bandwidth for another review so please iterate with @lilleyse and ping me if you need something specific.

Also looking forward to the blog on this!

pjcozzi · 2018-03-11T15:59:39Z

Source/Scene/DracoLoader.js

+    }
+
+    function addNewVertexBuffer(typedArray, model, context) {
+        var vertexBuffer = Buffer.createVertexBuffer({


Generally Cesium only calls GL functions in the render loop. Is that the case here and below? If not, is the worth the rework to change?

It looks like this is the case.

@ggetz for this you could try integrating the draco loading with createBuffers which manages buffer creation from typed arrays.

I've made changes in #6341 that ensure the buffers are being created during Model.update, does that cover this? Or would it be better to integrate it into createBuffers?

I think it would be better, only for the sake of utilizing the JobScheduler. If it's easy to integrate I would try going for it.

However I did look at the approach in #6341 and it seems solid too.

pjcozzi · 2018-03-11T16:02:43Z

Source/Workers/decodeDraco.js

+        for (var i = 0; i < numFaces; ++i) {
+            dracoDecoder.GetFaceFromMesh(dracoGeometry, i, faceIndices);
+
+            var offset = i * 3;


Old school optimization that won't matter, but why not offset += 3; after the assignments to indexArray to avoid the multiplication?

pjcozzi · 2018-03-11T16:04:59Z

Source/Workers/decodeDraco.js

+        '../Core/defined',
+        '../Core/IndexDatatype',
+        '../Core/RuntimeError',
+        '../ThirdParty/draco-decoder-gltf',


@fanzhanggoogle any chance you are able to quick a quick look at Cesium's use of the Draco API in this file and let us know if you have any feedback or tips? Thanks!

pjcozzi · 2018-03-11T16:06:38Z

Specs/Scene/ModelSpec.js

@@ -2330,6 +2336,49 @@ defineSuite([
        });
    });

+    it('loads a glTF with KHR_draco_mesh_compression extension', function() {


Do these tests have code coverage for all the new code?

Do we test both with and without quantization?

Do we test both with and without quantization?

I believe Draco + quantization was slated for the next PR.

Ah, I did not realize that quantization would be a separate PR. I was under the impression that Draco was "done." Let's please finish Draco with quantization before moving on to the GLSL extension.

ggetz added 4 commits February 6, 2018 16:01

Decode draco meshes, include sample models

7f27631

Merge branch 'master' into draco

253f397

Update CHANGES.md

c983d8c

Added draco loading spec

9d039a9

ggetz requested a review from lilleyse February 6, 2018 22:22

ggetz changed the title ~~Draco~~ Add support for Draco glTF models Feb 6, 2018

Use uint16 for indices

dce83b7

pjcozzi mentioned this pull request Feb 7, 2018

Draco mesh/point compression extension for glTF #5120

Closed

lilleyse reviewed Feb 7, 2018

View reviewed changes

Draco cleanup

f46a6c1

More draco tests

b1593ee

pjcozzi mentioned this pull request Feb 8, 2018

Update Draco gltf models using ratified bitstream KhronosGroup/glTF-Sample-Models#145

Merged

ggetz added 4 commits February 22, 2018 16:41

Modification for updated models

0ef63a8

Move draco decoding to web worker, decode in paralell

147ea11

Merge branch 'master' into draco

f516ed7

Updated Specs

c997b4e

ggetz force-pushed the draco branch from b1ab976 to c997b4e Compare March 1, 2018 21:27

mramato reviewed Mar 1, 2018

View reviewed changes

ggetz mentioned this pull request Mar 1, 2018

Exclude modules from source only used in workers #6298

Closed

Properly exlcude draco decodder module from main

a727dc6

ggetz force-pushed the draco branch from ae4f273 to a727dc6 Compare March 1, 2018 22:51

lilleyse reviewed Mar 5, 2018

View reviewed changes

ggetz added 4 commits March 7, 2018 10:31

Cleanup draco model loading

395f0c7

Model load cleanup

bdf8078

Exclude other worker third party modules from source

166a669

Clenaup specs

f860422

ggetz force-pushed the draco branch from 0610133 to f860422 Compare March 8, 2018 16:09

Merge remote-tracking branch 'origin/master' into draco

b37b146

lilleyse reviewed Mar 8, 2018

View reviewed changes

Add comment, cleanup

a25fb07

Fix race condition

d37b7c7

Use decoded indecies count instead of accessor count

feeb6dc

lilleyse reviewed Mar 9, 2018

View reviewed changes

Fix formatting, update comments

4bc5336

lilleyse merged commit 4ad27a4 into master Mar 9, 2018

lilleyse deleted the draco branch March 9, 2018 19:05

pjcozzi reviewed Mar 11, 2018

View reviewed changes

This was referenced Mar 16, 2018

Draco Fixes #6341

Merged

Draco with dequantization in shader #6357

Merged

Add support for Draco glTF models #6191

Add support for Draco glTF models #6191

Conversation

ggetz commented Feb 6, 2018 • edited Loading

cesium-concierge commented Feb 6, 2018

ggetz commented Feb 6, 2018

pjcozzi commented Feb 6, 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

ggetz commented Feb 7, 2018

lilleyse commented Feb 7, 2018

pjcozzi commented Feb 7, 2018

ggetz commented Feb 7, 2018

pjcozzi commented Feb 7, 2018

mramato commented Feb 8, 2018 • edited Loading

pjcozzi commented Feb 8, 2018

ggetz commented Mar 1, 2018 • edited Loading

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

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

lilleyse commented Mar 8, 2018

ggetz commented Mar 8, 2018

ggetz commented Mar 8, 2018

lilleyse commented Mar 8, 2018 • edited Loading

ggetz commented Mar 8, 2018

lilleyse commented Mar 9, 2018

ggetz commented Mar 9, 2018

mramato commented Mar 9, 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

lilleyse commented Mar 9, 2018

pjcozzi 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

lilleyse Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggetz commented Feb 6, 2018 •

edited

Loading

mramato commented Feb 8, 2018 •

edited

Loading

ggetz commented Mar 1, 2018 •

edited

Loading

lilleyse commented Mar 8, 2018 •

edited

Loading

lilleyse Mar 16, 2018 •

edited

Loading