Skip to content

Migrate Physical Camera to Core. #7146

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

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Conversation

stramit
Copy link
Contributor

@stramit stramit commented Feb 15, 2022

One half of this PR:
https://ono.unity3d.com/unity/unity/pull-request/139252/_/graphics/update-physical-camera

Purpose of this PR

This PR migrates a number of settings from HDRP down into core unity camera class so they can be shared between URP and HDRP. The change includes data upgraders and updated UI that is shared between URP and HDRP.


Testing status

Covered via automated testing.


Comments to reviewers

Needs to be reviewed in conjunction with:
https://ono.unity3d.com/unity/unity/pull-request/139252/_/graphics/update-physical-camera

@github-actions
Copy link

Hi! This comment will help you figure out which jobs to run before merging your PR. The suggestions are dynamic based on what files you have changed.
Link to Yamato: https://unity-ci.cds.internal.unity3d.com/project/902/
Search for your PR branch using the search bar at the top, then add the following segment(s) to the end of the URL (you may need multiple tabs depending on how many packages you change)

HDRP
/jobDefinition/.yamato%2Fall-hdrp.yml%23PR_HDRP_trunk
With changes to HDRP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

URP
/jobDefinition/.yamato%252Fall-urp.yml%2523PR_URP_trunk
With changes to URP packages, you should also run
/jobDefinition/.yamato%2Fall-lightmapping.yml%23PR_Lightmapping_trunk

SRP Core
You could run ABV on your branch before merging your PR, but it will start A LOT of jobs. Please be responsible about it and run it only when you feel the PR is ready:
/jobDefinition/.yamato%252F_abv.yml%2523all_project_ci_trunk
Be aware that any modifications to the Core package impacts everyone in the Graphics repo so please discuss the PR with your lead.

Depending on the scope of your PR, you may need to run more jobs than what has been suggested. Please speak to your lead or a Graphics SDET (#devs-graphics-automation) if you are unsure.

Copy link
Contributor

@alex-vazquez alex-vazquez left a comment

Choose a reason for hiding this comment

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

Thanks for the change:

  • I know that the inspector name is not from your PR, as you just move code arround. But if you could change it I will appreciate it
  • I've kicked the find missing docs Yamato, just to see if there are methods that need the public API Docs
    Edit: Missing Docs on the Core package:
[12:58:27.833 Information] Missing Doc on static System.Void UnityEditor.Rendering.CameraUI.PhysicalCamera::Drawer_PhysicalCamera_FocusDistance(UnityEditor.Rendering.ISerializedCamera, Editor)
Missing Doc on static System.Void UnityEditor.Rendering.CameraUI.PhysicalCamera::Drawer_PhysicalCamera_CameraBody_ISO(UnityEditor.Rendering.ISerializedCamera, Editor)
Missing Doc on static System.Void UnityEditor.Rendering.CameraUI.PhysicalCamera::Drawer_PhysicalCamera_CameraBody_ShutterSpeed(UnityEditor.Rendering.ISerializedCamera, Editor)
Missing Doc on static System.Void UnityEditor.Rendering.CameraUI.PhysicalCamera::Drawer_PhysicalCamera_Lens_Aperture(UnityEditor.Rendering.ISerializedCamera, Editor)
Missing Doc on static System.Void UnityEditor.Rendering.CameraUI.PhysicalCamera::Drawer_PhysicalCamera_ApertureShape(UnityEditor.Rendering.ISerializedCamera, Editor)
Missing Doc on UnityEditor.Rendering.CameraUI.PhysicalCamera.Styles.ISO
Missing Doc on UnityEditor.Rendering.CameraUI.PhysicalCamera.Styles.shutterSpeed
Missing Doc on UnityEditor.Rendering.CameraUI.PhysicalCamera.Styles.aperture
Missing Doc on UnityEditor.Rendering.CameraUI.PhysicalCamera.Styles.focusDistance
Missing Doc on UnityEditor.Rendering.CameraUI.PhysicalCamera.Styles.apertureShape
Missing Doc on UnityEditor.Rendering.CameraUI.PhysicalCamera.Styles.bladeCount
Missing Doc on UnityEditor.Rendering.CameraUI.PhysicalCamera.Styles.curvature
Missing Doc on UnityEditor.Rendering.CameraUI.PhysicalCamera.Styles.barrelClipping
Missing Doc on UnityEditor.Rendering.CameraUI.PhysicalCamera.Styles.anamorphism

Comment on lines 2073 to 2077
parameters.physicalCameraCurvature = camera.camera.curvature;
parameters.physicalCameraAnamorphism = camera.camera.anamorphism;
parameters.physicalCameraAperture = camera.camera.aperture;
parameters.physicalCameraBarrelClipping = camera.camera.barrelClipping;
parameters.physicalCameraBladeCount = camera.camera.bladeCount;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could introduce a local variable, physicalCamera

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

stramit and others added 8 commits February 16, 2022 10:45
…lCamera.Drawers.cs

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>
…lCamera.Drawers.cs

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>
…ine/Camera/HDAdditionalCameraData.Migration.cs

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>
…ine/Camera/HDAdditionalCameraData.cs

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>
…ine/Camera/HDAdditionalCameraData.cs

Co-authored-by: alex-vazquez <76204843+alex-vazquez@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants