Skip to content

Conversation

pmavridis
Copy link
Contributor

@pmavridis pmavridis commented Jun 10, 2020

Purpose of this PR

https://fogbugz.unity3d.com/f/cases/1240204/

This PR implements the UI/UX improvements for DoF that we recently discussed. After the changes, the UI looks like this:

image

Notably:

  • Quality settings are always visible and editable
  • Selecting a quality presets reflects the correct values in the UI controls
  • Editing a quality control automatically sets the quality to custom
  • Switching back to custom from a preset, remembers the last custom settings used (including override states).
  • The override check boxes are indented for this control, as seen in the screen (and discussed on Slack).

There is a lot of code in this PR to implement this functionality, but we should be able to re-use it when implementing the same for other volume components (apart from DoF). The new functions are not a public API, so it should be easy to change them in the future.

As expected, there are no changes (or overhead) in the runtime. All changes affect only the editor.


Testing status

I have tested changing various presets in DoF, clicking the checkboxes, disabling the control etc.

@pmavridis
Copy link
Contributor Author

Note: If we are OK with this, then I must also update the screenshot in the documentation before we merge.

@iM0ve
Copy link
Contributor

iM0ve commented Jun 11, 2020

Review Status: In Progress

Whats tested:

  • UX. The new UI workflow is way better than the current one.
  • Changing any value under Quality parameters switches mode to Custom
  • Undo functionality
  • Switching Manual/Use Physical Camera, retains all quality settings. Nothing is lost
  • Currently switching to Custom whenever you enable any parameter, even if you dont change the values (Fixed)
  • Verified that Advanced settings react in the same way as others, when switching Quality mode
  • Verified all Manual and Physical mode parameters still work
  • Verified that Aperature(phys camera) and DoF are still linked and work together

Issues
2 fixed, 1 active

  1. This is a general issue, will not be adressed in this PR .Changing Quality settings does not register for Undo. Can result in weird UI, for example will say Hight settings, but all the sliders would be set to Low.

bM7S1nqmYW

  1. Fixed. Not sure whats going on here. I switch to High, enable one of the parameters without changing it. I expect rendering not to change, but it does.

9qPy3FWKul

3)Fixed. Maybe related to issue 2, but it still occurs. Lowering a parameter reduces visual fidelity, but raising it back up doesnt revert the visual change. Only works if you rever the value via Quality mode (not via slider).

dofupdate

@pmavridis
Copy link
Contributor Author

pmavridis commented Jun 11, 2020

Maybe related to issue 2, but it still occurs. Lowering a parameter reduces visual fidelity, but raising it back up doesnt revert the visual change. Only works if you rever the value via Quality mode (not via slider).

These two issues are indeed related and both happen because the runtime values get out-of-sync with the reflected values in the Editor UI. I'm still trying to figure out what is causing this (it is related to the reflection system) and will post an update.

@pmavridis
Copy link
Contributor Author

pmavridis commented Jun 12, 2020

In the end, it was not exactly a bug, but more a UX issue. Here is what's happening:

  • In the quality settings you have a few parameters set to not be overridden.
  • By design, non-overridden parameters are replaced by the volume system with the default value for this parameter (which is not shown in the UI) if no other volume overrides this. This can be confusing, and initially I though it was a bug.
  • When the quality is set to custom, quality parameters behave similar to other parameters (so the ones that are set to "no override" will be replaced with default values).
  • When the quality is set to high (or another preset), all quality settings will be overridden with values from the presets (I believe this makes sense, because "high" has a specific meaning. If you want "high" but with one parameter modified, then it is not "high" anymore, it is custom).
  • So when you automatically switch to custom by moving one slider, because some parameters are set to not-override, for these parameters you are also switching from the high-preset to the default value.

So I'm making these "UX" changes to make things better:

  • When you switch to a preset, all quality parameters are set to "override" - all checkboxes will be active (this communicates to the user that all these values are actually used by the preset). This way, when you switch to custom by modifying one of them, you will get what you expect: the settings of the preset with only one of the quality parameter changed.
  • When the user sets the quality field to non-override (the "parent" of the quality settings), I grey-out all quality presets.

I believe there is still a UX issue (as mentioned in the second bullet): for non-override parameters we generally don't show what value is used. But this not specific to quality settings, so it should be addressed in another PR. I will start a discussion on Slack regarding this.

@FrancescoC-unity FrancescoC-unity requested a review from RSlysz June 12, 2020 10:37
Copy link
Contributor

@iM0ve iM0ve left a comment

Choose a reason for hiding this comment

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

All issues fixed (except the Undo, that is not this PR specific).
I think its a great quality of life improvement for Quality settings. We should use it as an example for other quality settings related components.

Copy link
Contributor

@RSlysz RSlysz left a comment

Choose a reason for hiding this comment

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

Please add UX in the loop when modifying UX.

Here I am worried as you add the indentation on the checkbox too. I am not against this change but I think it must be done everywhere at the same time and not only in this volume component. Having different display, even if new one is better, can be worst if not changed everywhere at same time :(

Check FrameSettings and other volume component please

/// <summary>
/// Like EditorGUI.indentLevel++ but this one will also indent the override checkboxes
/// </summary>
internal static void BeginIndent()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have prefered a scope here in order to be sure anyone using it will not forget to call also EndIndent :)

@pmavridis
Copy link
Contributor Author

pmavridis commented Jun 15, 2020

Please add UX in the loop when modifying UX.

We already had a few meetings with UX discussing these changes and there is also a slack thread where @TheoWong-pixel gives his input regarding indentation.

I agree that these changes need to be applied globally (not only the indentation but also the quality settings behaviour) and since @johnpars has exactly this task, perhaps he can continue working from this state and make a final PR for everything.

This PR was just addressing the linked FogBugz.

@pmavridis pmavridis closed this Jul 15, 2020
@pmavridis
Copy link
Contributor Author

Closing this PR since @johnpars will open a new one with improvements for all volume components with quality settings.

@johnpars
Copy link
Contributor

Opened a draft here for discussion (see reviewer notes): #1296

@sebastienlagarde sebastienlagarde deleted the HDRP/dof_ux_fix branch September 1, 2021 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants