Grab draco3d from npm instead of gstatic #9841

ebogo1 · 2021-09-28T17:49:39Z

Band-aid fix for #9787.

This PR is a temporary workaround for streamlining all our third party libraries to come from npm. Unfortunately, as mentioned in the above issue, the draco3d npm package seems to have a bug with quantization so this PR introduces a regression by disabling quantization in our decodeDraco worker.

The examples on the draco3d repo instantiate the .wasm module using the Decoder API, but doing so with our current npm build system doesn't play nicely with rollup because the Decoder module requires Node built-in libraries (fs and path) which are missing at build time. Because of all this, the PR changes are:

Add the draco3d npm package to package.json
In the postinstall script, copy the wrapper (now named draco_decoder_nodejs.js instead of draco_wasm_wrapper.js) and .wasm file from node_modules/ to their original locations in Source/ThirdParty/.
Comment out the lines dealing with quantization for meshes in the decodeDraco worker. A few specs failed because of this change so for now they are xit'd out.

This also updates Draco to 1.4.1.

cesium-concierge · 2021-09-28T17:49:43Z

Thanks for the pull request @ebogo1!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

ebogo1 · 2021-09-28T18:05:49Z

Source/WorkersES6/decodeDraco.js

-      dracoDecoder.SkipAttributeTransform(draco[attributesToSkip[i]]);
-    }
-  }
+  // var attributesToSkip = ["POSITION", "NORMAL", "COLOR", "TEX_COORD"];


@lilleyse Is this ok to merge as is? I remember you suggested commenting out the line in Model.js instead? I think it was -

cesium/Source/Scene/Model.js

Line 638 in e8f8fb9

this._dequantizeInShader = defaultValue(options.dequantizeInShader, true);

Yeah this is ok to merge as is.

Actually, can we keep this code and only enable "NORMAL"? Since it is working for normals we may as well take what we can get.

ebogo1 · 2021-09-28T18:10:09Z

Looks like CI/specs are failing with a few more that I missed, I think those should be xit'd too:

1) loads Duck
     Scene/GltfLoader
2) adds a dequantization function
     Scene/ModelExperimental/DequantizationPipelineStage
3) adds dequantization uniforms
     Scene/ModelExperimental/DequantizationPipelineStage
4) prepares Draco model for dequantization stage
     Scene/ModelExperimental/GeometryPipelineStage

lilleyse · 2021-09-28T18:03:05Z

Source/WorkersES6/decodeDraco.js

+  // var attributesToSkip = ["POSITION", "NORMAL", "COLOR", "TEX_COORD"];
+  // if (parameters.dequantizeInShader) {
+  //   for (var i = 0; i < attributesToSkip.length; ++i) {
+  //     dracoDecoder.SkipAttributeTransform(draco[attributesToSkip[i]]);
+  //   }
+  // }


To help keep the code clean let's delete this code and failing tests in a single isolated commit, then in a new issue mention that the commit should be reverted once the draco issue is fixed upstream and we update the npm package.

lilleyse · 2021-09-28T18:26:46Z

I just want to emphasize that this PR is an unfortunate but necessary tradeoff that we hope is just temporary.

Geometry memory usage for 3D Tiles will double because we can no longer dequantize data on the GPU due to a bug in newer versions of draco google/draco#742. We can no longer use our older working version of Draco due to a requirement to move all third party libraries to npm, which is further complicated by the fact that older versions of draco on npm use asm.js instead of WebAssemly and aren't compatible with our web worker system.

lilleyse · 2021-09-28T18:36:22Z

@ebogo1 just had an idea - even though draco will return the full decoded data now, we can still quantize the data ourselves in the worker so that we don't have to resort to doubling the memory usage. We'll just need to convert the Float32Array back to a Uint16Array or Uint8Array manually.

ebogo1 · 2021-09-30T19:28:54Z

Source/WorkersES6/decodeDraco.js

