Page MenuHome

Framebuffer effects:
ClosedPublic

Authored by Antonis Ryakiotakis (psy-fi) on Feb 10 2015, 11:49 AM.

Diff Detail

Repository
rB Blender
Branch
viewport_master_merge

Event Timeline

Compilation error (CMake/Linux):
../source/blender/editors/space_view3d/view3d_draw.c:98:26: fatal error: GPU_renderer.h: No such file or directory

  • Cleanup leftover code from WIP renderer
source/blender/gpu/intern/gpu_compositing.c
58

Since new GLs (and GL ES) have triangles but not quads would recommend to go ahead and use GL_TRIANGLES here so it's one less thing GL thing that needs updating in the future.

Could also be slightly more efficient to do one big triangle that goes offscreen rather than a quad or two triangles (because fragment shader gets invoked extra times on the shared border, even though those samples get discarded eventually).

Crappy ASCII art diagram, square at lower right is the viewport and big triangle completely covers it.

|\
|_\
|_|_\
486

Minor clarity suggestion: this is a pretty large function; these passes could be split out into separate functions that just get called in the proper sequence from GPU_fx_do_composite_pass.

Let's not get paranoid here, the cost is bound by fragment shading, not vertex shading. 2 triangles won't be an issue when scenes consist of 100s of thousands of triangles, and calculating appropriate coordinates for various effects is more involved in the triangle case. Agree about the QUADS and the code readability though.

source/blender/editors/render/render_opengl.c
465

Typo? Constant being ORd with itself

source/blender/makesdna/DNA_gpu_types.h
62

Would make an enum for these defines. Even if the SDNA field still has to be an int, there are places where functions are currently taking an 'int' param that could be an enum.

Let's not get paranoid here, the cost is bound by fragment shading, not vertex shading. 2 triangles won't be an issue when scenes consist of 100s of thousands of triangles, and calculating appropriate coordinates for various effects is more involved in the triangle case. Agree about the QUADS and the code readability though.

Yep, my comment did say fragment shading cost. When two triangles/quads are rendered there, batches of fragments are processed along the shared edge. Even though these fragments get discarded eventually (GL spec requires avoiding that overdraw) they are likely still processed partially.

source/blender/makesdna/DNA_gpu_types.h
62

I am a bit reluctant to make enums out of bitfields (enums in my head are usually more mutually exclusive)

Antonis Ryakiotakis (psy-fi) edited edge metadata.
  • Forward compatibility: Use triangle strips instead of quads
source/blender/makesdna/DNA_gpu_types.h
62

There's plenty of precedent for using enums this way in SDNA, see for example BrushFlags or ModifierMode.

One nice advantage of using an enum is that for variables declared with that type, gdb (and presumably other debuggers) can break down the value into its components, e.g. instead of saying '5' it will show 'GPU_FX_DEPTH_OF_FIELD|GPU_FX_HDR'.

Another advantage is it's more strongly typed, you'll get better warnings from the compiler.

  • Use an enum for enabled GPU Fx
  • Cleanup redundant state changes
Sergey Sharybin (sergey) requested changes to this revision.Feb 11 2015, 4:51 PM
Sergey Sharybin (sergey) edited edge metadata.

Just some quick scroll through..

SConstruct
767

Indentation is broken?

release/scripts/startup/bl_ui/space_view3d.py
2953

It seems the rna path is:

fxoptions.ssao_options.ssao_darkening

and so on. Not sure ssao_ prefix is needed in the property name.

source/blender/blenkernel/intern/screen.c
602

"{" should be on the new line.

This revision now requires changes to proceed.Feb 11 2015, 4:51 PM
Campbell Barton (campbellbarton) requested changes to this revision.Feb 11 2015, 5:16 PM
Campbell Barton (campbellbarton) edited edge metadata.

Generally looks good. noted some issues.

Theres a glitch when using the view3d without a camera, the FStop is 0.0 making everything out of focus - always. no matter what the focal distance - the focal distance is also 0.0 (probably should be set to something else).

Theres a glitch with SSAO defaulting to 0 samples too (outside the RNA range)

release/scripts/startup/bl_ui/space_view3d.py
2951

UI (text SSAO is a bit cryptic). Ambient Occlusion would fit just fine.

source/blender/gpu/GPU_compositing.h
49

should be GPUFxFlags

source/blender/gpu/shaders/gpu_shader_fx_dof_frag.glsl
50

120 line length.

source/blender/gpu/shaders/gpu_shader_fx_dof_vert.glsl
41

*picky* - 120 line length.

source/blender/gpu/shaders/gpu_shader_fx_ssao_frag.glsl
22

*picky* - 120 line length.

source/blender/makesdna/DNA_gpu_types.h
61

*picky* GPUFx vs GPUFX (both are used, slightly confusing)

source/blender/makesdna/DNA_view3d_types.h
224

Not sure its needed to malloc this struct? (possible more hassle then its worth)

source/blender/makesrna/intern/rna_camera.c
294–298

We discussed this a while back, and IIRC following camera focus settings doesn't work 100% the same, which is why the camera needs its own GPU settings.

The focal length in the gpu_dof is having an effect on the result though!

Is this intended?

source/blender/makesrna/intern/rna_scene.c
3850–3872

dof_ prefix is redundant, since the the struct is dof

eg: v3d.dof.dof_blah

3884–3909

ssao_ prefix is redundant, since the the struct is ssao

eg: v3d.ssao.ssao_blah

source/blender/makesrna/intern/rna_space.c
2215

*picky* (could call fx_options)

Some ideas:

  • Try to share focal length for real camera and DOF settings. Not really sure why you want them to be different.
  • Make more reasonable defaults for SSAO and DOF. So you can totally see what's happening when you first time enable the features.
  • It's kinda weird to have DOF applied in both camera view and non-camera view. Can totally see why you want that in camera view, but would also think in some cases you might not want DOF outside of camera view?
  • Focus on an active object when in non-camera view?

re: focus on active object when in non-camera view...

For small active objects its probably fine... but for larger objects (terrain, walls of building, trees etc)... this would get annoying fast.
We have similar problem with rotate around active selection preference. Small objects its fine but when it fails on large objects its annoying.


@Antonis Ryakiotakis (psy-fi), can you ask artists why they want DOF in non camera views?

Whats the use-case here?

Unlike SSAO, this isnt helping to add extra information to the scene, I can only imagine that an artist may want to set up a shot and quickly check what dof looks like in the viewport before adding a camera (a kind of preview of what the camera would look like).
Or to work in a scene where the background is known to be out of focus, this wouldn't draw unnecessary attention to it.

In that case, you probably dont want the focus jumping around based on selection. (unless its just to set the initial focus)
In camera view you can press: WKey->Dof-Distance we could do the same for the non camera view too so at least its quick to eyedrop the distance.

release/scripts/startup/bl_ui/space_view3d.py
2951

Ambient occlusion is a more "complete" algorithm, leave it as SSAO, it's well known from games.

source/blender/makesrna/intern/rna_camera.c
298

You have a point, focal length and sensor size can be reused from the camera here. Still a bit awkward that units are not properly converted though. We'll have to take a good look at that eventually.

Antonis Ryakiotakis (psy-fi) edited edge metadata.
  • Minor simplification
  • Cleanup naming, use sane defaults
Antonis Ryakiotakis (psy-fi) edited edge metadata.
  • Don't allocate FX, naming of eGPUFXFlags
  • More naming tidying up
  • Reuse focal length and sensor from camera
  • 3D Viewport only uses DOF when rendered from camera now.
This revision was automatically updated to reflect the committed changes.