[Fogbugz 1330769] Fixing bloom bicubic filtering for DRS #4370

kecho · 2021-04-28T18:34:11Z

Purpose of this PR

Bicubic filtering of bloom was using the wrong dimensions / coordinates for bloom. This PR applies the correct scaling to such parameters.
Fogbugz: https://fogbugz.unity3d.com/f/cases/1330769/
Repro steps in fogbugs

Tested, hardware drs, percentage of 10 to exaggerate artifact:

Before:

After:

Testing status

Tested on hardware DRS, vulkan and dx12 PC
Should be tested in consoles (one is enough, i.e. PS4)

…imensions / texel sizes

FrancescoC-unity · 2021-04-29T07:34:40Z

Did you try with SW dyn res too? I am a bit confused as HW dynamic res should have those constants you multiply with should be too far off 1 if a single view is there.

Changes look good I am just a tad confused on why the big difference, I guess we have another camera active? Or maybe depending on starting res the rounding is relevant as going down the mip chain?

FrancescoC-unity · 2021-04-29T07:38:40Z

...nity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.PostProcess.cs

@@ -3165,6 +3165,13 @@ void PrepareBloomData(RenderGraph renderGraph, in RenderGraphBuilder builder, Bl
            // the mip up 0 will be used by uber, so not allocated as transient.
            m_BloomBicubicParams = new Vector4(passData.bloomMipInfo[0].x, passData.bloomMipInfo[0].y, 1.0f / passData.bloomMipInfo[0].x, 1.0f / passData.bloomMipInfo[0].y);
            var mip0Scale = new Vector2(passData.bloomMipInfo[0].z, passData.bloomMipInfo[0].w);
+
+            //apply DRS on bloom bicubic params
+            m_BloomBicubicParams.x /= RTHandles.rtHandleProperties.rtHandleScale.x;


So these parameters should be baked in the actualWidth/actualHeight used here https://github.com/Unity-Technologies/Graphics/pull/4370/files#diff-617165acfec9a5b4c1b26809291f30938d7a152b8e9fec0e5ae2c661078201dbR3148 , by dividing we are undoing that and assuming full resolution source?

I might be misreading here as I didn't have my coffee yet, but worth double checking values :D

yes! we want exactly that, the actual full resolution of the texture. To understand why, you need to look at bicubic sampling. Thats were these params are used. Bicubic sampling function requires texel size to compute the offsets, but the texel size must be that of the physical texture (so the uv can be offset correctly).
If we use instead say texture.width here, then we have to write special code for hardware drs, because hardware drs hides the render - time resource size. So the easiest way is to just 'undo' the texture scale.

@FrancescoC-unity I have added a comment on this snippet to explain it

Yup it does make sense 👍

kecho · 2021-04-29T14:29:46Z

Did you try with SW dyn res too? I am a bit confused as HW dynamic res should have those constants you multiply with should be too far off 1 if a single view is there.

Changes look good I am just a tad confused on why the big difference, I guess we have another camera active? Or maybe depending on starting res the rounding is relevant as going down the mip chain?

Yes so the answer is that hardware res can also have a possible rtHandle scale that is not 1, due to for example planar reflections being present and sharing the same large core textures.
The problem is also aggravated by bicubic sampling, the offsets you see are actually for bicubic sampling, and the offsets are in iv space, so something like 1/hardware width. If that doesnt happen, you can get huge jumps and stair stepping.
I exagerated the problem in the screen shots, used a %10 scale to show the insanely bad offsets

FrancescoC-unity · 2021-04-29T14:32:13Z

Did you try with SW dyn res too? I am a bit confused as HW dynamic res should have those constants you multiply with should be too far off 1 if a single view is there.
Changes look good I am just a tad confused on why the big difference, I guess we have another camera active? Or maybe depending on starting res the rounding is relevant as going down the mip chain?

Yes so the answer is that hardware res can also have a possible rtHandle scale that is not 1, due to for example planar reflections being present and sharing the same large core textures.
The problem is also aggravated by bicubic sampling, the offsets you see are actually for bicubic sampling, and the offsets are in iv space, so something like 1/hardware width. If that doesnt happen, you can get huge jumps and stair stepping.
I exagerated the problem in the screen shots, used a %10 scale to show the insanely bad offsets

Yes I got that, but that implies that the problem is not there without extra cameras without this PR no? Does it go away?

The SW should show the issue even more for the same rationale (texture width differes from operating width a fair bit)

EDIT: Seeing the bicubic filter now, yeah the sw should do this even more likely as mismatch is likely wider

RemyUnity

Tested and fixe confirmed on PC.
@TomasKiniulis will check on console for final approval.

TomasKiniulis

Testing repro project on Xbox looks all good as well

* Fix null exception on Raytracing SSS volume component (#4322) * Add null check * changelog * Fix issue with Eye shader and area light (#4310) * Fix Ray Tracing Light Cluster debug mode (#4327) * Remove unneeded position from input layout of light cluster * Changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Skip adding the LOD's renderer to the checked renderer if it's null (#4313) * Skip adding the LOD's renderer to the checked renderer if it's null * changelog * Add LODgroup missing renderer to "check scene content for ray tracing" * formatting test * formatting Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Update doc of transparent motion vectors to specify TAA of object behind will be impacted (#4325) * Update doc * Fix doc * Fixed Look Dev breaking when docked and rendered at zero size. (#4331) * Fixed an issue where sometime a docked lookdev could be rendered at zero size and break. * Update changelog # Conflicts: # com.unity.render-pipelines.high-definition/CHANGELOG.md Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * [HDRP] Added a bit about scaling issues when dealing with history buffers (#4333) * Updated troubleshooting custom pass doc * Update Custom-Pass-Troubleshooting.md * Fixed an issue where runtime debug window UI would leak game objects. (#4337) * Fixed an issue where runtime debug window UI would leak game objects. * Update changelog * Updated documentation to support #4313 (#4336) Co-authored-by: Sebastien Lagarde <sebastien@unity3d.com> * Fixed NaNs when denoising pixels where the dot product between normal and view direction is near zero (#4342) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * updated tooltip (#4334) * Doc for custom matrix for fog (#4352) * [Fogbugz 1324000] Fixing unlit emissive reflections (#4340) * Fix ray tracing unlit emissive exposure. Changelog * Automated tests coverage * update scenes Co-authored-by: Remi Chapelain <remi.chapelain@unity3d.com> Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * [Fogbugz 1329173] Fixing wrong pyramid color dimensions when hardware DRS is on. (#4346) * Fixing bad pyramid color source texture size. DRS hanlder must be queried since the mip levels depend on the source hardware resolution * Changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix RTHandle scale bias [1329977] (#4320) * Fix RTHandle scale bias * Add changelog * Fix history sacle * Fix Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fixed emission material properties not being animable (#4366) * Fixed emission material properties not being animable * Updated changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix fog precision (#4364) Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix erroneous tile coordinate calculations in contact shadows (#4335) * Fix wrong tile coord calculations * Changelog * Update test scene for contact shadow to cover data * update references screenshots Co-authored-by: Sebastien Lagarde <sebastien@unity3d.com> * [HDRP]Fix history buffer allocation for AOVs when the request does not come in first frame (#4361) * Fix history buffer allocation for AOVs when the request does not come in first frame * Add a break in the for loop Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Fix for volumetric clouds performing Read and Write on same texture [Unsupported on some platforms] (#4356) * Fix for usage of camera color RW when not supported. * Re-enable on metal * changelog Co-authored-by: sebastienlagarde <sebastien@unity3d.com> * Update TestCaseFilters.asset * update metal screenshots * missing meta * [Fogbugz 1330769] Fixing bloom bicubic filtering for DRS (#4370) * Fixing bloom bicubic filtering for DRS, bicubic was using the wrong dimensions / texel sizes * Adding better comment to explain offseting of bicubic texture offset fix * update reference screenshots Co-authored-by: sebastienlagarde <sebastien@unity3d.com> Co-authored-by: FrancescoC-unity <43168857+FrancescoC-unity@users.noreply.github.com> Co-authored-by: Rémi Chapelain <57442369+remi-chapelain@users.noreply.github.com> Co-authored-by: JulienIgnace-Unity <julien@unity3d.com> Co-authored-by: Antoine Lelievre <antoinel@unity3d.com> Co-authored-by: Lewis Jordan <lewis.jordan@hotmail.co.uk> Co-authored-by: Pavlos Mavridis <pavlos.mavridis@unity3d.com> Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com> Co-authored-by: Kleber Garcia <kleber.garcia@unity3d.com> Co-authored-by: Remi Chapelain <remi.chapelain@unity3d.com> Co-authored-by: skhiat <55133890+skhiat@users.noreply.github.com>

kecho added needs-backport-10.x needs-backport-2021.1 labels Apr 28, 2021

kecho requested review from sebastienlagarde, FrancescoC-unity and a team April 28, 2021 18:34

Fixing bloom bicubic filtering for DRS, bicubic was using the wrong d…

35cb082

…imensions / texel sizes

kecho force-pushed the HDRP/FixDRSBloom branch from 48e6d50 to 35cb082 Compare April 28, 2021 18:36

sebastienlagarde approved these changes Apr 28, 2021

View reviewed changes

sebastienlagarde added the HDRP label Apr 29, 2021

FrancescoC-unity reviewed Apr 29, 2021

View reviewed changes

Adding better comment to explain offseting of bicubic texture offset fix

afd7e20

FrancescoC-unity approved these changes Apr 29, 2021

View reviewed changes

sebastienlagarde marked this pull request as ready for review April 30, 2021 07:59

Merge branch 'hd/bugfix' into HDRP/FixDRSBloom

aeea9e7

RemyUnity requested a review from TomasKiniulis April 30, 2021 14:37

RemyUnity approved these changes Apr 30, 2021

View reviewed changes

TomasKiniulis approved these changes Apr 30, 2021

View reviewed changes

update reference screenshots

2b7356c

sebastienlagarde merged commit d2c8c43 into hd/bugfix Apr 30, 2021

sebastienlagarde deleted the HDRP/FixDRSBloom branch April 30, 2021 16:55

sebastienlagarde mentioned this pull request Apr 30, 2021

Merge Hd/bugfix #4388

Merged

sebastienlagarde added backported-10.x backported-2021.1 and removed needs-backport-10.x needs-backport-2021.1 labels May 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Fogbugz 1330769] Fixing bloom bicubic filtering for DRS #4370

[Fogbugz 1330769] Fixing bloom bicubic filtering for DRS #4370

Uh oh!

kecho commented Apr 28, 2021

Uh oh!

FrancescoC-unity commented Apr 29, 2021 •

edited

Loading

Uh oh!

FrancescoC-unity Apr 29, 2021 •

edited

Loading

Uh oh!

kecho Apr 29, 2021

Uh oh!

kecho Apr 29, 2021

Uh oh!

FrancescoC-unity Apr 29, 2021

Uh oh!

kecho commented Apr 29, 2021

Uh oh!

FrancescoC-unity commented Apr 29, 2021 •

edited

Loading

Uh oh!

RemyUnity left a comment

Uh oh!

TomasKiniulis left a comment

Uh oh!

Uh oh!

[Fogbugz 1330769] Fixing bloom bicubic filtering for DRS #4370

[Fogbugz 1330769] Fixing bloom bicubic filtering for DRS #4370

Uh oh!

Conversation

kecho commented Apr 28, 2021

Purpose of this PR

Testing status

Uh oh!

FrancescoC-unity commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FrancescoC-unity Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kecho Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

kecho Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

FrancescoC-unity Apr 29, 2021

Choose a reason for hiding this comment

Uh oh!

kecho commented Apr 29, 2021

Uh oh!

FrancescoC-unity commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RemyUnity left a comment

Choose a reason for hiding this comment

Uh oh!

TomasKiniulis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FrancescoC-unity commented Apr 29, 2021 •

edited

Loading

FrancescoC-unity Apr 29, 2021 •

edited

Loading

FrancescoC-unity commented Apr 29, 2021 •

edited

Loading