Pick translucent geometry #4979

bagnell · 2017-02-09T22:03:04Z

Adds Scene.pickTranslucentDepth which can enable picking the depth of translucent pixels.

Decreases performance as the scene has to be rendered again where translucent fragments write depth. It may not be as bad because we use a culling frustum and scissor test for a single pixel.

Test performance
Add doc
Add tests
Update CHANGES.md

#4897 #4511 #4678

pjcozzi · 2017-02-09T22:21:07Z

CC @pmconne

Part of #4678
Part of #4897

pjcozzi · 2017-02-09T22:25:35Z

Source/Scene/Scene.js

@@ -2727,6 +2799,9 @@ define([
        var uniformState = context.uniformState;

        var drawingBufferPosition = SceneTransforms.transformWindowToDrawingBuffer(this, windowPosition, scratchPosition);
+        if (this.pickTranslucentDepth) {
+            renderForPickDepth(this, drawingBufferPosition);


Perhaps rename to renderTranslucentDepthForPick?

Are you sure pickTranslucentDepth need to be a member of Scene? It is only checked here.

Are you sure pickTranslucentDepth need to be a member of Scene?

Yes, unless you have a better suggestion. We call pickPosition in ScreenSpaceCameraController for panning, zooming, terrain collision, etc. If we don't have the option on Scene, would we do it by default?

pjcozzi · 2017-02-09T22:28:29Z

Source/Scene/Scene.js

+        // Update with previous frame's number and time, assuming that render is called before picking.
+        updateFrameState(scene, frameState.frameNumber, frameState.time);
+        frameState.cullingVolume = getPickCullingVolume(scene, drawingBufferPosition, 1, 1);
+        frameState.passes.render = true;


Is it better for this to be

frameState.passes.pick = true;

Or to do whatever the shadow map pass does since they only writes depth.

If doc, please add a comment.

Another thing to consider here is that we do not want a pick to cause 3D Tiles to make new requests or process new tiles (which could happen because the cull volume is different) so this needs to be pick/depth or we need to signal to pause requests.

The shadow map pass happens during the render pass. It uses derived commands that disables color writes and enables depth writes. Also, the framebuffer only has a depth attachment.

I could change it to pick, but the depth copies for each frustum only happen during the render pass. Another option is to add a new depth pass.

the depth copies for each frustum only happen during the render pass

I'm not following. What depth copies?

Another option is to add a new depth pass

Could work, but what about this: could we just derive commands from the previous frame's commands? Instead of calling all the update functions, we rely on the state of the frustums/commands from the previous frame. We would need to guarantee that WebGL resources pointed to by the commands were not deleted, we could even rename the function to indicate this.

What depth copies?

We copy the frustum depth to a texture after each frustum is rendered here.

I've updated this in e522ffe. There wasn't a need for derived commands. Everything is updated for the pick and depth pass which essentially queues pick commands for each command that was used in the previous render pass.

Do 3D Tiles work this way or does it select new tiles on pick?

pjcozzi · 2017-02-09T22:29:24Z

Source/Scene/Scene.js

@@ -2696,6 +2706,68 @@ define([
        return object;
    };

+    var scratchPickDepthPassState;
+
+    function renderForPickDepth(scene, drawingBufferPosition) {


Perhaps add a PERFORMANCE_IDEA about rendering translucent only and merging with the previous frame; I still think it could work if it checks the state is in sync.

pjcozzi · 2017-02-09T22:31:45Z

Source/Scene/Scene.js

+
+        var passState = scratchPickDepthPassState;
+        if (!defined(passState)) {
+            passState = scratchPickDepthPassState = new PassState(context);


What if the app has more than one context? Should scratchPickDepthPassState be part of the Scene?

pjcozzi · 2017-02-09T22:34:23Z

Source/Scene/Scene.js

+        var width = context.drawingBufferWidth;
+        var height = context.drawingBufferHeight;
+
+        var framebuffer = scene._pickDepthFramebuffer;


_pickDepthFramebuffer should be declared in the constructor function.

pjcozzi · 2017-02-09T22:35:33Z

Looks good. I expect performance will be reasonable.

bagnell · 2017-02-13T20:46:46Z

The performance is OK. The function is still bound by readPixels.

bagnell · 2017-02-15T00:02:20Z

@pjcozzi This is ready for another look.

pjcozzi · 2017-02-15T02:11:18Z

Do 3D Tiles work this way or does it select new tiles on pick?

3D Tiles use the same tiles for picking.

pjcozzi · 2017-02-15T02:33:16Z

Source/Scene/Scene.js

+        frameState.passes.pick = true;
+        frameState.passes.depth = true;
+
+        us.update(frameState);


Offline we discussed using the commands from the previous frame. Why didn't you take that approach? Because some primitives will want to generate different commands here? Which precisely since pick ids won't matter.

If we stick with this approach, let's add a PERFORMANCE_IDEA about reusing the commands.

I've been thinking about this in three levels of reuse/efficiency:

The most efficient is what was previously done; reuse the previous frame's depth buffer. This is not possible unless the render pass writes an extra depth buffer with translucent primitives.

Reuse the previous frame's commands. From a GL perspective, this is no more efficient, but it saves CPU overhead in Cesium.

Reuse nothing from the previous frame, and explicitly update/render.

pjcozzi · 2017-02-15T02:37:49Z

Source/Scene/Scene.js

+        var height = context.drawingBufferHeight;
+
+        var framebuffer = scene._pickDepthFramebuffer;
+        var pickDepthFBWidth = scene._pickDepthFramebufferWidth;


Is this and height below declared in the constructor function?

pjcozzi · 2017-02-15T02:38:45Z

Source/Scene/Scene.js

@@ -294,6 +302,9 @@ define([
        this._pickDepths = [];
        this._debugGlobeDepths = [];

+        this._pickDepthPassState = undefined;
+        this._pickDepthFramebuffer = undefined;


Should this be destroyed in the Scene's destroy function?

pjcozzi · 2017-02-15T02:39:41Z

Tweaked reference doc in be613f9.

pjcozzi · 2017-02-15T02:40:44Z

Also merge in master.

…om executing on depth only pass.

…s enabled, and color writes disabled. Revert PassState depth mask overwrites.

bagnell · 2017-02-15T22:01:14Z

@pjcozzi This is ready for another look.

I updated this to use the commands from the previous frame and generate derived commands like we discussed.

A simple way to test is to click the 'pick position' button in the picking Sandcastle example and replace the model entity code with:

    viewer.scene.pickTranslucentDepth = true;
    var modelEntity = viewer.entities.add({
        name : 'milktruck',
        position : Cesium.Cartesian3.fromDegrees(-123.0744619, 44.0503706),
        model : {
            uri : '../../SampleData/models/CesiumMilkTruck/CesiumMilkTruck-kmc.gltf',
            color : Cesium.Color.WHITE.withAlpha(0.5),
            colorBlendAmount : 0.5,
            colorBlendMode : Cesium.ColorBlendMode.MIX
        }
    });

pjcozzi · 2017-02-16T04:04:24Z

Source/Scene/Scene.js

+            var attributeLocations = shaderProgram._attributeLocations;
+            var fs = shaderProgram.fragmentShaderSource;
+
+            var writesFragDepth = false;


Perhaps combine this into one: writesDepthOrDiscards and simplify the logic below?

pjcozzi · 2017-02-16T04:06:01Z

Source/Scene/Scene.js

+    var fragDepthRegex = /\bgl_FragDepthEXT\b/;
+    var discardRegex = /\bdiscard\b/;
+
+    function getDepthOnlyShaderProgram(scene, context, shaderProgram) {


Perhaps add a comment here or somewhere that this is an optimization because we're not sure that drivers will replace the fragment shader when it doesn't discard/write-depth when the color mask is all falses and the framebuffer doesn't have a color attachment.

Who knows in the future, this might not be required.

pjcozzi · 2017-02-16T04:11:07Z

Source/Scene/Scene.js

+                attributeLocations : attributeLocations
+            });
+
+            cache[id] = shader;


Is it a concern that this cache always grows and never shrinks? Should it be possible for the original shader program to basically own this and free it when the ref count goes to zero? Instead of being an object literal, this could be its own shader program cache instance if useful.

Also, is all this being shared with the shadow map pass code?

Is it a concern that this cache always grows and never shrinks?

Yes, the derived commands for OIT are using a separate cache and has the same problem.

Also, is all this being shared with the shadow map pass code?

No. The shadow map derived commands don't have a cache. The source is generated each time the shader on the command changes. Also, it is never released.

pjcozzi · 2017-02-16T04:14:06Z

Specs/Scene/SceneSpec.js

+        });
+
+        scene.pickTranslucentDepth = true;
+        expect(scene).toRenderAndCall(function() {


Should this do a second pick and spy on each one or do some other check to make sure the cached derived commands work OK?

pjcozzi · 2017-02-16T04:15:28Z

This is super close.

…pth only derived commands.

…es. Updated depth only and OIT derived commands.

bagnell · 2017-02-17T00:15:21Z

@pjcozzi This is ready for another look.

pjcozzi · 2017-02-18T16:11:53Z

Source/Renderer/ShaderCache.js

@@ -138,14 +139,86 @@ define([
        return cachedShader.shaderProgram;
    };

+    ShaderCache.prototype.getDerivedShaderProgram = function(shaderProgram, keyword) {


Add unit tests for the new stuff here.

pjcozzi · 2017-02-18T16:14:09Z

Can you merge this into 3d-tiles and test before we merge this into master? Also, if any of the 3D Tiles renderer needs to be hooked up to this (e.g., for translucent/opaque styling), it would be good to do that right away in case it impacts the interface.

bagnell · 2017-02-22T19:28:49Z

Can you merge this into 3d-tiles and test before we merge this into master?

I tested in the 3d-tiles-pick-translucent branch. Everything works well. There wasn't any need to change the styling to use the cache because the derived commands only modify the render state and uniform map. The original commands have the modified shaders through the callbacks.

This should be ready to go. I can merge 3d-tiles-pick-translucent to 3d-tiles now or when this gets merged.

pjcozzi · 2017-02-22T19:46:58Z

Great! Please merge into 3d-tiles now however you see fit.

bagnell added 3 commits February 9, 2017 16:28

Add option to pick translucent geometry at a performance cost.

e4d1557

Remove commented out code.

832b36c

Fix whitespace.

002e2d9

pjcozzi reviewed Feb 9, 2017

View reviewed changes

Merge branch 'master' into pick-translucent

e43dc2e

bagnell added 5 commits February 13, 2017 17:31

Updates from review.

e4ed730

Add depth pass.

e522ffe

Add doc.

6d94f35

Add pick translucent test.

963e631

Update CHANGES.md.

0e2fb9c

pjcozzi reviewed Feb 15, 2017

View reviewed changes

Update reference doc

be613f9

bagnell added 5 commits February 15, 2017 14:25

Merge branch 'master' into pick-translucent

d3cba1c

Update doc after merge.

3bb0a9e

Use previous frame's commands. Disable compute and shadow commands fr…

a49219b

…om executing on depth only pass.

Create derived command with pass through fragment shader, depth write…

eb56783

…s enabled, and color writes disabled. Revert PassState depth mask overwrites.

Clean up resources and fix tests.

47efc79

pjcozzi reviewed Feb 16, 2017

View reviewed changes

bagnell added 4 commits February 16, 2017 14:02

Simplify code from review and add a comment about optimization for de…

547216f

…pth only derived commands.

Update test to ensure derived commands are cached.

94c3053

Add global derived shader cache that properly releases shader resourc…

ab65c94

…es. Updated depth only and OIT derived commands.

Change how derived command cache is implemented. Add support to shadows.

864433d

pjcozzi mentioned this pull request Feb 18, 2017

Apply color palette on render. #4953

Closed

7 tasks

Whitespace

e0c769b

pjcozzi reviewed Feb 18, 2017

View reviewed changes

bagnell added 2 commits February 22, 2017 13:57

Merge branch 'master' into pick-translucent

fe01bc0

Add ShaderCache tests.

4686c2f

This was referenced Feb 22, 2017

BIM roadmap #4678

Open

Depth pick translucent primitives #4511

Closed

pjcozzi merged commit 7a0a6e7 into master Feb 22, 2017

pjcozzi deleted the pick-translucent branch February 22, 2017 19:47

bkuster mentioned this pull request Jan 24, 2018

Clarify the need to call scene.render in between picks with pickTranslucentDepth in docs #6151

Closed

OmarShehata mentioned this pull request Sep 11, 2018

Make Scene.pick work for translucent polygons #7032

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pick translucent geometry #4979

Pick translucent geometry #4979

bagnell commented Feb 9, 2017 •

edited

Loading

pjcozzi commented Feb 9, 2017

pjcozzi Feb 9, 2017

pjcozzi Feb 9, 2017

bagnell Feb 13, 2017

pjcozzi Feb 9, 2017

pjcozzi Feb 9, 2017

bagnell Feb 13, 2017

pjcozzi Feb 13, 2017

bagnell Feb 14, 2017

bagnell Feb 14, 2017

pjcozzi Feb 9, 2017

pjcozzi Feb 9, 2017

pjcozzi Feb 9, 2017

pjcozzi commented Feb 9, 2017

bagnell commented Feb 13, 2017

bagnell commented Feb 15, 2017

pjcozzi commented Feb 15, 2017

pjcozzi Feb 15, 2017

pjcozzi Feb 15, 2017

pjcozzi Feb 15, 2017

pjcozzi commented Feb 15, 2017

pjcozzi commented Feb 15, 2017

bagnell commented Feb 15, 2017

pjcozzi Feb 16, 2017

pjcozzi Feb 16, 2017

pjcozzi Feb 16, 2017

bagnell Feb 16, 2017

pjcozzi Feb 16, 2017

pjcozzi commented Feb 16, 2017

bagnell commented Feb 17, 2017

pjcozzi Feb 18, 2017

pjcozzi commented Feb 18, 2017

bagnell commented Feb 22, 2017

pjcozzi commented Feb 22, 2017

Pick translucent geometry #4979

Pick translucent geometry #4979

Conversation

bagnell commented Feb 9, 2017 • edited Loading

pjcozzi commented Feb 9, 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

pjcozzi commented Feb 9, 2017

bagnell commented Feb 13, 2017

bagnell commented Feb 15, 2017

pjcozzi commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Feb 15, 2017

pjcozzi commented Feb 15, 2017

bagnell commented Feb 15, 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

pjcozzi commented Feb 16, 2017

bagnell commented Feb 17, 2017

Choose a reason for hiding this comment

pjcozzi commented Feb 18, 2017

bagnell commented Feb 22, 2017

pjcozzi commented Feb 22, 2017

bagnell commented Feb 9, 2017 •

edited

Loading