--Implementation of Nvidia's HBAO effect (soft shadows in corners and crevices) via Magnum #2192

jturner65 · 2023-08-22T17:38:05Z

Motivation and Context

This PR implements Nvidia's HBAO as per the shaders found here but using Magnum's graphics framework. This work was built from, and compared against, a reference implementation of the Nvidia sample code using Nvidia's graphics framework that was integrated into Habitat-Sim in this WIP PR.

Both Classic and Cache-Aware implementations are available, as well as Perpective and Orthographic sensor support (driven by the projection matrix configuration of the owning visual sensor).

TODO for This PR :

~~Update test images to match the working effect.~~
~~Gate test so that it does not attempt to run in WebGL builds~~ not necessary

TODO Next PR/Future Work :

Configuration support to allow easy customization of effect
Add MSAA support
Eventually retool and cleanup to be more generic (i.e. dispose of separate "classic" and "cache-aware" paths)
Validate effect when accessed via Habitat-Lab

Demonstration of with/without effect via CacheAware algorithm, both Perspective and Orthographic projection.

newCacheAwareHBAOHalfScreenExampl.mp4

How Has This Been Tested

New C++ tests to verify integrity of effect; existing c++ and python tests pass

Types of changes

Docs change / refactoring / dependency upgrade
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

0mdc · 2023-09-05T19:54:55Z

Pretty cool! Nice job!

src/tests/GfxBatchHbaoTest.cpp

mosra · 2023-09-08T10:42:23Z

src/tests/GfxBatchHbaoTest.cpp

+     esp::gfx_batch::HbaoConfiguration{}.flags(), 2.0f, 1.5f, 10.0f, 0.0f,
+     0.0f},
+    {"cache-aware, perspective, layered with image load store",
+     "hbao_tests/van-gogh-room.hbao-cache-layered.png", false, true,


The reason I originally used the same image for the defaults, geometry shader and image load/store was to be sure that they produce the same output (assuming all they do is speed improvements). If you have a dedicated image for each, you can no longer ensure the differences between the two are minimal.

In other words, there shouldn't be the van-gogh-room.hbao-cache-layered.png, van-gogh-room.hbao-cache-geom.png, van-gogh-room.hbao-cache-layered-ortho.png or van-gogh-room.hbao-cache-geom-ortho.png files, those should be compared to van-gogh-room.hbao-cache.png / van-gogh-room.hbao-cache-ortho.png as well. The "special blur" variant to my understanding, should produce a different / better output than the default blur so it has to be a different file, and the "strong effect" variant is obviously different too.

To save the correct ground truth images, use --save-diagnostics together with --abort-on-failure (or with --only), to ensure the ground truth image is generated from the "defaults" case and the GS etc. variants aren't overwriting it.

And the thresholds for the GS and image load/store variants should be reduced down to a reasonable minimum that still passes -- the 4.5f and 0.11f was what my initial (buggy) port had, you'll likely have it lower now. You can see the actual calculated thresholds for passing tests with --verbose.

src/tests/GfxBatchHbaoTest.cpp

mosra · 2023-09-08T11:02:16Z

src/tests/GfxBatchHbaoTest.cpp

+                    Cr::Containers::arraySize(TestData));
+}
+
+constexpr Mn::Vector2i Size{256, 256};


Got an idea for easy verification that the aspect ratio handling is implemented correctly: change this to something non-square (that still reasonably covers the scene, of course) and add a second test variant (testRotated()?) that uses Size.flipped() instead of Size and loads the images rotated by 90°:

/* This rotates the depth image by 90° */ Cr::Containers::Array<char> rotatedDepth{Cr::NoInit, depth->data.size()}; Cr::Utility::copy(depth->pixels().flipped<1>().transposed<0, 1>(), Cr::Containers::StridedArrayView2D<char>{rotatedDepth, {std::size_t(Size.y()), std::size_t(Size.x())}}); Mn::GL::Texture2D inputDepthTexture; inputDepthTexture .setStorage(1, Mn::GL::TextureFormat::DepthComponent32F, Size.flipped()) .setSubImage(0, {}, Mn::ImageView2D{depth->format(), Size.flipped(), rotatedDepth});

Similarly for the color image. The final comparison would be to the original image, but with the pixels rotated back:

CORRADE_COMPARE_WITH( output.read({{}, Size}, {Mn::PixelFormat::RGBA8Unorm}) .pixels<Color4ub>().transposed<1, 0>().flipped<1>(), Cr::Utility::Path::join(TEST_ASSETS, data.filename), (Mn::DebugTools::CompareImageToFile{data.maxThreshold, data.meanThreshold}));

If everything is correct in the implementation, the test should pass without any difference compared to the non-rotated case.

(Details about strided array views and their dimension flipping and transposition are in this blog post.)

Put these tests in and they are exposing a bias in the algorithm that seems like it exists in the shader, so I have marked them Expect to Fail and will investigate this further. The effect is present and visually pleasing, however, so I don't think this should hold up the PR.

