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

Jump to bottom

Backdrop and Skybands Nodes for DAG #2362

Merged
merged 7 commits into from
Jun 22, 2016
Merged

Backdrop and Skybands Nodes for DAG #2362

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3588be1
Add BackdropNode and SkyBandsNode
tdgunes Jun 17, 2016
c46c0e3
Remove unused methods from GraphicState
tdgunes Jun 18, 2016
6e9e276
Use setViewportToSideOf in PostProcessor
tdgunes Jun 19, 2016
086015c
Remove unused variables and methods from PostProcessor and WorldRende…
tdgunes Jun 19, 2016
5a77abf
Refactor according to feedback
tdgunes Jun 20, 2016
25fcce8
Move FBO variables to fields
tdgunes Jun 21, 2016
06f9fae
Remove wildcard import
tdgunes Jun 22, 2016
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
96 changes: 96 additions & 0 deletions engine/src/main/java/org/terasology/rendering/dag/BackdropNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
/*
* Copyright 2016 MovingBlocks
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.terasology.rendering.dag;

import org.terasology.config.Config;
import org.terasology.config.RenderingDebugConfig;
import org.terasology.monitoring.PerformanceMonitor;
import org.terasology.registry.In;
import org.terasology.rendering.backdrop.BackdropRenderer;
import org.terasology.rendering.cameras.Camera;
import org.terasology.rendering.opengl.FBO;
import org.terasology.rendering.opengl.FrameBuffersManager;
import org.terasology.rendering.world.WorldRenderer;

import static org.lwjgl.opengl.GL11.*;
Copy link
Member

Choose a reason for hiding this comment

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

No wildcard imports, please. See checkstyle.xml for the check.

import static org.terasology.rendering.opengl.OpenGLUtils.bindDisplay;
import static org.terasology.rendering.opengl.OpenGLUtils.enableWireframeIf;
import static org.terasology.rendering.opengl.OpenGLUtils.setRenderBufferMask;

/**
* TODO: Diagram of this node
*/
public class BackdropNode implements Node {

@In
private Config config;

@In
private BackdropRenderer backdropRenderer;

@In
private WorldRenderer worldRenderer;

@In
private FrameBuffersManager frameBuffersManager;

private RenderingDebugConfig renderingDebugConfig;
private Camera playerCamera;
private FBO sceneOpaque;

@Override
public void initialise() {
renderingDebugConfig = config.getRendering().getDebug();
playerCamera = worldRenderer.getActiveCamera();
}

@Override
public void process() {
PerformanceMonitor.startActivity("Render Sky");
sceneOpaque = frameBuffersManager.getFBO("sceneOpaque");

enableWireframeIf(renderingDebugConfig.isWireframe());
initialClearing();

sceneOpaque.bind();
setRenderBufferMask(sceneOpaque, true, true, true);

playerCamera.lookThroughNormalized();
/**
* Sets the state to render the Backdrop. At this stage the backdrop is the SkySphere
* plus the SkyBands passes.
*
* The backdrop is the only rendering that has three state-changing methods.
* This is due to the SkySphere requiring a state and the SkyBands requiring a slightly
* different one.
*/
setRenderBufferMask(sceneOpaque, true, false, false);
backdropRenderer.render(playerCamera);
}

/**
* Initial clearing of a couple of important Frame Buffers. Then binds back the Display.
*/
// It's unclear why these buffers need to be cleared while all the others don't...
private void initialClearing() {
FBO sceneReflectiveRefractive = frameBuffersManager.getFBO("sceneReflectiveRefractive");
sceneOpaque.bind();
glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT);
sceneReflectiveRefractive.bind();
glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
bindDisplay();
}
}
112 changes: 112 additions & 0 deletions engine/src/main/java/org/terasology/rendering/dag/SkyBandsNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
* Copyright 2016 MovingBlocks
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.terasology.rendering.dag;

import org.terasology.config.Config;
import org.terasology.config.RenderingConfig;
import org.terasology.config.RenderingDebugConfig;
import org.terasology.monitoring.PerformanceMonitor;
import org.terasology.registry.In;
import org.terasology.rendering.assets.material.Material;
import org.terasology.rendering.cameras.Camera;
import org.terasology.rendering.opengl.FBO;
import org.terasology.rendering.opengl.FrameBuffersManager;
import org.terasology.rendering.world.WorldRenderer;

import static org.lwjgl.opengl.GL11.GL_COLOR_BUFFER_BIT;
import static org.lwjgl.opengl.GL11.GL_DEPTH_BUFFER_BIT;
import static org.lwjgl.opengl.GL11.glClear;
import static org.terasology.rendering.opengl.OpenGLUtils.*;


