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

Improve Screen Space Refraction Default Behavior #6316

Merged
merged 29 commits into from Feb 16, 2022

Conversation

johnpars
Copy link
Contributor

@johnpars johnpars commented Nov 16, 2021

Purpose of this PR

Address https://fogbugz.unity3d.com/f/cases/1379733/

This PR changes the default behavior of screen space refraction when there is no reflection probe present.

Before, the behavior was to configure an infinite projection on-the-fly. With this PR, it will instead default to the renderer's bounding box to be used as the refraction proxy.

The result of this change is that a user will now get a meaningful default result for a material configured with a box refraction model (image below).

Left, Box Refraction Mode + Infinite Projection (Before)
Right, Box Refraction Mode + Bounding Box Projection (After)
image


Testing status

  • Yamato + Local Tests.

Comments to reviewers

I didn't think a backport was necessary as it is more of a change to a working behavior, rather than a bug fix.

@github-actions
Copy link

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 Nov 16, 2021
@johnpars johnpars requested review from FrancescoC-unity and a team November 16, 2021 22:28
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.

I'm still not sure how I feel about the 10 meter default, would it make sense the default proxy to be on the screen space refraction volume component?

I personally would want that. As stated on the internal discussion, The 10m radius sphere could be the new default but access to this proxy to revert to the previous behavior and/or change the radius could be interesting in some setup. Even though, the workaround is simple, is adding your own proxy but still, having it exposed might be a bit less confusing to the user where this projection comes from instead of it being hidden/hardcoded.

Copy link
Contributor

@Vic-Cooper Vic-Cooper left a comment

Choose a reason for hiding this comment

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

I've suggested tiny phrasing changes.

@JulienIgnace-Unity JulienIgnace-Unity requested a review from a team November 25, 2021 12:35
@johnpars johnpars changed the title Improve Screen Space Refraction Default Behavior [HOLD] Improve Screen Space Refraction Default Behavior Dec 6, 2021
@johnpars
Copy link
Contributor Author

johnpars commented Dec 6, 2021

Putting this PR on temporary hold as we revise our plan for the proxy volume - we plan to add the bounding box for transparent objects which will provide a much better default behavior.

@johnpars
Copy link
Contributor Author

This PR is dependent on this Ono PR and should not land before it: https://ono.unity3d.com/unity/unity/pull-request/138270

@iM0ve iM0ve removed the request for review from a team January 28, 2022 10:25
@sebastienlagarde sebastienlagarde changed the title [HOLD] Improve Screen Space Refraction Default Behavior Improve Screen Space Refraction Default Behavior Feb 2, 2022
@sebastienlagarde
Copy link
Collaborator

@johnpars can you merge master? there is conflcit. thanks

@AlixMi
Copy link
Contributor

AlixMi commented Feb 3, 2022

refractionbox
Hello, I noticed that having a reflection probe without proxy volume reset the refraction at infinity, maybe refraction should be at bounding box size again, and edit the message. I think a tooltip about proxy volume usage on the parameter would be good as well ("proxy volumes are used for screen space refraction" or something like that, are they used for something else ?)

@AlixMi
Copy link
Contributor

AlixMi commented Feb 3, 2022

I'm wondering if we should remove the set proxy volume by default now. (Sorry I know it has been changed back and forth).
Because now, I have the feeling that the prefered behavior would be to use bounding box instead of a proxy volume. So the more general set up would be to disable them and sometimes only have a proxy volume.

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.

Love this change. 👍
It gives you out of the box box (yes, two times) refraction as you'd expect it to work.

I personally like having the use influence volume as proxy volume checked by default.
This will ensure that if a user wants infinite projection it has to do something to have it, uncheck this or select infinite in the proxy set.

As Alix said, the only thing I'd like changed on my side would be to have proper tooltips (they are missing) on the proxy volume to explain a bit that this volume will change the way refraction and reflection is done in screen space.. etc

Also, the PR could be updated with the correct new behavior :)

@johnpars
Copy link
Contributor Author

johnpars commented Feb 4, 2022

Will incorporate this feedback (PR updated as well, sorry about that). Right now I am also fighting Yamato, which seems to be imploding for this branch.

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.

Tested again with more complex geometry and comparing with the pathtracer results for refraction as a reference.

I still think this is a huge improvement to use the bounding box by default and gives relatively good results without having to setup anything. The reflection probe + proxy still gives you the possibility to adjust the refraction shape for more complex scenarios.

Still need to add the tooltips but I'm still approving. ✔️

@johnpars
Copy link
Contributor Author

johnpars commented Feb 7, 2022

Yamato is still failing, investigating again the cause.

@sebastienlagarde sebastienlagarde marked this pull request as ready for review February 8, 2022 20:39
Copy link
Contributor

@Vic-Cooper Vic-Cooper left a comment

Choose a reason for hiding this comment

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

Further small language suggestions.

Copy link
Contributor

@Vic-Cooper Vic-Cooper left a comment

Choose a reason for hiding this comment

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

Docs changed approved.

@sebastienlagarde sebastienlagarde merged commit e86bfd1 into master Feb 16, 2022
@sebastienlagarde sebastienlagarde deleted the HDRP/fix-1379733 branch February 16, 2022 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants