Switch from f32 to Duration to measure time#122
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR migrates motiongfx timing from ChangesDuration timing core
Repository hygiene
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PlayerController
participant Timeline
participant ActionClip
PlayerController->>Timeline: advance playback or set frame timestamp
Timeline->>ActionClip: calculate progress at target Duration
ActionClip-->>Timeline: return normalized progress
Timeline-->>PlayerController: expose updated current and target time
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/track.rs (1)
139-149: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
delay()leavesTrackFragment::durationstale after shifting clips.Every sequence's clips are shifted by
delay, buttrack.durationis never updated to reflect the new (larger) span. The fragment's own test proves this: afterdelay(1.5, track)on a 2000ms clip,seq_a.end() == ms(3500)buttrack.durationis asserted to remainms(2000).
compile()'smax(seq.end(), self.duration)reconciliation (Lines 222-227) masks this fordelay(...).compile(), but if the delayed fragment is combined further viaord_chain/ord_flow/etc. beforecompile(), downstream combinators that readtrack.duration(asflow()does withother_track.duration, Line 125) will place subsequent clips using the understated span — reproducing the exactduration/clip-offset divergence the code comment abovecompile()and thechain_accumulation_matches_clip_offsetsregression test warn about.🐛 Proposed fix — mirror flow()'s duration accumulation
pub fn delay( delay: impl IntoDuration, mut track: TrackFragment, ) -> TrackFragment { let delay = delay.into_duration(); for sequence in track.sequences.values_mut() { sequence.delay(delay); } + track.duration += delay; track }Also update the
delay_applies_offsettest's expectation fromms(2000)toms(3500).🤖 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/track.rs` around lines 139 - 149, Update delay() to increase track.duration by the applied delay after shifting every sequence, matching flow()'s duration accumulation and keeping the fragment span consistent before compile(). Update the delay_applies_offset test to expect ms(3500) instead of ms(2000).
🤖 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/motiongfx/src/sequence.rs`:
- Around line 38-42: Update Sequence::duration to use a non-panicking duration
subtraction, such as saturating subtraction, so it returns zero when end()
precedes start(). Keep the existing duration calculation unchanged for valid
sequences and retain the current start/end accessors.
---
Outside diff comments:
In `@crates/motiongfx/src/track.rs`:
- Around line 139-149: Update delay() to increase track.duration by the applied
delay after shifting every sequence, matching flow()'s duration accumulation and
keeping the fragment span consistent before compile(). Update the
delay_applies_offset test to expect ms(3500) instead of ms(2000).
🪄 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: 547cce94-70a2-401a-ae3a-167a44e83d60
📒 Files selected for processing (16)
.gitignorecrates/bevy_motiongfx/src/controller.rscrates/motiongfx/benches/action_storage.rscrates/motiongfx/examples/custom_world.rscrates/motiongfx/src/action.rscrates/motiongfx/src/action/table.rscrates/motiongfx/src/lib.rscrates/motiongfx/src/pipeline.rscrates/motiongfx/src/sequence.rscrates/motiongfx/src/time.rscrates/motiongfx/src/timeline.rscrates/motiongfx/src/track.rscrates/motiongfx_editor/src/lib.rsexamples/bevy_examples/examples/slide_basic.rsexamples/bevy_examples/src/lib.rsexamples/vello_winit_example/examples/lissajous.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/bevy_motiongfx/src/controller.rs (1)
216-225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRestore the fluent API returns for
PassivePlayersetters.To maintain consistency with other components (like
FixedRatePlayer::set_playing) and prevent breaking downstream builder-pattern usages, consider restoring the fluent return (-> &mut Self) for these setter methods.♻️ Proposed refactor
#[inline] - pub fn set_time(&mut self, time: impl IntoDuration) { + pub fn set_time(&mut self, time: impl IntoDuration) -> &mut Self { self.time = time.into_duration(); + self } #[inline] - pub fn set_track_index(&mut self, track_index: usize) { + pub fn set_track_index(&mut self, track_index: usize) -> &mut Self { self.track_index = track_index; + self }🤖 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/controller.rs` around lines 216 - 225, Update PassivePlayer::set_time and PassivePlayer::set_track_index to return &mut Self after assigning their respective fields, preserving the existing setter behavior while restoring fluent builder-pattern chaining.
🤖 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/bevy_motiongfx/src/controller.rs`:
- Around line 216-225: Update PassivePlayer::set_time and
PassivePlayer::set_track_index to return &mut Self after assigning their
respective fields, preserving the existing setter behavior while restoring
fluent builder-pattern chaining.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d360a20d-6ea1-4ef9-aab0-e6aec535687e
📒 Files selected for processing (1)
crates/bevy_motiongfx/src/controller.rs
Jaghov
left a comment
Jaghov
left a comment
There was a problem hiding this comment.
Looks good! just don't see much of a point in the new time module. It could be heavily stripped down if we decide to keep it. Like replacing s() with the impl for IntoDuration for f64 and getting rid of IntoDuration entirely.
There was a problem hiding this comment.
Not convinced this addition is necessary. I'd rather just have, the api accept a core::time::Duration as an input in the public facing api's or convert from f64 to f32 at the few surfaces users interact with time.
At most, I think the shorthand functions for time could be helpful., but maybe just one shorthand for duration from secs f64.
There was a problem hiding this comment.
Hmm, the reason we migrate to Duration is to allow more precise timings. And that's by supporting integer as timings.
Using Duration as the param is fine, but it quickly becomes a hassle to write Duration::new() or Duration::from_secs etc. again and again.
I agree with f32 to f64 is unnecessary I'll fix it with Duration::from_secs_f32.
There was a problem hiding this comment.
I'll have a separate PR up for converting our existing examples into using cs, s, and ms more. ns is not really useful in this case, but I'm just keeping it for the sake of completeness.
2 participants
Resolves #115