Skip to content

[Fogbugz # 1398085] Fixing flicker when toggling hardware DRS #6868

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 31, 2022
Merged

[Fogbugz # 1398085] Fixing flicker when toggling hardware DRS #6868

merged 3 commits into from
Jan 31, 2022

Conversation

@kecho
Copy link
Contributor

@kecho kecho commented Jan 27, 2022

Purpose of this PR

Fogbugzes:

This PR fixes a flicker when enabling/disabling DRS on a camera. This issue only occurs on hardware DRS platforms (dx12, vulkan and console).

Reason:
We were calling UpdateAndUseCamera, which triggered an update. Then after we were calling SetCurrentCameraRequest(). This artifically delayed the ScalableBufferManager sending the new resolution to the runtime.
The fix is quite simple, do all the configuration first, then do a final Update() to flush the settings gathered.

Testing status

  • Tested same thing in these two fogbugz.
  • Tested on dx12, selecting hardware drs, and flicking DRS button on the camera. Both on play mode and with editor / multiple game views (all DRS techniques including dlss).
  • Hoping qa can test vulkan and test regression on software DRS

@kecho kecho requested a review from RemyUnity January 27, 2022 00:26
@github-actions
Copy link

github-actions bot commented Jan 27, 2022

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.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

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.

@github-actions github-actions bot added the HDRP label Jan 27, 2022
@kecho kecho requested review from FrancescoC-unity and removed request for RemyUnity January 27, 2022 00:26
@github-actions
Copy link

github-actions bot commented Jan 27, 2022

It appears that you made a non-draft PR!
Please convert your PR to draft (button on the right side of the page).
See the PR template for more information.
Thank you!

@kecho kecho requested a review from a team January 27, 2022 00:26
remi-chapelain
remi-chapelain requested changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@remi-chapelain remi-chapelain left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed for dx11 / dx12 ✔️

Found only one case where it's still an issue.

  • Vulkan renderer
  • Only in hardware mode (works fine in software)
  • In editor AND player
  • happens only when a DRS filter is active when setting AllowDynamicResolution to false on the camera (works fine if DLSS is active)
DLSS.2022-01-27.12-29-57.mp4
0a817e8a00fe163dc45d291e4d505088.mp4

@kecho
Copy link
Contributor Author

kecho commented Jan 27, 2022

Fixed for dx11 / dx12 ✔️

Found only one case where it's still an issue.

  • Vulkan renderer
  • Only in hardware mode (works fine in software)
  • In editor AND player
  • happens only when a DRS filter is active when setting AllowDynamicResolution to false on the camera (works fine if DLSS is active)

DLSS.2022-01-27.12-29-57.mp4
0a817e8a00fe163dc45d291e4d505088.mp4

Ok will investigate this. There is a chance this is a vulkan backend issue, but will definitely check this.

@kecho
Copy link
Contributor Author

kecho commented Jan 27, 2022

  • AllowDynamicResolution

@remi-chapelain I tried this, I cannot reproduce this locally. Are you %100 sure you have the right branch? I tried with a vulkan renderer, hardware mode, dlss off. Works just fine on my side.

@kecho kecho requested a review from remi-chapelain January 27, 2022 22:07
remi-chapelain
remi-chapelain approved these changes Jan 28, 2022
View reviewed changes
Copy link
Contributor

@remi-chapelain remi-chapelain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for wasting time here, it seems kleber is right because I can't repro anymore !
Approving the PR ✔️

@sebastienlagarde sebastienlagarde merged commit f1c6afa into master Jan 31, 2022
@sebastienlagarde sebastienlagarde deleted the HDRP/FixDRSOneFrameFlicker branch January 31, 2022 16:41
sebastienlagarde pushed a commit that referenced this pull request Feb 7, 2022
@kecho @sebastienlagarde
[Fogbugz # 1398085] Fixing flicker when toggling hardware DRS #6868
84090f6
@sebastienlagarde sebastienlagarde mentioned this pull request Feb 7, 2022
Merged
sebastienlagarde added a commit that referenced this pull request Feb 7, 2022
@sebastienlagarde @kecho
@pmavridis @emilybrown1 @anisunity
[HDRP] Backport various PRs (#7022)
3c06260 
* [Fogbugz # 1398085] Fixing flicker when toggling hardware DRS #6868

* [Fogbugz # 1381103] Fixing hardware DRS performance when active with multiple views #6862

* [Fogbugz#1388961] Fixing PBR Dof for DLSS #6651

* Do not use the dynResHandler while initializing the state. Ensuring we use the asset directly until we build up the drs state (#6966)

* [Docs] [HDRP] Update scripts in the accumulation API documentation (#6951)

* Update scripts in the accumulation API documentation

* Min frame rate should not be zero

* Update Ray-Tracing-Path-Tracing.md

* reformatted list (#6963)

* reformatted list

* Update UI-Best-Practices.md

* Update UI-Best-Practices.md

* Update UI-Best-Practices.md

* Fixing formatting #6967

* Fixed RTGI potentially reading from outside the half res pixels due to missing last pixel during the upscale pass (case 1400310). #6985

* add visual scripting to package list (#7018)

* Apply formatting changes

Co-authored-by: Kleber Garcia <[email protected]>
Co-authored-by: Pavlos Mavridis <[email protected]>
Co-authored-by: emilybrown1 <[email protected]>
Co-authored-by: anisunity <[email protected]>
Co-authored-by: [email protected] <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@remi-chapelain remi-chapelain remi-chapelain approved these changes

+2 more reviewers

@sebastienlagarde sebastienlagarde sebastienlagarde approved these changes

@FrancescoC-unity FrancescoC-unity FrancescoC-unity approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

backported-2021.2 backported-2022.1 HDRP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@kecho @sebastienlagarde @FrancescoC-unity @remi-chapelain