Objective

Previously, our cubic spline constructors would produce CubicCurve/RationalCurve output with no data when they themselves didn't hold enough control points to produce a well-formed curve. Attempting to sample the resulting empty "curves" (e.g. by calling CubicCurve::position) would crash the program (😓).

The objectives of this PR are:

1. Ensure that the curve output of bevy_math's spline constructions are never invalid as data.
2. Provide a type-level guarantee that CubicCurve and RationalCurve actually function as curves.

Solution

This has a few pieces. Firstly, the curve generator traits CubicGenerator, CyclicCubicGenerator, and RationalGenerator are now fallible — they have associated error types, and the curve-generation functions are allowed to fail:

/// Implement this on cubic splines that can generate a cubic curve from their spline parameters.
pub trait CubicGenerator<P: VectorSpace> {
    /// An error type indicating why construction might fail.
    type Error;

    /// Build a [`CubicCurve`] by computing the interpolation coefficients for each curve segment.
    fn to_curve(&self) -> Result<CubicCurve<P>, Self::Error>;
}

All existing spline constructions use this together with errors that indicate when they didn't have the right control data and provide curves which have at least one segment whenever they return an Ok variant.

Next, CubicCurve and RationalCurve have been blessed with a guarantee that their internal array of segments (segments) is never empty. In particular, this field is no longer public, so that invalid curves cannot be built using struct instantiation syntax. To compensate for this shortfall for users (in particular library authors who might want to implement their own generators), there is a new method from_segments on these for constructing a curve from a list of segments, failing if the list is empty:

/// Create a new curve from a collection of segments. If the collection of segments is empty,
/// a curve cannot be built and `None` will be returned instead.
pub fn from_segments(segments: impl Into<Vec<CubicSegment<P>>>) -> Option<Self> { //... }

All existing methods on CyclicCurve and CubicCurve maintain the invariant, so the direct construction of invalid values by users is impossible.

Testing

Run unit tests from bevy_math::cubic_splines. Additionally, run the cubic_splines example and try to get it to crash using small numbers of control points: it uses the fallible constructors directly, so if invalid data is ever constructed, it is basically guaranteed to crash.

Migration Guide

The to_curve method on Bevy's cubic splines is now fallible (returning a Result), meaning that any existing calls will need to be updated by handling the possibility of an error variant.

Similarly, any custom implementation of CubicGenerator or RationalGenerator will need to be amended to include an Error type and be made fallible itself.

Finally, the fields of CubicCurve and RationalCurve are now private, so any direct constructions of these structs from segments will need to be replaced with the new CubicCurve::from_segments and RationalCurve::from_segments methods.

Design

The main thing to justify here is the choice for the curve internals to remain the same. After all, if they were able to cause crashes in the first place, it's worth wondering why safeguards weren't put in place on the types themselves to prevent that.

My view on this is that the problem was really that the internals of these methods implicitly relied on the assumption that the value they were operating on was actually a curve, when this wasn't actually guaranteed. Now, it's possible to make a bunch of small changes inside the curve struct methods to account for that, but I think that's worse than just guaranteeing that the data is valid upstream — sampling is about as hot a code path as we're going to get in this area, and hitting an additional branch every time it happens just to check that the struct contains valid data is probably a waste of resources.

Another way of phrasing this is that even if we're only interested in solving the crashes, the curve's validity needs to be checked at some point, and it's almost certainly better to do this once at the point of construction than every time the curve is sampled.

In cases where the control data is supplied dynamically, users would already have to deal with empty curve outputs basically not working. Anecdotally, I ran into this while writing the cubic_splines example, and I think the diff illustrates the improvement pretty nicely — the code no longer has to anticipate whether the output will be good or not; it just has to handle the Result.

The cost of all this, of course, is that we have to guarantee that the new invariant is actually maintained whenever we extend the API. However, for the most part, I don't expect users to want to do much surgery on the internals of their curves anyway.