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

CMCL-1436: Integration with the new Cameras Overlay in 23.2 #854

Merged
merged 9 commits into from
Nov 15, 2023

Conversation

thomas-tu
Copy link
Collaborator

@thomas-tu thomas-tu commented Apr 3, 2023

Purpose of this PR

Jira ticket: CST-1310

This PR adds allows the new Cameras Overlay to take control of Cinemachine Cameras, which is an Editor only feature. In summary:

  • Cinemachine Cameras are visible and selectable from the Cameras Overlay.
  • Selected Cinemachine Camera will have a preview shown on the Overlay.
  • Users can look through the selected Cinemachine Camera within the SceneView.

This implementation doesn't take constraints into account.

image

Minimal supported version: Unity 23.2.0a14.

Testing status

[Explanation of what’s tested, how tested and existing or new automation tests. Can include manual testing by self and/or QA. Specify test plans. Rarely acceptable to have no testing.]

  • Added an automated test
  • Passed all automated tests
  • Manually tested

Documentation status

[Overview of how documentation is affected by this change. If there is no effect on documentation, explain why. Otherwise, state which sections are changed and why.]

  • Updated CHANGELOG
  • Updated README (if applicable)
  • Commented all public classes, properties, and methods
  • Updated user documentation

Technical risk

Low. It's scoped to the Cameras Overlay and the Scene View.

Package version

[Justification for updating either the patch, minor, or major version according to the semantic versioning rules]

  • Updated package version

@thomas-tu thomas-tu changed the title In-Camera Integration Integration with the new Cameras Overlay in 23.2 May 2, 2023
@thomas-tu thomas-tu marked this pull request as ready for review May 2, 2023 20:03
Copy link
Collaborator

@glabute glabute left a comment

Choose a reason for hiding this comment

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

LGTM, although I can't test it until the appropriate Unity version becomes available

@gaborkb
Copy link
Contributor

gaborkb commented May 3, 2023

I just finished building trunk unity, I am going to test it today or tomorrow.

@glabute
Copy link
Collaborator

glabute commented May 15, 2023

There are some interesting edge-cases to look at.

For example, what if I have a vcam with no procedural motion on Position (allowing the scene-view camera to drag it), but has a rotation tracking enabled? I would expect to be able to drag the camera's position, but while it's dragging the rotation should update so that it's always looking at the thing I'm tracking. Currently, that doesn't happen, and I have to wait for a refresh after I stop dragging to get the updated rotation.

@codecov-commenter
Copy link

codecov-commenter commented May 30, 2023

Codecov Report

Attention: 45 lines in your changes are missing coverage. Please review.

Comparison is base (9e2d555) 21.02% compared to head (7104141) 20.98%.

Files Patch % Lines
...ine/Editor/SceneView/CinemachineCameraViewpoint.cs 0.00% 45 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #854      +/-   ##
==========================================
- Coverage   21.02%   20.98%   -0.04%     
==========================================
  Files         244      245       +1     
  Lines       27482    27527      +45     
==========================================
  Hits         5777     5777              
- Misses      21705    21750      +45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thomas-tu thomas-tu changed the title Integration with the new Cameras Overlay in 23.2 CST-1310: Integration with the new Cameras Overlay in 23.2 May 31, 2023
@glabute glabute changed the title CST-1310: Integration with the new Cameras Overlay in 23.2 CMCL-1436: Integration with the new Cameras Overlay in 23.2 Jun 1, 2023
Copy link
Collaborator

@glabute glabute left a comment

Choose a reason for hiding this comment

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

Thomas, what are the controls to change the FOV and other lens properties via screen view? I can't figure it out

get => Target.Lens.FieldOfView;
set
{
var currentLens = Target.Lens;
Copy link
Collaborator

@glabute glabute Sep 12, 2023

Choose a reason for hiding this comment

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

It seems that this can be called with a negative number, resulting in negative FOV. In the inspector, this is protected against by calls to Validate() whenever the user changes something. Should the infrastructure be calling Validate() here? Or should the client (i.e. this code) be responsible for it?

Note: If we push a negative value and then undo/redo, Validate() does get called.

@glabute glabute merged commit 15a93d3 into main Nov 15, 2023
6 of 8 checks passed
@glabute glabute deleted the dev/in-camera-integration branch November 15, 2023 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants