A Curve trait for general interoperation — Part II #14700
Conversation
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
…e::reparametrize_linear
mweatherley
force-pushed
the
curve-trait-part-2
branch
from
628b91a
to
d295abe
Compare
mweatherley
added
C-Feature
crates/bevy_math/src/curve/cores.rs
Outdated
| /// Create a new [`EvenCore`] from the specified `domain` and `samples`. An error is returned | ||
| /// if there are not at least 2 samples or if the given domain is unbounded. | ||
| #[inline] | ||
| pub fn new(domain: Interval, samples: impl Into<Vec<T>>) -> Result<Self, EvenCoreError> { |
There was a problem hiding this comment.
The samples parameter would be more generic if it's defined as impl IntoIterator<Item=T>
There was a problem hiding this comment.
Yeah; the point of this is that it's really simple and doesn't require defining an iterator, but I'm becoming increasingly convinced that we should have a version of this that accepts an iterator, at least. That's kind of hairy, though, so I'll leave it to follow-ups.
Co-authored-by: Robert Walter <26892280+RobWalt@users.noreply.github.com>
| /// | ||
| /// # Invariants | ||
| /// This must always have a length of at least 2. | ||
| pub samples: Vec<T>, |
There was a problem hiding this comment.
I'm not thrilled with leaving this as a pub field with invariants to uphold, but there's already some serious Here There Be Dragons vibes from this code, so I don't think this is a big footgun.
There was a problem hiding this comment.
Yeah, I'm not necessarily huge on it either, but I didn't really see a good alternative. As far as I can tell, the best thing to do is to signal as best we can that this module is primarily for library authors (e.g. with its docs, excluding it from reexports, and so on).
crates/bevy_math/src/curve/cores.rs
Outdated
|
|
||
| impl<T> EvenCore<T> { | ||
| /// Create a new [`EvenCore`] from the specified `domain` and `samples`. An error is returned | ||
| /// if there are not at least 2 samples or if the given domain is unbounded. |
There was a problem hiding this comment.
Shouldn't this give an error if the samples are not evenly spaced, or am I misunderstanding something?
There was a problem hiding this comment.
Actually checking this seems prohibitively expensive, but a debug_assert or something might be useful? If nothing else, a note in the docs about why this isn't checked (and in the associated error type) would be nice.
There was a problem hiding this comment.
Shouldn't this give an error if the samples are not evenly spaced, or am I misunderstanding something?
I think the Even refers just to the time axis. It's an Interval which will be cut up into samples.len() - 1 evenly sized chunks.
alice-i-cecile
added
S-Ready-For-Final-Review
Objective
Finish what we started in #14630. The Curve RFC is here.
Solution
This contains the rest of the library from my branch. The main things added here are:
Curveitselfcoressubmodule that those data structures use to encapsulate sample interpolationThe weirdest thing in here is probably
ChunkedUnevenCoreincores, which is not used by anything in the Curve library itself but which is required for efficient storage of glTF animation curves. (See #13105.) We can move it into a different PR if we want to; I don't have strong feelings either way.Testing
New tests related to resampling are included. As I write this, I realize we could use some tests in
coresitself, so I will add some on this branch before too long.