APV touchup volumes #7017
APV touchup volumes #7017
Conversation
# Conflicts: # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.Debug.cs
FrancescoC-unity
requested review from
sebastienlagarde,
JulienIgnace-Unity and
adrien-de-tocqueville
|
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 SRP Core 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. |
|
It appears that you made a non-draft PR! |
| var touchupBound = touchup.Item1; | ||
| var touchupVolume = touchup.Item2; | ||
|
|
||
| if (touchupBound.Contains(cell.probePositions[i])) |
There was a problem hiding this comment.
Do we expect a lot of touchup volumes? If so, this could get slow I suppose (a bit like the deduplication that takes forever).
Shouldn't we do this outside of the loop and first do a pass to do TouchUpVolume/Cell collision then do the proper TouchUp/Probe tests?
There was a problem hiding this comment.
I don't expect a large amount of them, but yeah we could at the very prune the list before at cell granularity.
The deduplication is operating on a few order of magnitude difference I- the loop over position is there anyhow, here we just check against a few bounds.
com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.cs
Show resolved
Hide resolved
com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.cs
Show resolved
Hide resolved
com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeBrickPool.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Test status: In progress
What's tested:
This was only a quick test as the feature is still in development and needs to land before migration. Some issues/feature requests were noted below but they are not major and can follow in future PRS.
While testing I used 2 projects, HDRP template - only rebaked to check if any unwanted changes occur and ClassRoom where I actually replaced blocker cubes with touchup volumes and tested the feature. In general, I didn't find any regressions and the results of the new touch-up volumes were pretty good (adding showcase below). Further testing will be required when the quality of life features land in a separate pr, also on mac and more complex scenes.
Issues:
- [FIXED] There is a Size parameter and Scale is not used. So you can not input numerical values instead of using gizmos
EvaI8K0n6e.mp4
- With the default transparency, it is very hard to see the extent of the volumes. No way to adjust it. Can be addressed in a separate PR, but logging to not forget
qB5Ukc1xwV.mp4
- Would be good to have a debug mode where you could see what probes are affected by touchup volumes. Same as above can be separate PR
Demo
The touch-up volumes are placed and they take effect when the Probe Volume override is enabled. The scene is lit using APV volumes.
pdpju2SOfV.mp4
# Conflicts: # com.unity.render-pipelines.core/Editor/Lighting/ProbeVolume/ProbeGIBaking.cs # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeReferenceVolume.cs # com.unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeVolumeAsset.cs
An addition for the validity-based occlusion system. This will allow to:
Also moves all the computations for the occlusion weight mask at bake time vs runtime.
These can essentially be thought as a way to artificially inflate the size of thin walls.
It restore validity when invalidation via touchup volume was not necessary (i.e. no occluders in the interpolation grid).
Not entirely finished feature, but want to open a PR before SRP2Core.
Before: Notice the leak where the ceiling is super thin and therefore probes are not marked up as invalid and occlusion weigh
ting is not possible
After: A touchup volume is placed and the issue goes away as we force the probes to be invalid even if not inside actual geoemetry.
