Invert classification #5836

bagnell · 2017-09-18T19:57:24Z

Adds invertClassification and invertClassificationColor to Scene. When invertClassification is true, any 3D Tiles geometry that is not classified will have its color multiplied by invertClassificationColor. See the Classification Sandcastle example.

cesium-concierge · 2017-09-18T19:57:25Z

@bagnell, thanks for the pull request! Maintainers, we have a signed CLA from @bagnell, 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 am a bot who helps you make Cesium awesome! Thanks again.

… test.

pjcozzi · 2017-09-18T21:18:44Z

Very nice!

@lilleyse can you please take a first pass this week and then bump to me for a quick review?

lilleyse · 2017-09-19T13:58:53Z

Source/Renderer/UniformState.js

+         * The highlight color of unclassified 3D Tiles.
+         *
+         * @memberof UniformState.prototype
+         * @type {Number}


Should be Color.

lilleyse · 2017-09-19T14:03:19Z

Source/Scene/FrameState.js

+
+        /**
+         * When <code>false</code>, 3D Tiles will render normally. When <code>true</code>, classified 3D Tile geometry will render opaque and
+         * unclassified 3D Tile geometry will render translucent with the alpha set to {@link FrameState#invertClassificationAlpha}.


This description is still referring to alpha instead of highlight color.

lilleyse · 2017-09-19T14:27:03Z

Source/Scene/Scene.js

@@ -1988,10 +2104,17 @@ define([
                us.updateFrustum(frustum);
            }

+            var invertClassification;
+            if (!picking && environmentState.useInvertClassification && scene.frameState.invertClassificationColor.alpha < 1.0) {
+                // Fullscreen pass to copy unclassified fragments when alpha < 0.0.


lilleyse · 2017-09-19T14:34:36Z

It would be nice to have a Sandcastle demo that has translucency and uses inverse classification. Maybe the New York data set with a style that applies translucency to certain buildings, and then a ground primitive covering a single city block.

lilleyse · 2017-09-19T23:34:54Z

Setting the alpha of the classification color less than 1.0 throws this error:

bagnell · 2017-09-28T21:03:57Z

@lilleyse This is ready for another look.

lilleyse · 2017-10-02T19:04:07Z

While picking in the inspector sandcastle, what should be transparent-yellow becomes invisible.

Though it renders fine if the tile contains a mix of translucent / opaque gltf, like the Translucent/Opaque tileset in http://localhost:8080/Apps/Sandcastle/index.html?src=3D%20Tiles%20Formats.html&label=3D%20Tiles. So it must be something about whether features are opaque vs. translucent.

lilleyse · 2017-10-02T19:26:29Z

Actually ignore that - I forgot to rebuild and the Pass numbers were out of sync with the shaders.

lilleyse · 2017-10-02T19:44:04Z

The new changes look good.

lilleyse · 2017-10-02T19:44:15Z

@pjcozzi this is ready for you to review when you have a chance.

pjcozzi · 2017-10-03T16:04:59Z

@bagnell can you merge and fix the conflict in Classification.html?

pjcozzi · 2017-10-03T16:13:16Z

CHANGES.md

@@ -20,6 +20,7 @@ Change Log
 * Fixed removing multiple event listeners within event callbacks. [#5827](https://github.com/AnalyticalGraphicsInc/cesium/issues/5827)
 * Running `buildApps` now creates a built version of Sandcastle which uses the built version of Cesium for better performance.
 * Fixed a tileset traversal bug when the `skipLevelOfDetail` optimization is off. [#5869](https://github.com/AnalyticalGraphicsInc/cesium/issues/5869)
+* Adds `invertClassification` and `invertClassificationColor` to `Scene`. When `invertClassification` is `true`, any 3D Tiles geometry that is not classified will have its color multiplied by `invertClassificationColor`. [#5836](https://github.com/AnalyticalGraphicsInc/cesium/pull/5836)


Does this also apply to the classification primitives? I think so, right? If so, then update this.

Otherwise, if this is 3D Tiles specific, maybe rename to invert3DTilesClassification and invert3DTilesClassificationColor?

Longer-term, would this move to per-primitive/tileset properties that could use the bounding volume as a scissor? Maybe an event could notify the scene when these are enabled so that the main rendering pipeline still has the information it needs.

Updated. It applies to ClassificationPrimitive and GroundPrimitive.

Yes, longer-term we could move this to per-primitive/tileset properties.

bagnell · 2017-10-03T18:24:00Z

@pjcozzi The merge conflict is resolved and CHANGES.md has been updated.

pjcozzi · 2017-10-03T19:56:36Z

The Sandcastle example is good, but it is not 100% clear what the invert is doing since it goes from colored selection to darkened invert. Can the select and invert colors be the same? Not sure what to do about the colored trees - maybe this should be two Sandcastle examples or this example should have a drop down instead of buttons and the check box?

pjcozzi · 2017-10-03T19:57:06Z

Also, can you add an invert alpha example?

pjcozzi · 2017-10-03T19:59:14Z

Tests pass and profile looks OK.

pjcozzi · 2017-10-03T20:01:40Z

Source/Scene/Cesium3DTileBatchTable.js

@@ -1342,8 +1342,12 @@ define([
            // selection depth to the stencil buffer to prevent ancestor tiles from drawing on top
            derivedCommand = DrawCommand.shallowClone(command);
            var rs = clone(derivedCommand.renderState, true);
+            // Stencil test is masked to the most significant 4 bits so the reference is shifted.


For my knowledge, remind me what limitation the 4 bits creates, e.g., can't have more than 16 tilesets in the same pixel?

See this comment when traversing 3D Tiles with the skip LOD optimization: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Cesium3DTilesetTraversal.js#L167

Instead of a depth of 255, the depth would be limited to 15.

An unlikely, but potentially possible case now, right? But this is limited to just classification?

At the least, please add appropriate comments if you haven't already in case we need to investigate this in the future.

It's still very unlikely the selection depth will reach 15. I'll add a comment.

pjcozzi · 2017-10-03T20:02:11Z

Source/Scene/ClassificationPrimitive.js

@@ -344,6 +347,9 @@ define([
        return scene.context.stencilBuffer;
    };

+    var stencilReference = 0;
+    var stencilMask = 0x0F;


Perhaps comment where this came from.

pjcozzi · 2017-10-03T20:05:06Z

Source/Scene/InvertClassification.js

+        }
+    });
+
+    InvertClassification.isTranslucencySupported = function(context) {


Is this requirement/limitation mentioned in the reference doc?

pjcozzi · 2017-10-03T20:05:19Z

Source/Scene/InvertClassification.js

+    };
+
+    var stencilReference = 0;
+    var stencilMask = 0x0F;


Same comment.

pjcozzi · 2017-10-03T20:13:11Z

Source/Shaders/Builtin/Constants/passCesium3DTileClassificationIgnoreShow.glsl

+ *
+ * @see czm_pass
+ */
+const float czm_passCesium3DTileClassificationIgnoreShow = 6.0;


