Vector Tiles #4665

bagnell · 2016-11-18T00:51:47Z

Adds vector tile (polygons, polylines, and points), and geometry tile (boxes, cylinders, ellipsoids and spheres) for classifying 3D Tiles and terrain.
Adds classificationType to Cesium3DTileset so a b3dm tileset can classify other 3D Tiles and/or terrain.

Future work:

Geometry instancing
Polygon z-order

pjcozzi · 2016-11-22T21:59:52Z

@bagnell please update CesiumGS/3d-tiles#124 to keep the implementation and spec in sync.

pjcozzi · 2016-11-22T22:07:58Z

Apps/Sandcastle/gallery/3D Tiles - Vector Tiles.html

+viewer.scene.globe.depthTestAgainstTerrain = true;
+
+var tileset = scene.primitives.add(new Cesium.Cesium3DTileset({
+    url : 'http://localhost:8002/tilesets/VectorTile-Test/',


Add changing this to the tasklist if it isn't already.

pjcozzi · 2016-11-22T22:13:24Z

Source/Scene/Cesium3DTileFeature.js

@@ -41,12 +45,21 @@ define([
     *     }
     * }, Cesium.ScreenSpaceEventType.MOUSE_MOVE);
     */
-    function Cesium3DTileFeature(tileset, content, batchId) {
+    function Cesium3DTileFeature(tileset, content, batchId, billboardCollection, labelCollection) {


These changes make Cesium3DTileFeature really heavy especially for all the non-vector features that do not need most of these properties. Is it possible to cleanly add a Cesium3DTileVectorFeature so we only pay this cost when needed? Is it also possible to amortize some properties over multiple features, e.g., do all features with the same batch table also have the same billboard and label collection?

pjcozzi · 2016-11-22T22:14:25Z

Source/Scene/Cesium3DTileFeature.js

                return this._batchTable.getShow(this._batchId);
            },
            set : function(value) {
-                this._batchTable.setShow(this._batchId, value);
+                if (defined(this._labelCollection)) {


Style-wise throughout when members like this._labelCollection and this._batchId are used more than once in a function assign them to a local.

pjcozzi · 2016-11-22T22:15:40Z

Source/Scene/Cesium3DTileFeature.js

@@ -92,17 +116,301 @@ define([
         */
        color : {
            get : function() {
+                if (defined(this._labelCollection)) {
+                    var label = this._labelCollection.get(this._batchId);


What is this get function needed? Why can't we always keep reference to the label?

pjcozzi · 2016-11-22T22:16:25Z

Source/Scene/Cesium3DTileFeature.js

+        /**
+         * Gets and sets the point size of this feature.
+         * <p>
+         * Only applied when the feature is a point feature and <code>image</code> is <code>undefined</code>.


Does this mean "a point feature in a vector tile?"

pjcozzi · 2016-11-22T22:19:28Z

Source/Scene/Cesium3DTileFeature.js

+            },
+            set :function(value) {
+                this._pointSize = value;
+                if (defined(this._billboardCollection)) {


Is it worth optimizing for special cases when a PointPrimitiveCollection could be used? Seems like it would be hard to manage when setting some properties at runtime could cause it to split into a billboard collection. Add a PERFORMANCE_IDEA / roadmap item if you think this would be reasonable and useful for some cases.

pjcozzi · 2016-11-22T22:22:00Z

Source/Scene/Cesium3DTileFeature.js

+
+                    if (defined(value) && value !== '') {
+                        var billboard = this._billboardCollection.get(this._batchId);
+                        billboard.horizontalOrigin = HorizontalOrigin.RIGHT;


This doesn't put the label on top of the billboard?

pjcozzi · 2016-11-22T22:26:51Z

Source/Scene/Cesium3DTileFeature.js

+        b.setImage(textureId, createCallback(centerAlpha, cssColor, cssOutlineColor, newOutlineWidth, newPointSize));
+    }
+
+    function createCallback(centerAlpha, cssColor, cssOutlineColor, cssOutlineWidth, newPixelSize) {


This is from https://github.com/AnalyticalGraphicsInc/cesium/blob/4c6b691b4327a16a6065f5432d33471cddb7978a/Source/DataSources/PointVisualizer.js#L306, right? Can we share it as a @private function?

pjcozzi · 2016-11-22T22:27:24Z

Source/Scene/Cesium3DTileStyle.js

    'use strict';

    var DEFAULT_JSON_COLOR_EXPRESSION = 'color("#ffffff")';
+    var DEFAULT_JSON_OUTLINE_COLOR_EXPRESSION = 'color("#000000")';


Please update the styling spec when you update the 3D Tiles spec.

pjcozzi · 2016-11-22T22:27:49Z

Source/Scene/Cesium3DTileStyle.js

    var DEFAULT_JSON_BOOLEAN_EXPRESSION = true;
    var DEFAULT_JSON_NUMBER_EXPRESSION = 1.0;
+    var DEFAULT_LABEL_STYLE_EXPRESSION = LabelStyle.FILL;
+    var DEFAULT_FONT_EXPRESSION = '"30px sans-serif"';


Are you sure about this default here and below?

pjcozzi · 2016-11-22T22:35:27Z

Source/Scene/Cesium3DTileStyle.js

+         *
+         * @type {StyleExpression}
+         *
+         * @exception {DeveloperError} The style is not loaded.  Use Cesium3DTileStyle.readyPromise or wait for Cesium3DTileStyle.ready to be true.


Probably use {@link } here and throughout for the references.

pjcozzi · 2016-11-22T22:41:14Z

Source/Scene/Cesium3DTileStyle.js

+         *
+         * @example
+         * var style = new Cesium3DTileStyle({
+         *     image : '(${Temperature} > 90) ? "/url/to/image1" : "/url/to/image2"'


When you document the styling spec, this should state the validate image formats - it should be the same as those supported by the browser (probably including svg).

Also, please add a tasklist item to the spec PR to consider if this should be able to point to a texture atlas / glTF-style buffer/bufferView.

Probably something to consider for v2, but let's discuss in the context of the styling PR.

How to use it?

pjcozzi · 2016-11-22T22:42:58Z

Source/Scene/Cesium3DTileStyle.js

+         *
+         * @see {@link https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/master/Styling|3D Tiles Styling language}
+         */
+        outlineColor : {


Can you run coverage for these get/set functions? Most of them are not covered.

Same comment for new properties in Cesium3DTileFeature.js

Coverage is also pretty low for Vector3DTileContent.js at 85%

pjcozzi · 2016-11-22T22:49:11Z

Source/Scene/GroundPolylineBatch.js

+                shaderProgram : primitive._sp,
+                uniformMap : uniformMap,
+                boundingVolume : primitive._boundingVolume,
+                //pass : Pass.GROUND


Does this need a TODO so we add the GROUND_TRANSLUCENT pass before merging 3d-tiles into master?

pjcozzi · 2016-11-22T22:50:59Z

Source/Scene/GroundPrimitiveBatch.js

+        var batchedIndices = this._batchedIndices;
+        var length = batchedIndices.length;
+
+        var i;


Declare below inside the loop, it isn't used elsewhere, right?

Its used outside the loop as the found index.

pjcozzi · 2016-11-22T22:54:13Z

Source/Scene/Vector3DTileContent.js

+            var features = new Array(featuresLength);
+            for (var i = 0; i < featuresLength; ++i) {
+                if (defined(content._billboardCollection) && i < content._billboardCollection.length) {
+                    var billboardCollection = content._billboardCollection;


Could move this above and use the local in the if statement.

pjcozzi · 2016-11-22T22:57:02Z

Source/Scene/Vector3DTileContent.js

+            }
+        }
+
+        var batchId;


Is this local really needed?

pjcozzi · 2016-11-22T22:58:08Z

Source/Scene/Vector3DTileContent.js

+                Cartesian3.add(position, center, position);
+
+                var b = this._billboardCollection.add();
+                b.position = position;


Here and below, why not pass these to add? To avoid the object literal? I dunno, maybe that optimization is too soon given everything else this function does.

pjcozzi · 2016-11-22T22:59:06Z

Source/Scene/Vector3DTileContent.js

+            this._polylines.applyDebugSettings(enabled, color);
+        }
+
+        //TODO: debug settings for points/billboards/labels


Is this trivial to do now? If no, let's wait until the merge to 3d-tiles.

pjcozzi · 2016-11-22T23:01:10Z

I think I mentioned this in another PR, but it would be useful to have tests for the new @private batch primitives.

pjcozzi · 2016-11-22T23:01:16Z

Just those comments.

pjcozzi · 2016-11-23T13:18:44Z

Also, update CHANGES.md with any new classes: https://github.com/AnalyticalGraphicsInc/cesium/blob/3d-tiles/CHANGES.md

pjcozzi · 2017-01-04T11:56:38Z

Added

Update GroundPrimitiveBatch to use a dynamic bottom cap like Extend GroundPrimitive shadow volume based on screen space error #4751

to the tasklist.

Use b3dm tilesets for classification

lilleyse · 2017-12-20T14:13:56Z

Apps/Sandcastle/gallery/3D Tiles Point Cloud Classification.html

+    // Highlight the feature
+    highlighted.feature = pickedFeature;
+    Cesium.Color.clone(pickedFeature.color, highlighted.originalColor);
+    pickedFeature.color = Cesium.Color.WHITE.withAlpha(0.5);


Should the color be yellow instead?

lilleyse · 2017-12-20T14:25:42Z

Apps/Sandcastle/gallery/3D Tiles Photogrammetry Classification.html

@@ -0,0 +1,85 @@
+<!DOCTYPE html>


Will there be any Sandcastle examples using vector tiles in the more traditional sense - labels, billboards, polylines, etc? Also it's not completely clear which of the new demos are using b3dm vs. geom vs. vctr for classification. Each should have a comment about what tile format they are using.

I added a comment to each example.

The 3D Tiles Terrain Classification example is traditional vector tiles (polygons draped on terrain). There are no polylines or points/billboards/labels. This is the best I can do until I find better open data and better tiling.

I opened an issue to keep track of this: #6095

lilleyse · 2017-12-20T14:38:58Z

Apps/Sandcastle/gallery/3D Tiles Terrain Classification.html

+    }
+}, Cesium.ScreenSpaceEventType.MOUSE_MOVE);
+
+// Color a feature on selection and show metadata in the InfoBox.


Remove note about the InfoBox.

lilleyse · 2017-12-20T14:41:27Z

Apps/Sandcastle/gallery/3D Tiles Terrain Classification.html

+
+    // Highlight newly selected feature
+    pickedFeature.color = Cesium.Color.LIME;
+}, Cesium.ScreenSpaceEventType.LEFT_CLICK);


There should be a comment explaining why the selected feature doesn't stay selected when you zoom in and out, and link to CesiumGS/3d-tiles#265.

lilleyse · 2017-12-20T14:45:07Z

CHANGES.md

+   * `Cesium3DTileStyle` has expanded for styling point features. See the [styling specification](https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/vector-tiles/Styling#vector-data) for details.
+   * `Cesium3DTileFeature` can modify `color` and `show` properties for polygon, polyline, and geometry features.
+   * `Cesium3DTilePointFeature` can modify the styling options for a point feature.
+* Added `Cesium3DTileset.classificationType` to specify if a tileset classifies terrain, another 3D Tiles tileset, or both. This only applies to vector, geometry and batched 3D model tilesets. See [#6033](https://github.com/AnalyticalGraphicsInc/cesium/pull/6033) for limitations on the glTF contained in the b3dm tile.


Should the b3dm restrictions be mentioned here in CHANGES.md instead of linking to a PR?

lilleyse · 2017-12-22T19:47:05Z

Source/Scene/ClassificationModel.js

+            var rotation = Quaternion.unpack(gltfNode.rotation);
+            var scale = Cartesian3.fromArray(gltfNode.scale);
+            model._nodeMatrix = Matrix4.fromTranslationQuaternionRotationScale(translation, rotation, scale, model._nodeMatrix);
+        }


Can this section use ModelUtility.getTransform?

lilleyse · 2017-12-22T19:51:39Z

Source/Scene/ClassificationModel.js

+                model._rtcCenter = model._rtcCenter3D;
+            } else {
+                var center = model.boundingSphere.center;
+                var to2D = Transforms.wgs84To2DModelMatrix(projection, center, scratchComputedMatrixIn2D);


I tested 3D Tiles Photogrammetry Classification in 2D and CV and was happy to see that it works in both. Though switching modes causes the classification to be unaligned - probably related to #5955.

Yeah, I think it's the same problem.

lilleyse · 2017-12-22T19:53:32Z

Source/Scene/Geometry3DTileContent.js

+    when,
+    Cesium3DTileBatchTable,
+    Vector3DTileGeometry) {
+    'use strict';


Fix indentation on these includes.

lilleyse · 2017-12-22T20:04:54Z

Source/Scene/Vector3DTilePoints.js

+                return;
+            }
+
+            when(verticesPromise, function(result) {


Could this just be verticesPromise.then?

lilleyse · 2017-12-22T20:10:35Z

Source/Shaders/Builtin/Constants/passClassification.glsl

+ *
+ * @see czm_pass
+ */
+const float czm_passClassification = 8.0;


This should be 7.

lilleyse · 2017-12-22T20:28:22Z

Throughout any code related to pointColor needs to be just color, as reflected in the spec.

lilleyse · 2017-12-22T21:15:47Z

A couple new files have slightly low coverage:

…tion debug wireframe.

bagnell · 2018-01-02T22:45:12Z

@lilleyse This is ready for another look. I'm not sure why the tests are failing. They pass (both regular and the stub) locally.

…abelVerticalOrigin styles.

bagnell · 2018-01-04T20:10:19Z

This is now in sync with the spec.

…es or when used for classification.

lilleyse

Just one comment.

lilleyse · 2018-01-06T03:22:36Z

Source/Scene/Cesium3DTileset.js

+         * @type {Ellipsoid}
+         * @readonly
+         */
+        ellipsoid : {


This should be documented in CHANGES.md.

lilleyse · 2018-01-06T03:37:40Z

Does anyone else want to review? I think this is in good shape to merge on Monday otherwise.

pjcozzi · 2018-01-06T04:10:12Z

No need for me to look at this again.

bagnell · 2018-01-08T19:30:42Z

@lilleyse Updated.

lilleyse · 2018-01-08T21:14:31Z

Awesome! This was a big undertaking and I'm glad it's finally ready.

The two failing tests in the latest build are unrelated (VideoSynchronizer related)

cesium-concierge · 2018-01-08T21:14:41Z

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/eXH90obYoSs

If this issue affects any of these threads, please post a comment like the following:

The issue at #4665 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

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

🌍 🌎 🌏

pjcozzi reviewed Nov 22, 2016

View reviewed changes

Source/Scene/Vector3DTileContent.js Outdated

}

}

var batchId;

Copy link

Contributor

pjcozzi Nov 22, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this local really needed?

pjcozzi reviewed Nov 22, 2016

View reviewed changes

bagnell added 6 commits June 12, 2017 17:12

Change positionOffset to heightOffset, fix displaying anchor lines.

ada9029

Merge branch '3d-tiles-vector' into vector-tiles

ba5197d

Merge branch '3d-tiles-vector' into vector-tiles

7f642b8

Fixes after merge.

7e1297d

Merge branch '3d-tiles-vector' into vector-tiles

3171e6f

Fix picking vector tiles on 3D tiles.

6f30747

Merge pull request #6033 from AnalyticalGraphicsInc/b3dm-class

7d27a23

Use b3dm tilesets for classification

lilleyse reviewed Dec 22, 2017

View reviewed changes

bagnell added 5 commits December 28, 2017 15:33

Updates from review. Add arraySlice tests. Fix lower level classifica…

6188233

…tion debug wireframe.

More updates from review.

d42f320

Rename pointColor -> color.

b407913

Add tests for uncovered code.

459639a

Add exception tests for vector and geometry tiles.

bf9572d

bagnell added 4 commits January 3, 2018 15:47

Merge branch '3d-tiles-vector' into vector-tiles

0f5b30e

Update horizontalOrigin, verticalOrigin, labelHorizontalOrigin, and l…

5587f89

…abelVerticalOrigin styles.

Add positive infinity constant for styling.

695a60d

Align with revisions to spec. Update parsing and test tiles.

09a8fd2

lilleyse mentioned this pull request Jan 5, 2018

More Vector Tiles Sandcastle demos #6095

Open

bagnell added 2 commits January 5, 2018 15:41

Make skipLevelOfDetail a hint. Disable when using vector/geometry til…

fe81890

…es or when used for classification.

Merge branch '3d-tiles-vector' into vector-tiles

8350f31

lilleyse reviewed Jan 6, 2018

View reviewed changes

Source/Scene/Cesium3DTileset.js

* @type {Ellipsoid}

* @readonly

*/

ellipsoid : {

Copy link

Contributor

lilleyse Jan 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be documented in CHANGES.md.

Update CHANGES.md.

3340438

bagnell added 2 commits January 8, 2018 15:55

Update test tilesets to be uncompressed.

b40c2a9

Revert commits to Sandcastle example.

c379c63

lilleyse merged commit 4361ec7 into 3d-tiles-vector Jan 8, 2018

lilleyse deleted the vector-tiles branch January 8, 2018 21:14

bagnell mentioned this pull request Jan 8, 2018

Vector Tiles #6097

Merged

Vector Tiles #4665

Vector Tiles #4665

Conversation

bagnell commented Nov 18, 2016 • edited Loading

pjcozzi commented Nov 22, 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

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

pjcozzi commented Nov 22, 2016

pjcozzi commented Nov 22, 2016

pjcozzi commented Nov 23, 2016

pjcozzi commented Jan 4, 2017

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 commented Dec 22, 2017

lilleyse commented Dec 22, 2017

bagnell commented Jan 2, 2018 • edited Loading

bagnell commented Jan 4, 2018

lilleyse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Jan 6, 2018

pjcozzi commented Jan 6, 2018

bagnell commented Jan 8, 2018

lilleyse commented Jan 8, 2018

cesium-concierge commented Jan 8, 2018

bagnell commented Nov 18, 2016 •

edited

Loading

bagnell commented Jan 2, 2018 •

edited

Loading