Improvements to APV Virtual Offset #6174
Conversation
…s some negative offset bugs, improves chosen push direction from multiple same-length ray intersections.
|
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. 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. |
| if (voSettings.useMultiThreadedOffset) | ||
| ApplyVirtualOffsetsMultiThreaded(positions, out offsets, voSettings); | ||
| else | ||
| ApplyVirtualOffsetsSingleThreaded(positions, out offsets, voSettings); |
There was a problem hiding this comment.
Temporarily keep both options until after initial reviews.
| "references": [ | ||
| "GUID:df380645f10b7bc4b97d4f5eb6303d95" | ||
| "GUID:df380645f10b7bc4b97d4f5eb6303d95", | ||
| "GUID:2665a8d13d1b3f18800f46e256720795" |
There was a problem hiding this comment.
Optional Burst dependency
| const int kMinCommandsPerJob = 512; | ||
| const int kRayDirectionsPerPosition = 3 * 3 * 3 - 1; | ||
|
|
||
| static void ApplyVirtualOffsetsMultiThreaded(Vector3[] probePositions, out Vector3[] probeOffsets, VirtualOffsetSettings voSettings) |
There was a problem hiding this comment.
The core idea of all this code (and in fact most of the code itself) is still the same as ApplyVirtualOffsetsSingleThreaded
...unity.render-pipelines.core/Runtime/Lighting/ProbeVolume/ProbeVolumeBakingProcessSettings.cs
Outdated
Show resolved
Hide resolved
tlaedre
force-pushed
the
crp/apv-vo-jobs
branch
from
698d5bf to
df162bd
Compare
|
Im getting the same issue as mentioned here, preventing me from seeing the APV probes in debug. Making it hard to test |
There was a problem hiding this comment.
Review status: Done
The PR brings improvements to Virtual Offset, feature currently can be turned off(checkbox Use Multi-Threaded). While testing I did not find any regressions.
What's tested:
- Compared results of APV when feature is enabled vs disable
- Rebaked lighting of Classrom scene
FIXED 1) Errors. So far whenever I enable multi-threaded checkbox I get errors and doesn't bake. Not sure if im missing something or there is an issue.
Feature showcase
The virtual offset gained a visible improvement im my scene.
Feature turned off
Feature turned on
|
Yamato fails are all known now, merging |
4 participants
Purpose of this PR
The primary purpose of this PR is to improve APV probe position Virtual Offset performance. The current implementation runs entirely on the main thread and doesn't scale well to complex scenes, with the VO pass routinely taking longer to complete than actual (GPU) lightmapper raycasting. This PR implements a bursted, concurrent alternative, while also tweaking the heuristics slightly to handle certain cases better (in the scenes tested by the author).
Currently both the old and the new code can be ran in parallel (toggled by the Use Multi-Threaded checkbox). This makes it easy to compare them for validation and tweaks. The idea for a final PR if approved would be to remove the old implementation and put the MT one into its place.
So, performance first: in our production project the new code is ~200x faster than the old (on my local machine), reducing time spent from ~40s to ~200ms. In simpler test scenes the savings are a little more modest, usually coming in around ~75x faster for me.
The old implementation has a couple of cases where it doesn't yield optimal results. One of these is pushing probes that are already out in the open closer to, or in some cases even inside, geometry - the latter in particular reduces lighting quality since those end up being invalid probes that then have to be filled in by dilation.
Various incorrect push results:
Another case is that since there's no virtual offsetting of the origin, and because it purely chooses the best direction based on distance, it picks the first as opposed to the best direction when there's multiple rays with the same distance.
Lastly, because there's no way to get multiple ray-hits per collider, there's no reliable way to understand what a hit of an incoming ray actually represents, and that easily leads to false positives (think small objects or complex, concave shapes).
The primary change made to avoid many of these cases is removing the use of inbound rays. The other is a tweak of the hit surface and direction selection heuristics where surfaces with colinear normals are preferred for ray hits with distances within a very small threshold value.
Testing status
Tested in our current production for more than a month.
Tested in various artificial scenes.
Comments to reviewers
PR is currently not against master as it builds on a a different PR. This would be changed as the other PR lands.
This PR does not add a package dependency on Burst, it just uses it if available. (Which looks to be the case in HDRP/URP now)