Hdrp - Fix volumetric fog presets #2062
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
It appears that you made a non-draft PR! |
iM0ve
requested changes
Oct 5, 2020
There was a problem hiding this comment.
Its unclear what the feature does exactly from UX point of view, see below issues:
- Based on indentation, Quality should control 5 items. But only 3 are grayed out. Should quality setting affect Anisotropy? If not we should change identation
- When changing quality, for example med to low. None of the settings are refreshed, resulting in uncertainty of what is actually changed. I think this will be reworked by UX team, because its not only for fog. But still mentioning since its an issue, maybe its worth waiting for the rework before making the quality presets?
- To find out what do the Quality features do exactly, usually one would go to the Lighting Quality section and check. But regarding Fog, there is no such setting. Again resulting in uncertainty of what values are affected and how. Unclear why quality levels are not found in the expected location.
|
iM0ve
requested changes
Oct 14, 2020
|
@iM0ve can you please check the changes, i just pushed the fix for the review you made. |
iM0ve
approved these changes
Oct 22, 2020
Removing the reprojection frame setting. Making the volumetric scene view reprojection work. Reduce the cost of the volumetric lighting gaussian denoising. Adding the option to have only the directional light contributing to the volumetrics. Adding a new configuration mode for screen resolution and volume slices. Changing the maximal values for the volumetric resolution and slices. Adding a more intuitive way of defining the denoising process of the volumetrics. Changing what is in the advanced mode for volumetrics Renaming user-facing parameter names. Displaying an info message when the user triggers anistropy Restoring the right values for the tests
anisunity
force-pushed
the
HDRP/volumetric-fog-presets
branch
from
21b0e16 to
d8cb3b7
Compare
… include so git hooks stop complaining
johnpars
reviewed
Oct 26, 2020
| /// <summary>Controls the budget of the volumetric fog effect.</summary> | ||
| public float[] Fog_Budget = new float[s_QualitySettingCount]; | ||
| /// <summary>Controls how the budget is shared between screen resolution and depth.</summary> | ||
| public float[] Fog_DepthRatio = new float[s_QualitySettingCount]; |
There was a problem hiding this comment.
Hey, so it seems we currently only added quality presets for the Balance mode. (Fog Budget, DepthRatio), however the Manual mode still has none. Is it intended? I think we will need to add them, to comply with the Quality UI pattern, otherwise it will be unclear to users if they change quality preset and nothing happens.
There was a problem hiding this comment.
Yes it is intended, the manual mode is not used for the preset as it is considered "Advanced" so having default values for it doesnt make sense to me.
|
Updated the editor to support some of the new quality UX functionality:
|
|
Also, there was a small general issue with Quality UX override state that has been fixed here: #2382 |
sebastienlagarde
approved these changes
Oct 27, 2020
sebastienlagarde
changed the title
sebastienlagarde
pushed a commit
that referenced
this pull request
Oct 27, 2020
* Fixed the second pass in the denoising being done on the wrong axis. Removing the reprojection frame setting. Making the volumetric scene view reprojection work. Reduce the cost of the volumetric lighting gaussian denoising. Adding the option to have only the directional light contributing to the volumetrics. Adding a new configuration mode for screen resolution and volume slices. Changing the maximal values for the volumetric resolution and slices. Adding a more intuitive way of defining the denoising process of the volumetrics. Changing what is in the advanced mode for volumetrics Renaming user-facing parameter names. Displaying an info message when the user triggers anistropy Restoring the right values for the tests * Implement the quality UX functionality for relevant Fog quality settings * Apply the UX requested checkbox indentation Co-authored-by: John Parsaie <johnpa@unity3d.com>
jessebarker
added a commit
that referenced
this pull request
Dec 5, 2020
This reverts commit 1b06241.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is done on top of the volumetric user experience PR #1806
It adds quality presets of the control mode, budget and depthratio.
I changed the volumes in the HDRP tests to put them back to custom in order to not break the tests.
Testing status
HDRP tests are green locally.
checked that the right values where passed when changing from one preset to an other and to the custom mode (on the template)