From 93c17d105ae3d51633f38cd8c91986826b3abf82 Mon Sep 17 00:00:00 2001 From: Matty Date: Thu, 21 Mar 2024 14:58:51 -0400 Subject: [PATCH] Make cardinal splines include endpoints (#12574) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective - Fixes #12570 ## Solution Previously, cardinal splines constructed by `CubicCardinalSpline` would leave out their endpoints when constructing the cubic curve segments connecting their points. (See the linked issue for details.) Now, cardinal splines include the endpoints. For instance, the provided usage example ```rust let points = [ vec2(-1.0, -20.0), vec2(3.0, 2.0), vec2(5.0, 3.0), vec2(9.0, 8.0), ]; let cardinal = CubicCardinalSpline::new(0.3, points).to_curve(); let positions: Vec<_> = cardinal.iter_positions(100).collect(); ``` will actually produce a spline that connects all four of these points instead of just the middle two "interior" points. Internally, this is achieved by duplicating the endpoints of the vector of control points before performing the construction of the associated `CubicCurve`. This amounts to specifying that the tangents at the endpoints `P_0` and `P_n` (say) should be parallel to `P_1 - P_0` and `P_n - P_{n-1}`. --- ## Migration Guide Any users relying on the old behavior of `CubicCardinalSpline` will have to truncate any parametrizations they used in order to access a curve identical to the one they had previously. This would be done by chopping off a unit-distance segment from each end of the parametrizing interval. For instance, if a user's existing code looks as follows ```rust fn interpolate(t: f32) -> Vec2 { let points = [ vec2(-1.0, -20.0), vec2(3.0, 2.0), vec2(5.0, 3.0), vec2(9.0, 8.0), ]; let my_curve = CubicCardinalSpline::new(0.3, points).to_curve(); my_curve.position(t) } ``` then in order to obtain similar behavior, `t` will need to be shifted up by 1, since the output of `CubicCardinalSpline::to_curve` has introduced a new segment in the interval [0,1], displacing the old segment from [0,1] to [1,2]: ```rust fn interpolate(t: f32) -> Vec2 { let points = [ vec2(-1.0, -20.0), vec2(3.0, 2.0), vec2(5.0, 3.0), vec2(9.0, 8.0), ]; let my_curve = CubicCardinalSpline::new(0.3, points).to_curve(); my_curve.position(t+1) } ``` (Note that this does not provide identical output for values of `t` outside of the interval [0,1].) On the other hand, any user who was specifying additional endpoint tangents simply to get the curve to pass through the right points (i.e. not requiring exactly the same output) can simply omit the endpoints that were being supplied only for control purposes. --- ## Discussion ### Design considerations This is one of the two approaches outlined in #12570 — in this PR, we are basically declaring that the docs are right and the implementation was flawed. One semi-interesting question is how the endpoint tangents actually ought to be defined when we include them, and another option considered was mirroring the control points adjacent to the endpoints instead of duplicating them, which would have had the advantage that the expected length of the corresponding difference should be more similar to that of the other difference-tangents, provided that the points are equally spaced. In this PR, the duplication method (which produces smaller tangents) was chosen for a couple reasons: - It seems to be more standard - It is exceptionally simple to implement - I was a little concerned that the aforementioned alternative would result in some over-extrapolation ### An annoyance If you look at the code, you'll see I was unable to find a satisfactory way of doing this without allocating a new vector. This doesn't seem like a big problem given the context, but it does bother me. In particular, if there is some easy parallel to `slice::windows` for iterators that doesn't pull in an external dependency, I would love to know about it. --- crates/bevy_math/src/cubic_splines.rs | 63 ++++++++++++++++++++++++--- 1 file changed, 57 insertions(+), 6 deletions(-) diff --git a/crates/bevy_math/src/cubic_splines.rs b/crates/bevy_math/src/cubic_splines.rs index 422d5767e2..b7796b68f0 100644 --- a/crates/bevy_math/src/cubic_splines.rs +++ b/crates/bevy_math/src/cubic_splines.rs @@ -2,6 +2,7 @@ use std::{ fmt::Debug, + iter::once, ops::{Add, Div, Mul, Sub}, }; @@ -172,7 +173,8 @@ impl CubicGenerator

for CubicHermite

{ } /// A spline interpolated continuously across the nearest four control points, with the position of -/// the curve specified at every control point and the tangents computed automatically. +/// the curve specified at every control point and the tangents computed automatically. The associated [`CubicCurve`] +/// has one segment between each pair of adjacent control points. /// /// **Note** the Catmull-Rom spline is a special case of Cardinal spline where the tension is 0.5. /// @@ -183,8 +185,8 @@ impl CubicGenerator

for CubicHermite

{ /// Tangents are automatically computed based on the positions of control points. /// /// ### Continuity -/// The curve is at minimum C0 continuous, meaning it has no holes or jumps. It is also C1, meaning the -/// tangent vector has no sudden jumps. +/// The curve is at minimum C1, meaning that it is continuous (it has no holes or jumps), and its tangent +/// vector is also well-defined everywhere, without sudden jumps. /// /// ### Usage /// @@ -232,10 +234,28 @@ impl CubicGenerator

for CubicCardinalSpline

{ [-s, 2. - s, s - 2., s], ]; - let segments = self - .control_points + let length = self.control_points.len(); + + // Early return to avoid accessing an invalid index + if length < 2 { + return CubicCurve { segments: vec![] }; + } + + // Extend the list of control points by mirroring the last second-to-last control points on each end; + // this allows tangents for the endpoints to be provided, and the overall effect is that the tangent + // at an endpoint is proportional to twice the vector between it and its adjacent control point. + // + // The expression used here is P_{-1} := P_0 - (P_1 - P_0) = 2P_0 - P_1. (Analogously at the other end.) + let mirrored_first = self.control_points[0] * 2. - self.control_points[1]; + let mirrored_last = self.control_points[length - 1] * 2. - self.control_points[length - 2]; + let extended_control_points = once(&mirrored_first) + .chain(self.control_points.iter()) + .chain(once(&mirrored_last)) + .collect::>(); + + let segments = extended_control_points .windows(4) - .map(|p| CubicSegment::coefficients([p[0], p[1], p[2], p[3]], char_matrix)) + .map(|p| CubicSegment::coefficients([*p[0], *p[1], *p[2], *p[3]], char_matrix)) .collect(); CubicCurve { segments } @@ -1275,6 +1295,37 @@ mod tests { assert_eq!(bezier.ease(1.0), 1.0); } + /// Test that a simple cardinal spline passes through all of its control points with + /// the correct tangents. + #[test] + fn cardinal_control_pts() { + use super::CubicCardinalSpline; + + let tension = 0.2; + let [p0, p1, p2, p3] = [vec2(-1., -2.), vec2(0., 1.), vec2(1., 2.), vec2(-2., 1.)]; + let curve = CubicCardinalSpline::new(tension, [p0, p1, p2, p3]).to_curve(); + + // Positions at segment endpoints + assert!(curve.position(0.).abs_diff_eq(p0, FLOAT_EQ)); + assert!(curve.position(1.).abs_diff_eq(p1, FLOAT_EQ)); + assert!(curve.position(2.).abs_diff_eq(p2, FLOAT_EQ)); + assert!(curve.position(3.).abs_diff_eq(p3, FLOAT_EQ)); + + // Tangents at segment endpoints + assert!(curve + .velocity(0.) + .abs_diff_eq((p1 - p0) * tension * 2., FLOAT_EQ)); + assert!(curve + .velocity(1.) + .abs_diff_eq((p2 - p0) * tension, FLOAT_EQ)); + assert!(curve + .velocity(2.) + .abs_diff_eq((p3 - p1) * tension, FLOAT_EQ)); + assert!(curve + .velocity(3.) + .abs_diff_eq((p3 - p2) * tension * 2., FLOAT_EQ)); + } + /// Test that [`RationalCurve`] properly generalizes [`CubicCurve`]. A Cubic upgraded to a rational /// should produce pretty much the same output. #[test]