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

Post processing #5615

Merged
merged 127 commits into from May 4, 2018

Conversation

Projects
None yet
7 participants
@lilleyse
Copy link
Contributor

commented Jul 11, 2017

A start for post processing effects in Cesium. I trimmed down this PR so it only applies to scene post-processing. Per-entity post processing will be ready soon too, but I want to keep this PR a bit simpler.

@byumjin u_depthTexture now works so you can test out SSAO. However it uses the last frustum's depth only! This actually works okay for some situations, but to guarantee that only one frustum is rendered, set scene.farToNearRatio to a large number like 100000000.0.

I'll bump again when this is ready to review.

To-do:

  • Merge lens flare, HBAO, DOF, toon shading, bloom effects: #5498
  • Put PostProcessLibrary shaders in .glsl file
  • Fix FXAA - it is now a post process stage but isn't working. It seems like FXAA requires LINEAR filtering on its texture, but PostProcess uses NEAREST for everything
  • Scissor test considerations - no changes have been made to the post processing files related to #5225
  • Update sun post-process to use this, separate PR
  • Smarter resource allocation and sharing
  • Consolidate depth textures from all frustums, separate PR Log depth
  • Add entity support, separate PR
  • Lens flare artifact, #5932
  • Doc
  • Tests
  • Update CHANGES.md

postprocess

lilleyse and others added some commits Jun 15, 2017

added HBAO and DOF
added HBAO and DOF
Added stepsize parameter at DOF
Added stepsize parameter at DOF
add Bloom(Glow) abd EdgeDetection(Toon)
add Bloom(Glow) abd EdgeDetection(Toon)
Fixed a HBAO's artifact which draws unexpected lines

Glow -
http://localhost:8080/Apps/Sandcastle/?src=Hello%20World.html&label=Showcases&gist=d5fd85d0eaec2a91fb9548ec8d8fbea4

Toon -
http://localhost:8080/Apps/Sandcastle/?src=Hello%20World.html&label=Showcases&gist=87ac93ad6ac8df9c78894ba242416450
Ability to create output framebuffer in the postProcess instead of ex…
…ternally, and set stages show to true by default
clean up and adjust review
cleaned up improvements for PostProcess
Added sandcastles for Bloom and Edge detection
Reflected Sean Lilley's review

@pjcozzi pjcozzi referenced this pull request Sep 2, 2017

Open

Post process framework roadmap #5808

