Skip to content
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

Added a property on the HDRP asset to allow users to avoid ray tracing effects running at too low percentages (case 1342588). #5061

Merged
merged 5 commits into from Jul 9, 2021

Conversation

anisunity
Copy link
Contributor

After some thinking, the only viable solution we found was to add a parameter to define until which sub-resolution half res ray tracing effects are allowed.
https://fogbugz.unity3d.com/f/cases/1342588/

Testing status
Due to a bug, tests are failing randomly even on master (locally). So we'll have to run on yamato and make sure it's fine

@github-actions
Copy link

github-actions bot commented Jul 6, 2021

The following job should be run on every PR:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252F_abv.yml%2523General_Checks_2021.2

It looks like you're changing an HDRP Package or project
Please run the following jobs on your branch before merging your PR:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252Fall-hdrp.yml%2523PR_HDRP_2021.2

It looks like you're changing the Core SRP Package or project
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
https://yamato.cds.internal.unity3d.com/jobs/902-Graphics/tree/master/.yamato%252F_abv.yml%2523all_project_ci_2021.2
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

@github-actions github-actions bot added the SRP label Jul 6, 2021
@github-actions
Copy link

github-actions bot commented Jul 6, 2021

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!

@remi-chapelain remi-chapelain requested a review from a team July 6, 2021 09:18
Copy link
Contributor

@kecho kecho left a comment

Choose a reason for hiding this comment

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

Make sure to run the formatter, otherwise looks good.

@anisunity
Copy link
Contributor Author

Formatting fixed

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.

We need to update the documentation for this new checkbox in the dynamic resolution section of the HDRP asset.

Apart from that, feature is working well, it's not ideal but that's the best we can do for now.
If the screen percentage drops below the RT Half resolution threshold, all RT effects running at half resolution (RTR perf & RTGI perf) are force to full screen.

I added two new tests to prevent regression for this.
Basically, camera on the left uses DRS at 90% and the RT half threshold is 100%, so the half resolution get forced back to full resolution. Native camera on the right stays un-affected so keeps the half resolution.

1001_HalfResolutionThreshold_RTR
1001_HalfResolutionThreshold_RTGI

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.

All good 🟢

@anisunity
Copy link
Contributor Author

All green except formatting (that i need to get from master), test will probably be -reinstable after rebase
image

@anisunity
Copy link
Contributor Author

hallelujah

@sebastienlagarde sebastienlagarde merged commit 827835b into master Jul 9, 2021
@sebastienlagarde sebastienlagarde deleted the HDRP/fix-dlss-rt branch July 9, 2021 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants