Skip to content

Add missing tooltips in physical camera and improve the UI for aperture #1061

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
Merged
merged 9 commits into from
Jul 1, 2020
Merged

Add missing tooltips in physical camera and improve the UI for aperture #1061

merged 9 commits into from
Jul 1, 2020

Conversation

pmavridis
Copy link
Contributor

Purpose of this PR

https://fogbugz.unity3d.com/f/cases/1254916/
https://fogbugz.unity3d.com/f/cases/1256641/

The UI for the aperture now makes it more clear that we modify the f-number (and not directly the aperture):
image

Testing status

Things I have tested:

  • changing the aperture by typing in the text box works
  • if you type letters instead of numbers it ignores the input (and reverts to the last valid one)
  • changing the aperture by moving the slider works
  • undo works as expected
  • the aperture value is retained if you close the editor (As long as you have saved your changes)
  • checked that all physical camera fields now have tooltips (aperture, iso, blade count, curvature, etc)
  • the tooltips in the DoF volume are now correct

@pmavridis pmavridis added the HDRP label Jun 30, 2020
@pmavridis pmavridis requested review from sebastienlagarde, JordanL8, RSlysz and a team June 30, 2020 21:30
@sebastienlagarde sebastienlagarde requested a review from Chman July 1, 2020 07:15
iM0ve
iM0ve requested changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@iM0ve iM0ve 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.

+1 for the PR description :)

Tested:

  • The new tooltips are present and good addition
  • The Aperture UI and tooltip improved
  • Aperture still functions as intended
  • Undo works

Issues:

  1. Typo in Curvature tooltip. "Controls the curvuture of..."
  2. FB case 1256641 might not be addressed. Near start and Far start still use the same tooltip, same for end.

Screen Shot 2020-07-01 at 10 58 37

Chman
Chman approved these changes Jul 1, 2020
View reviewed changes
Copy link
Contributor

@Chman Chman left a comment

Choose a reason for hiding this comment

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

Side-note: I just want to point out that if we really want absolute correctness, we're not working with f-stops but t-stops. At the time we felt it would add unnecessary complexity to use t-stop in the UI as most users already know what f-stop stands for.

@pmavridis
Copy link
Contributor Author

pmavridis commented Jul 1, 2020 •
edited
Loading

Side-note: I just want to point out that if we really want absolute correctness, we're not working with f-stops but t-stops.

Yes, this is true for the exposure, but DoF computations should always use the f-stop: For example if you have a lens with f/1.4 but T/1.8, DoF should always use f/1.4, because it is the aperture radius that you are interested in. For exposure, transmittance should also be taken into account, so this needs to use the T-number. So my point is that if we want to have a T-number in the UI, then it should be an extra field in the physical camera, not replace the f-number.

@Chman
Copy link
Contributor

Chman commented Jul 1, 2020 •
edited
Loading

Yes, this is true for the exposure, but DoF computations should always use the f-stop

Yes, I was indeed thinking about exposure. The way we use our f-stop number right now for exposure is akin to a t-stop (or a physically perfect lens with "magic" glass lenses where its f-stop is in fact equal to its t-stop value). That's what I was pointing out as this PR makes some "correctness fixes" over the terminology we use.

You're right about our current (realtime) DOF. Although the way we use f-stop there is purely arbitrary as we have no relation between the sensor size and the actual lens aperture radius.

Maybe we should add a section about all of this in the manual at some point, for the sake of clarity?

The PR is good nonetheless.

@sebastienlagarde
Copy link
Contributor

Maybe we should add a section about all of this in the manual at some point, for the sake of clarity?
Agree. @JordanL8 can you track this item? thanks

