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.

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

Log depth buffer #5851

Merged
merged 82 commits into from Apr 12, 2018

Conversation

Projects
None yet
9 participants
@Vineg
Copy link
Contributor

Vineg commented Sep 28, 2017

Added option "logDepthBuffer" to Cesium.Viewer. With that option globe is rendered in 1 frustum with logarithmic depth. (fixes #4381)

@cesium-concierge

This comment has been minimized.

Copy link

cesium-concierge commented Sep 28, 2017

Welcome to the Cesium community @Vineg!

Can you please send in a Contributor License Agreement (CLA) so that we can review and merge this pull request?

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.

@Vineg

This comment has been minimized.

Copy link
Contributor Author

Vineg commented Sep 28, 2017

@cesium-concierge updated CHANGES.md. I am member of geoscan organization. We've already sent CLA

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Sep 28, 2017

Very cool, thanks @Vineg!

@AnimatedRNG @lilleyse and I have been talking about doing this. I'm pretty sure it will be a win to reduce the total number of frustums requires at the cost of losing early z, but I do not believe it will be robust enough to truly fit all scenes into one frustum without z fighting in the distance.

@lilleyse @bagnell can you review this?

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Sep 28, 2017

@Vineg I'll also add you to our CLA list so you do not get a CLA warning in future pull requests.

@pjcozzi pjcozzi requested review from lilleyse and bagnell Sep 28, 2017

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Sep 28, 2017

@Vineg if this requires a lot of changes to get into production, do you have the bandwidth to work with us to make the changes or do prefer that we make them?

@Vineg

This comment has been minimized.

Copy link
Contributor Author

Vineg commented Sep 28, 2017

@pjcozzi I think I can spend 1-2 more days on this feature. Not sure how much changes it is going to be. I've tested it with 1 frustum in the mountains, with pretty big tile model and didn't see z fighting.

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Sep 28, 2017

Sounds great, thanks for your support.

I've tested it with 1 frustum in the mountains, with pretty big tile model and didn't see z fighting.

lol, we need to test looking at a soda can on a table in a building and looking through a window to the International Space Station with the moon in the background. 😄

I expect we will enable log depth by default with the multi frustum, and for scenes that then only require one frustum since the frustum can be much longer, it will be a great win, and even for massive distance scenes, probably only two frustums will be required. We'll also need to test this with the upcoming depth-based post-processing framework and depth picking. It's a pretty central change to the rendering pipeline so it needs careful attention. @lilleyse and @bagnell can help support this.

@Vineg

This comment has been minimized.

Copy link
Contributor Author

Vineg commented Sep 28, 2017

I'll be away for 4 days. Will return back to this branch on tuesday.

@AnimatedRNG

This comment has been minimized.

Copy link
Contributor

AnimatedRNG commented Sep 28, 2017

@Vineg I talked with @lilleyse about this feature, and we think it would be best implemented with a 32-bit depth buffer rather than the 24-bit depth buffers that we've been using throughout the code right now. In particular the texture internal format DEPTH_COMPONENT32F or DEPTH32F_STENCIL8. This feature is only available in WebGL 2, meaning that the WebGL 1 client will have to use 24-bit depth textures.

You might find this blog post useful, as well as this post (for optimizations).

@Vineg

This comment has been minimized.

Copy link
Contributor Author

Vineg commented Oct 3, 2017

@AnimatedRNG Why do you want to use floating point values? If I understand it right, logarithmic depth is ought to be used with fixed-point values.

@Vineg

This comment has been minimized.

Copy link
Contributor Author

Vineg commented Oct 3, 2017

@AnimatedRNG Do we really need 32bit? I've tried to set far plane to 500000000000000 and still don't see any z-fighting in the mountains. I think we have enough precision even with 24bit.

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Oct 12, 2017

This does seem promising so far for the globe. In order to test the types of cases like @pjcozzi mentioned we'll need to support log depth throughout the engine - models, primitives, etc. Would you be up for this @Vineg, or would you like someone else to continue this work?

@Vineg

This comment has been minimized.

Copy link
Contributor Author

Vineg commented Oct 12, 2017

@lilleyse, Yes, I just noticed that GroundPrimitives, Primitives, Billboards, Polylines, PointClouds and 3D Tiles requires additional modifications. If I missed something - let me know, I am now working on it. Also it seems to be pretty difficult to make depth writes optional as I did with globe, so I enable it as long as EXT_frag_depth available.

@pjcozzi
Copy link
Member

pjcozzi left a comment

@bagnell please also see the comments I just left in this PR about the Sandcastle examples that are not part of this code review email.

* `Camera.distanceToBoundingSphere` now returns the signed distance to the bounding sphere. Positive values indicate that the bounding sphere is in the positive half-plane of the camera position and view direction while a negative value indicates it is in the negative half-plane.

##### Additions :tada:
* Added option `logDepthBuffer` to `Viewer`. With this option there is typically a single frustum using logarithmic depth rendered. This increases performance by issuing less draw calls to the GPU and helps to avoid artifacts on the connection of two frustums. [#5851](https://github.com/AnalyticalGraphicsInc/cesium/pull/5851)

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

Did we also change the default near value?

This comment has been minimized.

Copy link
@likangning93

likangning93 Apr 18, 2018

Contributor

Late to the party, but should this be logarithmicDepthBuffer in Scene?

This comment has been minimized.

Copy link
@likangning93
@@ -554,12 +554,14 @@ define([

for (j = 0; j < length; ++j) {
command = commands[j];
command = defined(command.derivedCommands.logDepth) ? command.derivedCommands.logDepth.logDepthCommand : command;

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

Sorry for the perhaps obvious question here, but is everything based on derived commands? If so, why there is there so much custom code for models, ellipsoid primitives, etc.

This comment has been minimized.

Copy link
@bagnell

bagnell Apr 5, 2018

Member

The custom code adds the function calls but the function bodies are surrounded by #ifdefs. The derived commands will add the defines when log depth is supported.

@@ -398,7 +423,10 @@ define([
}

// Recompile shader when material changes
if (materialChanged || lightingChanged || !defined(this._pickSP)) {
if (materialChanged || lightingChanged || !defined(this._pickSP) || useLogDepthChanged) {

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

useLogDepthChanged

If it makes things much easier, I don't know that users will want to be able to toggle log depth at runtime.

This comment has been minimized.

Copy link
@bagnell

bagnell Apr 5, 2018

Member

This wasn't difficult to add since we have to support the fallback anyway.

@@ -287,6 +287,10 @@ define([
this._primitives = new PrimitiveCollection();
this._groundPrimitives = new PrimitiveCollection();

this._logDepthBuffer = undefined;
this._logDepthBufferDirty = true;
this.logarithmicDepthBuffer = context.fragmentDepth;

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

Should this have the same name as the property get/set below?

This comment has been minimized.

Copy link
@bagnell

bagnell Apr 5, 2018

Member

Yes, this calls the setter below.

* @type {Number}
* @default 1e9
*/
this.logarithmicDepthFarToNearRatio = 1e9;

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

Better document the interaction between logarithmicDepthBuffer, farToNearRatio, and logarithmicDepthFarToNearRatio and use @see/@link to link them together. If I am following the boolean controls using one or the other ratios.

var vertexlogDepthRegex = /\s+czm_vertexLogDepth\(/;
var extensionRegex = /\s*#extension\s+GL_EXT_frag_depth\s*:\s*enable/;

function getLogDepthShaderProgram(context, shaderProgram) {

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

This is a lot of new code in Scene.js. Perhaps move these two new log depth shader functions to a new private utility class in a separate file?

@@ -870,6 +870,12 @@ define([
camera.frustum = camera3D.frustum.clone();
}

var frustum = camera.frustum;
if (scene._logDepthBuffer && !(frustum instanceof OrthographicFrustum || frustum instanceof OrthographicOffCenterFrustum)) {
frustum.near = 0.1;

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

Same comment here and below.

This comment has been minimized.

Copy link
@bagnell

bagnell Apr 5, 2018

Member

Same comment as what? That this should be added to CHANGES.md? Done.

This comment has been minimized.

Copy link
@bagnell

bagnell Apr 5, 2018

Member

Ah, there were some hidden comments.

This comment has been minimized.

Copy link
@bagnell

bagnell Apr 5, 2018

Member

This has the same problem as in master. When the scene mode switches, we reset to the default near far distances in the frustum constructor. The old ones aren't saved between scene mode changes (should happen, separate PR or open an issue). This just sets to better defaults when log depth is enabled.

Cartesian3.clone(cameraCV.position, camera.position);
Cartesian3.clone(cameraCV.direction, camera.direction);
Cartesian3.clone(cameraCV.up, camera.up);
Cartesian3.cross(camera.direction, camera.up, camera.right);
Cartesian3.normalize(camera.right, camera.right);
}

var frustum = camera.frustum;
if (scene._logDepthBuffer && !(frustum instanceof OrthographicFrustum || frustum instanceof OrthographicOffCenterFrustum)) {
frustum.near = 0.1;

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

Same.

@@ -0,0 +1,49 @@
#ifdef LOG_DEPTH
varying float v_logZ;

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

Here and in writeLogDepth.glsl, we generally don't add new varyings or uniforms when including a built-in GLSL function (well, I also just noticed writeDepthClampedToFarPlane.glsl). Instead, we try to make the functions standalone and pass in / return anything needed. Is that reasonable here? Or does that require too much shader patching elsewhere?

This comment has been minimized.

Copy link
@bagnell

bagnell Apr 5, 2018

Member

I opted to put it here instead of making the shader patching code more complicated.

@@ -251,6 +251,9 @@ defineSuite([
});

it('debugCommandFilter does not filter commands', function() {
var originalLogDepth = scene.logarithmicDepthBuffer;

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Apr 3, 2018

Member

Does this file or MultifrustumSpec.js need some new tests where it switches between log depth on/off and sees the number of (redundant) commands executed?

In general, how is code coverage? Does anything need more fine-grained tests? Shouldn't the new built-in GLSL functions have new tests?

This comment has been minimized.

Copy link
@bagnell

bagnell Apr 5, 2018

Member

The multi-frustum tests have been updated to disable log depth. One test has it enabled to show that log depth has less 1 frustum where the other had 3.

Code coverage seems ok, but it's hard to say. I can't add tests for the new GLSL built-ins since they don't return a value. They update gl_Position and gl_FragDepth.

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Apr 3, 2018

Not sure if this is related to this branch, but in the interior 3D Tiles Sandcastle example, if I zoom back the entire model disappears.

ok2

This happens on master as well as #6390. It is an edge case related to back faces and skip-lods. When refining the back faces of the parent are rendered, which kind of falls apart for single-sided geometry like this. I'll open an issue.

Without skip-lods:
back-face

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Apr 3, 2018

In the BIM 3D Tiles Sandcastle example, I get some flickering (looks like a full-screen quad) when inside and then zooming in/out close. Is this related to this branch or is this a skip LOD bug that may be fixed with the new refactor? Seems loading related.

There's a bunch of BIM rendering problems, but they happen on all branches I've tried:

(Geometry z-fighting)
zfighting
(Distorted flickers - I think a problem with the glTF/shaders)
flickers
(skip-lods flicker)
skiplodbug

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Apr 3, 2018

Not sure if it is related to this branch, but looks like the Draco compressed example in Sandcastle doesn't have depth testing:

It seems like it's just this branch.

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Apr 5, 2018

@bagnell all sounds good.

bagnell added some commits Apr 5, 2018

@bagnell

This comment has been minimized.

Copy link
Member

bagnell commented Apr 5, 2018

@pjcozzi @lilleyse This is ready for another look.

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Apr 5, 2018

@lilleyse please merge when ready.

@bagnell don't forget about the blog! 😄

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Apr 5, 2018

Sorry for not catching this earlier, but I noticed some problems in the KML example related to billboards:

master vs. log-depth-buffer

master
log-depth

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Apr 5, 2018

Also check the Cesium Inspector sandcastle. Some of the options aren't working for me.

@emackey emackey referenced this pull request Apr 9, 2018

Merged

Post processing #5615

11 of 12 tasks complete
@bagnell

This comment has been minimized.

Copy link
Member

bagnell commented Apr 9, 2018

@lilleyse I fixed the billboard issue in 6e9fc3e which you might want to review since it changes the billboard vertex shader to use eye coordinates instead of window coordinates.

@lilleyse
Copy link
Contributor

lilleyse left a comment

Cool, 6e9fc3e fixes the billboard problem and the existing demos still seem to work fine.

#ifdef LOG_DEPTH
czm_vertexLogDepth(czm_projection * positionEC);
#endif

vec4 positionWC = computePositionWindowCoordinates(positionEC, imageSize, scale, direction, origin, translate, pixelOffset, alignedAxis, validAlignedAxis, rotation, sizeInMeters);

This comment has been minimized.

Copy link
@lilleyse

lilleyse Apr 10, 2018

Contributor

Should positionWC and computePositionWindowCoordinates be renamed?

@@ -71,23 +71,23 @@ vec4 computePositionWindowCoordinates(vec4 positionEC, vec2 imageSize, float sca
positionEC.xy += halfSize;
}

vec4 positionWC = czm_eyeToWindowCoordinates(positionEC);
float mpp = czm_metersPerPixel(positionEC);

This comment has been minimized.

Copy link
@lilleyse

lilleyse Apr 10, 2018

Contributor

Is it possible to only compute this if sizeInMeters is true and still function properly with the new changes? If not, not a big deal.

This comment has been minimized.

Copy link
@bagnell

bagnell Apr 10, 2018

Member

No, since it's needed regardless of whether sizeInMeters is true or false;

@bagnell

This comment has been minimized.

Copy link
Member

bagnell commented Apr 10, 2018

@lilleyse This should be ready.

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Apr 12, 2018

Ok I have nothing else to add here. Ready to go!

@bagnell can you open an issue to remove the skinned model workarounds?

@lilleyse lilleyse merged commit 50b1d5d into AnalyticalGraphicsInc:master Apr 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.