Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pick translucent geometry #4979

Merged
merged 22 commits into from
Feb 22, 2017
Merged

Pick translucent geometry #4979

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
e4d1557
Add option to pick translucent geometry at a performance cost.
bagnell Feb 9, 2017
832b36c
Remove commented out code.
bagnell Feb 9, 2017
002e2d9
Fix whitespace.
bagnell Feb 9, 2017
e43dc2e
Merge branch 'master' into pick-translucent
bagnell Feb 13, 2017
e4ed730
Updates from review.
bagnell Feb 13, 2017
e522ffe
Add depth pass.
bagnell Feb 14, 2017
6d94f35
Add doc.
bagnell Feb 14, 2017
963e631
Add pick translucent test.
bagnell Feb 14, 2017
0e2fb9c
Update CHANGES.md.
bagnell Feb 15, 2017
be613f9
Update reference doc
pjcozzi Feb 15, 2017
d3cba1c
Merge branch 'master' into pick-translucent
bagnell Feb 15, 2017
3bb0a9e
Update doc after merge.
bagnell Feb 15, 2017
a49219b
Use previous frame's commands. Disable compute and shadow commands fr…
bagnell Feb 15, 2017
eb56783
Create derived command with pass through fragment shader, depth write…
bagnell Feb 15, 2017
47efc79
Clean up resources and fix tests.
bagnell Feb 15, 2017
547216f
Simplify code from review and add a comment about optimization for de…
bagnell Feb 16, 2017
94c3053
Update test to ensure derived commands are cached.
bagnell Feb 16, 2017
ab65c94
Add global derived shader cache that properly releases shader resourc…
bagnell Feb 16, 2017
864433d
Change how derived command cache is implemented. Add support to shadows.
bagnell Feb 16, 2017
e0c769b
Whitespace
pjcozzi Feb 18, 2017
fe01bc0
Merge branch 'master' into pick-translucent
bagnell Feb 22, 2017
4686c2f
Add ShaderCache tests.
bagnell Feb 22, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Source/Renderer/PassState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ define([], function() {
* @default undefined
*/
this.viewport = undefined;

this.depthMask = undefined;
}

return PassState;
Expand Down
15 changes: 8 additions & 7 deletions Source/Renderer/RenderState.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,8 +549,9 @@ define([
gl.colorMask(colorMask.red, colorMask.green, colorMask.blue, colorMask.alpha);
}

function applyDepthMask(gl, renderState) {
gl.depthMask(renderState.depthMask);
function applyDepthMask(gl, renderState, passState) {
var mask = defined(passState.depthMask) ? passState.depthMask : renderState.depthMask;
gl.depthMask(mask);
}

function applyStencilMask(gl, renderState) {
Expand Down Expand Up @@ -641,10 +642,10 @@ define([
applyDepthRange(gl, renderState);
applyDepthTest(gl, renderState);
applyColorMask(gl, renderState);
applyDepthMask(gl, renderState);
applyStencilMask(gl, renderState);
applyStencilTest(gl, renderState);
applySampleCoverage(gl, renderState);
applyDepthMask(gl, renderState, passState);
applyScissorTest(gl, renderState, passState);
applyBlending(gl, renderState, passState);
applyViewport(gl, renderState, passState);
Expand Down Expand Up @@ -686,10 +687,6 @@ define([
funcs.push(applyColorMask);
}

if (previousState.depthMask !== nextState.depthMask) {
funcs.push(applyDepthMask);
}

if (previousState.stencilMask !== nextState.stencilMask) {
funcs.push(applyStencilMask);
}
Expand Down Expand Up @@ -755,6 +752,10 @@ define([
if (previousRenderState !== renderState || previousPassState !== passState || previousPassState.context !== passState.context) {
applyViewport(gl, renderState, passState);
}

if (previousRenderState !== renderState || previousPassState !== passState || previousPassState.context !== passState.context) {
applyDepthMask(gl, renderState, passState);
}
};

RenderState.getState = function(renderState) {
Expand Down
75 changes: 75 additions & 0 deletions Source/Scene/Scene.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,17 +28,21 @@ define([
'../Core/Matrix4',
'../Core/mergeSort',
'../Core/Occluder',
'../Core/PixelFormat',
'../Core/ShowGeometryInstanceAttribute',
'../Core/Transforms',
'../Renderer/ClearCommand',
'../Renderer/ComputeEngine',
'../Renderer/Context',
'../Renderer/ContextLimits',
'../Renderer/DrawCommand',
'../Renderer/Framebuffer',
'../Renderer/Pass',
'../Renderer/PassState',
'../Renderer/PixelDatatype',
'../Renderer/ShaderProgram',
'../Renderer/ShaderSource',
'../Renderer/Texture',
'./Camera',
'./CreditDisplay',
'./CullingVolume',
Expand Down Expand Up @@ -96,17 +100,21 @@ define([
Matrix4,
mergeSort,
Occluder,
PixelFormat,
ShowGeometryInstanceAttribute,
Transforms,
ClearCommand,
ComputeEngine,
Context,
ContextLimits,
DrawCommand,
Framebuffer,
Pass,
PassState,
PixelDatatype,
ShaderProgram,
ShaderSource,
Texture,
Camera,
CreditDisplay,
CullingVolume,
Expand Down Expand Up @@ -548,6 +556,8 @@ define([
*/
this.useDepthPicking = true;

this.pickTranslucentDepth = true;

/**
* The time in milliseconds to wait before checking if the camera has not moved and fire the cameraMoveEnd event.
* @type {Number}
Expand Down Expand Up @@ -2696,6 +2706,68 @@ define([
return object;
};

var scratchPickDepthPassState;

function renderForPickDepth(scene, drawingBufferPosition) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

var context = scene._context;
var us = context.uniformState;
var frameState = scene._frameState;

// 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What depth copies?

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


us.update(frameState);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.


var passState = scratchPickDepthPassState;
if (!defined(passState)) {
passState = scratchPickDepthPassState = new PassState(context);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

passState.scissorTest = {
enabled : true,
rectangle : new BoundingRectangle()
};
passState.viewport = new BoundingRectangle();
passState.depthMask = true;
}

var width = context.drawingBufferWidth;
var height = context.drawingBufferHeight;

var framebuffer = scene._pickDepthFramebuffer;
Copy link
Contributor

Choose a reason for hiding this comment

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

_pickDepthFramebuffer should be declared in the constructor function.

var pickDepthFBWidth = scene._pickDepthFramebufferWidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this and height below declared in the constructor function?

var pickDepthFBHeight = scene._pickDepthFramebufferHeight;
if (!defined(framebuffer) || pickDepthFBWidth !== width || pickDepthFBHeight !== height) {
scene._pickDepthFramebuffer = scene._pickDepthFramebuffer && scene._pickDepthFramebuffer.destroy();
framebuffer = scene._pickDepthFramebuffer = new Framebuffer({
context : context,
depthStencilTexture : new Texture({
context : context,
width : width,
height : height,
pixelFormat : PixelFormat.DEPTH_STENCIL,
pixelDatatype : PixelDatatype.UNSIGNED_INT_24_8
})
});

scene._pickDepthFramebufferWidth = width;
scene._pickDepthFramebufferHeight = height;
}

passState.framebuffer = framebuffer;
passState.viewport.width = width;
passState.viewport.height = height;
passState.scissorTest.rectangle.x = drawingBufferPosition.x;
passState.scissorTest.rectangle.y = height - drawingBufferPosition.y;
passState.scissorTest.rectangle.width = 1;
passState.scissorTest.rectangle.height = 1;

updateAndExecuteCommands(scene, passState, scratchColorZero);
resolveFramebuffers(scene, passState);

context.endFrame();
}

var scratchPackedDepth = new Cartesian4();
var packedDepthScale = new Cartesian4(1.0, 1.0 / 255.0, 1.0 / 65025.0, 1.0 / 16581375.0);

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

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

Choose a reason for hiding this comment

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

Perhaps rename to renderTranslucentDepthForPick?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

}
drawingBufferPosition.y = this.drawingBufferHeight - drawingBufferPosition.y;

var camera = this._camera;
Expand Down