Added support for reflection probes as a fallback for ray traced reflections (case 1338644) #4809
Conversation
There was a problem hiding this comment.
Thanks for adding the tests. ✊
I updated the "both" and the "reflection" because they were using the same diffusion profile so "both" was not tested in it. I modified the probe box size for the test "both" to be able to see the probe contribution and the sky on the same capture.
I also renamed the volumes to avoid confusion.
What I tested :
- Perf Mode / Quality Mode
- Checked if Layer masks were still functioning properly
- Mixed mode
- Denoiser
Tests are still green locally, also launched yamato.
One last thing before approving, even though it seems logical what do you think of changing "Reflection probe and Sky" to "Reflection Probe then Sky" to emphasize the fact than there is an order ?
| @@ -176,7 +177,7 @@ static void RayMarchGBuffer(CommandBuffer cmd, in DeferredLightingRTRPassData da | |||
| cmd.SetComputeFloatParam(data.parameters.rayMarchingCS, HDShaderIDs._RayMarchingThicknessScale, thicknessScale); | |||
| cmd.SetComputeFloatParam(data.parameters.rayMarchingCS, HDShaderIDs._RayMarchingThicknessBias, thicknessBias); | |||
| cmd.SetComputeIntParam(data.parameters.rayMarchingCS, HDShaderIDs._RayMarchingSteps, data.parameters.raySteps); | |||
| cmd.SetComputeIntParam(data.parameters.rayMarchingCS, HDShaderIDs._RayMarchingReflectSky, data.parameters.raytracingCB._RaytracingIncludeSky); | |||
| cmd.SetComputeIntParam(data.parameters.rayMarchingCS, HDShaderIDs._RayMarchingReflectSky, 0); | |||
There was a problem hiding this comment.
I am not convince about the enum. Why to propose all the fallback mode?
If I understand correctly we just want to avoid cost of reflection fallback if we don't want it. But I think it introduce way to many settings, we should keep it simple.
I suggest just to be able to enable the fallback or reflection probe or not if performance is a concern but we always have the sky as a fallback. Thought? (And if adding the code of reflection probe don't imply cost (due to the dynamic branching and maybe because it doesn't add extra vgpr pressure), let's have it all the time.
| @@ -229,6 +230,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. | |||
| - Fixed support of Distortion with MSAA | |||
| - Fixed contact shadow debug views not displaying correctly upon resizing of view. | |||
| - Fixed an error when deleting the 3D Texture mask of a local volumetric fog volume (case 1339330). | |||
| - Fixed an issue where disbaled reflection probes were still sent into the the ray tracing light cluster. | |||
There was a problem hiding this comment.
disbaled -> disabled
|
Found a bug in the wieght mixing code of the probes, need to fix it. |
| @@ -141,6 +141,11 @@ public int rayMaxIterations | |||
| #endregion | |||
|
|
|||
| #region Ray Tracing | |||
| /// <summary> | |||
| /// When enabled, SSR handles sky reflection. | |||
There was a problem hiding this comment.
Out of date comment
…ections (case 1338644) Fixed an issue where disbaled reflection probes were still sent into the the ray tracing light cluster. When a ray hits the sky in the ray marching part of mixed ray tracing, it is considered a miss.
anisunity
force-pushed
the
HDRP/fix-1338644
branch
from
daca62c
to
3c18842
Compare
|
review fixes done |
Added support for reflection probes as a fallback for ray traced reflections (case 1338644)
Fixed an issue where disbaled reflection probes were still sent into the the ray tracing light cluster.
When a ray hits the sky in the ray marching part of mixed ray tracing, it is considered a miss.
https://fogbugz.unity3d.com/f/cases/1338644/
Testing status
Added new tests to cover this new feature.
The previous tests are still green.