Report ActionClip conflicts instead of panicking and fix field span offset - #142
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 2 reviews per rolling hour; 0 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds tracing-based conflict reporting for sequence insertion and extension. It also fixes cumulative field offsets in track compilation and adds regression coverage for field-to-lane lookups. ChangesSequence and track corrections
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR replaces debug-only panics with error reporting and corrects field span offsets while preserving existing clips; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
nixonyh
left a comment
There was a problem hiding this comment.
A small nit, good to merge once addressed.
Sequence::pushandSequence::extendguarded against overlappingclips with a
debug_assert!. That meant an authoring mistake took thewhole app down in a debug build, and in a release build the clips were
accepted silently — the animation came out wrong with nothing to point
at why.
Conflicts are now reported through
tracing::error!and the clips arekept. Reporting a conflict is not the same as resolving one, so the
animation is still wrong; you just find out immediately, in either
profile, instead of guessing.
Two things get reported, because baking walks a lane in list order
carrying one value forward (
start = endinpipeline::bake) whileplayback runs in time order:
the two orders simply disagree, so the later-listed clip opens on a
value that has not happened yet.
is named, not just the nearest one, so a clip buried under a long
one is reported even when the clips either side of it are clear.
Adds
tracingas an optional dependency, enabled by default. With thefeature off the checks compile out entirely.
Also fixes
field_offsetinTrackFragment::compileassigning ratherthan accumulating, which made every field lookup past the second one
point at the wrong lanes —
field_offset = field_lenoverwrote therunning position with the size of the previous run. The first boundary
was accidentally correct (the offset is still zero there), so it only
shows up with three or more distinct fields.
This is latent: nothing reads
field_lookupsyet, per the TODO onTrack. The added test fails without the fix and asserts the spanspartition the lanes , every lane reachable through exactly one field,
none skipped, none repeated.