-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Disallow empty cubic and rational curves #14382
Conversation
examples/math/cubic_splines.rs
Outdated
impl From<CubicCurve<Vec2>> for Curve { | ||
fn from(value: CubicCurve<Vec2>) -> Self { | ||
Self { inner: Some(value) } | ||
impl From<Option<CubicCurve<Vec2>>> for Curve { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up deleting even this one and just making Curve
a tuple struct for the sake of simplicity; the only reason this existed was to handle the old conversions out of curve constructors in the old version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. The pure functional people will tell you that we probably want to make the empty segments totally unrepresentable.
pub struct CubicCurve<P: VectorSpace> {
/// The first segment in the curve.
pub head: CubicSegment<P>,
/// The remaining segments in the curve.
pub tail: Vec<CubicSegment<P>>,
}
But in rust this is probably more cumbersome than it is worth. I'm fine with preserving the nonempty invariant by construction.
Does this supersede #13805? |
# Objective Apparently #14382 broke this, but it's not a part of CI, so it wasn't found until earlier today. ## Solution Update the benchmark like we updated the examples. ## Testing Running `cargo bench` actually works now.
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 callingCubicCurve::position
) would crash the program (😓).The objectives of this PR are:
bevy_math
's spline constructions are never invalid as data.CubicCurve
andRationalCurve
actually function as curves.Solution
This has a few pieces. Firstly, the curve generator traits
CubicGenerator
,CyclicCubicGenerator
, andRationalGenerator
are now fallible — they have associated error types, and the curve-generation functions are allowed to fail: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
andRationalCurve
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 methodfrom_segments
on these for constructing a curve from a list of segments, failing if the list is empty:All existing methods on
CyclicCurve
andCubicCurve
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 thecubic_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 aResult
), meaning that any existing calls will need to be updated by handling the possibility of an error variant.Similarly, any custom implementation of
CubicGenerator
orRationalGenerator
will need to be amended to include anError
type and be made fallible itself.Finally, the fields of
CubicCurve
andRationalCurve
are now private, so any direct constructions of these structs from segments will need to be replaced with the newCubicCurve::from_segments
andRationalCurve::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 theResult
.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.