/**
* TODO: Diagram of this node
* TODO: Separate this node into multiple SkyBandNode's
*/
public class SkyBandsNode implements Node {

@In
private Config config;

@In
private WorldRenderer worldRenderer;

@In
private FrameBuffersManager frameBuffersManager;

private RenderingConfig renderingConfig;
private Material blurShader;
private FBO sceneOpaque;
private FBO sceneSkyBand0;
private RenderingDebugConfig renderingDebugConfig;
private Camera playerCamera;

@Override
public void initialise() {
renderingConfig = config.getRendering();
renderingDebugConfig = renderingConfig.getDebug();
blurShader = worldRenderer.getMaterial("engine:prog.blur");
playerCamera = worldRenderer.getActiveCamera();
}

@Override
public void process() {
enableWireframeIf(renderingDebugConfig.isWireframe());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please enable AND disable wireframe in all nodes that were originally between the enableWireframeIf/disableWireframeIf statements in the WorldRendererImpl.render method. So, each of those nodes will need its own enable/disable pair. This of course has a small performance hit but we'll eventually solve it with state management.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you mean these two nodes also need a disableWireframeIf statement at the end of process() method? Should changed states for a certain node in OpenGL be reverted and left for our state management?

Copy link
Contributor

Choose a reason for hiding this comment

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

  • The answer to the first question is yes.
  • OpenGL state changes shouldn't be touched at this stage. I was very, very tempted of touching some in the SkyBands node but I decided not to suggest it so that we can continue focusing on wrapping things.
  • PerformanceMonitor is not an opengl thing so I don't mind touching it. The content of every process() method of every node should really be bracketed by startActivity()/endActivity() as a rule.


sceneOpaque = frameBuffersManager.getFBO("sceneOpaque");

setRenderBufferMask(sceneOpaque, true, true, true);
if (renderingConfig.isInscattering()) {
sceneSkyBand0 = frameBuffersManager.getFBO("sceneSkyBand0");
FBO sceneSkyBand1 = frameBuffersManager.getFBO("sceneSkyBand1");

generateSkyBand(sceneSkyBand0);
generateSkyBand(sceneSkyBand1);
}

sceneOpaque.bind();

playerCamera.lookThrough();
endRenderSkyActivity();
}

private void endRenderSkyActivity() {
PerformanceMonitor.endActivity();
Copy link
Contributor

Choose a reason for hiding this comment

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

Good coding choice to clarify what would otherwise be hard to understand. However I'd solve the problem by having Performance Monitor statements at the beginning and end of each node. This is a change from what we have now, grouping together a number of rendering steps within the same activity. My vision is to group together activities by using hierarchical activity names instead. I.e. you could name the activity in the BackdropNode "rendering/backdrop" and the other "rendering/backdrop/skybands". It will be up to DebugOverlay to group these together if the user wishes it. Let me know if this is clear enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean like:

PerformanceMonitor.startActivity("rendering/backdrop") // Executed inside Backdrop
...
PerformanceMonitor.startActivity("rendering/backdrop/skybands") // Executed inside SkyBand node
... 
PerformanceMonitor.endActivity("rendering/backdrop/skybands") // Executed inside SkyBand 

PerformanceMonitor.endActivity("rendering/backdrop") // Executed not inside SkyBand, but inside Backdrop

But the activity backdrop (rendering/backdrop) in total is going to include measurements from rendering/backdrop/skybands right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding the activity statements: correct. Just remember endActivity() takes no parameter. Which won't be a problem now that start/end are closer to each other.

Regarding your question: ideally, but only in a DebugOverlay. And initially I don't mind if it isn't. If the DebugOverlay is ordered alphabetically for example, related nodes will be listed near each other anyway.

There could be an argument for naming rendering/backdrop more specifically. Or perhaps naming rendering/backdrop/skybands to rendering/skybands instead. In fact, I would prefer it that way. I am not convinced that the skybands should be grouped with the backdrop. If I remember correctly the generate some distant haze that gets overimposed on the world rendering. So, they are atmospheric effects on the world, not on the sky.

So, I'd suggest rendering/backdrop and rendering/skybands for the time being.

}

private void generateSkyBand(FBO skyBand) {
blurShader.enable();
blurShader.setFloat("radius", 8.0f, true);
blurShader.setFloat2("texelSize", 1.0f / skyBand.width(), 1.0f / skyBand.height(), true);

if (skyBand == sceneSkyBand0) {
sceneOpaque.bindTexture();
} else {
sceneSkyBand0.bindTexture();
}

skyBand.bind();
setRenderBufferMask(skyBand, true, false, false);

setViewportToSizeOf(skyBand);
glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT); // TODO: verify this is necessary

renderFullscreenQuad();

bindDisplay(); // TODO: verify this is necessary
setViewportToSizeOf(sceneOpaque); // TODO: verify this is necessary
}
}
141 changes: 2 additions & 139 deletions engine/src/main/java/org/terasology/rendering/opengl/GraphicState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,45 +15,13 @@
*/
package org.terasology.rendering.opengl;

