Skip to content

Fix frame count in editor #3173

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 5 commits into from
Jan 21, 2021
Merged

Fix frame count in editor #3173

merged 5 commits into from
Jan 21, 2021

Conversation

adrien-de-tocqueville
Copy link
Contributor

@adrien-de-tocqueville adrien-de-tocqueville commented Jan 19, 2021 •
edited
Loading

Purpose of this PR

After a trunk change that reduced the number of editor repaint, the time and framecount managment of hdrp was broken.
This PR should fix everything when always refresh is enabled in a scene view or in game views (no change in players and in editor play mode)

  • Removed unused frameIndex parameters in some functions
  • Use per camera framecount for every effect doing accumulation (AO, density volume mask scrolling, camera dithering, sky update, hdri sky and cloud layer scrolling)
  • Fix time for camera by not using Time.time in the editor
  • Removed HDRenderPipeline variables time, lastTime and frameCount, because they are present inside the camera

Testing status

Tested effects mentionned above in the editor and in a player

@github-actions
Copy link

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

@adrien-de-tocqueville adrien-de-tocqueville marked this pull request as draft January 19, 2021 15:08
@anisunity
Copy link
Contributor

Do not merge as is, it will probably make the tests fail

@adrien-de-tocqueville
Copy link
Contributor Author

yes thats why i removed the reviewers, i am gonna look at what is failing

JulienIgnace-Unity
JulienIgnace-Unity reviewed Jan 19, 2021
View reviewed changes
...nity.render-pipelines.high-definition/Runtime/RenderPipeline/HDRenderPipeline.RenderGraph.cs
@@ -30,7 +30,7 @@ class TempPassData {};
{
scriptableRenderContext = renderContext,
commandBuffer = commandBuffer,
currentFrameIndex = GetFrameCount()
currentFrameIndex = m_FrameCount
Copy link
Contributor

Choose a reason for hiding this comment

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

Is intended to still use m_FrameCount here and not the one from the camera?

Copy link
Contributor Author

@adrien-de-tocqueville adrien-de-tocqueville Jan 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

it's intended, i investigated a bit how the frame count was used by the rendergraph, and it seems it should not be camera dependent, but you may want to confirm 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

I will take another look and try to confirm then.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the change is ok yes :)

JulienIgnace-Unity
JulienIgnace-Unity suggested changes Jan 19, 2021
View reviewed changes
Copy link
Contributor

@JulienIgnace-Unity JulienIgnace-Unity left a comment

Choose a reason for hiding this comment

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

Made a few comment. Overall looks ok to me but with all the time issues we had in the past only a thorough QA pass on this PR on all relevant effects will make sure of it :)

@adrien-de-tocqueville adrien-de-tocqueville marked this pull request as ready for review January 20, 2021 09:27
@adrien-de-tocqueville adrien-de-tocqueville requested a review from a team January 20, 2021 09:28
TomasKiniulis
TomasKiniulis approved these changes Jan 21, 2021
View reviewed changes
Copy link
Contributor

@TomasKiniulis TomasKiniulis left a comment

Choose a reason for hiding this comment

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

Looks good! I haven't ran into any PR specific issues.

What I've tested:

  • Checked @iM0ve "Time" testing scene with multiple docked undocked windows, carefully checking through density volumes, fog reprojection, shadergraphs with TAA
  • Entering fullscreen of Game/Scene view windows with Always Refresh enabled/disabled
  • Made sure AO accumulation works correctly now

Attaching comparison results:
Master- Expanding view causes exposure artifacts for a brief frame (leftmost view has a white rectangle for a split second when going in and out of fullscreen)
https://user-images.githubusercontent.com/50582134/105367554-39da4900-5c09-11eb-9317-b898af346fe7.mp4
PR - Expanding view on does not show any randomly artifacts for a brief frame
https://user-images.githubusercontent.com/50582134/105367620-4eb6dc80-5c09-11eb-9fd8-e9de69ac1e1a.mp4

Density Volumes + TAA + VFX with fixed exposure master:
https://user-images.githubusercontent.com/50582134/105368405-2bd8f800-5c0a-11eb-860a-de827fef056f.mp4

Density Volumes + TAA + VFX with fixed exposure PR:
https://user-images.githubusercontent.com/50582134/105368465-3dba9b00-5c0a-11eb-8d7a-32463fe7ea77.mp4

anisunity
anisunity approved these changes Jan 21, 2021
View reviewed changes
Copy link
Contributor

@anisunity anisunity left a comment

Choose a reason for hiding this comment

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

IF DXR and regular tests are still green, looks good to me

@sebastienlagarde sebastienlagarde merged commit 199c0f2 into master Jan 21, 2021
@sebastienlagarde sebastienlagarde deleted the hd/fix-frame-count branch January 21, 2021 15:56
sebastienlagarde pushed a commit that referenced this pull request Jan 21, 2021
@adrien-de-tocqueville @sebastienlagarde
Fix frame count in editor #3173
f361637
@sebastienlagarde sebastienlagarde mentioned this pull request Jan 22, 2021
Merged
@sebastienlagarde sebastienlagarde mentioned this pull request Feb 12, 2021
Closed
sebastienlagarde pushed a commit that referenced this pull request Mar 3, 2021
@adrien-de-tocqueville @sebastienlagarde
Fix frame count in editor #3173
e260270
@sebastienlagarde sebastienlagarde mentioned this pull request Mar 3, 2021
Merged
sebastienlagarde added a commit that referenced this pull request Mar 3, 2021
@sebastienlagarde @adrien-de-tocqueville
@alex-vazquez @anisunity
[Backport 2021.1][HDRP] Misc HDRP PR backport (#3746)
e705e20 
* Merge Hd/bugfix #3313

* Fix frame count in editor #3173

* Fixed the display name of a Volume Parameter when is defined the attribute InspectorName #3348

* Added the default quality settings to the HDRP asset for RTAO, RTR and RTGI (case 1304370). #3362

* Formatting

* Windows vulkan reference screenshots

Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com>
Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>
Co-authored-by: anisunity <42026998+anisunity@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JulienIgnace-Unity JulienIgnace-Unity JulienIgnace-Unity approved these changes

@anisunity anisunity anisunity approved these changes

@TomasKiniulis TomasKiniulis TomasKiniulis approved these changes

Assignees
No one assigned
Labels
backported-10.x backported-2021.1 HDRP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@adrien-de-tocqueville @anisunity @JulienIgnace-Unity @TomasKiniulis @sebastienlagarde