3D Tiles - Support batch table binary #4228

lilleyse · 2016-08-23T22:10:39Z

This adds support for a batch table binary section to b3dm, i3dm, and pnts.

To do:

Need instancedWithBatchTableBinary tileset for testing. (This causes one test to fail)
~~Possibly remove handleTranslucent check for pnts, should be able to support transparent and opaque together.~~

pjcozzi · 2016-08-24T12:24:09Z

In the Sandcastle example with the new batched point cloud, picking works well when zoomed out, but when zoomed in, it often flickers between the back and front faces of the sphere as the mouse moves, which is expected.

No action now, but when we do, for example, the edge-preserving blur, we'll need to determine how to "blur" the pick ids, e.g., think more like nearest texture filtering, not linear.

pjcozzi · 2016-08-24T12:25:52Z

Source/Core/ComponentDatatype.js

+     *
+     * @exception {DeveloperError} name is not a valid value.
+     */
+    ComponentDatatype.fromName = function(name) {


Add to CHANGES.md since this is a public class.

Could also cherry-pick this into master.

pjcozzi · 2016-08-24T13:27:48Z

Can you verify that test coverage is solid?

Looks great!

lilleyse · 2016-08-24T14:09:18Z

In the Sandcastle example with the new batched point cloud, picking works well when zoomed out, but when zoomed in, it often flickers between the back and front faces of the sphere as the mouse moves, which is expected.

I should be able to make this better by using normals and doing back-face culling on the points.

lilleyse · 2016-08-24T14:13:25Z

Ah..... I accidentally committed some large pnts tiles, I'll squash my changes with smaller versions.

lilleyse · 2016-08-25T15:41:37Z

Updated.

This also lays the foundation for dynamic styling in the shader. Here is a point cloud that stores a secondaryColor (shades of red) and temperature (3d noise values) as per-point properties in the batch table, loaded in as vertex attributes and styled in the vertex shader.

pjcozzi · 2016-08-25T19:17:57Z

This also lays the foundation for dynamic styling in the shader...

Very nice.

pjcozzi · 2016-08-25T19:18:33Z

Do you still plan to do this in this PR?

Possibly remove handleTranslucent check for pnts, should be able to support transparent and opaque together.

pjcozzi · 2016-08-25T19:23:17Z

Should the arraybuffer/view memory concerns we discussed offline be addressed in this PR or another?

pjcozzi · 2016-08-25T19:25:36Z

Source/Scene/Model.js

@@ -51,7 +51,7 @@ define([
        '../ThirdParty/gltfDefaults',
        '../ThirdParty/Uri',
        '../ThirdParty/when',
-        './getModelAccessor',
+        './getBinaryAccessor',


Can you cherry-pick this into master?

Ah yes, thanks for reminding me.

lilleyse · 2016-08-25T19:28:39Z

Do you still plan to do this in this PR?

Possibly remove handleTranslucent check for pnts, should be able to support transparent and opaque together.

I think I may pass on that for later. I'm a bit worried about the performance hit associated with doing all the pass/translucent checks per point. As of now supporting opaque and translucent together is only an issue for batched points, and maybe just setting the render state to translucent may be enough.

pjcozzi · 2016-08-25T19:28:57Z

Source/Scene/Points3DTileContent.js

+        var hasStyleableProperties = defined(styleableProperties);
+
+        // Use per-point normals to hide back-facing points.
+        var backFaceCulling = true;


Check out hidden point removal in http://www.crs4.it/vic/cgi-bin/multimedia-page.cgi?id=160

Could be a nice reading group paper when we are ready to implement it.

Also add a TODO here for how to expose this, e.g., will the Cesium API have a point cloud specific style, is this a tileset property, etc.

lilleyse · 2016-08-25T19:29:20Z

Should the arraybuffer/view memory concerns we discussed offline be addressed in this PR or another?

I'll handle it here.

pjcozzi · 2016-08-25T19:29:43Z

I think I may pass on that for later. I'm a bit worried about the performance hit associated with doing all the pass/translucent checks per point. As of now supporting opaque and translucent together is only an issue for batched points, and maybe just setting the render state to translucent may be enough.

Good plan.

pjcozzi · 2016-08-25T19:31:39Z

Source/Scene/Points3DTileContent.js

+            for (var name in styleableProperties) {
+                if (styleableProperties.hasOwnProperty(name)) {
+                    var property = styleableProperties[name];
+                    // TODO : this will not handle matrix types currently


Not part of this PR?

I can probably get something working for this, but I'm not sure of the benefits. Would the styling language even support the concept of matrices?

It could...and it likely will at some point...but I would rather roadmap this since it is not in the most promient use cases, update CesiumGS/3d-tiles#2

Added to roadmap.

pjcozzi · 2016-08-25T19:38:38Z

Source/Scene/Points3DTileContent.js

@@ -403,10 +562,21 @@ define([
            vs += '    vec3 position = a_position; \n';
        }

+        // TODO : only for testing


Keep this for now, right?

I'm going to remove it. This was mainly for anyone reviewing the PR to see how it works.

Ah, look at the shader generation in the Batch Table, but I think we want to prefix these more specifically than style_.

pjcozzi · 2016-08-25T19:41:40Z

Just those comments. Really liking the direction here.

pjcozzi · 2016-08-25T20:29:49Z

The points batched example in Sandcastle comes up blank here: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/batch-table-binary/Apps/Sandcastle/index.html?src=3D%20Tiles.html&label=Showcases

Does it for you?

lilleyse · 2016-08-26T15:25:53Z

Updated.

The points batched example in Sandcastle comes up blank here: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/batch-table-binary/Apps/Sandcastle/index.html?src=3D%20Tiles.html&label=Showcases

This works now.

The memory issues are also addressed. Model doesn't hold on to the array buffer: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Model.js#L3697.

pjcozzi · 2016-08-26T16:03:09Z

Source/Scene/Batched3DModel3DTileContent.js

@@ -238,6 +238,8 @@ define([
            if (batchTableBinaryByteLength > 0) {
                // Has a batch table binary
                batchTableBinary = new Uint8Array(arrayBuffer, byteOffset, batchTableBinaryByteLength);
+                // Copy the batchTableBinary section and let the underlying ArrayBuffer be freed
+                batchTableBinary = new Uint8Array(batchTableBinary);


Does any part of Model.js need to change?

pjcozzi · 2016-08-26T16:03:44Z

Part of #3241

pjcozzi · 2016-08-26T16:09:20Z

Source/Scene/Points3DTileContent.js

+
+        // TODO : How to expose this? Will this be part of the point cloud styling or a property of the tileset?
+        // Use per-point normals to hide back-facing points.
+        var backFaceCulling = false;


I changed this to false by default since many users will want go inside point clouds, e.g., like the church model.

pjcozzi reviewed Aug 24, 2016
View reviewed changes

lilleyse mentioned this pull request Aug 24, 2016

Added ComponentDatatype.fromName #4230

Merged

Batch table binary

1699954

lilleyse force-pushed the batch-table-binary branch 3 times, most recently from ee2dc07 to 2806d11 Compare August 25, 2016 15:24

pjcozzi reviewed Aug 25, 2016
View reviewed changes

Refactoring and updates

232234f

lilleyse force-pushed the batch-table-binary branch from e2451ee to 232234f Compare August 26, 2016 14:43

lilleyse mentioned this pull request Aug 26, 2016

Renamed getModelAccessor to getBinaryAccessor #4238

Merged

More updates

b048042

pjcozzi reviewed Aug 26, 2016
View reviewed changes

Change point backface culling to false

62a6fbd

pjcozzi reviewed Aug 26, 2016
View reviewed changes

pjcozzi merged commit af6c677 into pnts-updates Aug 26, 2016

pjcozzi deleted the batch-table-binary branch August 26, 2016 16:09

This was referenced Aug 26, 2016

Move Batch Table to after features payload (glTF, points, vector data)? CesiumGS/3d-tiles#103

Closed

3D Tiles Transform Tests #4256

Merged

mramato mentioned this pull request Sep 12, 2016

3DTiles spamming console.log with thousands of messages. #4313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3D Tiles - Support batch table binary #4228

3D Tiles - Support batch table binary #4228

lilleyse commented Aug 23, 2016 •

edited

Loading

pjcozzi commented Aug 24, 2016

pjcozzi Aug 24, 2016

pjcozzi Aug 24, 2016

pjcozzi commented Aug 24, 2016

lilleyse commented Aug 24, 2016

lilleyse commented Aug 24, 2016

lilleyse commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

pjcozzi Aug 25, 2016

lilleyse Aug 25, 2016

lilleyse commented Aug 25, 2016

pjcozzi Aug 25, 2016

pjcozzi Aug 25, 2016

lilleyse commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

pjcozzi Aug 25, 2016

lilleyse Aug 25, 2016

pjcozzi Aug 25, 2016

lilleyse Aug 25, 2016

pjcozzi Aug 25, 2016

lilleyse Aug 25, 2016

pjcozzi Aug 25, 2016

pjcozzi commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

lilleyse commented Aug 26, 2016

pjcozzi Aug 26, 2016

lilleyse Aug 26, 2016

pjcozzi commented Aug 26, 2016

pjcozzi Aug 26, 2016

3D Tiles - Support batch table binary #4228

3D Tiles - Support batch table binary #4228

Conversation

lilleyse commented Aug 23, 2016 • edited Loading

pjcozzi commented Aug 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Aug 24, 2016

lilleyse commented Aug 24, 2016

lilleyse commented Aug 24, 2016

lilleyse commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Aug 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

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

pjcozzi commented Aug 25, 2016

pjcozzi commented Aug 25, 2016

lilleyse commented Aug 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Aug 26, 2016

Choose a reason for hiding this comment

lilleyse commented Aug 23, 2016 •

edited

Loading