Is this used anywhere?

No. The only czm_pass* constant that is used is czm_passTranslucent.

So should this be removed?

These constants mirror the Pass enum. I'm OK with removing this because it's not used, but then why not remove the rest?

pjcozzi · 2017-10-03T20:14:11Z

Source/Scene/InvertClassification.js

+    /**
+     * @private
+     */
+    function InvertClassification() {


Is it reasonable to have a unit test for this file?

If not, how is the current coverage?

Coverage is at 91%. The only thing not covered is the destroy function.

pjcozzi · 2017-10-03T20:14:31Z

@bagnell this looks good, just those comments.

bagnell · 2017-10-18T20:25:01Z

@pjcozzi This is ready for another look.

pjcozzi · 2017-10-18T21:38:08Z

Sandcastle example looks great!

@lilleyse can you make a final pass and merge?

lilleyse · 2017-10-19T19:43:53Z

Looks good!

bagnell added 2 commits September 18, 2017 15:51

Add option to invert classification.

8e7b0a6

Merge branch 'master' into invert-classification

950afbf

bagnell added 4 commits September 18, 2017 15:58

Update CHANGES.md.

f56364c

Fix tests.

87088d4

Fix blinking when using skip LODs. Missing update to 3D Tiles stencil…

601e050

… test.

Add comment about 3D Tiles stencil test.

24582c3

pjcozzi requested a review from lilleyse September 18, 2017 21:18

bagnell mentioned this pull request Sep 18, 2017

Invertable classification with alpha #5755

Merged

lilleyse reviewed Sep 19, 2017

View reviewed changes

bagnell added 4 commits September 28, 2017 15:47

Merge branch 'master' into invert-classification

bd5f009

Updates from review.

3bdf457

Fix crash.

a89214c

Update doc.

a05fc1c

bagnell added 2 commits October 2, 2017 14:39

Merge branch 'master' into invert-classification

94eb17b

Update Sandcastle example after merge.

abaed6e

pjcozzi reviewed Oct 3, 2017

View reviewed changes

bagnell added 3 commits October 3, 2017 14:16

Merge branch 'master' into invert-classification

a2b12fb

Update Sandcastle example after merge.

b9ab3a8

Update CHANGES.md.

9f4cab0

pjcozzi reviewed Oct 3, 2017

View reviewed changes

Source/Scene/InvertClassification.js Outdated

};

var stencilReference = 0;

var stencilMask = 0x0F;

Copy link

Contributor

pjcozzi Oct 3, 2017

Choose a reason for hiding this comment

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

Same comment.

pjcozzi reviewed Oct 3, 2017

View reviewed changes

bagnell added 4 commits October 18, 2017 14:57

Merge branch 'master' into invert-classification

2a53685

Updates from review.

5f60fa3

Update Sandcastle example.

d617556

Merge branch 'master' into invert-classification

d89350b

bagnell added 2 commits October 18, 2017 17:41

Update comment.

87289df

Merge branch 'master' into invert-classification

c4e886f

lilleyse merged commit d568775 into master Oct 19, 2017

lilleyse deleted the invert-classification branch October 19, 2017 19:43

Invert classification #5836

Invert classification #5836

Conversation

bagnell commented Sep 18, 2017

cesium-concierge commented Sep 18, 2017

pjcozzi commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Sep 19, 2017

lilleyse commented Sep 19, 2017 • edited Loading

bagnell commented Sep 28, 2017

lilleyse commented Oct 2, 2017

lilleyse commented Oct 2, 2017 • edited Loading

lilleyse commented Oct 2, 2017

lilleyse commented Oct 2, 2017

pjcozzi commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagnell commented Oct 3, 2017

pjcozzi commented Oct 3, 2017

pjcozzi commented Oct 3, 2017

pjcozzi commented Oct 3, 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

Choose a reason for hiding this comment

pjcozzi commented Oct 3, 2017

bagnell commented Oct 18, 2017

pjcozzi commented Oct 18, 2017

lilleyse commented Oct 19, 2017

lilleyse commented Sep 19, 2017 •

edited

Loading

lilleyse commented Oct 2, 2017 •

edited

Loading