Added Draco support to point clouds #6559

lilleyse · 2018-05-03T19:58:29Z

Adds support for reading Draco encoded pnts files.

Merge #6558 first - one of the tests here fails without that fix.

To-do:

Host sample draco point cloud on ion
Add IE support back once Use legacy environment enabled build for draco decoder module #6404 is done
Open PR for spec changes into 3d-tiles (post-1.0)

cesium-concierge · 2018-05-03T19:58:30Z

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

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

⚠️ 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.

🌍 🌎 🌏

lilleyse · 2018-05-03T20:01:15Z

Source/Workers/decodeDraco.js

-        dracoDecoder.GetAttributeInt32ForAllPoints(dracoGeometry, attribute, attributeData);
+        var vertexArray = new Uint16Array(vertexArrayLength);
+        var attributeData = new draco.DracoUInt16Array();
+        dracoDecoder.GetAttributeUInt16ForAllPoints(dracoGeometry, attribute, attributeData);


While working on decodeDracoPointCloud I realized my suggestion to use Int16Array for oct-encoded attributes was incorrect, so I fixed it here as well.

ggetz · 2018-05-03T20:31:35Z

Source/Workers/decodeDracoPointCloud.js

+                    }
+
+                    dracoModule(wasmConfig).then(function (compiledModule) {
+                        initWorker(compiledModule);


What happens if we are decoding a Draco model and a Draco point cloud at the same time? Since it's a fairly large module, do we want to load it twice?

Hm that's a good point.

I guess caching the binary is an option but I'll see if combining decodeDraco and decodeDracoPointCloud into the same worker is doable. That would also address https://github.com/AnalyticalGraphicsInc/cesium/pull/6559/files/c5870f8cd097b235f67e2e460e7430d8c4506bc4#r185930196.

ggetz · 2018-05-03T20:36:17Z

Source/Workers/decodeDraco.js

                    };
                }
                draco.destroy(transform);

                transform = new draco.AttributeOctahedronTransform();
                if (transform.InitFromAttribute(attribute)) {
                    quantization = {
-                        quantizationBits : transform.quantization_bits(),
-                        octEncoded : true


ModelUtility.modifyShaderForQuantizedAttributes will need to be updated accordingly.

This also explains the test failures. I'll add octEncoded back in.

ggetz · 2018-05-03T20:42:10Z

Source/Workers/decodeDracoPointCloud.js

+                attributeData = new draco.DracoFloat32Array();
+                vertexArray = new Float32Array(vertexArrayLength);
+                dracoDecoder.GetAttributeFloatForAllPoints(dracoGeometry, attribute, attributeData);
+                break;


This seems like a lot of duplicate code with decodeDraco, and this function in particular is identical. Can we share this somehow?

pjcozzi · 2018-05-06T00:56:16Z

CHANGES.md

@@ -7,6 +7,10 @@ Change Log

 * Fixed a bug causing custom TilingScheme classes to not be able to use a GeographicProjection. [#6524](https://github.com/AnalyticalGraphicsInc/cesium/pull/6524)

+##### Additions :tada:
+
+* Added support for loading Draco encoded Point Cloud tiles [#6559](https://github.com/AnalyticalGraphicsInc/cesium/pull/6559)


Add a tad more detail, e.g., "compressed", not "encoded", and something like "up to 10x" or whatever compression.

pjcozzi · 2018-05-06T00:57:59Z

Source/Scene/PointCloud3DTileContent.js

+                throw new RuntimeError('DRACO.semantics, DRACO.byteOffset, and DRACO.byteLength must be defined');
+            }
+
+            var dracoHasPositions = dracoSemantics.indexOf('POSITION') >= 0;


Here and below, maybe put the righthand side in parentheses.

pjcozzi · 2018-05-06T01:00:12Z

Source/Scene/PointCloud3DTileContent.js

+                u_quantizedVolumeScaleAndOctEncodedRange : function() {
+                    var scratch = scratchQuantizedVolumeScaleAndOctEncodedRange;
+                    if (defined(content._quantizedVolumeScale)) {
+                        scratch.x = content._quantizedVolumeScale.x;


Use Cartesian3.clone?

lilleyse · 2018-05-08T16:10:33Z

@ggetz updated so that point clouds and models use the same worker.

There is a failing test but it should get fixed once #6558 is merged.

ggetz

Only the wasm decoder file was updated? Does the fallback js module also need an update?

ggetz · 2018-07-05T13:59:41Z

Source/Scene/PointCloud3DTileContent.js

+            hasNormals = defined(dracoFeatureTableProperties.NORMAL);
+            hasBatchIds = defined(dracoFeatureTableProperties.BATCH_ID);
+            isTranslucent = defined(dracoFeatureTableProperties.RGBA);
+            isQuantizedDraco = hasPositions && content._dequantizeInShader;


With the model implementation, we determine per attribute if quantization (and oct-encoding) is is available using AttributeQuantizationTransform.initFromAttribute rather than assuming it's present. Is that an approach we want to take here?

ggetz · 2018-07-05T14:04:07Z

Specs/Data/Cesium3DTiles/PointCloud/PointCloudDraco/tileset.json

@@ -0,0 +1,27 @@
+{
+  "asset": {
+    "version": "0.0"


Change to 1.0

ggetz · 2018-07-05T14:04:31Z

Specs/Data/Cesium3DTiles/PointCloud/PointCloudDraco/tileset.json

+    },
+    "geometricError": 0,
+    "content": {
+      "url": "pointCloudDraco.pnts"


url -> uri

ggetz · 2018-07-05T14:04:45Z

Specs/Data/Cesium3DTiles/PointCloud/PointCloudDracoBatched/tileset.json

@@ -0,0 +1,27 @@
+{
+  "asset": {
+    "version": "0.0"


ggetz · 2018-07-05T14:04:58Z

Specs/Data/Cesium3DTiles/PointCloud/PointCloudDracoBatched/tileset.json

+    },
+    "geometricError": 0,
+    "content": {
+      "url": "pointCloudDracoBatched.pnts"


url -> uri

ggetz · 2018-07-05T14:05:29Z

Specs/Data/Cesium3DTiles/PointCloud/PointCloudDracoPartial/tileset.json

@@ -0,0 +1,27 @@
+{
+  "asset": {
+    "version": "0.0"


ggetz · 2018-07-05T14:05:39Z

Specs/Data/Cesium3DTiles/PointCloud/PointCloudDracoPartial/tileset.json

+    },
+    "geometricError": 0,
+    "content": {
+      "url": "pointCloudDracoPartial.pnts"


url -> uri

ggetz · 2018-07-05T14:14:34Z

Source/Workers/decodeDraco.js

+
+        if (parameters.dequantizeInShader) {
+            dracoDecoder.SkipAttributeTransform(draco.POSITION);
+            dracoDecoder.SkipAttributeTransform(draco.NORMAL);


Do we want to include generic attributes here, for RGB, RGBA, etc?

I didn't include them because the other attributes like RGB, RGBA, intensity, and classification usually decode to 8-bit values and decoding to quantized form won't provide any GPU memory savings.

ggetz · 2018-07-05T14:25:45Z

Source/Workers/decodeDraco.js


-                quantization = undefined;
+    function decodePointCloud(parameters) {
+        var dracoDecoder = new draco.Decoder();


I don't think we are destroying the decoder anywhere once we're done.

ggetz · 2018-07-05T14:37:59Z

Source/Scene/PointCloud3DTileContent.js

+                        var scale = quantization.range / (1 << quantization.quantizationBits);
+                        content._quantizedVolumeScale = Cartesian3.fromElements(scale, scale, scale);
+                        content._quantizedVolumeOffset = Cartesian3.unpack(quantization.minValues);
+                        content._quantizedRange = (1 << quantization.quantizationBits) - 1.0;


._quantizedRange !== quantization.range? Maybe clarify terminology here.

Yeah this is kind of confusing. I'll add a comment to the code in #6721 because the behavior changed slightly:
https://github.com/AnalyticalGraphicsInc/cesium/pull/6721/files#diff-e12a976367111c8fb95abea65e0b6ab0R1228

Comment added here: 6161a96

ggetz · 2018-07-05T14:40:36Z

Source/Scene/PointCloud3DTileContent.js


    PointCloud3DTileContent.prototype.update = function(tileset, frameState) {
+        var context = frameState.context;
+        var decoding = decodeDraco(this, context);


Can be written more concisely as

if (decodeDraco(this, context)) { return; }

ggetz · 2018-07-05T14:44:48Z

As part of this, should we also finish up CesiumGS/3d-tiles#310 ?

ggetz · 2018-07-05T15:33:47Z

I think there may be something wrong with the boundingSphere for draco compressed models now. This is the view that viewer zooms to:

lilleyse · 2018-07-05T19:38:14Z

As part of this, should we also finish up CesiumGS/3d-tiles#310 ?

Yes! I need to get back to that.

lilleyse · 2018-07-05T19:40:41Z

I think there may be something wrong with the boundingSphere for draco compressed models now. This is the view that viewer zooms to:

This happens in master too.

At a glance the bounding volumes look correct but it's almost like it's using the wrong one for the zoom. I'm not sure if it's a model problem or a decode problem, but I don't think it's related to this PR. Maybe we should open a separate issue to investigate it?

lilleyse · 2018-07-05T19:44:32Z

Only the wasm decoder file was updated? Does the fallback js module also need an update?

Good point, and I think there has been another Draco release since then. I'll update the files tonight.

lilleyse · 2018-07-05T20:30:00Z

Opened a 3d-tiles-tools PR for the sample tilesets: CesiumGS/3d-tiles-validator#150

lilleyse · 2018-07-06T00:23:23Z

@ggetz updated the Draco files to 1.3.3.

ggetz · 2018-07-06T17:36:25Z

Source/Scene/PointCloud3DTileContent.js

        }

        content._parsedContent = {
            positions : positions,
            colors : colors,
            normals : normals,
            batchIds : batchIds,
-            styleableProperties : styleableProperties
+            styleableProperties : styleableProperties,
+            draco : {


This draco property should probably remain undefined unless the extension is used.

ggetz · 2018-07-06T17:44:33Z

I'm not sure if it's a model problem or a decode problem, but I don't think it's related to this PR. Maybe we should open a separate issue to investigate it?

Yeah we should open an issue if it's happening master.

ggetz · 2018-07-06T17:45:08Z

@lilleyse besides those two comments, this looks looks good, thanks!

lilleyse · 2018-07-08T23:19:58Z

Opened #6784 for the zooming bug.

Updated.

ggetz · 2018-07-09T13:48:34Z

Thanks @lilleyse ! Looks good.

Is #6721 up to date with this branch and ready to be reviewed?

lilleyse · 2018-07-09T15:03:15Z

Is #6721 up to date with this branch and ready to be reviewed?

Yup, should be ready to review now.

Added Draco loading to pnts

9cfba49

lilleyse force-pushed the point-clouds-draco branch from 068629e to 9cfba49 Compare May 3, 2018 19:59

lilleyse commented May 3, 2018

View reviewed changes

Updated CHANGES.md

c5870f8

ggetz requested changes May 3, 2018

View reviewed changes

pjcozzi reviewed May 6, 2018

View reviewed changes

lilleyse added 6 commits May 7, 2018 15:20

Merge branch 'master' into point-clouds-draco

ccc04ee

Combine point cloud and model into the same draco loader

706a918

Formatting updates

f618fb0

Tweaked CHANGES.md

4ef9401

Change names of test data, update link in Sandcastle demo

929ce88

Merge branch 'master' into point-clouds-draco

b153553

lilleyse changed the base branch from stats-bug to master May 8, 2018 15:42

lilleyse added 2 commits May 8, 2018 12:02

Removed isPointCloud parameter

845846c

Revert changes in #6558

a79b93f

lilleyse force-pushed the point-clouds-draco branch from 5803b15 to a79b93f Compare May 8, 2018 16:04

Remove decodeDracoPointCloud

054fc5b

lilleyse added 2 commits May 11, 2018 14:24

Merge branch 'master' into point-clouds-draco

4156778

Revert a79b93f

ebd98ca

lilleyse mentioned this pull request May 18, 2018

Draco compression for pnts CesiumGS/3d-tiles#310

Merged

1 task

Update Draco format

8e551be

lilleyse force-pushed the point-clouds-draco branch from fded814 to 8e551be Compare May 23, 2018 03:32

lilleyse added 6 commits May 23, 2018 11:19

Support 8-bit Draco quantized points

2f940d7

Fix when batch table doesn't exist

a552a9f

Fix for batched points

99040bc

Adjustments to support a mix of draco and non-draco properties

4951e17

Update tilesets and specs

c54add9

Remove IE checks

258ebf8

pjcozzi assigned ggetz Jul 4, 2018

ggetz requested changes Jul 5, 2018

View reviewed changes

ggetz reviewed Jul 5, 2018

View reviewed changes

lilleyse added 2 commits July 5, 2018 15:17

Set isQuantized and isOctEncoded after Draco decode

d9c3dee

Fix tilesets

3e96229

Destroy decoder

8efda1b

Merge branch 'master' into point-clouds-draco

c7948c4

lilleyse mentioned this pull request Jul 5, 2018

Generate Draco and Time Dynamic Point Cloud tilesets CesiumGS/3d-tiles-validator#150

Merged

Update draco files

9e502b7

ggetz reviewed Jul 6, 2018

View reviewed changes

Move draco object creation

620d975

lilleyse mentioned this pull request Jul 8, 2018

Model bounding sphere wrong after forward axis conversion change #6784

Closed

ggetz approved these changes Jul 9, 2018

View reviewed changes

ggetz merged commit b913760 into master Jul 9, 2018

lilleyse deleted the point-clouds-draco branch July 9, 2018 14:47

Added Draco support to point clouds #6559

Added Draco support to point clouds #6559

Conversation

lilleyse commented May 3, 2018 • edited Loading

cesium-concierge commented May 3, 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

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 May 8, 2018

ggetz 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

ggetz commented Jul 5, 2018

ggetz commented Jul 5, 2018

lilleyse commented Jul 5, 2018

lilleyse commented Jul 5, 2018

lilleyse commented Jul 5, 2018

lilleyse commented Jul 5, 2018

lilleyse commented Jul 6, 2018

Choose a reason for hiding this comment

ggetz commented Jul 6, 2018

ggetz commented Jul 6, 2018

lilleyse commented Jul 8, 2018

ggetz commented Jul 9, 2018

lilleyse commented Jul 9, 2018

lilleyse commented May 3, 2018 •

edited

Loading