import org.lwjgl.BufferUtils;
import org.lwjgl.opengl.GL11;
import org.lwjgl.opengl.GL20;
import org.terasology.math.geom.Vector3f;

import java.nio.IntBuffer;

import static org.lwjgl.opengl.EXTFramebufferObject.GL_COLOR_ATTACHMENT0_EXT;
import static org.lwjgl.opengl.GL11.GL_ALWAYS;
import static org.lwjgl.opengl.GL11.GL_BACK;
import static org.lwjgl.opengl.GL11.GL_BLEND;
import static org.lwjgl.opengl.GL11.GL_COLOR_BUFFER_BIT;
import static org.lwjgl.opengl.GL11.GL_CULL_FACE;
import static org.lwjgl.opengl.GL11.GL_DECR;
import static org.lwjgl.opengl.GL11.GL_DEPTH_BUFFER_BIT;
import static org.lwjgl.opengl.GL11.GL_DEPTH_TEST;
import static org.lwjgl.opengl.GL11.GL_FILL;
import static org.lwjgl.opengl.GL11.GL_FRONT;
import static org.lwjgl.opengl.GL11.GL_FRONT_AND_BACK;
import static org.lwjgl.opengl.GL11.GL_INCR;
import static org.lwjgl.opengl.GL11.GL_KEEP;
import static org.lwjgl.opengl.GL11.GL_LEQUAL;
import static org.lwjgl.opengl.GL11.GL_LINE;
import static org.lwjgl.opengl.GL11.GL_NOTEQUAL;
import static org.lwjgl.opengl.GL11.GL_ONE;
import static org.lwjgl.opengl.GL11.GL_ONE_MINUS_SRC_ALPHA;
import static org.lwjgl.opengl.GL11.GL_ONE_MINUS_SRC_COLOR;
import static org.lwjgl.opengl.GL11.GL_SRC_ALPHA;
import static org.lwjgl.opengl.GL11.GL_STENCIL_BUFFER_BIT;
import static org.lwjgl.opengl.GL11.GL_STENCIL_TEST;
import static org.lwjgl.opengl.GL11.glBlendFunc;
import static org.lwjgl.opengl.GL11.glClear;
import static org.lwjgl.opengl.GL11.glCullFace;
import static org.lwjgl.opengl.GL11.glDepthMask;
import static org.lwjgl.opengl.GL11.glDisable;
import static org.lwjgl.opengl.GL11.glEnable;
import static org.lwjgl.opengl.GL11.glStencilFunc;
import static org.lwjgl.opengl.GL11.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the project's coding standards are against this. Did you use Checkstyle?

Copy link
Contributor Author

@tdgunes tdgunes Jun 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Just rechecked. Seems like import static statements with wildcards are not against CheckStyle.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's ask @msteiger or @Cervator, they know better.

Copy link
Member

Choose a reason for hiding this comment

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

Static imports are considered "different" somehow, but I can't actually remember why. A quick Google search and a page find for "static" gets you a little bit of reasoning: http://stackoverflow.com/questions/2067158/what-is-the-proper-style-for-listing-imports-in-java

That thread also finally made me realize why regular import star is bad - not just for some optimization reason, but if new classes are added that would suddenly qualify due to the wildcard bad things can suddenly happen. Which differs with static imports... somehow.

Copy link
Contributor

@emanuele3d emanuele3d Jun 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@Cervator: so..... import star for static imports is ok? Can you confirm?

Copy link
Member

Choose a reason for hiding this comment

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

Can't confirm completely, no. I wouldn't go out of my way to star import statics if they're split out already, but it may make sense in general. @immortius could probably call it, or @flo / @msteiger

Copy link
Member

Choose a reason for hiding this comment

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

I don't see any advantage of using wildcard imports. Any reasonable IDE would fold imports, so you don't even see them. Most notable drawback imho is that you don't immediately see which class is used if multiple wildcards are used. You can also run into conflicts if two classes with the same name are imported.

import static org.lwjgl.opengl.GL20.glStencilOpSeparate;
import static org.terasology.rendering.opengl.OpenGLUtils.bindDisplay;
import static org.terasology.rendering.opengl.OpenGLUtils.setRenderBufferMask;

