A Curve trait for general interoperation — Part I#14630
Merged
alice-i-cecile merged 12 commits intobevyengine:mainfrom Aug 9, 2024
Merged
A Curve trait for general interoperation — Part I#14630alice-i-cecile merged 12 commits intobevyengine:mainfrom
alice-i-cecile merged 12 commits intobevyengine:mainfrom
Conversation
mweatherley
added
A-Math
mweatherley
force-pushed
the
curve-trait-part-1
branch
from
5cd06ca to
2594516
Compare
mweatherley
force-pushed
the
curve-trait-part-1
branch
from
2594516 to
48a5ecc
Compare
15 tasks
DasLixou
reviewed
Aug 5, 2024
alice-i-cecile
added
the
S-Needs-Review
|
|
||
| /// Get the start of this interval. | ||
| #[inline] | ||
| pub fn start(self) -> f32 { |
Member
There was a problem hiding this comment.
I think I prefer left/right naming for these fields, which would make these consistent with the other methods.
Contributor
Author
There was a problem hiding this comment.
It should at least consistently use the terminology of start/end everywhere now. Hopefully that's a little better at least. (This is kind of a choice between metaphors, since start and end evoke time, while left and right are more like geometry on the real line)
alice-i-cecile
added
S-Waiting-on-Author
IQuick143
reviewed
Aug 6, 2024
IQuick143
reviewed
Aug 6, 2024
IQuick143
approved these changes
Aug 6, 2024
soqb
reviewed
Aug 6, 2024
Contributor
soqb
left a comment
soqb
left a comment
There was a problem hiding this comment.
it's been a long time since i last read the RFC, so take this as a layman's review. i think this is generally really solid!
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
eckz
reviewed
Aug 7, 2024
Contributor
Author
|
I think with the most recent round of changes, I've addressed the existing comments to my satisfaction. Of course, more are always welcome. :) |
mweatherley
removed
the
S-Waiting-on-Author
alice-i-cecile
approved these changes
Aug 8, 2024
alice-i-cecile
added
the
S-Ready-For-Final-Review
soqb
approved these changes
Aug 9, 2024
github-merge-queue bot
pushed a commit
that referenced
this pull request
Aug 15, 2024
# Objective Finish what we started in #14630. The Curve RFC is [here](https://github.com/bevyengine/rfcs/blob/main/rfcs/80-curve-trait.md). ## Solution This contains the rest of the library from my branch. The main things added here are: - Bulk sampling / resampling methods on `Curve` itself - Data structures supporting the above - The `cores` submodule that those data structures use to encapsulate sample interpolation The weirdest thing in here is probably `ChunkedUnevenCore` in `cores`, 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 `cores` itself, so I will add some on this branch before too long. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Robert Walter <26892280+RobWalt@users.noreply.github.com>
Member
|
Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1669 if you'd like to help out. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
This PR implements part of the Curve RFC. See that document for motivation, objectives, etc.
Solution
For purposes of reviewability, this PR excludes the entire part of the RFC related to taking multiple samples, resampling, and interpolation generally. (This means the entire
coressubmodule is also excluded.) On the other hand, the entireIntervaltype and all of the functionalCurveadaptors are included.Testing
Test modules are included and can be run locally (but they are also included in CI).