Skip to content

Fix local fog volumes z axis calculation #6068

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Nov 4, 2021
Merged

Fix local fog volumes z axis calculation #6068

merged 12 commits into from
Nov 4, 2021

Conversation

RemyUnity
Copy link
Contributor

@RemyUnity RemyUnity commented Oct 19, 2021 •
edited
Loading

Purpose of this PR

The local volumetric fog was sampling 3D texture with an inverted Z axis. This PR fixes it.

Before:
Before

After:
After

This change needed to compensate also the scrolling direction in Z.
For a scrolling speed of (0.5, 0.5, 0.5):
Before the PR:
af9538053f160788a72281f1f6854a54

After Z axis inversion:
4054395a48304c7c0a0947a151ec473c

After Z axis inversion and scroll direction correction:
e0e98c8fb881f06966437044fdf3e95e

Testing status

Tested manually with this project, resulting to the screen captures above.

Images and test scene has been updated, but automation is still falling for reasons unrelated to my changes :/ : https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/hd%252Ffix%252Flocal-volumetric-fog-z-axis/.yamato%252Fall-hdrp.yml%2523Nightly_HDRP_trunk/9533137/job

Consoles PR: https://github.cds.internal.unity3d.com/unity/ScriptableRenderPipelinePrivate/pull/300

Comments to reviewers

Pretty small change, but will alter the rendering of existing projects if they use the feature. Might be totally invisible in the case of noise 3D textures, but more problematic with specifically created textures.
Informations will be added in upgrade guides.

@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/jobDefinition/.yamato%2Fall-hdrp.yml%23HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%252Fall-lightmapper.yml%2523PR_LightMapper_trunk

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

sebastienlagarde
sebastienlagarde reviewed Oct 19, 2021
View reviewed changes
Copy link
Contributor

@sebastienlagarde sebastienlagarde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add comment in upgrade guide of 21.2

JMargevics
JMargevics approved these changes Oct 20, 2021
View reviewed changes
Copy link
Contributor

@JMargevics JMargevics left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing covered looks sufficient. Approving as is 👍✔

@sebastienlagarde sebastienlagarde marked this pull request as ready for review October 21, 2021 15:58
@kecho kecho self-requested a review October 22, 2021 19:08
@kecho
Copy link
Contributor

kecho commented Oct 22, 2021

Dx12 fog d3d12 tests are failing here:

d3d12: failed to create a buffer of size 384 d3d12 : CreateCommittedResource 'BufferD3D12::CreateBufferResource() Buffer' (48 x 1) format 0 failed (887a0005). Device Remove Reason (HRESULT): 0x887a0005 Device Memory Stats: Local AvailableForReservation: 4972 MB, Budget: 9391 MB, CurrentUsage: 2675 MB, CurrentReservation: 64 MB Non-local AvailableForReservation: 3890 MB, Budget: 7372 MB, CurrentUsage: 642 MB, CurrentReservation: 0 MB

I recommend running them locally on dx12, and then verifying the results again. There might be an issue at device creation, such as a negative offset being passed in the resource description or something.

I also recommend merging with latest, tests are green now.

@RemyUnity
Merge branch 'master' into hd/fix/local-volumetric-fog-z-axis
3f0de09 
# Conflicts:
#	com.unity.render-pipelines.high-definition/CHANGELOG.md
@kecho
Copy link
Contributor

kecho commented Nov 1, 2021

I still some dx12 errors, did you manage to figure out if they are related to the PR at all?

@sebastienlagarde
Copy link
Contributor

crash isn't related to the PR (the DX12 one), DX12 was green on a previous run: https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/hd%252Ffix%252Flocal-volumetric-fog-z-axis/.yamato%252Fhdrp-win-dx12.yml%2523HDRP_Win_DX12_playmode_XR_mono_Linear_trunk/9525830/job . Merging

