Initial work on ssao #91
Conversation
There was a problem hiding this comment.
I'm afraid this will need to wait (together with #89) until September to get merged -- I'm too busy, sorry about that. Here's an initial review pass to get the ball rolling :)
If you want to save me some time, could you add a doc/ssao.dox file that:
- is based off one of the other files (for example the arcball camera)
- references a 800x600 screenshot image
- lists you as an author
- properly credits the Armadillo model
- lists controls
- lists all example source files
It doesn't need to be perfect, just something to get started.
This is great work, thanks a lot! 👍
src/ssao/SSAOExample.cpp
Outdated
| Shaders::SSAOApply _ssaoApply{NoCreate}; | ||
| Shaders::SSAO _ssao{NoCreate}; | ||
|
|
||
| ::Magnum::Shaders::Phong _phong{NoCreate}; |
There was a problem hiding this comment.
Exactly because of this I wouldn't introduce another Shaders namespace here.
I'd suggest GeometryShader, SsaoApplyShader and SsaoShader instead, and for consistency having different capitalization, SsaoExample as well. Sorry for the boring rename work and thanks in advance :)
src/ssao/SSAOExample.cpp
Outdated
| { | ||
| const Vector2 dpiScaling = this->dpiScaling({}); | ||
| Configuration conf; | ||
| conf.setTitle("Magnum SSAOApply Example") |
There was a problem hiding this comment.
| conf.setTitle("Magnum SSAOApply Example") | |
| conf.setTitle("Magnum SSAO Example") |
probably some find&replace gone wild? :)
src/ssao/SSAOExample.cpp
Outdated
| std::uniform_real_distribution<float> distr(-1,1); | ||
| Containers::Array<Vector4> noise(Containers::NoInit, 16); | ||
| for (Vector4& n : noise) | ||
| n = Vector4{distr(engine),distr(engine),0,0}; |
There was a problem hiding this comment.
That reminds me I need to merge mosra/magnum#432 as well 🙈
| @@ -0,0 +1,10 @@ | |||
| layout(location = 0) | |||
There was a problem hiding this comment.
Can you add the same license header as you have in the SSAOExample.cpp to all shaders and other cpp/h files as well?
src/ssao/Shaders/Geometry.h
Outdated
| @@ -0,0 +1,43 @@ | |||
|
|
|||
| #pragma once | |||
There was a problem hiding this comment.
For consistency, can you use the usual include guards as elsewhere? #ifndef Magnum_Examples_<classname>_h etc
src/ssao/Shaders/SSAO.cpp
Outdated
| GL::Shader vert{GL::Version::GL420, GL::Shader::Type::Vertex}; | ||
| GL::Shader frag{GL::Version::GL420, GL::Shader::Type::Fragment}; |
There was a problem hiding this comment.
| GL::Shader vert{GL::Version::GL420, GL::Shader::Type::Vertex}; | |
| GL::Shader frag{GL::Version::GL420, GL::Shader::Type::Fragment}; | |
| GL::Shader vert{GL::Version::GL330, GL::Shader::Type::Vertex}; | |
| GL::Shader frag{GL::Version::GL330, GL::Shader::Type::Fragment}; |
I assume you wanted 330 since you're asserting for that above? :)
src/ssao/Shaders/SSAO.h
Outdated
|
|
||
| UnsignedInt _projectionUniform = 0; | ||
|
|
||
| Containers::Array<Vector3> _randomSamples; |
There was a problem hiding this comment.
Hmm, do we actually need to store this if it's uploaded in the constructor and never used again?
| THE AUTHORS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER | ||
| IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN | ||
| CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. | ||
| */ |
There was a problem hiding this comment.
The indent of this license header seems off compared to other files :)
src/ssao/Shaders/SSAOApply.h
Outdated
| NormalUnit = 1, | ||
| AlbedoUnit = 2, | ||
| OcclusionFactorUnit = 3 | ||
| }; |
There was a problem hiding this comment.
Could be probably defined in an unnamed namespace inside the cpp, as it's not used publicly in any way.
Same in the SSAO shader.
src/ssao/SSAOExample.cpp
Outdated
| } | ||
|
|
||
| _profiler.endFrame(); | ||
| _profiler.printStatistics(10); |
There was a problem hiding this comment.
You're awesome -- I just wanted to suggest hooking up the profiler and you already did that. Can you share the numbers from it? I'm interested, especially comparing fragment and compute implementation.
src/ssao/SSAOExample.cpp
Outdated
| _framebuffer.attachTexture(GL::Framebuffer::BufferAttachment::Depth, _depth, 0) | ||
| .attachTexture(GL::Framebuffer::ColorAttachment{0}, _albedo, 0) | ||
| .attachTexture(GL::Framebuffer::ColorAttachment{1}, _positions, 0) | ||
| .attachTexture(GL::Framebuffer::ColorAttachment{2}, _normals, 0) |
There was a problem hiding this comment.
This together with checkStatus() would be probably better to be done in constructor, instead of in every frame?
src/ssao/SSAOExample.cpp
Outdated
| _framebuffer.mapForDraw({ | ||
| {Shaders::SSAO::AmbientOcclusionOutput, GL::Framebuffer::ColorAttachment{3}}, | ||
| }); | ||
| _ssao.draw(GL::Mesh{}.setCount(3)); |
There was a problem hiding this comment.
This (and below) also (it's still a VAO creation + destruction every time, not sure how much effort that needs in the driver).
implementation. rework and add imgui
This patch provides a ssao example. The computation of the occlusion factor can either be done with a fragment shader or with a compute shader. Three TODOs:
Here is a little sneak peak
This PR is in commission for Vhite Rabbit.