+  }
+  var range = max[0] - min[0];
+  for (i = 1; i < numComponents; ++i) {
+    range = Math.max(max[i] - min[i], range);


This is not what I expected - the draco source code says range is the "max delta over all components." I first thought this would mean the magnitude of max - min, but doing it this way matches the range returned by the decoder in main.

Oh, I didn't see this, maybe ignore my comment in the PR review 😅

ptrgags

@ebogo1 looks pretty good, though I'm noticing one problem with the NYC dataset, picking is now selecting way too many buildings:

Maybe check if the _BATCH_ID property is going down the correct path?

The milk truck model worked fine though!

Source/WorkersES6/decodeDraco.js

ptrgags · 2021-10-01T14:30:37Z

@ebogo1 code looks good (aside from the tests that we want to temporarily remove). NYC dataset is picking correctly again!

let me know when you remove the broken tests and when you've documented what commits need to be reverted after draco is fixed, then I'll merge.

ptrgags · 2021-10-01T14:57:58Z

@ebogo1 changes look good, thanks!

ebogo1 · 2021-10-01T15:03:27Z

Thanks @ptrgags - I opened #9847 to follow up.

ebogo1 added 2 commits September 28, 2021 13:36

postinstall to copy files from node_modules

811584e

remove fdescribes

51706bc

ebogo1 requested a review from lilleyse September 28, 2021 17:49

ebogo1 commented Sep 28, 2021

View reviewed changes

lilleyse reviewed Sep 28, 2021

View reviewed changes

brute force math

c9ac980

ebogo1 commented Sep 30, 2021

View reviewed changes

ptrgags reviewed Sep 30, 2021

View reviewed changes

Source/WorkersES6/decodeDraco.js Outdated Show resolved Hide resolved

ptrgags reviewed Sep 30, 2021

View reviewed changes

Source/WorkersES6/decodeDraco.js Outdated Show resolved Hide resolved

ebogo1 added 6 commits September 30, 2021 20:17

only quantize POSITION, COLOR, and TEX_COORD

b2475ac

revert xit changes in Specs

2ee0e7c

revert decodeDraco worker

d466c87

Update gulpfile.cjs

be462e6

remove SkipAttributeTransform and failing specs

1a3b697

use prepare instead of postinstall

53d112e

Gupdate GltfLoader and ModelExperimental specs

97c186c

ptrgags merged commit 1de05af into main Oct 1, 2021

ptrgags deleted the no-quantization branch October 1, 2021 14:58

ebogo1 mentioned this pull request Oct 1, 2021

Re-enable Draco quantization #9847

Closed

lilleyse mentioned this pull request Nov 1, 2021

Add Draco quantization workaround for point clouds #9908

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grab draco3d from npm instead of gstatic #9841

Grab draco3d from npm instead of gstatic #9841

ebogo1 commented Sep 28, 2021 •

edited

Loading

cesium-concierge commented Sep 28, 2021

ebogo1 Sep 28, 2021

lilleyse Sep 28, 2021

lilleyse Sep 28, 2021 •

edited

Loading

ebogo1 commented Sep 28, 2021

lilleyse Sep 28, 2021

lilleyse commented Sep 28, 2021

lilleyse commented Sep 28, 2021

ebogo1 Sep 30, 2021 •

edited

Loading

ptrgags Sep 30, 2021

ptrgags left a comment

ptrgags commented Oct 1, 2021

ptrgags commented Oct 1, 2021

ebogo1 commented Oct 1, 2021

Grab draco3d from npm instead of gstatic #9841

Grab draco3d from npm instead of gstatic #9841

Conversation

ebogo1 commented Sep 28, 2021 • edited Loading

cesium-concierge commented Sep 28, 2021

ebogo1 Sep 28, 2021

Choose a reason for hiding this comment

lilleyse Sep 28, 2021

Choose a reason for hiding this comment

lilleyse Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

ebogo1 commented Sep 28, 2021

lilleyse Sep 28, 2021

Choose a reason for hiding this comment

lilleyse commented Sep 28, 2021

lilleyse commented Sep 28, 2021

ebogo1 Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

ptrgags Sep 30, 2021

Choose a reason for hiding this comment

ptrgags left a comment

Choose a reason for hiding this comment

ptrgags commented Oct 1, 2021

ptrgags commented Oct 1, 2021

ebogo1 commented Oct 1, 2021

ebogo1 commented Sep 28, 2021 •

edited

Loading

lilleyse Sep 28, 2021 •

edited

Loading

ebogo1 Sep 30, 2021 •

edited

Loading