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

Add Light Unit Slider (Light Intensity, Temperature, Exposure) #1691

Merged
merged 66 commits into from Sep 22, 2020

Conversation

johnpars
Copy link
Contributor

@johnpars johnpars commented Aug 27, 2020

Purpose of this PR

This PR introduces the proposed improvement to UI/UX for light units. It adds a new slider UI drawer that loads a table of helpful value ranges, providing more context to the user about light unit values; through icons, markers, and tooltips. This will help the user to more easily reason about the variety of physically based values in HDRP, when authoring lighting and exposure.

Light Intensity Units (EV, Nits, Lumen, Candela, Lux)

LightIntensity

Temperature Units (Kelvin)

Temperature

Exposure Units (EV)

Exposure

Editor Theme

EditorThemes

Slider is constant across all punctual light intensity units and spot reflector

ConstantSlider


Testing status

  • Checked new UI names with UX convention
  • Tested UI multi-edition + Undo/Redo + Prefab overrides + Alignment in Preset
  • C# and shader warnings (supress shader cache to see them)
  • Checked new resources path for the reloader (in developer mode, you have a button at end of resources that check the paths)

Comments to reviewers

  • This PR implements the new UI drawer for Light Intensities, Temperature, and Exposure (volume component), but has not yet been integrated to the scene view override exposure, or the physically based sky exposure.
  • The slider for light intensities and exposure is modeled as a piece-wise function. At the cost of not being continuous and not monotonically increasing, it has the benefit of easily being able to accommodate the wide value distributions for certain light units, The PR does include an exponential version (which fits a curve to a low, mid, and high value), but its currently unused and can be removed. I have sent an email to our artists to gather additional feedback on this piece-wise distribution.
  • For icon loading, currently they are not loaded with the HDRP editor resource loading mechanism (reloader). This is because we need to take special care to load icons so that they are skinned properly for the editor and it's color theme. Maybe it's possible to add Reloader support for icon case?

@github-actions github-actions bot added the HDRP label Aug 27, 2020
@johnpars johnpars changed the title Light Unit UI/UX (Light Intensity) Light Unit Slider UI/UX (Light Intensity) Aug 27, 2020
@johnpars johnpars changed the title Light Unit Slider UI/UX (Light Intensity) Add Light Unit Slider (Light Intensity) Aug 27, 2020
@johnpars
Copy link
Contributor Author

Hi @iM0ve, thanks for the extra review.

To address your notes:

  • 3: Fixed
  • 7: Fixed after discussion with artists in thread: the filter/temperaturewill be the default.
  • 8: Fixed, however there is an ongoing discussion with lighting artist on how this behavior should work when spot reflector is enabled.
  • 5: Not Fixed, this is a more UI/UX note which I will forward to @MaddalenaUnity

@johnpars
Copy link
Contributor Author

Hi @fredericv-unity3d, thanks also for your review.

Your notes should all be addressed (note: for your note regarding Humanizing tooltip value, there is ongoing discussion with artists if it should be abbreviated or not).

If it's alright, I will request another re-review. I just pushed one last significant change to the slider that makes it always operate internally in Lumen. This was done to directly fix @iM0ve feedback number 8.

It removed a lot of code (in the settings), added a slider specifically for converting between light units, and it elevated the access level of ConvertLightIntensity in HDLightUI to internal.

Copy link
Contributor

@fredericv-unity3d fredericv-unity3d left a comment

Choose a reason for hiding this comment

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

I found very minor issues that should be changed, can you update them?

@fredericv-unity3d
Copy link
Contributor

You can request as many reviews as you need, it is always helpful!
Thanks for your work!

@iM0ve
Copy link
Contributor

iM0ve commented Sep 17, 2020

No new issues found. Updated the review. Only issue 5 still remains. Overall PR is looking good. I will wait for all the changes to land and do one last check before approving.

…or (keeping the slider constant between light unit)
@johnpars
Copy link
Contributor Author

johnpars commented Sep 18, 2020

Thanks @fredericv-unity3d - I implemented your requests, and also added one last improvement that should satisfy the request of the artists (basically, need to satisfy the case of spot reflector; where the slider should be unchanged between light units). There was an unfortunate limitation to the LightUtil conversion that required me manually recompute some things (more info in the comment). Instead of improving the LightUtil I decided to accomplish it all in the slider, as I was nervous about changing light unit conversion at this point, since it might actually impact things on the runtime (I do not want to accidentally undo an optimization).

@johnpars
Copy link
Contributor Author

johnpars commented Sep 18, 2020

Hi @iM0ve, for issue 5, it's certainly a doable change but I haven't been able to confirm alignment on it with UX. Additionally, I assume the change would also require new icons which I don't think are currently on hand from icon artist. If the change to the tooltip is enough however, it should be quickly doable.

Note also: I pushed a change to the light intensity slider that should be an improvement for how it behaves with spot reflector. Its probably the area of most attention in your next round of testing.

Copy link
Contributor

@fredericv-unity3d fredericv-unity3d left a comment

Choose a reason for hiding this comment

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

This is good for me!

@johnpars
Copy link
Contributor Author

Note: This PR now fully implements the originally designed spec. However, we have the following feedback from artists:

  • Improvement of iconography for temperature slider (specifically, waiting for two new icons - info is updated in the icon google sheet. Note that adding this should address the remaining issue flagged by @iM0ve).
  • Add a right-click context menu on the slider, offering a dropdown of presets.

@sebastienlagarde sebastienlagarde marked this pull request as ready for review September 22, 2020 09:00
@sebastienlagarde
Copy link
Collaborator

Hey, so I will merge the current PR and we will do the improvement in a following one. THanks

@sebastienlagarde sebastienlagarde merged commit 58a91f2 into HDRP/staging Sep 22, 2020
@sebastienlagarde sebastienlagarde deleted the HDRP/physical-light-unit-ux branch September 22, 2020 09:03
@iM0ve
Copy link
Contributor

iM0ve commented Sep 22, 2020

Still wanted to do a final pass before merge :/ please warn next time.
Did a quick check on reflector and Undo. Found that undo doesnt work with reflector properly and switching between certain light value types. For example works if Candela->Lumen->Undo and doesnt work Candela->Lux->Undo

type
refl

@sebastienlagarde
Copy link
Collaborator

arf sorry about that. was guessing it was done. Good catch

@sebastienlagarde sebastienlagarde mentioned this pull request Sep 22, 2020
6 tasks
@sebastienlagarde sebastienlagarde restored the HDRP/physical-light-unit-ux branch September 22, 2020 17:29
@sebastienlagarde sebastienlagarde deleted the HDRP/physical-light-unit-ux branch September 25, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants