-
Notifications
You must be signed in to change notification settings - Fork 857
Adding adaptive performance as optional dependency #375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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.
Welcome to the Unity SRP repo!
Please make sure to fill out the PR template as best you can to give reviewers as much information as possible.
If you have any questions (and you are a Unity employee) go to "#devs-renderpipe"
Please use Draft PR if you don't need review, thank you |
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.
LGTM. I'd suggest to add a setting in the pipeline asset to enable/disable adaptive performance as well.
The UI for adaptive performance would appear if the package is installed.
if (!cameraData.isStereoEnabled) | ||
{ | ||
cameraData.cameraTargetDescriptor.width = (int)(cameraData.camera.pixelWidth * cameraData.renderScale); | ||
cameraData.cameraTargetDescriptor.height = (int)(cameraData.camera.pixelHeight * cameraData.renderScale); |
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.
We have to be careful with render scaling, as this kind of RT scale recreates render textures. The render scale in URP is meant to be done once to select your game resolution (720p, 1080p, 1440p... )
for something dynamic as adaptive performance is probably better to have a different scaling system.
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 to know! For GLES I'd have though that might be the case, it this also on Vulcan? It's currently used as fall-back if dynamic resolution would not be available.
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.
That's for all platforms. I'm not sure this is a valid fallback for dynamic resolution, HDRP and XR has a system to scale based on viewport, that would be a suitable fallback but sadly URP doesn't support it.
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.
I'd suggest to not use this render scale with adaptive performance, it will possibly allocate textures per-frame and cause issues.
"expression": "2.1.0-preview.1", | ||
"define": "ADAPTIVE_PERFORMANCE_2_1_0_OR_NEWER" |
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.
This should be 2.0.0-preview.x (7 imho) (as we will land the feature as 2.0.0 feature)
Which would put the define as ADAPTIVE_PERFORMANCE_2_0_0_OR_NEWER
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.
Please add changelog.md entry and xml doc for the new public API.
set { m_ColorGradingLutSize = Mathf.Clamp(value, k_MinLutSize, k_MaxLutSize); } | ||
} | ||
|
||
public bool useAdaptivePerformance |
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.
Add public API docs. https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/xmldoc/
if (!cameraData.isStereoEnabled) | ||
{ | ||
cameraData.cameraTargetDescriptor.width = (int)(cameraData.camera.pixelWidth * cameraData.renderScale); | ||
cameraData.cameraTargetDescriptor.height = (int)(cameraData.camera.pixelHeight * cameraData.renderScale); |
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.
I'd suggest to not use this render scale with adaptive performance, it will possibly allocate textures per-frame and cause issues.
…#513) * Adding adaptive performance as optional dependency * Update adaptive performance package version and defines * Add enable flag for adaptive performance in URP asset * Add Adaptive performance reference to URP Editor asmdef * Revert "Add Adaptive performance reference to URP Editor asmdef" This reverts commit 28e2f21. * Enable adaptive performance by default if available * Add adaptive performance documentation * Update changelog.md Co-authored-by: Lukas Chodosevicius <lukasc@unity3d.com>
…#658) * Adding adaptive performance as optional dependency * Add enable flag for adaptive performance in URP asset * Enable adaptive performance by default if available * Add adaptive performance documentation * Update changelog.md * Fix spelling error Co-authored-by: Lukas Chodosevicius <lukasc@unity3d.com> Co-authored-by: Elvar Örn Unnþórsson <ellioman@ellioman.com>
…#514) * Adding adaptive performance as optional dependency * Fix adaptive performance version and defines * Add enable flag for adaptive performance in URP asset * Add adaptive performance documentation * Update changelog.md * Fix spelling error Co-authored-by: Lukas Chodosevicius <lukasc@unity3d.com>
Purpose of this PR
Adding adaptive performance as optional dependency and hooking up CameraData/RenderingData patching from AP settings.
Testing status
Manual Tests
Boat attack demo with adaptive performance enabled/disabled
Automated Tests
In adaptive performance repo for now. https://yamato.prd.cds.internal.unity3d.com/jobs/100-com.unity.adaptiveperformance/tree/indexer-api-tests/.yamato%252Fproject-test.yml%2523test_URP_Win_DX11_Playmode_2020.1
Links
Yamato: (Select your branch) https://yamato.prd.cds.internal.unity3d.com/jobs/902-Graphics
Comments to reviewers
This PR is currently preview and I created it in order to find out if it can be backported up to 7.x.x!
NO REVIEW IS NEEDED YET