Skip to content

Automatically show custom DoF parameters when quality is set to custom. [Hold] #476

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
Closed
wants to merge 1 commit into from
Closed

Automatically show custom DoF parameters when quality is set to custom. [Hold] #476

wants to merge 1 commit into from

Conversation

pmavridis
Copy link
Contributor

Purpose of this PR

https://fogbugz.unity3d.com/f/cases/1240204/
There were two issues with the Post process UI:

  • As described in the bug ticket, the custom options were hidden when the user was selecting "Custom", unless the advanced options were enabled.
  • When advanced was selected and the quality was NOT custom, then the information on the disabled UI elements was just misleading (because it reflects serialezed properties that are used only for custom settings and not the values in the HDRP quality presets)

As suggested by Remi, the solution I implemented in this PR is to show the properties that you can customise only when the custom option is selected.

Keep in mind that after I finish the DoF improvements (at the moment looks more like a rewrite), the UI might/will change again.

Manual Tests

  • Open an HDRP scene (for example the template)
  • Add a Post Processing volume override
  • Select custom quality ===> The options to customise the quality now appear automatically.

@github-actions github-actions bot added the HDRP label May 14, 2020
FrancescoC-unity
FrancescoC-unity approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@FrancescoC-unity FrancescoC-unity left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We should probably discuss this UX pattern more deeply. Having advanced settings not show up as advanced because custom is on is a bit defeating the purpose of advanced settings at all, but then again I understand why the bug was filed, so it is a bit of an odd situations we are in.

@pmavridis
Copy link
Contributor Author

Having advanced settings not show up as advanced because custom is on

It is rather arguable what is an "advanced" setting and what is "custom".
IMHO, if you override one of the presets that you define in the asset, then you have a "custom" setting, not an advanced one. So these were incorrectly marked as advanced in the first place.
(And btw they are not marked as advanced in the HDRP asset, pp quality settings)

@FrancescoC-unity
Copy link
Contributor

FrancescoC-unity commented May 14, 2020 •
edited
Loading

Having advanced settings not show up as advanced because custom is on

It is rather arguable what is an "advanced" setting and what is "custom".
IMHO, if you override one of the presets that you define in the asset, then you have a "custom" setting, not an advanced one. So these were incorrectly marked as advanced in the first place.
(And btw they are not marked as advanced in the HDRP asset, pp quality settings)

For these specific ones I tend to agree, they were made advanced before we had the concept of presets; in this case indeed custom == advanced.

For other volumes (Thinking for example AO where we have something like max radius in pixels) there are several settings that most users, if not power-one shouldn't be concerned about, but they are part of the presets.
Not everything that is advanced is part of a preset and not everyone that is part of a preset is advanced.

Also, if we go by "hide everything that is not customizable" (which was by the way the first design but got changed to show the values that are used) we need to do that as a general pattern which is not followed by any other volume. In this case all stuff custom was hidden because advanced, but for most of other cases we have mix bag of advanced/non-advanced stuff that are part of a preset.

As I said, is a much wider discussion, nothing to do with this PR specifically, it is just a note that we need to standardize on a behaviour.

@FrancescoC-unity FrancescoC-unity self-requested a review May 14, 2020 13:13
@FrancescoC-unity
Copy link
Contributor

Actually this creates more problem than just UX, letting @JulienIgnace-Unity chime in with the details but there is slack conversation about it.

@JulienIgnace-Unity
Copy link
Contributor

Yeah, there was a big conversation a couple days ago about this sort of logic in Volume inspectors:
https://unity.slack.com/archives/GHD5LADU7/p1589288025027100
Bottom line was that hiding parameters is not good in general because then you can't override parameters individually. The problem now is to still have a UI that makes sense :)
A meeting is scheduled do discuss this.

@pmavridis pmavridis marked this pull request as draft May 14, 2020 13:26
@pmavridis
Copy link
Contributor Author

Converting to draft until we have a general consensus on what to do in these cases.

JordanL8
JordanL8 reviewed May 14, 2020
View reviewed changes
com.unity.render-pipelines.high-definition/Documentation~/Post-Processing-Depth-of-Field.md
| **Property** | **Description** |
| ------------------ | ------------------------------------------------------------ |
| **Focus Mode** | Use the drop-down to select the mode that HDRP uses to set the focus for the depth of field effect.<br />&#8226; **Off**: Select this option to disable depth of field.<br />&#8226; **Use Physical Camera**: Select this option to use the physical [Camera](HDRP-Camera.html) to set focusing properties for the depth of field effect. For information on what Camera properties affect depth of field, see [Physical Camera settings](#PhysicalCameraSettings).<br />&#8226; **Manual**: Select this option to use custom values to set the focus of the depth of field effect. |
| **Quality** | Select one of the **Low**, **Medium** or **High** quality presets or **Custom** quality to override some or all the settings localy.|
Copy link
Contributor

Choose a reason for hiding this comment

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

Specifies the quality level to use for the effect. You can select either the Low, Medium, or High quality presets or, if you want to override properties locally, select Custom quality and provide your own values.

JordanL8
JordanL8 reviewed May 14, 2020
View reviewed changes
com.unity.render-pipelines.high-definition/Documentation~/Post-Processing-Depth-of-Field.md

Depth Of Field includes [more options](More-Options.html) that you must manually expose.

Depth Of Field includes properties that you define in the quality presets (**Project Settings > Quality > HDRP > Post-processing quality settings**). You can override these properties locally when using the Custom quality option. In this case, more options will appear in the UI.
Copy link
Contributor

Choose a reason for hiding this comment

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

Depth Of Field includes properties which you can define in your Project's quality presets (Project Settings > Quality > HDRP > Post-processing quality settings). If you do not want to use a quality preset and instead want to override these properties locally, select Custom from the Quality drop-down. This makes more properties appear in the UI which you can then edit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Lewis. I will keep this PR on hold for awhile until we have a meeting with UX, and then integrate your changes in the final version.

Base automatically changed from HDRP/staging to master May 19, 2020 23:35
@pmavridis pmavridis changed the base branch from master to HDRP/staging May 20, 2020 10:39
@sebastienlagarde sebastienlagarde changed the title Automatically show custom DoF parameters when quality is set to custom. Automatically show custom DoF parameters when quality is set to custom. [Hold] Jun 4, 2020
@pmavridis
Copy link
Contributor Author

I'm closing this PR, the new one (based on our UX meetings and discussions) is here:
#812

@pmavridis pmavridis closed this Jun 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sebastienlagarde sebastienlagarde Awaiting requested review from sebastienlagarde

@FrancescoC-unity FrancescoC-unity Awaiting requested review from FrancescoC-unity

1 more reviewer

@JordanL8 JordanL8 JordanL8 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

HDRP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pmavridis @FrancescoC-unity @JulienIgnace-Unity @JordanL8