@sebastienlagarde sebastienlagarde merged commit f643e8a into HDRP/staging Jul 1, 2020
@sebastienlagarde sebastienlagarde deleted the HDRP/case_1254916 branch July 1, 2020 11:03
sebastienlagarde added a commit that referenced this pull request Jul 2, 2020
@alelievr @JulienIgnace-Unity
@fabien-unity @anisunity @adrien-de-tocqueville @remi-chapelain @sebastienlagarde @FrancescoC-unity @pmavridis
Allow ShaderGraph generation to generate multiple render state [Skip …
f8b473d 
…CI] (#1039)

* Custom Passes for Render Graph (#1009)

* Implemented Probe Volume light list build.

* Implement transparency overdraw.

* Implement XR occlusion mesh

* Fix when no movecs are available.

* Remove unnecessary workaround.

* Fixed AO texture binding and usage.

* Implement transparent SSR and fixed transparent depth prepass

* Fixed a leak with the resource purge system.

Fixed a gcalloc
Properly bind normal and movec textures to SSR.

* Temporary fix to a texture binding error with deferred compute shaders.

* Fixed null ref

* Fixed area shadows

* Enabled test 2501 LightLayers

* Enabled SSAO tests 2601/2602

* Fixed debug mode background color

Re-enabled test 3002 DebugLightLayers

* Fixed GCAlloc in PushFullScreenDebug

Re-enabled test 4013 TransparentMotionVector

* Fixed GCAlloc in Resolve MSAA functions

Re-enabled test 5009 HDRISkyFlow

* Reenabled 8106

* Fixed MSAA prepass and camera motion vector

Re-enabled tests 9401

* Re-enabled tests 9402

* Re-enabled 9500, 9603 and 9604

* Fixed object motion vector pass with MSAA.

Re-enabled tests 9601 and 9602

* Fixed AOV output

Re-enabled test 9901

* Fixed binding of depth texture to volumetric lighting shader.

* Custom Passes implementation WIP

* Remove comment

* Re-enabled tests 9700 9701 for custom passes.

* Add comment.

* typo

* Fixed a null ref and binding of color pyramid.

* Implemented MSAA resolve for custom pass.

* fix typo in the Render Pipeline Wizard (#1037)

* Fixed a number of small issue when running XR and Render Graph at the same time. (#1018)

* Fixed shader graph code generation for HDRP unlit shader

* Fixed render state leak in Depth forward only pass

* Renamed the debug name from SSAO to ScreenSpaceAmbientOcclusion (1254974). (#1044)

* Added an example CloudLayer render texture + rotation for cloud layer (#976)

* Added an example CloudLayer render texture + rotation for cloud layer

* Fix shader warnings

* Better Inspector. Hacky fix for PBR sky

* Add name of default tetxure in doc

* Section for duplicating asset

* Improve doc

* Fix perf framework ui (#1049)

* Fix test scene description asset UI

* Added doc

* Added an option to extend the camera culling for skinned mesh animation in ray tracing effects (1258547). (#1042)

* Added an option to extend the camera culling for skinned mesh animation in ray tracing effects (1258547).

* Improved tooltip

Co-authored-by: Remi Chapelain <remi.chapelain@unity3d.com>
Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Change transparent ssr name in frame settings (#1056)

* Change transparent SSR display name

* changelog

* Add frame settings docs update

* Move CloudLayer Material editor

* Correct fallback for ray tracing and light layers (1258837) (#1053)

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Add missing tooltips in physical camera and improve the UI for aperture (#1061)

* Added missing tooltips in the physical camera UI

* Fix tooltips for DoF

* Revert doc change

* Custom drawing for the aperture

* Code cleanup

* Add missing comment

* Changelog

* Clamp the aperture to the correct range, avoid using hard-coded constants

* Fixed more tooltips + typo

* Fixed reseed on play overdraw vfx (#1050)

* Add defines guards around shadows hlsl files (#1067)

* Add guards to shadow includes

* changelog

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Fix issue throwing exceptions when trying to do implicit conversions on null rthandles (#1051)

* Fix exception spam

* Revert "Fix exception spam"

This reverts commit 8d91753.

* Fix exception on null RTHandle at the root of the problem.

* Force scene view redraw upon change of override exposure (#1071)

* Force scene repaint on change of overridden exposure

* changelog

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Fixed an error when clearing the SSGI history texture at creation time (1259930). (#1070)

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

* Slight change of parametrization behaviour (#1080)

* Shader stacks fixes (#1085)

* Fixed the non-editable transparent writes motion vector and back then front rendering

* Fixed alpha to mask reset in the material UI

* Updated changelog

* Fixed show after post process being visible in Lit SG material UI

* Fixed displacement mode not showing in the material UI

* Bump material version on HDRPProjectSettings

* Moved depth offset keyword to base unlit

* Fixed material multi-selection at several places

* Moved common.hlsl and shaderpass.cs.hlsl before define block in shadergraph

* Fixed depth offset displayed in material but disabled in SG

* update HDRP test material

* update DXR material

* Update 709_CameraExtendedCulling.png

Co-authored-by: Antoine Lelièvre <antoinel@unity3d.com>

* Revert: Correct fallback for ray tracing and light layers (1258837) (#1053)

* Fixed Look Dev with Light Theme (#1006)

* Fixed an issue where opening the look dev window with the light theme would make the window blink and eventually crash unity.

* Update changelog.

Co-authored-by: sebastienlagarde <sebastien@unity3d.com>

Co-authored-by: JulienIgnace-Unity <julien@unity3d.com>
Co-authored-by: Fabien Houlmann <44069206+fabien-unity@users.noreply.github.com>
Co-authored-by: anisunity <42026998+anisunity@users.noreply.github.com>
Co-authored-by: Adrien de Tocqueville <adrien.tocqueville@unity3d.com>
Co-authored-by: Remi Chapelain <remi.chapelain@unity3d.com>
Co-authored-by: sebastienlagarde <sebastien@unity3d.com>
Co-authored-by: FrancescoC-unity <43168857+FrancescoC-unity@users.noreply.github.com>
Co-authored-by: Pavlos Mavridis <pavlos.mavridis@unity3d.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Chman Chman Chman approved these changes

@sebastienlagarde sebastienlagarde sebastienlagarde approved these changes

@JordanL8 JordanL8 JordanL8 approved these changes

@iM0ve iM0ve iM0ve approved these changes

@RSlysz RSlysz Awaiting requested review from RSlysz

Assignees
No one assigned
Labels
HDRP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pmavridis @Chman @sebastienlagarde @JordanL8 @iM0ve