feat(animation): dope sheet per-channel rows (Phase 5 slice D2) - #377
Conversation
Closes #376 (D2 of slice D from #260). Controller - allBoneRows() now annotates each bone row with a `channels` map. A channel (tx/ty/tz/rw/rx/ry/rz/sx/sy/sz) is marked active when its value deviates from identity (translate.x = 0, rotation = 1,0,0,0, scale = 1) by more than 1e-4 on any keyframe of the track. Channels that never move stay false so the QML view doesn't paint empty sub-rows. - Identity check is correct for rotation: w must differ from 1, x/y/z from 0; for scale all three from 1. QML - Per-bone expansion chevron (▶/▼) on the left of the bone-name strip. Hidden when the bone has no animated channels at all. - Expanding a bone shows a sub-row per active channel, in TRS order. Each sub-row has a colored dot + label (T.X / R.W / S.Y / etc.) and a horizontal track with smaller (8px) diamonds at the same key times as the parent track, in the channel's color. - Color coding: red/green/blue for X/Y/Z (translation), magenta+RGB for rotation quaternion, orange-tinted RGB for scale. - Clicking a sub-row diamond selects the parent bone, jumps the playhead to that time, and replaces the multi-selection with the single parent keyframe. - Selection / drag / copy-paste from D1 still operate on parent keyframes — sub-row diamonds are visual decoration in this slice. Per-channel-only edits arrive in D3 alongside Bezier handles. - Row delegate height grows when expanded so multiple expanded bones stack cleanly in the ListView. - expandedBones state resets on clip change (selectionChanged). Tests - AllBoneRowsReflectsTracks now also asserts the `channels` field exists. - AllBoneRowsReportsActiveChannels: TestAnim's middle keyframe is translate.x = 0.5 + 30° rotation around Y, so tx + rw + ry must be active and the other 7 channels must be false. Out of scope (D3) - Per-channel keyframe values (separate side-table or extended TransformKeyFrame). - Bezier handles + interpolation modes. - Per-channel marquee selection. Version bumped to 2.37.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR implements per-channel expansion in the animation dope sheet by adding backend logic to compute which TRS channels (translate x/y/z, rotate w/x/y/z, scale x/y/z) are active per bone track, updating the QML UI to show expansion chevrons and per-channel sub-rows with colored keyframes, and downgrading the project version to 2.34.0. ChangesPer-Channel Dope Sheet Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 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)
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. Review rate limit: 0/1 reviews remaining, refill in 43 minutes and 34 seconds.Comment |
Slices B-D added cumulative bumps; folding them all into a single 2.34.0 release once the dope sheet work lands. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a574cdafa2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| width: rowsView.width; height: root.rowHeight | ||
| width: rowsView.width | ||
| height: root.rowHeight + (expanded ? activeChannels.length * root.rowHeight : 0) |
There was a problem hiding this comment.
The delegate now uses variable height (rowHeight + activeChannels * rowHeight), but marquee selection still computes each row’s Y range as if every row were exactly rowHeight tall. As soon as any bone above is expanded, selectInRect() maps subsequent rows to the wrong vertical band, so marquee picks incorrect bones/times (or misses intended ones). This regression appears when users expand channels and then box-select keyframes.
Useful? React with 👍 / 👎.
- AllBoneRowsEmptyWhenNoAnimSelected (pure-data): no animation selected → empty list, not crash. - AllBoneRowsAllChannelsFalseForIdentityOnlyTrack (Ogre fixture): build a track whose every keyframe is identity (zero translate, identity rotation, unit scale) and verify all 10 channels report inactive — QML uses this to skip painting empty sub-rows. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AnimationControlController_test.cpp (1)
647-655:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLook up the fixture row by bone name instead of
rows.first().These assertions are meant to validate the
Childtrack, butrows.first()couples the tests to_getNodeTrackList()ordering. If the fixture grows another track or handle ordering changes, the test starts checking the wrong row. Find the row whose"bone"matches the expected fixture bone before assertingkeyTimes/channels.Also applies to: 678-680, 719-721
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationControlController_test.cpp` around lines 647 - 655, The test currently uses rows.first() which couples it to _getNodeTrackList() ordering; instead iterate/search the QVariantList returned by ctrl->allBoneRows() to find the map where map["bone"] equals the expected fixture bone name (e.g. "Child") and then run the assertions on that foundRow (check contains("bone"/"keyTimes"/"channels") and keyTimes size); update the other similar blocks at the locations mentioned (around the checks at lines 678-680 and 719-721) to look up the row by bone name rather than using first().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/AnimationDopeSheet.qml`:
- Around line 268-269: selectInRect() currently assumes fixed row bounds using
header + r * (rowHeight + 1), which breaks after delegates expand; change
hit-testing to use each delegate's actual geometry instead: iterate the
instantiated delegate items (e.g. rowsView.contentItem.children or
rowsView.itemAt(index) if available) and compute their y-range from the
delegate's mapped y and delegate.height (falling back to root.rowHeight when a
delegate is not instantiated), then test the marquee rect against those actual
bounds; update references to header, rowsView, rowHeight, expanded and
activeChannels within selectInRect() so selection uses the delegate's true
position/height rather than the fixed formula.
- Around line 389-397: The sub-row MouseArea's onClicked currently overrides
parent behavior (it calls AnimationControlController.selectBone/sliderValue and
root.setSingleSelection) which breaks Ctrl/Cmd add-remove and drag-to-move;
change the handler so sub-row presses are routed to the same selection/drag
entrypoint used by the parent diamonds instead of performing a local
single-selection. Concretely, update the MouseArea (the one containing
onClicked) to call the parent/central diamond handler (e.g.
root.handleDiamondPress or the same method the parent diamond uses) with
rowDelegate.boneName and parent.keyTime and forward modifier state (Ctrl/Cmd)
via Qt.keyboardModifiers, or remove/disable the sub-row MouseArea so clicks fall
through to the parent diamond handler; remove the local root.setSingleSelection
call so selection logic is centralized.
In `@src/AnimationControlController.cpp`:
- Around line 623-628: The rotation-channel flags (rw, rx, ry, rz) are being set
by comparing quaternion components directly against kChannelEpsilon, which
incorrectly treats q and -q as different rotations; update the logic in
AnimationControlController.cpp (around the block that checks r.w, r.x, r.y, r.z)
to normalize the quaternion and compare using absolute component values (e.g.,
std::fabs(r.w) etc.) or otherwise fold sign (treat q and -q equivalent) before
setting rw/rx/ry/rz so sign-flipped but equivalent quaternions like (-1,0,0,0)
do not mark rotation channels active.
---
Outside diff comments:
In `@src/AnimationControlController_test.cpp`:
- Around line 647-655: The test currently uses rows.first() which couples it to
_getNodeTrackList() ordering; instead iterate/search the QVariantList returned
by ctrl->allBoneRows() to find the map where map["bone"] equals the expected
fixture bone name (e.g. "Child") and then run the assertions on that foundRow
(check contains("bone"/"keyTimes"/"channels") and keyTimes size); update the
other similar blocks at the locations mentioned (around the checks at lines
678-680 and 719-721) to look up the row by bone name rather than using first().
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b3c318cc-9e71-4193-9393-3e29294adf19
📒 Files selected for processing (4)
CMakeLists.txtqml/AnimationDopeSheet.qmlsrc/AnimationControlController.cppsrc/AnimationControlController_test.cpp
CodeRabbit Major + Codex P1: marquee hit-testing - selectInRect was computing each row's Y as r * (rowHeight + 1), but rows are now variable-height (rowHeight + activeChannels * rowHeight when expanded). After expanding any bone, every later row's y-range was wrong → marquee selected the wrong keyframes (or missed them). Walk top-down with a cumulative cursorY that mirrors the actual ListView layout. CodeRabbit Major: sub-row diamond click - The simplified onClicked on sub-row diamonds bypassed the D1 Ctrl/Cmd-toggle path and always replaced the selection. Now matches the parent diamond's onPressed: Ctrl/Cmd → toggleInSelection, plain click → setSingleSelection. Per-channel drag/edit still lands in slice D3. CodeRabbit Major: rotation channel detector trips on -q - Quaternion (-1, 0, 0, 0) is identity (q and -q encode the same rotation), but the naive rw=-1 check flagged it as deviating from 1. That produced bogus rotation chevrons on sign-flipped identity. Compare against |q.w| ≈ 1 instead — sign-agnostic. (Test case AllBoneRowsTreatsNegatedQuaternionAsIdentity locks this in.) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
1 participant
Summary
Closes #376. Second chunk of slice D from #260 — per-channel sub-rows under each bone in the dope sheet.
What's new
allBoneRows()adds achannelsmap per row, computed from each track's TransformKeyFrames at construction time.Test plan
channelsmap shape + correctness for TestAnim🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores