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-0000: add spline smoother #1017

Merged
merged 18 commits into from
Jul 30, 2024
Merged

CMCL-0000: add spline smoother #1017

merged 18 commits into from
Jul 30, 2024

Conversation

glabute
Copy link
Collaborator

@glabute glabute commented Jul 10, 2024

Purpose of this PR

Added CinemachineSplineSmoother for creating smooth splines suitable for camera paths. This replicates the behaviour of CinemachineSmoothPath in CM2.

See https://forum.unity.com/threads/replicating-cm2s-smoothpath-in-cm3s-splines.1610553/#post-9931731 for the full story.

image

SplineSmoother is now automatically added when upgrading CinemchineSmoothPath from CM2, and when creating dolly spline or spline cart from Cinemachine menu.

Testing status

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

Documentation status

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

Technical risk

low

@codecov-commenter
Copy link

codecov-commenter commented Jul 10, 2024

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 0% with 159 lines in your changes missing coverage. Please review.

Project coverage is 26.81%. Comparing base (eee8004) to head (aeb0a88).

Files Patch % Lines
...om.unity.cinemachine/Runtime/Core/SplineHelpers.cs 0.00% 73 Missing ⚠️
.../Editor/Editors/CinemachineSplineSmootherEditor.cs 0.00% 34 Missing ⚠️
...chine/Runtime/Helpers/CinemachineSplineSmoother.cs 0.00% 31 Missing ⚠️
....cinemachine/Editor/Upgrader/UpgradeObjectToCm3.cs 0.00% 20 Missing ⚠️
...achine/Runtime/Behaviours/CinemachineSplineRoll.cs 0.00% 1 Missing ⚠️

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

Additional details and impacted files
@@                    Coverage Diff                    @@
##           dev/splineroll-easing    #1017      +/-   ##
=========================================================
- Coverage                  26.95%   26.81%   -0.15%     
=========================================================
  Files                        254      256       +2     
  Lines                      28439    28593     +154     
=========================================================
  Hits                        7666     7666              
- Misses                     20773    20927     +154     

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

Copy link
Collaborator

@sebastienduverne sebastienduverne left a comment

Choose a reason for hiding this comment

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

Ok to me – I committed minor doc fixes.

@glabute glabute assigned eakesy and unassigned eakesy Jul 19, 2024
@glabute glabute requested review from eakesy and sim-bz July 19, 2024 17:26
Copy link

@sim-bz sim-bz left a comment

Choose a reason for hiding this comment

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

Some nitpicks. I may be missing part of the bigger picture, so feel free to disregard if not relevant.

ux.Add(new HelpBox("Spline Smoother adjusts the spline's knot settings to maintain smoothness "
+ "suitable for camera paths. Do not adjust the tangents manually; they will be overwritten by the smoother.",
HelpBoxMessageType.Info));
ux.Add(new PropertyField(serializedObject.FindProperty("AutoSmooth")));
Copy link

Choose a reason for hiding this comment

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

Maybe use nameof to avoid the hardcoded string.

Suggested change
ux.Add(new PropertyField(serializedObject.FindProperty("AutoSmooth")));
ux.Add(new PropertyField(serializedObject.FindProperty(nameof(CinemachineSplineSmoother.AutoSmooth))));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wish I had known about nameof earlier! Thanks for this. Elsewhere I used some awkward expressions to avoid hardcoded strings. I like this much better.

{
#if UNITY_EDITOR
UnityEditor.Undo.RecordObject(container, "Smooth Spline");
#endif
Copy link

Choose a reason for hiding this comment

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

Should RecordObject be part of this function or called beforehand in CinemachineSplineSmootherEditor? It may be strange from an API perspective to add to the undo stack when calling this function from the editor.

Copy link
Collaborator Author

@glabute glabute Jul 22, 2024

Choose a reason for hiding this comment

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

I think you're right, this isn't the correct place for this

@glabute glabute requested a review from sim-bz July 23, 2024 12:33
Base automatically changed from dev/splineroll-easing to main July 30, 2024 13:39
@glabute glabute merged commit 86ad795 into main Jul 30, 2024
3 of 48 checks passed
@glabute glabute deleted the dev/add-spline-smoother branch July 30, 2024 13:41
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.

5 participants