Velyst integration - #110
Conversation
This is achieved through `KanvaPhase`s that takes in pure functions that mutates `Kanva` via modifiers
- `MoveTo` should not be rendered
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Velyst animation integration behind a new ChangesVelyst Integration and Supporting Changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BevyApp
participant Velyst
participant MotionGfx
participant Typst
BevyApp->>Velyst: initialize Velyst and load plot scene
BevyApp->>MotionGfx: create Kanva groups and timeline
MotionGfx->>Velyst: update Kanva path animations
Typst->>Velyst: render plot from circle coordinates
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note: this should just be a feature of |
1ff824b to
2e19a2e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/peniko_motiongfx/src/trace.rs (1)
101-129: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftThis still loses
MoveToboundaries when subpaths share an endpoint.The new heuristic only emits
move_towhensub.start()differs fromlast_end. That misses legitimate boundaries where a fresh subpath starts exactly at the previous endpoint—for example after a closed contour, or two distinct subpaths that intentionally meet at one point. In those cases the traced result collapses two subpaths into one joined path, which changes stroke joins/caps and can render incorrectly. Preserving boundaries here needs subpath-aware iteration rather than reconstructing them from point equality after callingsegments().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/peniko_motiongfx/src/trace.rs` around lines 101 - 129, The subpath boundary detection in trace logic is still collapsing distinct subpaths when they share the same endpoint. Update the tracing loop in the segment-processing code to preserve `MoveTo` boundaries using subpath-aware iteration instead of inferring boundaries from `last_end` and `sub.start()` equality after `BezPath::segments()`. Keep the existing `move_to`/`push` flow in the tracing routine, but ensure a new subpath is emitted whenever the original path had one, even if the start point matches the previous segment’s end.
🧹 Nitpick comments (2)
crates/motiongfx/src/interpolation.rs (1)
44-44: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrecision loss for large
i64/u64/usizevalues.Lerping via
f64loses precision once magnitudes exceed ~2^53, so interpolating large integer values (e.g. large IDs/counters) could produce slightly incorrect results. Likely fine for typical animated counts, but worth keeping in mind if this macro is ever used for large-magnitude fields.Also applies to: 50-54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/motiongfx/src/interpolation.rs` at line 44, The integer interpolation in the lerp macro currently converts through f64, which can lose precision for large i64/u64/usize values. Update the interpolation logic in the macro used by the Integer impls to avoid floating-point rounding for large magnitudes, or constrain/document that these types are only safe for smaller ranges; make sure the fix is applied consistently across the affected integer implementations in interpolation.rs.Cargo.toml (1)
29-29: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin these git dependencies to a commit.
Using a moving branch here makes fresh resolution non-reproducible and can break the workspace without any change in this repo. Prefer a fixed
revonce you've validated the Bevy 0.19 fork state.Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` at line 29, The git dependencies are currently using a moving branch instead of a fixed commit, which makes resolution non-reproducible. Update the affected entries in Cargo.toml, including the bevy_vello dependency and the other referenced git dependency, to use a pinned rev after validating the Bevy 0.19 fork state. Keep the existing dependency settings, but replace branch-based sourcing with an immutable commit reference so builds remain stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/bevy_motiongfx/README.md`:
- Around line 132-149: The Velyst README example uses a non-existent plugin name
and won’t compile. Update the example to use the actual public plugin exported
from the crate, `BevyMotionGfxPlugin`, and keep the Velyst feature description
aligned with how `lib.rs` wires `VelystIntegrationPlugin` internally. Fix both
the import and the `add_plugins` call in the example so they reference the real
plugin symbol.
In `@crates/bevy_motiongfx/src/velyst_integration.rs`:
- Around line 322-334: Validate `anim.path_window` in the path timing logic in
`velyst_integration.rs` before computing `stagger` and `local_t` inside the loop
over `range.enumerate()`. The issue is that `anim.path_window` can be zero or
greater than one, which causes invalid division and can also make the stagger
negative; fix this by clamping or rejecting values outside `(0.0, 1.0]` before
the `stagger` calculation and the `local_t` computation, using the existing
`anim.path_window`, `stagger`, and `local_t` symbols to locate the affected
code.
In `@crates/peniko_motiongfx/README.md`:
- Around line 53-54: The README license links used by the crate docs are
relative paths that only work on GitHub, so update the license references in the
peniko_motiongfx README to absolute URLs. Keep the existing license section in
sync with the docs pulled in by the crate’s lib.rs, and replace the LICENSE-MIT
and LICENSE-APACHE targets with full web URLs so they render correctly on
docs.rs and crates.io as well.
In `@examples/bevy_examples/examples/velyst_demo.rs`:
- Line 126: Guard the FPS calculation in velyst_demo so it does not divide by
zero when time.delta_secs_f64() returns 0.0 on the first frame. Update the logic
around the FPS computation in the relevant update/render path to handle a zero
delta safely, e.g. by skipping the calculation or using a fallback value before
formatting the displayed text. Use the existing time.delta_secs_f64() usage as
the locator and keep the FPS text update behavior unchanged for normal nonzero
frame times.
---
Outside diff comments:
In `@crates/peniko_motiongfx/src/trace.rs`:
- Around line 101-129: The subpath boundary detection in trace logic is still
collapsing distinct subpaths when they share the same endpoint. Update the
tracing loop in the segment-processing code to preserve `MoveTo` boundaries
using subpath-aware iteration instead of inferring boundaries from `last_end`
and `sub.start()` equality after `BezPath::segments()`. Keep the existing
`move_to`/`push` flow in the tracing routine, but ensure a new subpath is
emitted whenever the original path had one, even if the start point matches the
previous segment’s end.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 29: The git dependencies are currently using a moving branch instead of a
fixed commit, which makes resolution non-reproducible. Update the affected
entries in Cargo.toml, including the bevy_vello dependency and the other
referenced git dependency, to use a pinned rev after validating the Bevy 0.19
fork state. Keep the existing dependency settings, but replace branch-based
sourcing with an immutable commit reference so builds remain stable.
In `@crates/motiongfx/src/interpolation.rs`:
- Line 44: The integer interpolation in the lerp macro currently converts
through f64, which can lose precision for large i64/u64/usize values. Update the
interpolation logic in the macro used by the Integer impls to avoid
floating-point rounding for large magnitudes, or constrain/document that these
types are only safe for smaller ranges; make sure the fix is applied
consistently across the affected integer implementations in interpolation.rs.
🪄 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: 60d23754-2972-47da-a2a2-1c5e41a3fa28
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.gitignoreCargo.tomlcrates/bevy_motiongfx/Cargo.tomlcrates/bevy_motiongfx/README.mdcrates/bevy_motiongfx/src/lib.rscrates/bevy_motiongfx/src/velyst_integration.rscrates/motiongfx/src/interpolation.rscrates/motiongfx/src/track.rscrates/peniko_motiongfx/README.mdcrates/peniko_motiongfx/src/lib.rscrates/peniko_motiongfx/src/trace.rsexamples/bevy_examples/Cargo.tomlexamples/bevy_examples/assets/typst/Monokai Pro.tmThemeexamples/bevy_examples/assets/typst/monokai_pro.typexamples/bevy_examples/assets/typst/velyst_demo.typexamples/bevy_examples/examples/recording.rsexamples/bevy_examples/examples/velyst_demo.rs
| let n = range.len(); | ||
| let stagger = if n > 1 { | ||
| (1.0 - anim.path_window) / (n - 1) as f32 | ||
| } else { | ||
| 0.0 | ||
| }; | ||
|
|
||
| for (i, path_idx) in range.enumerate() { | ||
| // Clamp to [0, 1] so completed paths always hold their final | ||
| // state rather than reverting to the raw Typst fill. | ||
| let local_t = ((anim.t - i as f32 * stagger) | ||
| / anim.path_window) | ||
| .clamp(0.0, 1.0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate path_window before dividing by it.
Line 332 divides by anim.path_window directly. 0.0 yields NaN/inf, and values above 1.0 make Line 324 produce a negative stagger, so later paths can start before earlier ones. Clamp or reject values outside (0.0, 1.0] before computing stagger and local_t.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/bevy_motiongfx/src/velyst_integration.rs` around lines 322 - 334,
Validate `anim.path_window` in the path timing logic in `velyst_integration.rs`
before computing `stagger` and `local_t` inside the loop over
`range.enumerate()`. The issue is that `anim.path_window` can be zero or greater
than one, which causes invalid division and can also make the stagger negative;
fix this by clamping or rejecting values outside `(0.0, 1.0]` before the
`stagger` calculation and the `local_t` computation, using the existing
`anim.path_window`, `stagger`, and `local_t` symbols to locate the affected
code.
| - MIT License ([LICENSE-MIT](/LICENSE-MIT) or [http://opensource.org/licenses/MIT](http://opensource.org/licenses/MIT)) | ||
| - Apache License, Version 2.0 ([LICENSE-APACHE](/LICENSE-APACHE) or [http://www.apache.org/licenses/LICENSE-2.0](http://www.apache.org/licenses/LICENSE-2.0)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use absolute license URLs here.
crates/peniko_motiongfx/src/lib.rs Line 1 pulls this README into the crate docs, so /LICENSE-MIT and /LICENSE-APACHE only resolve on GitHub and break on docs.rs/crates.io. Please switch these to absolute URLs so the links stay valid in every rendered context.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/peniko_motiongfx/README.md` around lines 53 - 54, The README license
links used by the crate docs are relative paths that only work on GitHub, so
update the license references in the peniko_motiongfx README to absolute URLs.
Keep the existing license section in sync with the docs pulled in by the crate’s
lib.rs, and replace the LICENSE-MIT and LICENSE-APACHE targets with full web
URLs so they render correctly on docs.rs and crates.io as well.
| diag: Res<DiagnosticsStore>, | ||
| ) { | ||
| let Ok(mut text) = q.single_mut() else { return }; | ||
| let fps = (1.0 / time.delta_secs_f64() * 100.0).round() / 100.0; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against zero delta time in FPS calculation.
time.delta_secs_f64() can be 0.0 on the first frame, producing inf in the displayed FPS text.
🩹 Proposed fix
- let fps = (1.0 / time.delta_secs_f64() * 100.0).round() / 100.0;
+ let delta = time.delta_secs_f64();
+ let fps = if delta > 0.0 { (1.0 / delta * 100.0).round() / 100.0 } else { 0.0 };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let fps = (1.0 / time.delta_secs_f64() * 100.0).round() / 100.0; | |
| let delta = time.delta_secs_f64(); | |
| let fps = if delta > 0.0 { (1.0 / delta * 100.0).round() / 100.0 } else { 0.0 }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/bevy_examples/examples/velyst_demo.rs` at line 126, Guard the FPS
calculation in velyst_demo so it does not divide by zero when
time.delta_secs_f64() returns 0.0 on the first frame. Update the logic around
the FPS computation in the relevant update/render path to handle a zero delta
safely, e.g. by skipping the calculation or using a fallback value before
formatting the displayed text. Use the existing time.delta_secs_f64() usage as
the locator and keep the FPS text update behavior unchanged for normal nonzero
frame times.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/motiongfx/src/interpolation.rs (1)
36-40: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valuePrecision loss for large
i64/u64/usizevalues viaf64round-trip.
f64can only exactly represent integers up to 2^53; convertingi64/u64/usizevalues above that throughf64for lerp/round can silently produce imprecise results. Likely a non-issue for animation-scale values (e.g., path indices, counters), but worth a doc note if these impls are ever used with large magnitudes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/motiongfx/src/interpolation.rs` around lines 36 - 40, The `interp` implementation for the integer `Interpolatable` impls in `interpolation.rs` converts values through `f64`, which can lose precision for large `i64`/`u64`/`usize` inputs. Add a short documentation note on the affected `Interpolatable` impls or the `interp` macro expansion explaining that interpolation is only exact up to `f64` integer precision and may be imprecise for very large magnitudes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/motiongfx/src/interpolation.rs`:
- Around line 36-40: The `interp` implementation for the integer
`Interpolatable` impls in `interpolation.rs` converts values through `f64`,
which can lose precision for large `i64`/`u64`/`usize` inputs. Add a short
documentation note on the affected `Interpolatable` impls or the `interp` macro
expansion explaining that interpolation is only exact up to `f64` integer
precision and may be imprecise for very large magnitudes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ce6c067a-23b3-45b7-9ed5-28312ed28f2f
📒 Files selected for processing (2)
crates/bevy_motiongfx/README.mdcrates/motiongfx/src/interpolation.rs
✅ Files skipped from review due to trivial changes (1)
- crates/bevy_motiongfx/README.md
Use pointer cursor on timeline. Also some minor visual changes.
|
This is mostly feature complete, just waiting on: |
2 participants
Closes #75
Closes #45