CMCL-0000: Improvements to PR-928: Spline position units (#929)

* Update CinemachineSplineDolly.cs & CinemachineSplineDollyEditor.cs

* Converted PositionUnits to Property

* Update `SplineContainerExtensions.cs`

* Created `ConvertDistance` method & friends
(Thanks Wybren van den Akker for helping with  math)

* Update `SplineContainerExtensions.cs`

* Added `ArgumentOutOfRangeException`s to internal code (could theoretically be accessed if users use ASMREFs)

* Update `CinemachineTrackedDolly.cs` (UpgradeToCm3)

* Changed the order of upgrading. `m_Path` has to be done first, `CinemachineSplineDolly` needs it before its `PositionUnits` is set.

* Update `SplineDollyCameraTest.cs`

* Added Unit Tests for my changes

* Added small comment why I use OnValidate the way that I do.

* Update `SplineContainerExtensions.cs`

* Added XML Documentation to new Conversion methods.

* Update `SplineDollyCameraTest.cs`

* `PositionUnits_DoesNotChangeCameraPosition_WhenPositionUnitsSame()` - Added additional asserts, not very important because as the code is now even just one should be enough. But still.

* Update `SplineContainerExtensions.cs`

* Fixed small error made in XML comments

* Update `CinemachineSplineDollyEditor.cs` & `CinemachineSplineDolly.cs`

* Moved my positionUnitsBackingfield change tracking from OnValidate to the custom editor.

* refactor as struct with PropertyDrawer, include SplineCart, handle multi-select

* Update CHANGELOG.md

* Rename SplinePosition to SplineSettings, to break less API

* add legacy support

* propertydrawer layout tweak

* handle prefabs better

* clean up spline samples

---------

Co-authored-by: Walter Hulsebos <walterhulsebos@outlook.com>
Co-authored-by: Walter Hulsebos <77513543+Walter-Hulsebos@users.noreply.github.com>

com.unity.cinemachine/CHANGELOG.md

@@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Added CinemachineShotQualityEvaluator which is a standalone version of the evaluation code in Deoccluder.
 
 ### Changed
+- SplineDolly and SplineCart: position on spline is preserved when units change.
 - SimplePlayerAimController sample upgraded to work on arbitrary surfaces (no longer depends on world up).
 - SimplePlayerController sample has logic to avoid input gimbal lock when player is upside-down relative to the camera.
 - PlayerOnSphere sample is now PlayerOnSurface - can walk on arbitrary surfaces.

com.unity.cinemachine/Editor/Editors/CinemachineSplineCartEditor.cs

@@ -13,10 +13,8 @@ class CinemachineSplineCartEditor : UnityEditor.Editor
         public override VisualElement CreateInspectorGUI()
         {
             var ux = new VisualElement();
-            ux.Add(new PropertyField(serializedObject.FindProperty(() => Target.Spline)));
+            ux.Add(new PropertyField(serializedObject.FindProperty(() => Target.SplineSettings)));
             ux.Add(new PropertyField(serializedObject.FindProperty(() => Target.UpdateMethod)));
-            ux.Add(new PropertyField(serializedObject.FindProperty(() => Target.PositionUnits)));
-            ux.Add(new PropertyField(serializedObject.FindProperty(() => Target.SplinePosition)));
 
             var autoDollyProp = serializedObject.FindProperty(() => Target.AutomaticDolly);
             ux.Add(new PropertyField(autoDollyProp));

com.unity.cinemachine/Editor/Editors/CinemachineSplineDollyEditor.cs

@@ -1,6 +1,7 @@
 using UnityEditor;
 using UnityEditor.UIElements;
 using UnityEngine.UIElements;
+using UnityEngine.Splines;
 
 namespace Unity.Cinemachine.Editor
 {
@@ -17,14 +18,8 @@ public override VisualElement CreateInspectorGUI()
             this.AddMissingCmCameraHelpBox(ux);
             var noSplineHelp = ux.AddChild(new HelpBox("A Spline is required.", HelpBoxMessageType.Warning));
 
-            var splineProp = serializedObject.FindProperty(() => Target.Spline);
+            var splineProp = serializedObject.FindProperty(() => Target.SplineSettings);
             ux.Add(new PropertyField(splineProp));
-
-            var row = ux.AddChild(InspectorUtility.PropertyRow(
-                serializedObject.FindProperty(() => Target.CameraPosition), out _));
-            row.Contents.Add(new PropertyField(serializedObject.FindProperty(() => Target.PositionUnits), "") 
-                { style = { flexGrow = 2, flexBasis = 0 }});
-
             ux.Add(new PropertyField(serializedObject.FindProperty(() => Target.SplineOffset)));
             ux.Add(new PropertyField(serializedObject.FindProperty(() => Target.CameraRotation)));
             ux.Add(new PropertyField(serializedObject.FindProperty(() => Target.AutomaticDolly)));

com.unity.cinemachine/Editor/PropertyDrawers/SplineSettingsPropertyDrawer.cs

@@ -0,0 +1,58 @@
+using UnityEditor;
+using UnityEngine.UIElements;
+using UnityEditor.UIElements;
+using UnityEngine.Splines;
+
+namespace Unity.Cinemachine.Editor
+{
+    [CustomPropertyDrawer(typeof(SplineSettings))]
+    class SplineSettingsPropertyDrawer : PropertyDrawer
+    {
+        public override VisualElement CreatePropertyGUI(SerializedProperty property)
+        {
+            SplineSettings def = new ();
+            var splineProp = property.FindPropertyRelative(() => def.Spline);
+            var positionProp = property.FindPropertyRelative(() => def.Position);
+            var unitsProp = property.FindPropertyRelative(() => def.Units);
+
+            var ux = new VisualElement();
+            ux.Add(new PropertyField(splineProp));
+            var row = ux.AddChild(InspectorUtility.PropertyRow(positionProp, out var posField));
+            posField.style.flexGrow = 1;
+            posField.style.flexBasis = 0;
+
+            var initialUnits = (PathIndexUnit)unitsProp.enumValueIndex;
+            var targets = property.serializedObject.targetObjects;
+            for (int t = 0; t < targets.Length && initialUnits == (PathIndexUnit)unitsProp.enumValueIndex; ++t)
+                if (new SerializedObject(targets[t]).FindProperty(unitsProp.propertyPath).enumValueIndex != (int)initialUnits)
+                    initialUnits = (PathIndexUnit)(-1); // mixed values
+
+            var unitsField = row.Contents.AddChild(new EnumField(
+                initialUnits) { style = { flexGrow = 2, flexBasis = 0 } });
+            unitsField.RegisterValueChangedCallback((evt) => 
+            {
+                var newUnits = (PathIndexUnit)evt.newValue;
+                for (int i = 0; i < targets.Length; ++i)
+                {
+                    var o = new SerializedObject(targets[i]);
+                    var pU = o.FindProperty(unitsProp.propertyPath);
+                    var oldUnits = (PathIndexUnit)pU.enumValueIndex;
+                    if (oldUnits != newUnits)
+                    {
+                        // Convert the position to the new units
+                        var pS = o.FindProperty(splineProp.propertyPath);
+                        var spline = pS.objectReferenceValue as SplineContainer;
+                        if (spline.IsValid())
+                        {
+                            var pP = o.FindProperty(positionProp.propertyPath);
+                            pP.floatValue = spline.Spline.ConvertIndexUnit(pP.floatValue, oldUnits, newUnits);
+                        }
+                        pU.enumValueIndex = (int)newUnits;
+                        o.ApplyModifiedProperties();
+                    }
+                }
+            });
+            return ux;
+        }
+    }
+}

com.unity.cinemachine/Runtime/Behaviours/CinemachineSplineCart.cs

@@ -1,4 +1,5 @@
 using UnityEngine;
+using UnityEngine.Serialization;
 using UnityEngine.Splines;
 
 namespace Unity.Cinemachine
@@ -14,9 +15,10 @@ namespace Unity.Cinemachine
     [HelpURL(Documentation.BaseURL + "manual/CinemachineSplineCart.html")]
     public class CinemachineSplineCart : MonoBehaviour
     {
-        /// <summary>The Spline container to which the camera will be constrained.</summary>
-        [Tooltip("The Spline container to which the cart will be constrained.")]
-        public SplineContainer Spline;
+        /// <summary>
+        /// Holds the Spline container, the spline position, and the position unit type
+        /// </summary>
+        public SplineSettings SplineSettings = new () { Units = PathIndexUnit.Normalized };
 
         /// <summary>This enum defines the options available for the update method.</summary>
         public enum UpdateMethods
@@ -33,53 +35,81 @@ public enum UpdateMethods
         [Tooltip("When to move the cart, if Speed is non-zero")]
         public UpdateMethods UpdateMethod = UpdateMethods.Update;
 
-        /// <summary>How to interpret the Spline Position</summary>
-        [Tooltip("How to interpret the Spline Position.  If set to Knot, values are as follows: "
-            + "0 represents the first knot on the path, 1 is the second, and so on.  "
-            + "Values in-between are points on the spline in between the knots.  "
-            + "If set to Distance, then Spline Position represents distance along the spline.")]
-        public PathIndexUnit PositionUnits = PathIndexUnit.Distance;
-
         /// <summary>Controls how automatic dollying occurs</summary>
         [FoldoutWithEnabledButton]
-        [NoSaveDuringPlay]
         [Tooltip("Controls how automatic dollying occurs.  A tracking target may be necessary to use this feature.")]
         public SplineAutoDolly AutomaticDolly;
 
         /// <summary>Used only by Automatic Dolly settings that require it</summary>
         [Tooltip("Used only by Automatic Dolly settings that require it")]
         public Transform TrackingTarget;
 
-        /// <summary>The cart's current position on the spline, in position units</summary>
-        [NoSaveDuringPlay]
-        [Tooltip("The position along the spline at which the cart will be placed.  "
-            + "This can be animated directly or, if the velocity is non-zero, will be updated automatically.  "
-            + "The value is interpreted according to the Position Units setting.")]
-        public float SplinePosition;
+        /// <summary>The Spline container to which the cart will be constrained.</summary>
+        public SplineContainer Spline
+        {
+            get => SplineSettings.Spline;
+            set => SplineSettings.Spline = value;
+        }
+
+        /// <summary>The cart's current position on the spline, in spline position units</summary>
+        public float SplinePosition
+        {
+            get => SplineSettings.Position;
+            set => SplineSettings.Position = value;
+        }
+
+        /// <summary>How to interpret PositionOnSpline:
+        /// - Distance: Values range from 0 (start of Spline) to Length of the Spline (end of Spline).
+        /// - Normalized: Values range from 0 (start of Spline) to 1 (end of Spline).
+        /// - Knot: Values are defined by knot indices and a fractional value representing the normalized
+        /// interpolation between the specific knot index and the next knot."</summary>
+        public PathIndexUnit PositionUnits
+        {
+            get => SplineSettings.Units;
+            set => SplineSettings.ChangeUnitPreservePosition(value);
+        }
 
         CinemachineSplineRoll m_RollCache; // don't use this directly - use SplineRoll
 
+        // In-editor only: CM 3.0.x Legacy support =================================
+        [SerializeField, HideInInspector, FormerlySerializedAs("SplinePosition")] private float m_LegacyPosition = -1;
+        [SerializeField, HideInInspector, FormerlySerializedAs("PositionUnits")] private PathIndexUnit m_LegacyUnits;
+        [SerializeField, HideInInspector, FormerlySerializedAs("Spline")] private SplineContainer m_LegacySpline;
+        void PerformLegacyUpgrade()
+        {
+            if (m_LegacyPosition != -1)
+            {
+                SplineSettings.Position = m_LegacyPosition;
+                SplineSettings.Units = m_LegacyUnits;
+                m_LegacyPosition = -1;
+                m_LegacyUnits = 0;
+            }
+            if (m_LegacySpline != null)
+            {
+                SplineSettings.Spline = m_LegacySpline;
+                m_LegacySpline = null;
+            }
+        }
+        // =================================
+
         private void OnValidate()
         {
-            if (AutomaticDolly.Method != null)
-                AutomaticDolly.Method.Validate();
+            PerformLegacyUpgrade(); // only called in-editor
+            AutomaticDolly.Method?.Validate();
         }
 
         void Reset()
         {
-            Spline = null;
+            SplineSettings = new SplineSettings { Units = PathIndexUnit.Normalized };
             UpdateMethod = UpdateMethods.Update;
-            PositionUnits = PathIndexUnit.Distance;
             AutomaticDolly.Method = null;
             TrackingTarget = null;
-            SplinePosition = 0;
         }
 
         void OnEnable()
         {
             RefreshRollCache();
-            if (AutomaticDolly.Method != null)
-                AutomaticDolly.Method.Reset();
+            AutomaticDolly.Method?.Reset();
         }
 
         void FixedUpdate()

com.unity.cinemachine/Runtime/Components/CinemachineSplineDolly.cs

@@ -21,30 +21,10 @@ namespace Unity.Cinemachine
     [HelpURL(Documentation.BaseURL + "manual/CinemachineSplineDolly.html")]
     public class CinemachineSplineDolly : CinemachineComponentBase
     {
-        /// <summary>The Spline container to which the camera will be constrained.</summary>
-        [Tooltip("The Spline container to which the camera will be constrained.")]
-        public SplineContainer Spline;
-
-        /// <summary>The position along the spline at which the camera will be placed. This can be animated directly,
-        /// or set automatically by the Auto-Dolly feature to get as close as possible to the Follow target.
-        /// The value is interpreted according to the Position Units setting.</summary>
-        [Tooltip("The position along the spline at which the camera will be placed.  "
-            + "This can be animated directly, or set automatically by the Auto-Dolly feature to "
-            + "get as close as possible to the Follow target.  The value is interpreted "
-            + "according to the Position Units setting.")]
-        public float CameraPosition;
-
-        /// <summary>How to interpret the Spline Position:
-        /// - Distance: Values range from 0 (start of Spline) to Length of the Spline (end of Spline).
-        /// - Normalized: Values range from 0 (start of Spline) to 1 (end of Spline).
-        /// - Knot: Values are defined by knot indices and a fractional value representing the normalized
-        /// interpolation between the specific knot index and the next knot."</summary>
-        [Tooltip("How to interpret the Spline Position:\n"
-            + "- Distance: Values range from 0 (start of Spline) to Length of the Spline (end of Spline).\n"
-            + "- Normalized: Values range from 0 (start of Spline) to 1 (end of Spline).\n"
-            + "- Knot: Values are defined by knot indices and a fractional value representing the normalized " 
-            + "interpolation between the specific knot index and the next knot.\n")]
-        public PathIndexUnit PositionUnits = PathIndexUnit.Normalized;
+        /// <summary>
+        /// Holds the Spline container, the spline position, and the position unit type
+        /// </summary>
+        public SplineSettings SplineSettings = new () { Units = PathIndexUnit.Normalized };
 
         /// <summary>Where to put the camera relative to the spline position.  X is perpendicular 
         /// to the spline, Y is up, and Z is parallel to the spline.</summary>
@@ -131,21 +111,67 @@ public struct DampingSettings
 
         CinemachineSplineRoll m_RollCache; // don't use this directly - use SplineRoll
 
+        // In-editor only: CM 3.0.x Legacy support =================================
+        [SerializeField, HideInInspector, FormerlySerializedAs("CameraPosition")] private float m_LegacyPosition = -1;
+        [SerializeField, HideInInspector, FormerlySerializedAs("PositionUnits")] private PathIndexUnit m_LegacyUnits;
+        [SerializeField, HideInInspector, FormerlySerializedAs("Spline")] private SplineContainer m_LegacySpline;
+        void PerformLegacyUpgrade()
+        {
+            if (m_LegacyPosition != -1)
+            {
+                SplineSettings.Position = m_LegacyPosition;
+                SplineSettings.Units = m_LegacyUnits;
+                m_LegacyPosition = -1;
+                m_LegacyUnits = 0;
+            }
+            if (m_LegacySpline != null)
+            {
+                SplineSettings.Spline = m_LegacySpline;
+                m_LegacySpline = null;
+            }
+        }
+        // =================================
+
+        /// <summary>The Spline container to which the camera will be constrained.</summary>
+        public SplineContainer Spline
+        {
+            get => SplineSettings.Spline;
+            set => SplineSettings.Spline = value;
+        }
+
+        /// <summary>The position along the spline at which the camera will be placed. This can be animated directly,
+        /// or set automatically by the Auto-Dolly feature to get as close as possible to the Follow target.
+        /// The value is interpreted according to the Position Units setting.</summary>
+        public float CameraPosition
+        {
+            get => SplineSettings.Position;
+            set => SplineSettings.Position = value;
+        }
+
+        /// <summary>How to interpret the Spline Position:
+        /// - Distance: Values range from 0 (start of Spline) to Length of the Spline (end of Spline).
+        /// - Normalized: Values range from 0 (start of Spline) to 1 (end of Spline).
+        /// - Knot: Values are defined by knot indices and a fractional value representing the normalized
+        /// interpolation between the specific knot index and the next knot."</summary>
+        public PathIndexUnit PositionUnits
+        {
+            get => SplineSettings.Units;
+            set => SplineSettings.ChangeUnitPreservePosition(value);
+        }
+
         void OnValidate()
         {
+            PerformLegacyUpgrade(); // only called in-editor
             Damping.Position.x = Mathf.Clamp(Damping.Position.x, 0, 20);
             Damping.Position.y = Mathf.Clamp(Damping.Position.y, 0, 20);
             Damping.Position.z = Mathf.Clamp(Damping.Position.z, 0, 20);
             Damping.Angular = Mathf.Clamp(Damping.Angular, 0, 20);
-            if (AutomaticDolly.Method != null)
-                AutomaticDolly.Method.Validate();
+            AutomaticDolly.Method?.Validate();
         }
 
         void Reset()
         {
-            Spline = null;
-            CameraPosition = 0;
-            PositionUnits = PathIndexUnit.Normalized;
+            SplineSettings = new SplineSettings { Units = PathIndexUnit.Normalized };
             SplineOffset = Vector3.zero;
             CameraRotation = RotationMode.Default;
             Damping = default;
@@ -157,8 +183,7 @@ protected override void OnEnable()
         {
             base.OnEnable();
             RefreshRollCache();
-            if (AutomaticDolly.Method != null)
-                AutomaticDolly.Method.Reset();
+            AutomaticDolly.Method?.Reset();
         }
 
         /// <summary>True if component is enabled and has a spline</summary>

com.unity.cinemachine/Runtime/Core/SplineContainerExtensions.cs

@@ -1,6 +1,12 @@
+using System;
+using System.Runtime.CompilerServices;
+using static System.Runtime.CompilerServices.MethodImplOptions;
+
 using UnityEngine;
 using UnityEngine.Splines;
 
+using static UnityEngine.Splines.PathIndexUnit;
+
 namespace Unity.Cinemachine
 {
     /// <summary>
@@ -126,7 +132,7 @@ static class SplineContainerExtensions
         /// <param name="splineLength">The length of the spline, in distance units.  
         /// Passed as parameter for efficiency because length calculation is slow.
         /// If a negative value is passed, length will be calculated.</param>
-        /// <returns></returns>
+        /// <returns>The clamped position value, respecting the specified units</returns>
         public static float StandardizePosition(
             this Spline spline, float t, PathIndexUnit unit, float splineLength = -1)
         {