Add peniko (2d graphics) animation support#103
Draft
nixonyh wants to merge 13 commits intonixon/lazy-registrationfrom
Draft
Add peniko (2d graphics) animation support#103nixonyh wants to merge 13 commits intonixon/lazy-registrationfrom
peniko (2d graphics) animation support#103nixonyh wants to merge 13 commits intonixon/lazy-registrationfrom
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/motiongfx/src/timeline.rs (1)
438-472:⚠️ Potential issue | 🔴 CriticalAdd the missing
'staticbound to bothactandact_stepmethods.Both methods call
ActionBuilder::with_interp, which requiresT: 'static(defined atcrates/motiongfx/src/action.rs:380). The current signatures omit this bound and will fail to compile.🔧 Suggested fix
pub fn act<I, S, T, M>( &mut self, target: I, field_acc: FieldAccessor<S, T>, action: impl Action<T>, ) -> InterpActionBuilder<'_, T> where W: SubjectSource<M, I, S> + 'static, I: SubjectId, S: 'static, - T: Interpolation<M> + Clone + ThreadSafe, + T: Interpolation<M> + Clone + ThreadSafe + 'static, @@ action: impl Action<T>, ) -> InterpActionBuilder<'_, T> where W: SubjectSource<M, I, S> + 'static, I: SubjectId, S: 'static, - T: Clone + ThreadSafe, + T: Clone + ThreadSafe + 'static,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/motiongfx/src/timeline.rs` around lines 438 - 472, The act and act_step methods call ActionBuilder::with_interp which requires the interpolated type T to be 'static, so add the missing T: 'static bound to both function where-clauses (the act(...) and act_step(...) methods) so their generics include T: 'static alongside the existing bounds; ensure the signatures for act and act_step mirror the required trait bounds used by ActionBuilder::with_interp.
🧹 Nitpick comments (1)
crates/motiongfx/src/interpolation.rs (1)
1-8: ⚡ Quick winClarify the
tcontract in docs.At Line 7, please document whether
tis expected in[0.0, 1.0]or whether extrapolation is intentionally supported. That will prevent divergent downstream implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/motiongfx/src/interpolation.rs` around lines 1 - 8, Document the contract for the interpolation parameter `t` on the Interpolation trait: update the doc comment for trait Interpolation and the interp method (Interpolation::interp) to state explicitly whether `t` is clamped to [0.0, 1.0] or if extrapolation outside that range is allowed, and if clamped describe who is responsible for clamping (caller or implementor); include expected behavior for edge cases (e.g., t == 0.0 returns a, t == 1.0 returns b) so downstream implementors have a single authoritative spec to follow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/kurbo_motiongfx/src/trace.rs`:
- Around line 24-26: The public function trace_bez_path currently treats
multi-subpath BezPath as a single stroke; fix by either preserving subpaths or
enforcing single-subpath input: implement detection of MoveTo elements and split
the input BezPath into subpaths, call the existing single-subpath tracing logic
for each subpath, and append results to a new output BezPath preserving MoveTo
boundaries (ensuring you handle closing/First/ClosePath logic), or change
trace_bez_path to return a Result/Option or panic when more than one subpath is
detected and document the behavior; locate and update trace_bez_path and any
helpers that operate on path elements (see comments around lines 44–55) to
perform the split-or-validate step and then aggregate per-subpath traces into
the final BezPath (or return an error) accordingly.
- Around line 7-17: The trace_cubic_bez and trace_quad_bez helpers currently
take an unbounded t and forward it to curve.subsegment, causing inconsistent
behavior with trace_bez_path which clamps t; update both trace_cubic_bez and
trace_quad_bez to clamp t into the [0.0, 1.0] range (e.g. min(max(t, 0.0), 1.0))
before converting to f64 and calling curve.subsegment so their prefix semantics
match trace_bez_path.
---
Outside diff comments:
In `@crates/motiongfx/src/timeline.rs`:
- Around line 438-472: The act and act_step methods call
ActionBuilder::with_interp which requires the interpolated type T to be 'static,
so add the missing T: 'static bound to both function where-clauses (the act(...)
and act_step(...) methods) so their generics include T: 'static alongside the
existing bounds; ensure the signatures for act and act_step mirror the required
trait bounds used by ActionBuilder::with_interp.
---
Nitpick comments:
In `@crates/motiongfx/src/interpolation.rs`:
- Around line 1-8: Document the contract for the interpolation parameter `t` on
the Interpolation trait: update the doc comment for trait Interpolation and the
interp method (Interpolation::interp) to state explicitly whether `t` is clamped
to [0.0, 1.0] or if extrapolation outside that range is allowed, and if clamped
describe who is responsible for clamping (caller or implementor); include
expected behavior for edge cases (e.g., t == 0.0 returns a, t == 1.0 returns b)
so downstream implementors have a single authoritative spec to follow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2bd8cfa6-0cff-45be-9f85-eca05403b7c1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.tomlcrates/bevy_motiongfx/Cargo.tomlcrates/bevy_motiongfx/src/interpolation.rscrates/bevy_motiongfx/src/lib.rscrates/kurbo_motiongfx/Cargo.tomlcrates/kurbo_motiongfx/src/interpolation.rscrates/kurbo_motiongfx/src/lib.rscrates/kurbo_motiongfx/src/trace.rscrates/motiongfx/Cargo.tomlcrates/motiongfx/docs/world.rscrates/motiongfx/examples/custom_world.rscrates/motiongfx/src/interpolation.rscrates/motiongfx/src/lib.rscrates/motiongfx/src/timeline.rsexamples/bevy_examples/examples/custom_ease.rsexamples/bevy_examples/examples/custom_interp.rsexamples/bevy_examples/examples/easings.rsexamples/bevy_examples/examples/hello_world.rsexamples/bevy_examples/examples/minimal.rsexamples/bevy_examples/examples/recording.rsexamples/bevy_examples/examples/slide_basic.rs
💤 Files with no reviewable changes (1)
- crates/bevy_motiongfx/src/lib.rs
Comment on lines
+7
to
+17
| pub fn trace_cubic_bez(curve: &CubicBez, t: f32) -> CubicBez { | ||
| let t = t as f64; | ||
| curve.subsegment(0.0..t) | ||
| } | ||
|
|
||
| /// Returns the prefix of a [`QuadBez`] traced from start to `t`. | ||
| #[inline] | ||
| pub fn trace_quad_bez(curve: &QuadBez, t: f32) -> QuadBez { | ||
| let t = t as f64; | ||
| curve.subsegment(0.0..t) | ||
| } |
There was a problem hiding this comment.
Clamp t in curve helpers for API consistency.
At Line 7 and Line 14, these functions accept raw t while trace_bez_path (Line 27/30) clamps by behavior. Align them to avoid inconsistent prefix semantics across tracing APIs.
Suggested patch
pub fn trace_cubic_bez(curve: &CubicBez, t: f32) -> CubicBez {
- let t = t as f64;
+ let t = t.clamp(0.0, 1.0) as f64;
curve.subsegment(0.0..t)
}
@@
pub fn trace_quad_bez(curve: &QuadBez, t: f32) -> QuadBez {
- let t = t as f64;
+ let t = t.clamp(0.0, 1.0) as f64;
curve.subsegment(0.0..t)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/kurbo_motiongfx/src/trace.rs` around lines 7 - 17, The trace_cubic_bez
and trace_quad_bez helpers currently take an unbounded t and forward it to
curve.subsegment, causing inconsistent behavior with trace_bez_path which clamps
t; update both trace_cubic_bez and trace_quad_bez to clamp t into the [0.0, 1.0]
range (e.g. min(max(t, 0.0), 1.0)) before converting to f64 and calling
curve.subsegment so their prefix semantics match trace_bez_path.
- Rename interpolation module to interp - Add CubicBez and QuadBez interpolation - Add trace module with trace_bez_path, trace_cubic_bez, trace_quad_bez - Remove prelude in favour of direct module imports
Make `act` first class for creating animation and use `act_builder` for custom interpolation method.
nixonyh
force-pushed
the
nixon/kurbo-support
branch
from
535f251 to
3eacd70
Compare
nixonyh
changed the title
nixonyh
changed the title
peniko (2d graphics) animation support
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
1 participant
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.
Stack on top of #100
peniko_motiongfxcrate that covers common interpolation & tracing implementationsInterpolation<M>trait intomotiongfxand add a marker type to workaround the orphan rule.Tracer<T>type inpeniko_motiongfxto handle tracing operations.peniko, rendered viavello.