Fix APV error on first time it is enabled and improve debug probe perf by 2x #5423
Conversation
FrancescoC-unity
requested review from
JulienIgnace-Unity and
sebastienlagarde
|
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! |
| - Fixed lens flare not rendering correctly with TAAU or DLSS | ||
| - Fixed lens flare not rendering correctly with TAAU or DLSS. | ||
| - Fixed assert failure when enabling the probe volume system for the first time. | ||
| - Significantly improved performance of APV probe debug. |
There was a problem hiding this comment.
note for later: APV isn't an official feature (yet) so we don't need to put changelog.
| } | ||
|
|
||
| if (IsAPVEnabled()) | ||
| if (IsAPVEnabled() && ProbeReferenceVolume.instance.isInitialized) |
There was a problem hiding this comment.
Shouldn't we be able to test isInitialized inside the cleanup function and avoid having this public?
If we do it this way it means that any SRP needs to replicate this not too obvious pattern
There was a problem hiding this comment.
I did think about that, but the problem is that if cleaning up happens anywhere else but this specific use case, then if the system is not initialized something is probably wrong.
Now, one could argue in that case we could just not cleanup, but I wondered if that "hides" potential issues.
sebastienlagarde
deleted the
HDRP/fix-error-message-and-smaller-sphere
branch
| @@ -1,17 +1,11 @@ | |||
| fileFormatVersion: 2 | |||
| guid: 9e0af751bc36ea146940ba245193e28c | |||
| guid: f7f013a81640a284ea38df61c509ff9a | |||
There was a problem hiding this comment.
This change of guid will lead to some lost references: https://github.com/Unity-Technologies/Graphics/search?q=9e0af751bc36ea146940ba245193e28c
See this conversation.
There was a problem hiding this comment.
Yup, reverting it as we speak (cannot open the conversation tho :) )
…f by 2x (#5423) * Cleanup only when initialized. * changelog * More decimated sphere * Changelog * Move init check inside Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
4 participants
This PR has two things in it:
Fix an error that was kicking when APV is enabled for the first time. The reason is that the cleanup seems to happen with new settings instead of old, so we try to cleanup when not init. Fix is simple enough to just check if enabled.
Significantly decimated the Sphere fbx (though still to a decent level). It was way too detailed to be a debug mesh for such usage. This improved the performance of the debug probe view more than 2x (still fairly slow if a lot of probes). A nightmare case I had went from 180ms per frame to 81ms