-
Notifications
You must be signed in to change notification settings - Fork 855
Change the way we handle emissive with RTGI/SSGI/Mixed and APV + Remove ForceForwardEmissive code #5577
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.
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
Conversation
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. HDRP 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. |
com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/LightLoop.hlsl
Show resolved
Hide resolved
...s.high-definition/Runtime/Lighting/ScreenSpaceLighting/ScreenSpaceGlobalIllumination.compute
Show resolved
Hide resolved
com.unity.render-pipelines.high-definition/Runtime/Material/Builtin/BuiltinData.cs
Show resolved
Hide resolved
com.unity.render-pipelines.high-definition/Runtime/Material/BuiltinGIUtilities.hlsl
Show resolved
Hide resolved
com.unity.render-pipelines.high-definition/Runtime/Material/BuiltinGIUtilities.hlsl
Show resolved
Hide resolved
com.unity.render-pipelines.high-definition/Runtime/Material/BuiltinUtilities.hlsl
Show resolved
Hide resolved
com.unity.render-pipelines.high-definition/Runtime/Material/Lit/Lit.hlsl
Show resolved
Hide resolved
outGBuffer3 = float4(builtinData.bakeDiffuseLighting * surfaceData.ambientOcclusion + builtinData.emissiveColor, 0.0); | ||
// Pre-expose lighting buffer | ||
outGBuffer3.rgb *= GetCurrentExposureMultiplier(); | ||
outGBuffer3.xz = AO_IN_GBUFFER3_TAG.xz; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FrancescoC-unity . I finally decided to do some kind of hack to store AO. See the comment, but short: if we don't have emissive, then we store AO. Should solve 90% of our use case and don't require extra buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sounds like a valid solution indeed
com.unity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.LightLoop.cs
Show resolved
Hide resolved
...h-definition/Runtime/RenderPipeline/Raytracing/HDRenderPipeline.RaytracingIndirectDiffuse.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved after changes
com.unity.render-pipelines.high-definition/Runtime/Lighting/LightLoop/LightLoop.hlsl
Outdated
Show resolved
Hide resolved
com.unity.render-pipelines.high-definition/Runtime/Material/Lit/Lit.hlsl
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR simplifies greatly the setup and need for comprehension when having a combinaison of real-time/baked global illumination technique with emissive materials.
Before, in deferred, users needed to disable receive SSGI flag or enable Force Forward Emissive per material which created much confusion.
Now, everything is supposed to work properly out of the box without having to enable anything new.
PR have been tested thoroughly to ensure no unexpected regressions.
What's tested:
- Upgrade a project with SSGI and Force Forward Emissive ✔️
- SSGI / RTGI / Mixed still replaces lightmaps ✔
- Check transparent emissive still not contributing except when in pre-refraction for SSGI ✔️
- Decal Emissive still not contributing in RTGI or Mixed mode ✔️
- Building APV and relying on probe data to fill SSGI artefacts with emissive objects in the scenes ✔️
- Material with Diffuse, AO, Emissive with SSGI and/or APV ✔️
(in Deferred, with SSGI or APV, we lose material AO contribution, but not with Lightmaps)
(in Forward, everything works in all cases) - SSAO/RTAO still contributes as expected ✔️
- Can still multiply indirect lighting contribution for Lightmaps / APV / SSGI / RTGI / Mixed ✔️
- Emissive still works with SSS ✔️
- Emissive still works with RT SSS 🔴 (not related to this PR > reported here)
- DXR Tests are all green locally ✔️
- Performance (roughly same frame time in 4K in a scene with SSGI with lots of emissive / ao) ✔️
…ove ForceForwardEmissive code #5577
@Vic-Cooper : Sorry I realize I forget to add you for doc checking on this PR. Could you add it in your backlog? Thanks a lot |
Purpose of this PR
This PR is a big cleanup PR and also change the way we handle emissive to works correctly with RTGI/SSGI/Mixed and APV.
This PR:
Explanation about how APV and SSGI/RTGI/Mixed effects will handle emissve with this PR and the steps.
All effects will output only Emissive inside the lighting buffer (gbuffer3) in case of deferred (For APV this is done only if we are not a lightmap). The Lightmaps/Lightprobes contribution is 0 for those cases. Code enforce it in SampleBakedGI(). The remaining code of Material pass (and the debug code) is exactly the same with or without effects on, including the EncodeToGbuffer. builtinData.isLightmap is used by APV to know if we have lightmap or not and is harcoded based on preprocessor in InitBuiltinData()
AO is also store with a hack in Gbuffer3 if possible. Otherwise it is set to 1.
In case of regular deferred path (when effects aren't enable) AO is already apply on lightmap and emissive is added on to of it. All is inside bakeDiffuseLighting and emissiveColor is 0. AO must be 1
When effects are enabled and for APV we don't have lightmaps, bakeDiffuseLighting is 0 and emissiveColor contain emissive (Either in deferred or forward) and AO should be the real AO value.
Then in the lightloop in below code we will evalaute APV or read the indirectDiffuseTexture to fill bakeDiffuseLighting.
We will then just do all the regular step we do with bakeDiffuseLighting in PostInitBuiltinData()
No code change is required to handle AO, it is the same for all path.
Note: With current approach, Decals Emissive aren't taken into account by RTGI and by the Mixed method.
Note2: With current approach, Transparent Emissive works with SSGI and RTGI but can cause artifact with Mixed (RayMarch part could get a hit and thus avoid Raytrace path)
Note3: Forward opaque emissive work in all cases. The current code flow with Emissive store in GBuffer3 is only to manage the case of Opaque Lit Material with Emissive in case of deferred
Note4: Only APV can handle backFace lighting, all other effects are front face only.
TODO: complete documentation
Testing status
TODO (no test done yet) Test Plan:
Scenario are: SSGI, RTGI, Mixed, APV, SSGI with APV with forward and deferred case
With scene that have emissive decal, emissive transparent unlit, emissive opaque unlit lit opaque emissive.
Test all debug lighting mode: lighting only, emissive only, override emissive for all scenario
Test with probe volume on/off when APV is enable
Test indirect diffuse multiplier for all scenario
Test that AO behave correctly in all scenario
Comments to reviewers
Notes for the reviewers you have assigned.