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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post-Processing Nodes for DAG #2388

Merged
merged 7 commits into from Jul 6, 2016

Conversation

Projects
None yet
5 participants
@tdgunes
Copy link
Member

tdgunes commented Jul 1, 2016

Contains

Finally, the last nodes! 馃帄 馃帀

Seven new nodes for DAG (Directed Acyclic Graph), wrapping tasks that are related with Post-Processing.

  • BloomPassesNode
  • BlurPassesNode
  • DownSampleSceneAndUpdateExposureNode
  • FinalPostProcessingNode
  • InitialPostProcessingNode
  • ToneMappedSceneNode
  • LightShaftsNode

How to test

Check if there are any weirdness related to rendering of post-processing effects like bloom, blur, tone mapping (HDR), light shafts and so on.

@GooeyHub GooeyHub added the in progress label Jul 1, 2016

@tdgunes tdgunes changed the title [WIP] Post-Processing Nodes for DAG [RFR] Post-Processing Nodes for DAG Jul 1, 2016

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jul 1, 2016

Hooray Jenkins reported success with all tests good!

1 similar comment
@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jul 1, 2016

Hooray Jenkins reported success with all tests good!

@@ -93,7 +94,7 @@ public void process() {
*/
// It's unclear why these buffers need to be cleared while all the others don't...
private void initialClearing() {
FBO sceneReflectiveRefractive = frameBuffersManager.getFBO("sceneReflectiveRefractive");

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Good catch, thank you!

sceneHighPass.bind();

setViewportToSizeOf(sceneHighPass);
glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Let's mark this with // TODO: verify this is necessary

renderFullscreenQuad();

bindDisplay();
setViewportToSizeOf(sceneOpaque);

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

And also these two.

renderFullscreenQuad();

bindDisplay();
setViewportToSizeOf(sceneOpaque);

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Again, let's add // TODO: verify if necessary in these two statements AND the glClear one, above.

@Override
public void process() {
sceneOpaque = frameBuffersManager.getFBO("sceneOpaque");
scenePrePost = frameBuffersManager.getFBO("scenePrePost");

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Shouldn't these be within the IF block and within the PerformanceMonitor statements, for consistency?

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

On second thought, these are used only in method downsampleSceneInto1x1pixelsBuffer(). They should go there.


frameBuffersManager.swapReadbackPBOs();

ByteBuffer pixels = frameBuffersManager.getCurrentReadbackPBO().readBackPixels();

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Let's make this a private variable please.

sceneOpaque = frameBuffersManager.getFBO("sceneOpaque");
scenePrePost = frameBuffersManager.getFBO("scenePrePost");
if (renderingConfig.isEyeAdaptation()) {
PerformanceMonitor.startActivity("rendering/downsamplesceneandupdateexposure");

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Let's use the string "rendering/updateExposure" here. Then, in the downsampleSceneInto1x1pixelsBuffer method, let's use "rendering/updateExposure/downsampleScene" instead of Render Eye Adaptation.

targetExposure = hdrTargetLuminance / currentSceneLuminance;
}

float maxExposure = hdrMaxExposure;

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Let's turn this and float targetExposure above into private variables please.

PerformanceMonitor.startActivity("Rendering eye adaption");

downSampler.enable();
FBO downSampledFBO;

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Into a private variable please.

downSampledFBO.bind();

setViewportToSizeOf(downSampledFBO);
glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Add // TODO: verify this is necessary

generateHighPass();
generateBloom(sceneBloom0);
generateBloom(sceneBloom1);
generateBloom(sceneBloom2);

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Let's add to this block: // TODO: review - would it make sense to split these operations into one highpass node and three blur nodes with different parameters?

Interestingly, this hints to a bigger issue: if a number of "elementary" nodes are working closely together to achieve a more complex effect, how can they be "grouped" together? Perhaps this could be handled by considering some nodes as a graph within the graph, which is functionality we probably need anyway, i.e. to handle the eye-based parallel graphs for stereoscopic displays.


ocDistortion = worldRenderer.getMaterial("engine:prog.ocDistortion");
finalPost = worldRenderer.getMaterial("engine:prog.post"); // TODO: rename shader to finalPost
debug = worldRenderer.getMaterial("engine:prog.debug");

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Add //TODO: rethink debug strategy in light of the DAG-based architecture - it should be possible to inspect the output of any rendering node. I'll also write an issue in your fork on this.

renderingDebugConfig = renderingConfig.getDebug();

ocDistortion = worldRenderer.getMaterial("engine:prog.ocDistortion");
finalPost = worldRenderer.getMaterial("engine:prog.post"); // TODO: rename shader to finalPost

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

A node getting a handle to more than one shader is probably a good candidate to become multiple nodes. Nothing to worry about right now, just something to keep in mind for a later stage.

} else {
sceneFinal.bind();

glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Add: // TODO: verify this is necessary

sceneFinal = frameBuffersManager.getFBO("sceneFinal");
sceneOpaque = frameBuffersManager.getFBO("sceneOpaque");

fullScale = sceneOpaque.dimensions();

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

I am somewhat concerned that all these variables are reset here rather than close to where they are used. But I'm ok with leaving them as they are, as we will handle buffers and dimensions very differently, eventually.

sceneFinal.bind();
} else {
ocUndistorted.bind();
}

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

This IF block only confuses things here as these bindings are used only if we are dealing with the left eye: when the right eye is being rendered the display or sceneFinal are bound respectively, overriding what the if blocks does.

I'd say we move the IF block to within the LEFT_EYE case. Let's also verify it all still works though... =)

PerformanceMonitor.startActivity("rendering/initialpostprocessing");
disableWireframeIf(renderingDebugConfig.isWireframe());
FBO scenePrePost = frameBuffersManager.getFBO("scenePrePost");
FBO sceneOpaque = frameBuffersManager.getFBO("sceneOpaque");

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Private variables pls...

public void process() {
// Initial Post-Processing: chromatic aberration, light shafts, 1/8th resolution bloom, vignette
PerformanceMonitor.startActivity("rendering/initialpostprocessing");
disableWireframeIf(renderingDebugConfig.isWireframe());

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

This looks out of place. It wasn't there in the PostProcessor.initialPostProcessing() method. Why is it here?

@Override
public void process() {
// Initial Post-Processing: chromatic aberration, light shafts, 1/8th resolution bloom, vignette
PerformanceMonitor.startActivity("rendering/initialpostprocessing");

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

Let's make sure to use lowerCamelCase in all startACtivity() calls please. I just noticed you use all smaller case also in other nodes.

/**
* TODO: Add diagram of this node
*/
public class ToneMappedSceneNode implements Node {

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

I'd rename this to ToneMappingNode.

renderingPipeline.add(toneMappedSceneNode);
renderingPipeline.add(bloomPassesNode);
renderingPipeline.add(blurPassesNode);
renderingPipeline.add(finalPostProcessingNode);

This comment has been minimized.

@emanuele3d

emanuele3d Jul 1, 2016

Contributor

In light of where we are heading I'd recommend you to rename this method to "initRenderGraph" and the variable renderingPipeline to renderGraph. The renderPipeline (shorter!) will be the list of commands generated out of the graph.

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jul 2, 2016

Hooray Jenkins reported success with all tests good!

1 similar comment
@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jul 2, 2016

Hooray Jenkins reported success with all tests good!


downsampleSceneInto1x1pixelsBuffer();
downSampleSceneInto1x1pixelsBuffer();

This comment has been minimized.

@emanuele3d

emanuele3d Jul 2, 2016

Contributor

Hehe, actually in this case it would be correct to keep it as "downsample" as it is actually a one-word verb, like upscaling for example. But don't bother, it's ok to leave it as it is.

@@ -219,36 +219,36 @@ private void initPipeline() {
Node lightShaftsNode = createInstance(LightShaftsNode.class, context);
Node initialPostProcessingNode = createInstance(InitialPostProcessingNode.class, context);
Node downSampleSceneAndUpdateExposure = createInstance(DownSampleSceneAndUpdateExposureNode.class, context);
Node toneMappedSceneNode = createInstance(ToneMappedSceneNode.class, context);
Node toneMappedSceneNode = createInstance(ToneMappingNode.class, context);

This comment has been minimized.

@emanuele3d

emanuele3d Jul 2, 2016

Contributor

I would say the name of the variable should also change to toneMappingNode.

switch (renderingStage) {
case LEFT_EYE:
if (postProcessor.isNotTakingScreenshot()) { // TODO: verify if this works

This comment has been minimized.

@emanuele3d

emanuele3d Jul 2, 2016

Contributor

Errr... you should verify this before you even push a new commit! =)

This comment has been minimized.

@tdgunes

tdgunes Jul 3, 2016

Author Member

Since I can not verify (Oculus VR Support is disabled as default in the settings, so that part of this code is disabled), after checking assignments seems like this modification should not affect current implementation. But I would like to revisit this again while we are working on multiple cameras.

This comment has been minimized.

@emanuele3d

emanuele3d Jul 3, 2016

Contributor

Can you try by overriding temporarily the RenderConfig.isOculusVrSupport() method to always return true?

In any case I'm getting more and more tempted to find the time and eradicate VR support from the code...

This comment has been minimized.

@tdgunes

tdgunes Jul 3, 2016

Author Member

Oh, shame that I didn't think about it for testing. I am wondering about the rendering result without having an actual VR headset. 馃槃

I wouldn't mind eradicating it! 馃榾

This comment has been minimized.

@emanuele3d

emanuele3d Jul 3, 2016

Contributor

It just renders two images side-by-side allegedly from slightly different point of views and appropriately distorted for the lenses in the Oculus headset. It won't break your monitor. =)

This comment has been minimized.

@tdgunes

tdgunes Jul 4, 2016

Author Member

I just tried now, unfortunately it doesn't work if the if-clause is moved inside the case:

screen shot 2016-07-04 at 11 32 16

Previously it was:
screen shot 2016-07-04 at 11 31 16

I think it is better to revert that change and leave it as is.

This comment has been minimized.

@emanuele3d

emanuele3d Jul 4, 2016

Contributor

I'm an idiot. It's obvious it can't work. The bindings in the IF block at the beginning of the method affect the quad rendering at the beginning of the RIGHT_EYE case. Somehow I missed that line. If it hadn't been there it would have worked. Sorry for the waste of time.

This comment has been minimized.

@tdgunes

tdgunes Jul 5, 2016

Author Member

After focusing on individual FBO bindings, I noticed that too. But this was all after uploading the screenshots and reading your comment. 馃槃

@emanuele3d

This comment has been minimized.

Copy link
Contributor

emanuele3d commented Jul 2, 2016

@tdgunes: notice I answered to a number of comments in the outdate diffs.You might want to expand them all to double-check.

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jul 3, 2016

Hooray Jenkins reported success with all tests good!

@emanuele3d

This comment has been minimized.

Copy link
Contributor

emanuele3d commented Jul 3, 2016

@tdgunes: you have gone back to -not- acknowledging my comments. A simple thumb up when you have addressed the issue or just a short written comment will do.

sceneFinal.bind();
} else {
ocUndistorted.bind();
}

This comment has been minimized.

@emanuele3d

emanuele3d Jul 4, 2016

Contributor

Let's move this IF block back out of the switch. Apologies again for my mistake.

Once this is done, I think this PR is good to go. @msteiger ?

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jul 5, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes tdgunes changed the title [RFR] Post-Processing Nodes for DAG [RFM] Post-Processing Nodes for DAG Jul 5, 2016

@Cervator

This comment has been minimized.

Copy link
Member

Cervator commented Jul 5, 2016

As a quick side comment the conflict now marked in the GitHub UI is trivial, it is literally a single letter in a monitor statement, won't be a problem at all to merge and I don't mind doing that part if needed when ready :-)

@emanuele3d & @msteiger: any final feedback on this one or all set?

@emanuele3d

This comment has been minimized.

Copy link
Contributor

emanuele3d commented Jul 5, 2016

From my perspective it is all set. But I don't know if @msteiger has had any chance to review it yet.

@msteiger

This comment has been minimized.

Copy link
Member

msteiger commented Jul 6, 2016

Thanks for waiting - looks good to me!

tdgunes added some commits Jun 17, 2016

Add post-processing nodes
Add OutlineNode

Rename ChunksRefractiveReflective

Add four post-processing nodes to DAG

Add BloomPassesNode

Remove bloom related rendering steps from PostProcessor and BloomPassesNode to pipeline

Add FinalPostProcessingNode, remove unused methods from PostProcessor

Remove unnecessary variables from WorldRendererImpl

@tdgunes tdgunes force-pushed the tdgunes:dag-postprocessing branch from ce05bf0 to 10d6522 Jul 6, 2016

@GooeyHub

This comment has been minimized.

Copy link
Member

GooeyHub commented Jul 6, 2016

Hooray Jenkins reported success with all tests good!

@tdgunes

This comment has been minimized.

Copy link
Member Author

tdgunes commented Jul 6, 2016

I rebased this branch with develop to include the quick fix #2396. It's ready for merge! 馃帀 馃帄

@msteiger msteiger merged commit 6f133fa into MovingBlocks:develop Jul 6, 2016

1 check passed

default Build finished. 503 tests run, 0 skipped, 0 failed.
Details

@GooeyHub GooeyHub removed the in progress label Jul 6, 2016

@Cervator Cervator added this to the Alpha 3 milestone Jul 7, 2016

@tdgunes tdgunes changed the title [RFM] Post-Processing Nodes for DAG Post-Processing Nodes for DAG Jul 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.