15 of 46 tasks complete
// and instead shadowing is built-in. In this case execute the command regularly below.
command.derivedCommands.shadows.receiveCommand.execute(context, passState);
} else {
command.execute(context, passState);

This comment has been minimized.

Copy link
@lilleyse

lilleyse Oct 19, 2017

Author Contributor

All these changes are just reorganization without code edits. The debug show bounding volume code is just moved to its own function.

@@ -300,7 +302,7 @@ define([
this._globeDepth = globeDepth;
this._depthPlane = new DepthPlane();
this._oit = oit;
this._fxaa = new FXAA();
this._sceneFramebuffer = new SceneFramebuffer();

This comment has been minimized.

Copy link
@lilleyse

lilleyse Oct 19, 2017

Author Contributor

The FXAA file was renamed to SceneFramebuffer and the FXAA specific stuff was moved to PostProcessLibrary


const float fxaaQualitySubpix = 0.5;
const float fxaaQualityEdgeThreshold = 0.125;
const float fxaaQualityEdgeThresholdMin = 0.0833;

void main()
{
vec2 fxaaQualityRcpFrame = vec2(1.0) / czm_viewport.zw;

This comment has been minimized.

Copy link
@lilleyse

lilleyse Oct 19, 2017

Author Contributor

@bagnell you might have to double check if this is correct. I removed the uniform value and just calculate this in the shader. This may be related to the multiple-viewport changes in #5225

This comment has been minimized.

Copy link
@lilleyse

lilleyse Feb 5, 2018

Author Contributor

Bump - in case this is important.

This comment has been minimized.

Copy link
@bagnell

bagnell Feb 6, 2018

Member

This is fine for now. This would need to change for multiple viewports, but a lot more work would need to be done to get post-processing per-viewport.

@@ -0,0 +1,176 @@
define([

This comment has been minimized.

Copy link
@lilleyse

lilleyse Oct 19, 2017

Author Contributor

This file is a placeholder for the AO in #5673

@lilleyse

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2017

Quick summary of the code:

A PostProcess takes an input texture and output texture and applies a list of PostProcessStage sequentially. It uses a context-wide texture cache for ping-ponging. But it does not support down-scaled textures.

PostProcessStage contains a fragment shader and custom uniforms.

More complicated effects like AO may require custom stages which "inherit" from PostProcessStage. PostProcessAmbientOcclusionStage for example contains an internal PostProcess that is used to generate the grayscale AO texture. The fragment shader of the stage blends the AO texture with the color texture.

PostProcessCompositeStage is "inherited" from PostProcessStage and is a way to define a stage that contains multiple stages. I don't know if this code is actually used, but its convenient for effects like a blur stage, which would be composed of blurX and blurY stages.

PostProcessLibrary is a global store of post processing stages. PostProcessScene contains a PostProcess with common stages grabbed from PostProcessLibrary (all disabled by default).

@lilleyse

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2017

@bagnell @pjcozzi the todo list is up-to-date.

lilleyse added some commits Oct 19, 2017

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Oct 19, 2017

CC #5808

@lilleyse lilleyse referenced this pull request Oct 20, 2017

Open

Renderer roadmap #3001

9 of 24 tasks complete

bagnell added some commits Oct 20, 2017

@bagnell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

@lilleyse updated. This should be ready.

@lilleyse

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2018

Looks ready to me. I think this is safe to merge for 1.45 but can push if anyone prefers it wait.

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Apr 22, 2018

If we're confident this is pretty well isolated - taking into account OIT, sun bloom, etc.- and well tested, OK with me to merge for 1.45. Otherwise, let's not take the risk with everything else already going in, e.g., log depth, ion, etc.

@mramato

This comment has been minimized.

Copy link
Member

commented Apr 23, 2018

  1. I'm against this going into 1.45, there's no way it's tested enough.
  2. I think the dev guide needs to cover renderer refactors and features like this to ensure consistent and sufficient testing. For example: Did we test this on all browsers, including ones our team rarely uses like Safari? What about iOS/Android? Did we run through every Sandcastle like we do in release testing? Changes like this PR need to have way more scrutiny.

Example A: IE11 is completely broken in this branch. Cesium Viewer renders a black canvas.

@bagnell

This comment has been minimized.

Copy link
Member

commented May 2, 2018

@lilleyse This is ready. I fixed the issues in IE and Edge. Depth textures aren't supported in IE so any stage added to the collection that requires a depth texture will not be run. If one stage in a composite won't run, then neither does any stage in the composite (also goes for composites of composites).

Can you review this and #6476 soon? I'd like to merge then as soon as possible so they have a while to sit in master before the next release.

@lilleyse

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2018

Add a test that checks that post processing works as expected when depth textures are not supported.

@lilleyse

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2018

Should requiresDepthTexture be replaced with a generic isSupported function?

@bagnell

This comment has been minimized.

Copy link
Member

commented May 3, 2018

@lilleyse updated.

* @see {Context#depthTexture}
* @see {@link http://www.khronos.org/registry/webgl/extensions/WEBGL_depth_texture/|WEBGL_depth_texture}
*/
PostProcessStage.prototype.isSupported = function(context) {

This comment has been minimized.

Copy link
@lilleyse

lilleyse May 3, 2018

Author Contributor

Since it's part of the public API it should probably take scene instead.

Annoyingly, there will probably need to be a duplicate function that takes context, for internal use.

* @see {Context#depthTexture}
* @see {@link http://www.khronos.org/registry/webgl/extensions/WEBGL_depth_texture/|WEBGL_depth_texture}
*/
PostProcessStage.prototype.isSupported = function(context) {

This comment has been minimized.

Copy link
@lilleyse

lilleyse May 3, 2018

Author Contributor

There are a few areas in the post processing code that check if ao is enabled by checking if depth textures are supported. Those areas should call this function instead.

bagnell and others added some commits May 3, 2018

@lilleyse

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

The new changes look good. I tested in as many browsers as I could (Chrome, Firefox, Edge, IE, Safari) and Safari on an iPad. Internet Explorer ignores unsupported post processing stages correctly. As a final safeguard, it might be good to check isSupported and log a message in the AO/DOF/Edge detection Sandcastle demos so that people using IE or devices without depth textures understand why the post process isn't showing up.

The only artifact that I noticed is AO graininess on the iPad. It used to be worse before 5e11e9e but it's still worth showing what's happening. Whether or not this is something we can fix, I don't know.

image

Also there is a failing test in CI:

  Scene/PostProcessStageComposite
    ✗ does not run a stage that requires depth textures when depth textures are not supported
	Error: Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL.
	    at <Jasmine>

bagnell added some commits May 4, 2018

@bagnell

This comment has been minimized.

Copy link
Member

commented May 4, 2018

@lilleyse I fixed the test and updated the Sandcastles to report when a post process isn't supported. I think we should merge this and open a bug for the iOS issue. I'll look at it before the next release. I just want this to sit in master for a while

@lilleyse

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

Travis failed due to an unrelated failure in makeZipFile. Just in case, I restarted the build and will merge when it passes.

I'll look at #6476 soon as well.

@lilleyse

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2018

Ok it passed. Thanks @bagnell!

@lilleyse lilleyse merged commit 13998f2 into master May 4, 2018

5 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deployment Deployed
Details
npm package Deployed
Details
zip file Deployed
Details

@bagnell bagnell deleted the post-processing-1.0 branch Jun 4, 2018

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.