Draco with dequantization in shader #6357

ggetz · 2018-03-21T14:46:59Z

Skip dequantization in the draco module and instead dequantizes in the shader, for an optionally specified list of attribute types to skip.

lilleyse

The mechanics look solid here. Once quantization is on by default it might be good to go through all the draco sample models and make sure they still work. Another thing to try is loading the draco version of the NYC tileset and comparing the total memory usage in the 3D Tiles inspector with quantization on and off.

Finally, does it ever make sense to draco-encode a model with pre-quantized attributes? Or is the preferred method to do what is done here, converting draco-encoded to quantized form at runtime? I would think the second case, but just want to be sure.

lilleyse · 2018-03-21T20:41:31Z

Source/Scene/DracoLoader.js

@@ -130,7 +130,7 @@ define([
     *
     * @private
     */
-    DracoLoader.parse = function(model) {
+    DracoLoader.parse = function(model, dequantizeInShader) {


Could we just grab model._dequantizeInShader instead of passing it through separately?

lilleyse · 2018-03-21T20:44:57Z

Source/Scene/Model.js

@@ -288,6 +288,7 @@ define([
     * @param {Color} [options.silhouetteColor=Color.RED] The silhouette color. If more than 256 models have silhouettes enabled, there is a small chance that overlapping models will have minor artifacts.
     * @param {Number} [options.silhouetteSize=0.0] The size of the silhouette in pixels.
     * @param {ClippingPlaneCollection} [options.clippingPlanes] The {@link ClippingPlaneCollection} used to selectively disable rendering the model.
+     * @param {String[]} [options.dequantizeInShader] The list of {@link https://github.com/KhronosGroup/glTF/blob/master/extensions/2.0/Khronos/KHR_draco_mesh_compression/README.md|KHR_draco_mesh_compression} encoded attributes to dequantize in the shader.


I think this is too fine grained of an option. It should just be a boolean and default to true, meaning all attributes will be dequantized in the shader. (Under the hood there might be exceptions, in case there's an issue with bone indices or something).

Also it should be set to false if the draco extension is not in use. In its current form a model crashes if it doesn't use draco but a value like ['POSITION'] is passed in here.

We can only skip attributes by attribute type (https://github.com/google/draco/blob/master/src/draco/javascript/emscripten/draco_web_decoder.idl#L22), not each attribute. I think it may be best to limit to POSITION, NORMAL,
COLOR, and TEX_COORD.

(Three.js only skips quantization for position)

Ok, hardcoding those to be quantized sounds fine.

lilleyse · 2018-03-21T20:55:44Z

Source/Scene/Model.js

@@ -1710,6 +1713,7 @@ define([
                        programPrimitives = [];
                        model._programPrimitives[programId] = programPrimitives;
                    }
+                    primitive.id = id + '.primitive.' + i;


If possible we should avoid dynamically adding a property to the glTF in this way.

I can't remember why we cache quantized primitives according to their program, but maybe the step can be simplified?

It was implemented for WEB3D_quantized_attributes, I'll see if it can be simplified.

I turned the list into a dictionary, and used the id as the key, so I am no longer adding the id attribute to the primitive itself.

lilleyse · 2018-03-21T21:00:21Z

Source/Scene/Model.js

+
+        if (!defined(primitives)) {
+            return shader;
+        }


What is this check for?

If the program primitives weren't cached, they are not quantized, and the shader does not need to be modified.

I'll add a comment if unclear.

lilleyse · 2018-03-21T21:15:09Z

Source/Workers/decodeDraco.js

@@ -82,6 +84,21 @@ define([
                        componentDatatype : ComponentDatatype.fromTypedArray(vertexArray)
                    }
                };
+
+                transform = new draco.AttributeQuantizationTransform();


It should be ok to remove the var transform; above and have this be var transform = new draco.AttributeQuantizationTransform();

lilleyse · 2018-03-21T21:31:12Z

Source/Workers/decodeDraco.js

+
+                transform = new draco.AttributeQuantizationTransform();
+                if (transform.InitFromAttribute(attribute)) {
+                    var minValues = new Float32Array(numComponents);


It doesn't really matter, but could this be a plain Array instead? Since the array is pretty small using a Float32Array almost seems misleading.

ggetz · 2018-03-22T13:53:16Z

Finally, does it ever make sense to draco-encode a model with pre-quantized attributes? Or is the preferred method to do what is done here, converting draco-encoded to quantized form at runtime? I would think the second case, but just want to be sure.

Preferred method is the second, see google/draco#18

bagnell · 2018-03-22T18:56:17Z

Finally, does it ever make sense to draco-encode a model with pre-quantized attributes? Or is the preferred method to do what is done here, converting draco-encoded to quantized form at runtime? I would think the second case, but just want to be sure.

Preferred method is the second, see google/draco#18

Why isn't it the first? With the second, you would need to re-quantize after decompression. After reading that issue, it seems like you would quantize attributes and then draco compress with the option to remove the built-in quantization.

ggetz · 2018-03-22T20:21:22Z

Why isn't it the first? With the second, you would need to re-quantize after decompression. After reading that issue, it seems like you would quantize attributes and then draco compress with the option to remove the built-in quantization.

With the second the quantization values are stored in the draco geometry itself, so there's not need to store them separately. With the current iteration in glTF-pipeline, I don't believe it is possible to quantize the attributes then draco encode.

ggetz · 2018-03-23T14:44:16Z

Thanks @lilleyse, updated!

ggetz · 2018-03-23T15:20:33Z

Numbers with NYC tileset:

Dequantize	Memory	GPU Memory
CPU	910,756K	684,219K
GPU	553,675K	664,426K

lilleyse

Getting close! In a bit I should be able to test the final memory usage numbers.

lilleyse · 2018-03-27T13:45:19Z

Source/Scene/DracoLoader.js

@@ -131,11 +136,12 @@ define([
     * @private
     */
    DracoLoader.parse = function(model) {
-        if (!hasExtension(model)) {
+        if (!this.hasExtension(model)) {


I think it's better style to use DracoLoader.hasExtension instead here and below since it is a static functions.

lilleyse · 2018-03-27T13:50:49Z

Source/Scene/Model.js

@@ -1429,7 +1433,7 @@ define([

        // Only ARRAY_BUFFER here.  ELEMENT_ARRAY_BUFFER created below.
        ForEach.bufferView(model.gltf, function(bufferView, id) {
-            if (bufferView.target === WebGLConstants.ARRAY_BUFFER) {
+            if (bufferView.target === WebGLConstants.ARRAY_BUFFER && !DracoLoader.hasExtension(model)) {


Is the extension check needed? Did you run into a situation where a buffer view was marked as ARRAY_BUFFER but shouldn't have been?

lilleyse · 2018-03-27T13:59:50Z

Source/Workers/decodeDraco.js

@@ -4,13 +4,15 @@ define([
        '../Core/IndexDatatype',
        '../Core/RuntimeError',
        '../ThirdParty/draco-decoder-gltf',
+        '../ThirdParty/GltfPipeline/byteLengthForComponentType',


It doesn't matter in terms of functionality, but use the Cesium version instead: ComponentDatatype.getSizeInBytes

lilleyse · 2018-03-27T14:05:43Z

Source/Scene/Model.js

@@ -288,6 +288,7 @@ define([
     * @param {Color} [options.silhouetteColor=Color.RED] The silhouette color. If more than 256 models have silhouettes enabled, there is a small chance that overlapping models will have minor artifacts.
     * @param {Number} [options.silhouetteSize=0.0] The size of the silhouette in pixels.
     * @param {ClippingPlaneCollection} [options.clippingPlanes] The {@link ClippingPlaneCollection} used to selectively disable rendering the model.
+     * @param {Boolean} [options.dequantizeInShader=true] Determines if a {@link https://github.com/google/draco|Draco} encoded model is dequantized on the GPU. This decreases total memory usage for encoded models.


Mention in CHANGES.md that this flag is added. It can go alongside the Draco bullet.

lilleyse · 2018-03-27T14:22:59Z

Source/Workers/decodeDraco.js

                    attributeData = new draco.DracoInt32Array();
-                    // Uint16Array is used becasue there is not currently a way to retreive the maximum
+                    vertexArray = new Int16Array(vertexArrayLength);
+                    dracoDecoder.GetAttributeInt32ForAllPoints(dracoGeometry, attribute, attributeData);


When the attribute is oct encoded I think we can use a smaller array length since the number of components should be 2 instead of 3.

Also quantization should use a Uint16Array while oct-encoding should use a Int16Array.

When the attribute is oct encoded, the loader already return 2 for the number of components, so the array length is being set to the correct length.

I have updated Uint16 vs Int16 however.

Ah ok, I should have checked that myself. Thanks!

ggetz · 2018-03-27T17:02:23Z

Thanks @lilleyse, updated!

lilleyse · 2018-03-27T19:18:39Z

Thanks, looks good.

ggetz added 5 commits March 20, 2018 17:44

Dequntize draco in shader

74578e0

Optionally pass list of attributes to dequantize

842589d

Cleanup dcumentation

524663c

Add spec for draco with quantization

decbbe3

Cleanup function declaration

a422861

ggetz requested a review from lilleyse March 21, 2018 19:27

lilleyse reviewed Mar 21, 2018

View reviewed changes

ggetz added the type - enhancement label Mar 22, 2018

ggetz added 2 commits March 23, 2018 10:34

Cleanup, use oct-encoding where available

c7d71ef

Update Specs

846b17a

Fix quantized byteLength

2044d9a

ggetz force-pushed the draco-quantization branch from a4dd562 to 2044d9a Compare March 26, 2018 15:44

lilleyse reviewed Mar 27, 2018

View reviewed changes

Cleanup

535bc9b

lilleyse merged commit 593af7a into master Mar 27, 2018

lilleyse deleted the draco-quantization branch March 27, 2018 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draco with dequantization in shader #6357

Draco with dequantization in shader #6357

ggetz commented Mar 21, 2018

lilleyse left a comment

lilleyse Mar 21, 2018

lilleyse Mar 21, 2018

ggetz Mar 22, 2018

lilleyse Mar 22, 2018

lilleyse Mar 21, 2018

ggetz Mar 22, 2018

ggetz Mar 23, 2018

lilleyse Mar 21, 2018

ggetz Mar 22, 2018

ggetz Mar 22, 2018

lilleyse Mar 21, 2018

lilleyse Mar 21, 2018

ggetz commented Mar 22, 2018

bagnell commented Mar 22, 2018

ggetz commented Mar 22, 2018

ggetz commented Mar 23, 2018

ggetz commented Mar 23, 2018

lilleyse left a comment

lilleyse Mar 27, 2018

lilleyse Mar 27, 2018

lilleyse Mar 27, 2018

lilleyse Mar 27, 2018

lilleyse Mar 27, 2018

ggetz Mar 27, 2018

lilleyse Mar 27, 2018

ggetz commented Mar 27, 2018

lilleyse commented Mar 27, 2018

Draco with dequantization in shader #6357

Draco with dequantization in shader #6357

Conversation

ggetz commented Mar 21, 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

ggetz commented Mar 22, 2018

bagnell commented Mar 22, 2018

ggetz commented Mar 22, 2018

ggetz commented Mar 23, 2018

ggetz commented Mar 23, 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

ggetz commented Mar 27, 2018

lilleyse commented Mar 27, 2018