-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Curve trait for general interoperation #12932
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
Conversation
crates/bevy_math/src/curve.rs
Outdated
if points < 2 { | ||
return None; | ||
} | ||
let step = self.length() / (points - 1) as f32; |
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.
What if the interval is infinite?
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.
Good catch!
crates/bevy_math/src/curve.rs
Outdated
/// of interpolable data can be represented instead (or in addition). | ||
pub trait Curve<T> | ||
where | ||
T: Interpolable, |
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.
As I've discussed on discord, I don't think this bound should be placed on Curve<T>
itself, only at the part implementing resample
(about which we could also debate some more).
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 think we all pretty much agree that that would be better, so I've limited it to just the resample
methods and the curve constructs that require it now (SampleCurve
and UnevenSampleCurve
).
crates/bevy_math/src/curve.rs
Outdated
/// Sample a point on this curve at the parameter value `t`, extracting the associated value. | ||
fn sample(&self, t: f32) -> T; | ||
|
||
/// Sample a point on this curve at the parameter value `t`, returning `None` if the point is |
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.
What does/should sample
do when it's not "checked" like this?
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.
Yeah, that's a good question. Generally speaking, the idea is that sampling behavior outside of the domain interval is implementation-specific. We might want to consider renaming some of the methods to discourage using unchecked sampling in user-space, but it's pretty important to have access to it in the API, since doing otherwise introduces required overhead.
crates/bevy_math/src/curve.rs
Outdated
let step = self.domain.length() / subdivs as f32; | ||
let t_shifted = t - self.domain.start(); | ||
let lower_index = (t_shifted / step).floor() as usize; | ||
let upper_index = (t_shifted / step).ceil() as usize; |
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.
Are we sure this cannot go out of bounds due to numerical error in f32
calculations?
ie:
t = domain.end;
// step could be slightly shorter due to numerical error
step = (domain.end - domain.start) / subdivs as f32;
// t_shifted could be slightly too much due to numerical error
t_shifted = t - domain.start = domain.end - domain.start
// t_shifted / step could be therefore slightly bigger than subdivs, using .ceil() on it could therefore give the next integer, causing and out of bounds access.
upper_index = (t_shifted / step).ceil()
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 rewrote this to make it less prone to this kind of thing and easier to reason about.
Closing in favor of breaking this up to be more easily reviewed — #14630 |
This is a PR containing an implementation of the curve API described in this RFC.
See #13213 for Curves Working Group tasks.
Objective
Motivation is described in the RFC (linked above) but briefly summarized here.
The goal is to create a trait (
Curve<T>
) to provide a general functional API surface for working with curves of arbitrary origin. This allows different areas in Bevy to avoid reinventing the wheel, and paves the way for things like the following:Vec3
or similar.Put another way, the goal is to allow many consumers to be made effectively generic over curves rather than needing to rely on specific concrete constructions (e.g.
CubicCurve
,VariableCurve
, and so on). With a trait, this can be done without sacrificing runtime performance.Solution
The interface itself is described in detail in the RFC itself. Here is a brief summary.
The
Curve
trait is generic in a single parameterT
. In general, arbitraryT
are allowed, butT: StableInterpolate
guarantees access to some niceties. This bound will generally not hold for types from user-space.The fundamentals of a
Curve
are as follows:domain
, which is anInterval
(this type represents nonempty closed intervals that are possibly unbounded in either direction).sample
) atf32
parameter values to spit out values of typeT
.The sampling has to return data for any
f32
input, but its behavior outside of the domain interval is implementation-specific. There are additional sampling functions which do actually use the domain explicitly:sample_checked
: This is likesample
, but it returns anOption<T>
takingNone
values when sampling outside of the domain.sample_clamped
: This is likesample
, but it clamps the input parameter to the domain interval before sampling.Furthermore, there are a number of operations afforded to
Curve
as part of its API:map
: Given aCurve<S>
and a functionf: S -> T
, this produces aCurve<T>
whose samples are those of the former curve mapped through the latter.reparametrize
: Given aCurve<T>
and a functionf: f32 -> f32
, this produces aCurve<T>
whose samples at parametert
are those of the given curve at parameterf(t)
.graph
: Given aCurve<T>
, this produces aCurve<(f32, T)>
whose samples are those of the given curve coupled with the sampling parameter.In addition to these functional operations, the API also supports a couple of resampling operations. In their general form, these depend on an explicitly given interpolation
I: Fn(&T, &T, f32) -> T
.The resampling operations are as follows:
resample
: Given aCurve<T>
, a number of samples to use, and an explicit interpolation function, this produces aSampleCurve<T, I>
, which is aCurve<T>
whose sampled values are determined by interpolating between evenly-spaced samples.resample_uneven
: Given aCurve<T>
, an iterator overf32
parameter values, and an explicit interpolation function, this produces anUnevenSampleCurve<T, I>
, which is aCurve<T>
whose sampled values are determined by interpolating between samples taken from the original curve at the given parameter values. (That is, this works like keyframes.)These
SampleCurve
types are supported by acores
submodule that is intended for use in user-space (and especially by people like library authors) to define curves that are determined by sample interpolation. It provides a number of structs and access patterns that encapsulate, for example, the notions of evenly-spaced sampling and keyframe sampling; these can just be dropped into structs in user-space together with interpolation metadata.The
EvenCore
documentation gives an example of how this works:The API has a few more bells and whistles than this, but this hits the major points.