CMCL-0000: Improvements to PR-928: Spline position units #929
Conversation
* Converted PositionUnits to Property
* Created `ConvertDistance` method & friends (Thanks Wybren van den Akker for helping with math)
* Added `ArgumentOutOfRangeException`s to internal code (could theoretically be accessed if users use ASMREFs)
* Changed the order of upgrading. `m_Path` has to be done first, `CinemachineSplineDolly` needs it before its `PositionUnits` is set.
* Added Unit Tests for my changes
* Added XML Documentation to new Conversion methods.
* `PositionUnits_DoesNotChangeCameraPosition_WhenPositionUnitsSame()` - Added additional asserts, not very important because as the code is now even just one should be enough. But still.
* Fixed small error made in XML comments
* Moved my positionUnitsBackingfield change tracking from OnValidate to the custom editor.
glabute
changed the title
| + "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; |
There was a problem hiding this comment.
We can't change the API without a major bump 4.X.X
There was a problem hiding this comment.
Changed it to break less API
|
Maybe we can make an exception. IMO this isn't enough of a reason to bump the major version. |
|
I changed it so that less API is broken (new properties have the same names as the old fields). Maybe that is good enough? |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #929 +/- ##
==========================================
+ Coverage 26.64% 26.82% +0.17%
==========================================
Files 248 250 +2
Lines 27644 27786 +142
==========================================
+ Hits 7367 7453 +86
- Misses 20277 20333 +56 ☔ View full report in Codecov by Sentry. |
It would still make it so prior configurations are lost, since this wouldn't preserve serialized values from before this change, no? |
Yes, unfortunately. With a little effort I could possibly fix that (stream into some private place using FormerlySerlializedAs, then find an opportune moment to patch the values into the right place). There are places in CM where this sort of thing happens, but it's always a little iffy imo (search for PerformLegacyUpgrade). |
|
|
Purpose of this PR
I really like the initiative of PR-928, it significantly improves the UX and is worth implementing. However, I do have problems with the implementation:
I've refactored it here by introducing a SplinePosition struct with its own PropertyDrawer. This allows it to be easily used by multiple clients, including SplineDolly and SplineCart, and potentially other custom classes provided by the user.
The downside of this PR is that it breaks backwards-compatibility for serialization of SplineCart and SplineDolly (because members have moved into a new struct and there is no simple way to handle such changes). I'm willing to bite the bullet on that one, and just document the issue in the changelog.
Samples were updated for this change.
Testing status
Documentation status