src/tests/GfxBatchHbaoTest.cpp

src/tests/CMakeLists.txt

src/shaders/gfx_batch/hbao/bilateralblur.frag

0mdc

I left some minor comments.

Did you have a change to measure rendering performance with/without HBAO?

src/esp/bindings/SimBindings.cpp

src/esp/gfx_batch/Hbao.h

src/esp/gfx_batch/Hbao.cpp

mosra

Accidentally opened a GitHub notification so had to go through the code and write comments :)

src/esp/gfx_batch/Hbao.h

mosra · 2023-09-13T08:39:02Z

src/esp/gfx_batch/Hbao.cpp

+
+Hbao& Hbao::operator=(Hbao&&) noexcept = default;
+
+void Hbao::setConfiguration(const HbaoConfiguration& configuration) {


Wait a moment, I thought we agreed that you'd just replace the instance when configuration changes? This setter is only adding extra potential for bugs as it may not reininitialize all internal state properly.

In other words, this is what I mean:

Hbao hbao; ... // when settings change hbao = Hbao{newHbaoConfiguration};

That way the instance is, when constructed, always correct, and there's no chance some state may be stale because setConfiguration() forgot to overwrite something that was only populated in a constructor.

For changing actual runtime parameters (as we discussed), there should be direct setters instead, separate from the HbaoConfiguration class which should have only the initial setup.

I'll rework this when I work on the user-driven attributes-based customization functionality.

src/esp/gfx_batch/Hbao.cpp

src/tests/GfxBatchHbaoTest.cpp

jturner65 · 2023-09-13T12:44:30Z

Did you have a change to measure rendering performance with/without HBAO?

~~Not yet, I am going to investigate that today.~~ Took a backseat to the PBR skin rendering. I'll investigate it soon though.

aclegg3

Looks good for initial merge, thanks @jturner65 and @mosra!
We can follow-up with additional polish.

src/tests/GfxBatchHbaoTest.cpp

src/esp/gfx_batch/Hbao.h

src/esp/gfx_batch/Hbao.cpp

src/tests/GfxBatchHbaoTest.cpp

mosra · 2023-09-21T17:38:38Z

src/tests/GfxBatchHbaoTest.cpp

+
+constexpr const char* baseTestFilename = "van-gogh-room";
+
+const std::string SourceTestPlyDir =


Just FYI, Utility::Path::join(), Utility::format() all return Containers::String, and the APIs taking strings use StringViews.

Not that I'd want that changed away from std::string across all of Habitat, but existing code in gfx_batch is using the Corrade ones exclusively so it'd be great if it was kept consistent. (I'm probably just annoying at this point, sorry.)

Not at all. I wonder about these things, but I don't want to overly annoy you with questions about these little things. :)

I'll revisit this in HBAO version 1.1 :)

Also, default should be always have blur - blur should be disabled only for debugging (since no blur looks awful).

Functionality specified with "blur" in the original nvidia code explicitly meant using the special blur algorithm, not both blur and special blur.

-add test cases using layers and special blur

--rotation/de-rotation test results in images that are outside the bounds of our current thresholds, and so this test is marked as expect to fail until it can be repaired. This behavior is not likely

…e HBAO tests.

--currently we do not allow for the Geometry-based layer handling if we are not using the cache-aware/interleaving algorithm, so this cleans up some inappropriate but never-reached code in the shader.

--direct flag-setting access was removed due to interdependencies/mutual exclusivity between certain states.

jturner65 requested a review from 0mdc August 22, 2023 17:38

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Aug 22, 2023

jturner65 requested review from mosra, aclegg3 and eundersander and removed request for 0mdc August 22, 2023 17:38

mosra force-pushed the HBAO_WIP_GenericShader branch 2 times, most recently from ead30c4 to 026b403 Compare August 23, 2023 19:55

jturner65 force-pushed the HBAO_WIP_GenericShader branch from 4a070aa to cf7392c Compare August 29, 2023 19:51

jturner65 changed the title ~~--[WIP]-HBAO implementation using a generic shader~~ --[WIP]-HBAO implementation via Magnum/Habitat Aug 29, 2023

jturner65 force-pushed the HBAO_WIP_GenericShader branch 2 times, most recently from 25e20af to 8762518 Compare August 30, 2023 20:36

jturner65 changed the title ~~--[WIP]-HBAO implementation via Magnum/Habitat~~ --Implementation of Nvidia's HBAO effect (soft shadows in corners and crevices) via Magnum Sep 5, 2023

jturner65 marked this pull request as ready for review September 5, 2023 17:45

jturner65 requested a review from 0mdc September 5, 2023 20:51

This comment was marked as outdated.

Sign in to view

mosra reviewed Sep 8, 2023

View reviewed changes

src/shaders/gfx_batch/hbao/bilateralblur.frag Outdated Show resolved Hide resolved

jturner65 force-pushed the HBAO_WIP_GenericShader branch 2 times, most recently from 2f6edbf to b09150b Compare September 12, 2023 13:15

0mdc reviewed Sep 12, 2023

View reviewed changes

mosra reviewed Sep 13, 2023

View reviewed changes

jturner65 force-pushed the HBAO_WIP_GenericShader branch 3 times, most recently from 29ab4a8 to e3ed691 Compare September 19, 2023 18:04

aclegg3 approved these changes Sep 20, 2023

View reviewed changes

mosra reviewed Sep 21, 2023

View reviewed changes

jturner65 force-pushed the HBAO_WIP_GenericShader branch from f4e326c to 0d46a30 Compare September 21, 2023 21:16

jturner65 added 27 commits September 22, 2023 12:55

--remove unnecessary constructor; streamline testing

dce98f6

--shader fixes; precision; naming conventions (blur shaders)

bab3292

--shader fixes; precision; naming conventions (depth linearize)

9e57092

--shader fixes; precision; naming conventions (de/re interleave)

648230f

Also, default should be always have blur - blur should be disabled only for debugging (since no blur looks awful).

--shader fixes; precision; naming conventions (viewnormal)

e9066e6

--compiler direct AO_DEINTERLEAVED

80d030f

--fix incorrect blur bug/assumptions.

93c9d2c

Functionality specified with "blur" in the original nvidia code explicitly meant using the special blur algorithm, not both blur and special blur.

--minor

d25573b

--clean up AO_SPECIAL_BLUR

d175b5e

--update copyrights

a5108b7

--make hbao.frag compatible with es300

f335490

--fix issue with non-layered cache-aware alg

4f59d36

--fix issue with inappropriate type and handling for texLinearDepth

1717166

--change blur flag to enable no-blur instead.

942a1e4

--fix improper layered load/store framebuffer config

1819e50

--update and expand tests

ca523ea

-add test cases using layers and special blur

--tighten up test bounds

bc24e50

-change size and aspect ratio not square

ff6e09e

--move ply mesh to test assets; add perspective rotation test;

2b11f35

--rotation/de-rotation test results in images that are outside the bounds of our current thresholds, and so this test is marked as expect to fail until it can be repaired. This behavior is not likely

--pull the most recent test assets to get the van-gogh ply used in th…

4a3e285

…e HBAO tests.

--only build hbao for supported camera types; stub out cubemap support

502dadd

--uniform naming conventions

752adda

--address geometry shader/passthrough confusion regarding texCoords

00d3f28

--currently we do not allow for the Geometry-based layer handling if we are not using the cache-aware/interleaving algorithm, so this cleans up some inappropriate but never-reached code in the shader.

--reviewer suggestions.

59f9f72

--format; copyright header;

b28501f

--reviewer suggestions

6f97652

--improve test config setup; temp bypass of bench code;

f72ab20

--direct flag-setting access was removed due to interdependencies/mutual exclusivity between certain states.

jturner65 force-pushed the HBAO_WIP_GenericShader branch from 9437aad to f72ab20 Compare September 22, 2023 17:22

jturner65 merged commit 536cb4d into main Sep 22, 2023
10 checks passed

jturner65 deleted the HBAO_WIP_GenericShader branch September 22, 2023 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--Implementation of Nvidia's HBAO effect (soft shadows in corners and crevices) via Magnum #2192

--Implementation of Nvidia's HBAO effect (soft shadows in corners and crevices) via Magnum #2192

jturner65 commented Aug 22, 2023 •

edited

0mdc commented Sep 5, 2023

This comment was marked as outdated.

mosra Sep 8, 2023

mosra Sep 8, 2023

jturner65 Sep 20, 2023

0mdc left a comment •

edited

mosra left a comment

mosra Sep 13, 2023

jturner65 Sep 20, 2023

jturner65 commented Sep 13, 2023 •

edited

aclegg3 left a comment •

edited

mosra Sep 21, 2023

jturner65 Sep 21, 2023

jturner65 Sep 22, 2023


		Hbao& Hbao::operator=(Hbao&&) noexcept = default;

		void Hbao::setConfiguration(const HbaoConfiguration& configuration) {


		constexpr const char* baseTestFilename = "van-gogh-room";

		const std::string SourceTestPlyDir =

--Implementation of Nvidia's HBAO effect (soft shadows in corners and crevices) via Magnum #2192

--Implementation of Nvidia's HBAO effect (soft shadows in corners and crevices) via Magnum #2192

Conversation

jturner65 commented Aug 22, 2023 • edited

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

0mdc commented Sep 5, 2023

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0mdc left a comment • edited

Choose a reason for hiding this comment

mosra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturner65 commented Sep 13, 2023 • edited

aclegg3 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jturner65 commented Aug 22, 2023 •

edited

0mdc left a comment •

edited

jturner65 commented Sep 13, 2023 •

edited

aclegg3 left a comment •

edited