/**
* The GraphicState class aggregates a number of methods setting the OpenGL state
Expand Down Expand Up @@ -144,30 +112,6 @@ public void setSceneShadowMap(FBO newShadowMap) {
buffers.sceneShadowMap = newShadowMap;
}

/**
* Initial clearing of a couple of important Frame Buffers. Then binds back the Display.
*/
// It's unclear why these buffers need to be cleared while all the others don't...
public void initialClearing() {
buffers.sceneOpaque.bind();
glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT | GL_STENCIL_BUFFER_BIT);
buffers.sceneReflectiveRefractive.bind();
glClear(GL_COLOR_BUFFER_BIT | GL_DEPTH_BUFFER_BIT);
bindDisplay();
}

/**
* Readies the state to render the Opaque scene.
*
* The opaque scene includes a number of successive passes including the backdrop (i.e. the skysphere),
* the landscape (chunks/blocks), additional objects associated with the landscape (i.e. other players/fauna),
* overlays (i.e. the cursor-cube) and the geometry associated with the first person view, i.e. the objects
* held in hand.
*/
public void preRenderSetupSceneOpaque() {
buffers.sceneOpaque.bind();
setRenderBufferMask(buffers.sceneOpaque, true, true, true);
}

/**
* Resets the state after the rendering of the Opaque scene.
Expand All @@ -180,16 +124,6 @@ public void postRenderCleanupSceneOpaque() {
bindDisplay();
}

/**
* Sets the state to render in wireframe.
*
* @param wireframeIsEnabledInRenderingDebugConfig If True enables wireframe rendering. False, does nothing.
*/
public void enableWireframeIf(boolean wireframeIsEnabledInRenderingDebugConfig) {
if (wireframeIsEnabledInRenderingDebugConfig) {
GL11.glPolygonMode(GL_FRONT_AND_BACK, GL_LINE);
}
}

/**
* Disables wireframe rendering. Used together with enableWireFrameIf().
Expand All @@ -203,36 +137,6 @@ public void disableWireframeIf(boolean wireframeIsEnabledInRenderingDebugConfig)
}


/**
* Sets the state to render the Backdrop. At this stage the backdrop is the SkySphere
* plus the SkyBands passes.
*
* The backdrop is the only rendering that has three state-changing methods.
* This is due to the SkySphere requiring a state and the SkyBands requiring a slightly
* different one.
*/
public void preRenderSetupBackdrop() {
setRenderBufferMask(buffers.sceneOpaque, true, false, false);
}

/**
* Sets the state to generate the SkyBands.
*
* See preRenderSetupBackdrop() for some more information.
*/
public void midRenderChangesBackdrop() {
setRenderBufferMask(buffers.sceneOpaque, true, true, true);
}

/**
* Resets the state after the rendering of the Backdrop.
*
* See preRenderSetupBackdrop() for some more information.
*/
public void postRenderCleanupBackdrop() {
buffers.sceneOpaque.bind();
}

/**
* Sets the state to render the First Person View.
*
Expand Down Expand Up @@ -417,48 +321,7 @@ public void postRenderCleanupChunk() {
GL11.glPopMatrix();
}

/**
* Once an FBO is bound, opengl commands will act on it, i.e. by drawing on it.
* Meanwhile shaders might output not just colors but additional per-pixel data. This method establishes on which
* of an FBOs attachments, subsequent opengl commands and shaders will draw on.
*
* @param fbo The FBO holding the attachments to be set or unset for drawing.
* @param color If True the color buffer is set as drawable. If false subsequent commands and shaders won't be able to draw on it.
* @param normal If True the normal buffer is set as drawable. If false subsequent commands and shaders won't be able to draw on it.
* @param lightBuffer If True the light buffer is set as drawable. If false subsequent commands and shaders won't be able to draw on it.
*/
// TODO: verify if this can become part of the FBO.bind() method.
public void setRenderBufferMask(FBO fbo, boolean color, boolean normal, boolean lightBuffer) {
if (fbo == null) {
return;
}

int attachmentId = 0;

IntBuffer bufferIds = BufferUtils.createIntBuffer(3);

if (fbo.colorBufferTextureId != 0) {
if (color) {
bufferIds.put(GL_COLOR_ATTACHMENT0_EXT + attachmentId);
}
attachmentId++;
}
if (fbo.normalsBufferTextureId != 0) {
if (normal) {
bufferIds.put(GL_COLOR_ATTACHMENT0_EXT + attachmentId);
}
attachmentId++;
}
if (fbo.lightBufferTextureId != 0) {
if (lightBuffer) {
bufferIds.put(GL_COLOR_ATTACHMENT0_EXT + attachmentId);
}
}

bufferIds.flip();

GL20.glDrawBuffers(bufferIds);
}

private class Buffers {
public FBO sceneOpaque;
Expand Down
Loading