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
Merged

Post processing #5615

merged 127 commits into from
May 4, 2018

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse 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: Lens Flare #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 Incremental renderer updates for multiple viewports #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, Lens Flare artifact #5932
  • Doc
  • Tests
  • Update CHANGES.md

postprocess

lilleyse and others added 13 commits July 11, 2017 16:10
added HBAO and DOF
Added stepsize parameter at DOF
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
…ternally, and set stages show to true by default
cleaned up improvements for PostProcess
Added sandcastles for Bloom and Edge detection
Reflected Sean Lilley's review
@pjcozzi pjcozzi mentioned this pull request Sep 2, 2017
46 tasks
// 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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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();
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 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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bump - in case this is important.

Copy link
Contributor

Choose a reason for hiding this comment

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

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([
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is a placeholder for the AO in #5673

@lilleyse
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 19, 2017

CC #5808

@lilleyse lilleyse mentioned this pull request Oct 20, 2017
24 tasks
@bagnell
Copy link
Contributor

bagnell commented Apr 19, 2018

@lilleyse updated. This should be ready.

@lilleyse
Copy link
Contributor Author

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

@pjcozzi
Copy link
Contributor

pjcozzi 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
Copy link
Contributor

mramato 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
Copy link
Contributor

bagnell 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
Copy link
Contributor Author

lilleyse commented May 3, 2018

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

@lilleyse
Copy link
Contributor Author

lilleyse commented May 3, 2018

Should requiresDepthTexture be replaced with a generic isSupported function?

@bagnell
Copy link
Contributor

bagnell 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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lilleyse
Copy link
Contributor Author

lilleyse 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
Copy link
Contributor

bagnell 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
Copy link
Contributor Author

lilleyse 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
Copy link
Contributor Author

lilleyse commented May 4, 2018

Ok it passed. Thanks @bagnell!

@lilleyse lilleyse merged commit 13998f2 into master May 4, 2018
@bagnell bagnell deleted the post-processing-1.0 branch June 4, 2018 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants