-
Notifications
You must be signed in to change notification settings - Fork 855
Scene templates and default settings tweaks #948
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"
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.
Concerns regarding Outdoor scene
-
Physical sky is being used. This can cause confusion for beginners when they add objects below or near 0 altitude
-
Fixed Directiontional light is using color filter, maybe we want to showcase Temperature mode instead?
-
Fixed Sun intensity is 100 000 that translates to Bright day, but the scene looks dark, remaining of the evening. Maybe Limit Max exposure of 16 could be bumped down to 14 or 15
-
Fixed We are using Dynamic Ambient mode. Some time ago we have a meeting where I recommended to make Dynamic by default. But we didnt do it because of the performance hit and considered it to be a bad practice. I have no issue with this one but want to clarify if this is still the case and we should recommend Static lighting or is it ok.
Concerns regarding Indoor scene
- Fixed Dynamic Gradient sky is used for Ambient lighting. Not sure if better recommended workflow could be using Visual Environment:None with black camera backround. Would this make sense performance/workflow wise?
General issues
General Suggestions
- Fixed Rename Directional Light to Sun in outdoor scene
- Add HDRP default settings to the scene for discoverability. Effectively it wont do anything, but help people understand we have HDRP default settings applied. Priority -1
Test coverage:
- Create new Indoor scene
- Create new Outdoor scene
- Indoor and Outdoor in DXR
- Running HDRP wizard
If I recall correctly, we had a lot of trouble with fog/volumetric fog when no sky was set in visual environment. (it was in 7.x package though) |
Tweaked a few things:
Notes:
|
It should also be noted that I don't have any way of testing the DXR templates, any help is appreciated :) |
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 will check DXR template. |
Issues I've found were fixed or discussed. I also tested DXR its exactly the same as regular templates, but has DXR SSR instead of regular SSR. We might want to add more features, even if they are turned off by default - for discoverability. Found one new issue with unreliable reproduction. After running DXR wizard clicking on random scenes for 10-40 seconds will end up with an error message. This is an edge case, not sure if we can fix fast or just merge this version. |
On DXR I have just added disable Raytracing Settings, but there is nothing to add more. PR is good, thanks for that! Merging |
Purpose of this PR
Mode
is now set toAutomatic (Histogram)
.Limit Min
is now set to-4
.Limit Max
is now set to+16
.Testing status
Manual Tests:
Created new scenes using each available template. Note: these templates assume you have an HDRP asset setup already or that the Wizard has been ran at least once.
Comments to reviewers
Adding @JordanL8 to validate the template naming scheme and descriptions (look for the four
*.scenetemplate
files). I'm not entirely sure about the naming used here but I followed the one from the two hardcoded templates used with the builtin renderer.