Improved SSAO2 when samples <16. Added more control over SSAO2 denoising filter.

[fooware]

Related to this thread in the forums:
https://forum.babylonjs.com/t/ssao2-blur-shader-issues/38924/25

SSAO calculation fix

Fixes an error in SSAO2 calculations that caused massive amounts of incorrect occlusions on flat surfaces when samples were <16. This change is automatically applied with the new epsilon parameter defaulting to 0.02, demonstrated below with samples = 8 (fixed on the right). Setting the new parameter to 0.0 will revert back to the old behavior.

See #13652 for discussions and more screenshots.

Terrain (#N96NXC#127)

Cat (#T3K2SA#1)

Room (#LCUPCU#14)

Denoising / Blur filter is more configurable

The "expensiveBlur" filter has been improved with new features and their associated parameters.

This change was done with care so that the new code keeps all the quirks and bugs in the current code, while adding options to mitigate them on a per scene/project basis. This was none of the existing deployments are affected since they might have already tried to compensate for them in other ways.

Summary of changes

• Adds option to disable the blur filters for easier tuning of the SSAO2 parameters.
• Made the "expensive filter" configurable (number of samples, shape of kernel, tolerance when rejecting samples) while keeping the defaults to be exactly like before the PR.
• Exposed the new settings in the Inspector.
• Cleaned up the code, reducing rather big code duplication.
• Added and updated comments and documentation.
• Added some optimizations based on feedback from @Popov72.

This is the first time I contribute to Babylon.js, so please let me know if I need to do or change something.

[bjsplat]

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

[fooware]

I would very much like to run this commit with the demo bellow to evaluate the results, is that possible?
https://www.babylonjs.com/demos/ssao2/

[fooware]

Related issue: #12630
This PR however tries to improve things within the scope of non-breaking changes and no reduced performance.

SSAO2 however needs a big overhaul and modernization. It really should look better than it does and there are improvements to be made in the SSAO step as well as using more advanced denoising filters, perhaps a "guided filter" or something even more recent.

The current bilateral filter also needs fixing. It doesn't take normals into account and thus can be fooled quite easily by some geometry in the depth buffer. That however is out of scope for this PR since it would break the "not slower" performance criteria.

The SSAO2 shader as a whole probably also needs overhauling. It looks much more "hacky" and primitive than for example the blur postprocessing shaders. There are also synergies that can come from extracting common things there, like the gaussian filter code and optimizations.

[bjsplat]

Snapshot stored with reference name:
refs/pull/13621/merge

Test environment:
https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13621/merge/index.html

To test a playground add it to the URL, for example:

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13621/merge/index.html#WGZLGJ#4600

Links to test babylon tools with this snapshot:

https://playground.babylonjs.com/?snapshot=refs/pull/13621/merge
https://sandbox.babylonjs.com/?snapshot=refs/pull/13621/merge
https://gui.babylonjs.com/?snapshot=refs/pull/13621/merge
https://nme.babylonjs.com/?snapshot=refs/pull/13621/merge

To test the snapshot in the playground with a playground ID add it after the snapshot query string:

https://playground.babylonjs.com/?snapshot=refs/pull/13621/merge#BCU1XR#0

[Popov72]

Thanks for the PR, I will have a look tomorrow!

I would very much like to run this commit with the demo bellow to evaluate the results, is that possible?

You can browse this PG locally: http://localhost:1338/#T3K2SA#1

[bjsplat]

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13621/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

[fooware]

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13621/merge/testResults/webgl2/index.html

First, a noob question, is the left or the right side the stored reference?

Secondly, could someone help me to figure out why "Light Projection Texture" test completely changed to/from a black screen? I don't see how it is connected to SSAO2.

[fooware]

There is also a failing unit test that I don't understand how it is affected by my changes:

Summary of all failing tests
FAIL packages/dev/core/test/unit/DeviceInput/babylon.inputManager.test.ts (21.79 s)
● InputManager › Doesn't fire onPointerOberservable for POINTERTAP when ExclusiveDoubleClickMode is enabled

[fooware]

I need to look into the lower blur quality of the bilateral denoiser in the Cat scene. Probably a side effect of moving from a box filter to a modified gaussian filter while being limited to using the same number of samples as before.

[Popov72]

Don't worry with the first visualization test that fails, it's not related to your PR but a long standing problem we have with this test...

Regarding the 2nd one, have you merged your PR with the latest changes from master? It could explain why it is failing it you didn't do it.

[fooware]

Don't worry with the first visualization test that fails, it's not related to your PR but a long standing problem we have with this test...

OK, got it. 👍

Regarding the 2nd one, have you merged your PR with the latest changes from master? It could explain why it is failing it you didn't do it.

I think I did, but both those tests looks like they are using SSAO, so some change would be expected. Where can I find the source of those tests so I can see how they are setup?

[Popov72]

Good job!

I also noticed that we are generating mipmaps for the SSAO effect and blur textures, which doesn't make sense! You should change the two occurrences of TRILINEAR_SAMPLINGMODE in _createBlurPostProcess to BILINEAR_SAMPLINGMODE to fix this.

[Popov72]

First, a noob question, is the left or the right side the stored reference?

It is on the right.

I think I did, but both those tests looks like they are using SSAO, so some change would be expected. Where can I find the source of those tests so I can see how they are setup?

I didn't see there were other visualization tests failing, I thought you were speaking of the unit test!

You can find the PG ids corresponding to the visualization tests in packages/tools/tests/test/visualization/config.json.

When you look at this PG and hit the "3" key, we can see there's clearly a problem (comparison with existing SSAO2: https://playground.babylonjs.com/#N96NXC#119), the blur is not applied everywhere it should. I don't know if using gaussian instead of box explains everything(?)

Let's try to see what's going on before looking at the visualization test.

[fooware]

I also noticed that we are generating mipmaps for the SSAO effect and blur textures, which doesn't make sense! You should change the two occurrences of TRILINEAR_SAMPLINGMODE in _createBlurPostProcess to BILINEAR_SAMPLINGMODE to fix this.

How about the one in _randomTexture, should that stay?

[bjsplat]

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13621/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

[bjsplat]

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13621/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

[bjsplat]

Visualization tests for webgl2 have failed. If some tests failed because the snapshots do not match, the report can be found at

https://babylonsnapshots.z22.web.core.windows.net/refs/pull/13621/merge/testResults/webgl2/index.html

If tests were successful afterwards, this report might not be available anymore.

[Popov72]

How about the one in _randomTexture, should that stay?

You can also change it for this one, but this is not as much of a problem as the others, because no mipmap is created for this texture, so TRILINEAR will be treated as BILINEAR internally. But in the other cases, defining a TRILINEAR filter will force the system to generate mipmaps for the post-processes!

[Popov72]

I'm a bit uneasy with your latest changes, where you test 0.1 / 0.05 to make it look better in your example test, but it could be worse in other tests...

I think this value should be a parameter, that way people would be able to change it as they see fit.

Also, we probably need a parameter to enable the new code because I fear the changes could adversely impact people who are using the existing SSAO2 and may have tweaked the parameters for their use case.

[fooware]

I'm a bit uneasy with your latest changes, where you test 0.1 / 0.05 to make it look better in your example test, but it could be worse in other tests...

That is true. I tweaked it to look as crisp as possible in the different test cases I tried, without reducing the actual denoising in any of them. But still, those four tests is only a sample of every possible way.

The result is a bit less "crisp" edges since the sample rejection test was very strict before, that is what the tweak was about. The biggest upside is the reduction of the pretty glaring ghosting you get from the old box-filter. You can see it in the cat's mouth and nose for example, or the sharp edges in the terrain boxes which got duplicated with the previous filter.

The real fix would be to implement normal-based sample rejection as well, but given the hard requirement of "never slower" that wasn't really feasible here.

I think this value should be a parameter, that way people would be able to change it as they see fit.

Also, we probably need a parameter to enable the new code because I fear the changes could adversely impact people who are using the existing SSAO2 and may have tweaked the parameters for their use case.

I'm a bit unsure how to add that without getting even more settings on the SSAO2 postprocess. Because after you add it you can't really remove it again?

Ideally, I wanted to have some sort of enum-based setting to switch between different filter types, that way one could easily add more filters in the future in a safe way, even though I guess it would default to the old not-very-good filter.

@Popov72 , I would really appreciate your help to get a "denoising algorithm" (enum) setting into place in a way that follows best practices and is backward compatible.

The old "expensiveBlur" would map to "Bilateral Box" (true, default) and "Gaussian" (false) with such an enum.

[fooware]

Actually, I have another suggestion @Popov72 .

What if I back out all the potential breaking changes regarding the "expensiveBlur" and remove the new stuff, basically keeping this as a bug-fix / cleanup task to prepare for a larger rewrite later? Then I take the code changes I started with here and continue on them to create new filter(s), including any new options to tweak, and work on them until we are certain that they are well tested.

[Popov72]

What if I back out all the potential breaking changes regarding the "expensiveBlur" and remove the new stuff, basically keeping this as a bug-fix / cleanup task to prepare for a larger rewrite later?

I agree, this PR is becoming a bit heavy for something we plan to replace in the short/mid term.

Then I take the code changes I started with here and continue on them to create new filter(s), including any new options to tweak, and work on them until we are certain that they are well tested.

Actually, we planned to do as for SSR: write a new separate implementation and deprecate the old one. There is probably a newer SSAO algorithm we could use, and we won't bother to make the old and the new one work side by side in the same implementation. Having a separate implementation for the new algorithm is easier to manage.

[fooware]

All done @Popov72 , please take a new look when you have a chance.

Since you said you were going to start over with "SSAO3" and not reuse this class I went in this direction:

1. I added new settings to control the "sample size", "softness" and "tolerance" of the current "expensive blur" filter.
2. With the default values kept (16, 0.0, 0.0) the code that executes in the shader produces identical results and looks to be pixel-perfect with the shader code on master.
3. I did not fix any of the errors or quirks in the existing "expensive blur" filter implementation, but instead made very sure to keep them around as well as adding notes here and there.
4. I removed my optimized gaussian blur filters, and reverted back to the previous "blur13bilateral" filter when "expensiveBlur" is set to false, as you have on master.
5. I updated the documentation on "expensiveBlur" to match point 4 above.
6. I added documentation on all the new settings as well as exposing them in the Inspector.
7. I cleaned up the shader and pipeline code and I tried to make sure all your previous code review notes got implemented. Please take an extra look though, easy to miss one.

Tweaking the new settings works fairly well and can get an almost perfect cat for example. And at the same time, with the default settings there should be no changes in the output at all. Fingers crossed.

[fooware]

Looking at the code again this morning with fresh eyes, I noticed that there were several uniforms being declared and bound to the blur filter that has never been used in the blur shader, so I removed those. Along with one for the SSAO shader.

Also, how come _forceGeometryBuffer doesn't have a @serialize() like the rest of the similar types of properties?

[Popov72]

Great job!

[Popov72]

I agree with all your comments!

Regarding:

Also, how come _forceGeometryBuffer doesn't have an @serialize() like the rest of the properties of the same type?

Yes, please add serialization to this property, as well as to textureType. The Parse function will need to be updated to pass the forceGeometryBuffer and textureType values read from the serialization to the SSAO2 constructor.

Thanks!

[fooware]

Some code review fixes has been commited, but there is still some work left.

I got sidetracked trying to figure out why the SSAO2 algorithm performes so badly for samples <16, and think I figured it out. The fix is in PR #13652

[Popov72]

Yes, please add serialization to this property, as well as to textureType. The Parse function will need to be updated to pass the forceGeometryBuffer and textureType values read from the serialization to the SSAO2 constructor.

Regarding this one:

• you need to add @serialize() in front of private _forceGeometryBuffer: boolean = false;
• save the textureType parameter we pass to the constructor in a private variable and add @serialize():

@serialize()
private _textureType: number;
...
constructor(...) {
  ...
  this._textureType = textureType;
}

• Update the Parse function:

public static Parse(source: any, scene: Scene, rootUrl: string): SSAO2RenderingPipeline {
    return SerializationHelper.Parse(() => new SSAO2RenderingPipeline(source._name, scene, source._ratio, undefined, source._forceGeometryBuffer, source._textureType) , source, scene, rootUrl);
}

I think it should do it!

[fooware]

The serialization code is in now. I think that was the last known thing. =)

The declaration of _textureType caused an eslint warning. But looking at the eslint config it looks like underscored parameters should be exempt. I guess it is a local bug because it was really persistent:

[sebavan]

Great job on the PR !!!

@Popov72 feel free to merge when you see fit :-)

@fooware May I ask why using textureLod everywhere instead of change the sampling type of the input texture and ensuring we do not generate their mip levels ?

[fooware]

@fooware May I ask why using textureLod everywhere instead of change the sampling type of the input texture and ensuring we do not generate their mip levels ?

@sebavan Because Popov72 told me to. =)

You should use textureLod with a lod=0.0 instead of texture2D to avoid uniformity analysis problems in WebGPU. Same thing below for all texture reads.

It was my understanding that we did disabled the MIP-map generation as well by changing from TRILINEAR to BILINEAR in the PR.

[sebavan]

Makes much more sense completely missed this in the comment !!! thanks. All good to me.

[Popov72]

This is a great first contribution, grats!