Implement Clip Overlap Resolution - #134
Conversation
Use pointer cursor on timeline. Also some minor visual changes.
Stacks on top of #122
<img width="1392" height="864" alt="image" src="https://github.com/user-attachments/assets/84a034ee-080e-4c93-91ca-3e98de40d38c" /> - Added `motiongfx_editor_ui` - Added `motiongfx_editor_ui_kernel` - Moved `motiongfx_editor` into `editor/` folder, and make it a binary instead of a lib. - Removed `editor.rs` example and move its content to `motiongfx_editor/src/main.rs` as a stub content until we have proper serialization implementation. Relates to #70 & #89
Support serialization of `motiongfx` via `motiongfx_scene`! `motiongfx_scene` is backend agnostic, which means it's not just bevy, it can work with any backend world/renderer/app you like! This PR also made double confirmation on `no_std` support for `motiongfx` & `motiongfx_scene` by compiling it using the `thumbv7em-none-eabihf` target.
Follows the builder no longer accumulating tracks itself. `Tracks` wraps a `NonEmpty<Track>`, so a timeline can't be built with zero tracks, and `compile` takes `impl Into<Tracks>` with a `From<Track>` impl so the single-track case stays `b.compile(track)`. Multiple tracks go through `Tracks(nonempty![a, b])`, with `nonempty` re-exported from `motiongfx` so callers don't depend on it directly. `CompileError::EmptyTimeline` goes with it - the type now rules that case out, leaving the variant unconstructible.
<img width="2784" height="1728" alt="image" src="https://github.com/user-attachments/assets/7939ea6c-7c12-4980-8de1-fef13aacc312" />
The goal here is to encapsulate ui elements and widgets into their own modules so private `const`s, `fn`s, `struct`s stays private without leaking into unnecessary places. This PR also deletes most of the marker components where it's become useless because of the use of `moxie_ui_kernel` bindings & watchers.
|
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:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds overlap-aware sequence merging, timeline sampling, track compilation, and pipeline baking. Clips remain ordered by start time. Later-starting clips take precedence during overlaps. Latest-finishing clips determine gap and track-end values. ChangesOverlap-Aware Clip Playback
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Sequence
participant Track
participant Timeline
participant Pipeline
Sequence->>Track: merge sorted overlapping clips
Track->>Timeline: provide compiled clip timing
Timeline->>Timeline: resolve visible clip for sample time
Pipeline->>Timeline: resolve lane state during baking
Timeline-->>Pipeline: return interpolated clip state
Pipeline-->>Pipeline: accumulate and write baked segments
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/motiongfx/src/action/table.rs (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the counter arithmetic with the documented invariant.
Line 38 documents
next_orderas monotonic.wrapping_addbreaks that after 2^32 actions: a later-authored action then receives a lowerorder, and bothresolve_overlaps(other.order > clip.order) andresolve_clip(max_by_key(|clip| clip.order)) invert their precedence. The count is unreachable in practice, so this is a robustness and documentation point only.saturating_addkeeps the invariant in the degenerate case, at the cost of ties at the ceiling.♻️ Proposed change
let order = self.next_order; - self.next_order = self.next_order.wrapping_add(1); + self.next_order = self.next_order.saturating_add(1);🤖 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/action/table.rs` around lines 84 - 85, Update the counter increment in the action-order assignment using saturating arithmetic instead of wrapping arithmetic, so next_order remains monotonic at its maximum value and preserves precedence in resolve_overlaps and resolve_clip.
🤖 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/action/table.rs`:
- Around line 84-85: Update the counter increment in the action-order assignment
using saturating arithmetic instead of wrapping arithmetic, so next_order
remains monotonic at its maximum value and preserves precedence in
resolve_overlaps and resolve_clip.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e33393d2-74f4-4b1d-bf24-e30920d3196b
📒 Files selected for processing (8)
crates/bevy_motiongfx/Cargo.tomlcrates/bevy_motiongfx/src/manager.rscrates/motiongfx/src/action.rscrates/motiongfx/src/action/table.rscrates/motiongfx/src/lib.rscrates/motiongfx/src/sequence.rscrates/motiongfx/src/timeline.rscrates/motiongfx/src/track.rs
There was a problem hiding this comment.
This is my first pass review. Generally I'm not happy with how overlaps is handled here. I would prefer the more naive and easier way to just store clips as is in the Sequence / TrackFragment / Track and only resolve it during baking (see motiongfx::pipeline::bake<..>()).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/motiongfx/src/pipeline.rs (2)
223-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOptional: the gap fast path duplicates
resolve_clip's fallback.When
max_end < clip.start, every clip inbakedhas already finished.resolve_clip(&baked, clip.start)then finds no covering clip and falls through to itsmax_by_key(clip.end())branch, which returns the same index withSampleMode::End.max_by_keykeeps the last maximum, and the>=test at line 303 keeps the last maximum too, so both paths break ties the same way today.That leaves two independent tie-break rules that must stay in sync. If you drop the fast path,
max_endandmax_idxbecome unnecessary and the precedence rule lives in one place.The behavior at a touching boundary does not change:
max_end == clip.startalready falls through toresolve_clip.♻️ Proposed simplification
- // The furthest end so far and which clip reached it. Ties go - // to the later clip, as in `resolve_clip`. - let mut max_end = Duration::ZERO; - let mut max_idx = 0; - for clip in ctx.track.clips(*span) {- // Nothing is still running, so nothing covers this - // clip. - let (i, mode) = if max_end < clip.start { - (max_idx, SampleMode::End) - } else { - resolve_clip(&baked, clip.start) - }; + // Whatever the lane shows at `clip.start`: the last + // covering clip, or the last one to finish. + let (i, mode) = resolve_clip(&baked, clip.start);- if clip.end() >= max_end { - max_end = clip.end(); - max_idx = baked.len() - 1; - } }🤖 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/pipeline.rs` around lines 223 - 248, Remove the gap fast path that checks max_end < clip.start in the clip-opening logic, and always call resolve_clip(&baked, clip.start) for non-empty baked state. Then remove the now-unused max_end and max_idx tracking and related updates, leaving tie-breaking solely in resolve_clip while preserving the existing touching-boundary behavior.
289-296: 🎯 Functional Correctness | 🔵 TrivialTODO recorded for the missing-interpolation fallback.
An action with no interpolation opens the next clip on
segment.end, a value the lane never displayed. The rest of the block is correct: the easedtis applied before interpolation, and the value is read from the localsegmentsbuffer rather than the table.Do you want me to open an issue to track this fallback?
🤖 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/pipeline.rs` around lines 289 - 296, Replace the `None => segment.end.clone()` fallback in the interpolation logic with behavior that uses the value actually displayed by the lane for actions without interpolation, rather than advancing to `segment.end`. Preserve the existing eased-`t` interpolation and local `segments` buffer usage for interpolated actions.
🤖 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/pipeline.rs`:
- Around line 223-248: Remove the gap fast path that checks max_end < clip.start
in the clip-opening logic, and always call resolve_clip(&baked, clip.start) for
non-empty baked state. Then remove the now-unused max_end and max_idx tracking
and related updates, leaving tie-breaking solely in resolve_clip while
preserving the existing touching-boundary behavior.
- Around line 289-296: Replace the `None => segment.end.clone()` fallback in the
interpolation logic with behavior that uses the value actually displayed by the
lane for actions without interpolation, rather than advancing to `segment.end`.
Preserve the existing eased-`t` interpolation and local `segments` buffer usage
for interpolated actions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 49b3bdaa-49e2-4f44-8a66-65e1fd80af96
📒 Files selected for processing (4)
crates/motiongfx/src/pipeline.rscrates/motiongfx/src/sequence.rscrates/motiongfx/src/timeline.rscrates/motiongfx/src/track.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/motiongfx/src/sequence.rs
nixonyh
left a comment
nixonyh
left a comment
There was a problem hiding this comment.
In general, I dun think we should be sorting the clips. And some nits.
| /// Clips **may overlap**. Where they do, the one later in the list | ||
| /// wins. The list is sorted by start, so that is whichever began | ||
| /// most recently, not whichever was authored last. Clips it covers | ||
| /// show again when it ends, each at its own progress. |
There was a problem hiding this comment.
I dun think we're sorting anything, but we have a guarantee that the [ActionClip::start] will be in order.
3 participants
Overlapping clips no longer panic or corrupt
Two animations driving the same field of the same subject at the same time
used to panic in debug and silently corrupt the lane in release. This PR
makes that case defined behaviour: both clips are kept, and at any instant the
one later in its lane is the one on screen.
Why it broke
Sequencerefused any clip that did not begin after the previous one ended.That rejected genuine overlaps — and also rejected clips that merely arrived
out of order, which
all/any/flowproduce whenever a fragment listedfirst is delayed past one listed later. Clips seconds apart could panic purely
on listing order.
Underneath,
bakechained values: each clip opened on the previous clip's end.That assumption is what made the rejection necessary in the first place.
What changed
Resolution moves to where it is observed. A new
resolve_clipanswers"which clip is on screen at this time" for both baking and playback: the last
clip in the lane amon
last.
Baking opens on what is visible. Each clip now starts from the value the
lane is showing at
clip.start, rather than from the previous clip's end. Anoverlapping clip takes over seamlessly instead of teleporting.
Failures log instead of panicking. Both
debug_asserts are removed.Sequenceemitserror!when a clip starts before the one ahead of it, oroverlaps one already present — at build time, so the author hears about it
while writing the animation. Adds an optional
tracingdependency, on bydefault. The overlap test is strict on both ends, so
chainstays quiet.Track::duration now maxes over every clip rather than each lane's last with clips stored as listed, the last-stored clip isn't always the last to finish, so a lane could clamp the playhead before its own animation ended. (require review at (
TrackFragment::compile, crates/motiongfx/src/track.rs:223) )Other fix.
compile()hadfield_offset = field_lenwhere it meant+=, so field lookups pointed at the wrong lanes from the third distinctfield onward.
Design decision: precedence
Where clips overlap, position in the lane decides — the clip listed later
wins while it covers the playhead. Listing order is what the author writes;
start times are derived by the combinators.
Compatibility
No animation that worked before changes behaviour.
mainrejectedoverlapping lanes outright, so every lane it could represent is sequential —
and on a sequential lane the new resolution returns exactly what
main'sbinary_search_byreturned, for clips, gaps, boundaries and the pre-lanevalue alike. The new rule only decides territory
maincould not express.Known limitations
resumes at its own curve position rather than continuing from what was on
screen. A clip has a single segment, so it cannot be reopened part way
through; fixing it means splitting clips at coverage boundaries — a data
model change beyond this PR.
never displayed (
TODOinpipeline.rs).