-
Notifications
You must be signed in to change notification settings - Fork 748
Fix to reduce the amount of allocations required when using MSVO and Dynamic Resolution #901
base: v2
Are you sure you want to change the base?
Conversation
…esolution instead of allocating them each frame at different sizes when dynamic resolution is on. This should reduce memory pressure and fragmentation.
Ahh good testing Yamato, so the min supported version is 2018.4 and it appears that useDynamicScale in a RenderTextureDescriptor doesn't exist until 2019.1. This will make things more tricky to fix in a way that doesn't break the behavior in 2018.4. I would have to keep the existing behavior in 2018.4 so that it worked but allocated lots and then in 2019.4 make it move over to using dynamic intermediate targets. |
I think I see how to do that. |
…sing the existing codepath as the new codepath shows up a bug that needs fixing in Unity.
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.
Looks a good improvement to me. Shame I didn't did it this way the first time :)
…o calculate the texture size as one size but the m_Scaled(Width|Height) calculation was not quite the same and caused the wrong number of dispatches to be generated. And example is a base image size of 2160p scaled down to 2142p causing a mismatch in the height.
m_ScaledHeights[i] = (m_ScaledHeights[0] + (div - 1)) / div; | ||
// Scaled width and heights have to match their calculations like will happen with dynamic resolution otherwise with odd numbers you can get a difference between what the dynamic resolution version | ||
// generates and what we use here. | ||
m_ScaledWidths[i] = Mathf.CeilToInt(m_Widths[i] * widthScalingFactor); |
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.
Just comparing the maths to what the Switch does for scaling, Switch clamps the values between 1 and Width or 1 and Height. That's probably just the switch code being overly cautious e.g. the scale factor shouldn't be more than 1, but just double checking that wouldn't be needed here.
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.
Good call, I've checked the engine code and the scaling factors are clamped to a max of 1 so we shouldn't need to clamp on the top end but I suppose it's possible this will produce a 0 sized render target in some cases which it sounds like Switch catches, I'm not sure if we catch this case on all platforms and might end up with a 0 sized RT which I don't imagine would produce good results.
The existing dynamic resolution strategy manually scales the temporary intermediate textures to the correct size required for the current dynamic resolution scale. This allows the effect to run correctly and for a given fixed scaling factor will keep the memory usage constant however if dynamic resolution is being scaled over time then every time it changes scaling factor the temporary intermediate textures will be reallocated causing an increase in memory pressure while this happens and possibly generating more VRAM fragmentation (depending on the platform).
This PR make the temporary intermediate textures be full size dynamic resolution textures themselves. The behavior of this PR and the existing behavior should match when dynamic resolution is not being used. However when dynamic resolution is being used this PR changes the following:
Fixing this behavior showed up a bug in how Unity handles viewport setting when using dynamic resolution on some render targets at least on PS4 and possibly other dynamic resolution platforms. This PR requires a fixed Unity version to work correctly otherwise the temporary intermediate textures are not rendered to correctly when the scale is not 1.0.