@sebastienlagarde sebastienlagarde merged commit 85fe5bd into master Nov 4, 2021
@sebastienlagarde sebastienlagarde deleted the hd/fix/local-volumetric-fog-z-axis branch November 4, 2021 19:55
sebastienlagarde pushed a commit that referenced this pull request Nov 7, 2021
@RemyUnity @sebastienlagarde
Fix local fog volumes z axis calculation #6068
63c8e52
@sebastienlagarde sebastienlagarde mentioned this pull request Nov 7, 2021
Merged
sebastienlagarde added a commit that referenced this pull request Nov 7, 2021
@sebastienlagarde @kecho
@gmitrano-unity @anisunity @johnpars @pmavridis @SeanPuller @adrien-de-tocqueville @eturquin @alelievr @emilybrown1 @FrancescoC-unity @RemyUnity @slunity @James-Burr-Unity @tlaedre
[HDRP][Backport 21.2] Backport various PRs (#6231)
a9d7146 
* Increasing HDRP fined pruned light tile count to 63. #5771

* Fix Upscaled Film Grain & Dither in HDRP (#6111)

When using particular types of upscaling, (CAS, FSR, Catmull) the
film grain quality produced by HDRP was lower than expected.

This was caused by errors in the UV scale calculations for the grain
texture.

This change fixes the issue by modifying the scale calculations to
use the final viewport size rather than the size of the current
scaled/unscaled viewport. This works because film grain is always
handled by "FinalPass" which always renders at final viewport size
anyways.

This change also fixes a few cases where the current resolution group
would have been incorrect when CAS or FSR were being used as the
upscaling method.

* Update CHANGELOG.md

* Fix regression that was introduced in a previous PR on the diffuse denoiser #6119

* Fix Light Loop Variant Warnings (1372256) #6135

* Bump the HDRP Template IET Framework Package Version #6134

* Update ray tracing and path tracing docs regarding custom interpolator limitations (#6126)

* [HDRP] Disable DoF for orthographic cameras #6124

* update occlusion radius for directional light sample (#6154)

Co-authored-by: Sean Puller <sean.puller@unity3d.com>

* Clean up Public API Documentation #6151

* Fix Sky override not taken into account #6127

* [Fogbugz # 1372245] Hdrp/drs fixing pyramid blur #6136

* Improved Area Light Support for Hair (#6157)

* Initial commit MRP implementation

* Barn door application

* Add dominant marschner lobe direction skeleton

* Finalize rect light MRP for marschner, handle forward + backward hemisphere scattering.

* Force pathtracer to far-field in case users use geometry with bad normals.

* com.unity.render-pipelines.high-definition/Runtime/Material/Hair/MultipleScattering/HairMultipleScattering.hlsl

* Fix SH ringing for multiple scattering

* Add trivial support for cookies

* Add Kajiya support for area light approximation (non-LTC)

* remove Line area light MRP for now

* Remove area rect reference

* Update area light hair test scene and reference images

* Select cookie mip based on solid angle, rename some functions, temporarily add back rect ref

* Remove the useless div-by-1 and add a comment to explain the removal of the divide by PI

* Add a comment to further explain the normalization we do on the longitudinal distribution

* Remove the ref once again

* Update test images for metal and vulkan

* Add a zero-div guard to silence compiler warning

* Update CHANGELOG.md

* [HDRP][Path Tracing] Exposed 3 methods related to path tracing and accumulation. #6197

* [HDRP] Fix infitnite material import loop materials #6185

* fixed typo: users -> uses (#6162)

* Don't blur if nothing to blur (saves memory due to RG) (#6183)

* Fix local fog volumes z axis calculation #6068

* fixed passive voice (#6092)

* Fixed grammar errors

* fixed passive voice in decal doc

* Update Decal.md

* Update Decal.md

* Update Decal.md

* AxF Raytracing: fix performance mode distillation that was broken and removed in #9d6db83478c213bc066e5f728d94b7faa6662543 (#6104)

* Add scale parameter in eye shader #6132

* updated Override Fog page (#6200)

* updated fog override screenshot

* changed Depth Extent to Volumetric Fog Distance

* Fix missing pragma (#6204)

* [HDRP] Fix custom pass motion vector texture access #6205

* Fix typo in MipBias tooltip in HDRenderPipelineUI.Skin (#6210)

* Replace gizmos FindObjectsOfType with list registrations to avoid constant slowdown in large scenes. Merged editor guards blocks. (#6168)

* [HDRP] Fix APV tooltip for Geometry Distance Offset (#6150)

* Update tooltip

* Update ProbeVolumeUI.Skin.cs

* Fixed the point distribution for the diffuse denoiser sometimes not being properly intialized. #6194

* Fixed the bad blending between the sun and the clouds (case 1373282). #6193

* Fix shadow mask fade and optimize it at same time (#6083)

* [HDRP][Path Tracing] Minor fix for SSS when combined with fog (#6215)

* Minor fix for SSS + fog.

* Comments.

* Update 1401_HairGraph_Area_Light.png

* update screenshots

Co-authored-by: Kleber Garcia <kleber.garcia@unity3d.com>
Co-authored-by: gmitrano-unity <89797527+gmitrano-unity@users.noreply.github.com>
Co-authored-by: anisunity <42026998+anisunity@users.noreply.github.com>
Co-authored-by: John Parsaie <johnpa@unity3d.com>
Co-authored-by: Pavlos Mavridis <pavlos.mavridis@unity3d.com>
Co-authored-by: Sean Puller <43151199+SeanPuller@users.noreply.github.com>
Co-authored-by: Sean Puller <sean.puller@unity3d.com>
Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com>
Co-authored-by: Emmanuel Turquin <emmanuel@turquin.org>
Co-authored-by: Antoine Lelievre <antoinel@unity3d.com>
Co-authored-by: emilybrown1 <88374601+emilybrown1@users.noreply.github.com>
Co-authored-by: FrancescoC-unity <43168857+FrancescoC-unity@users.noreply.github.com>
Co-authored-by: RemyUnity <32760367+RemyUnity@users.noreply.github.com>
Co-authored-by: slunity <37302815+slunity@users.noreply.github.com>
Co-authored-by: James-Burr-Unity <61244495+James-Burr-Unity@users.noreply.github.com>
Co-authored-by: Torbjorn Laedre <271210+tlaedre@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebastienlagarde sebastienlagarde sebastienlagarde approved these changes

@JulienIgnace-Unity JulienIgnace-Unity JulienIgnace-Unity approved these changes

@JMargevics JMargevics JMargevics approved these changes

@kecho kecho Awaiting requested review from kecho

Assignees
No one assigned
Labels
backported-2021.2 HDRP testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RemyUnity @kecho @sebastienlagarde @JulienIgnace-Unity @JMargevics