feat(animation): multi-bone dope sheet (Phase 5 slice C) - #373
Merged
Conversation
Closes #372 (slice C of #260). - New AnimationDopeSheet.qml — one row per animated bone, diamond markers per keyframe, click-and-drag a marker to move its time. Wheel zooms around the cursor (20–2000 px/s); middle-drag pans. Reuses the existing yellow-diamond / red-selected styling. - New AnimationControlController APIs: - allBoneRows() — { bone, keyTimes[] } per animated bone - moveKeyframe(bone, oldT, newT) — pushes a MoveKeyframeCommand - boneRowsChanged() signal driven from refreshSliderTicks() - New commands/MoveKeyframeCommand — find keyframe at oldTime, capture TRS, remove + recreate at newTime, restore TRS. 1 ms match tolerance. Refuses moves that would collide with another keyframe on the same track. Undo/redo round-trips cleanly. - New QDockWidget at the bottom of the main window hosts the QML view. Toggleable via the View menu (uses the dock's built-in toggleViewAction so we don't have to edit mainwindow.ui). Visibility changes emit "ui.action" Sentry breadcrumbs. - Shares state with the existing single-track Animation Control panel — selecting a bone or a keyframe in either view updates the other. Tests: - AnimationControlControllerPlaybackTest: pure-data API safety (allBoneRows empty, moveKeyframe no-op without selection). - AnimationControlControllerTest: rows reflect tracks; move shifts time; collision rejected. - MoveKeyframeCommandTest: redo moves, undo restores, TRS preserved across move, missing search-time is a no-op. Out of scope (deferred to slice D / curve editor): - Multi-select rectangle + bulk move/copy/paste - Per-channel rows (T.X / R.W etc.) - Bezier handles Version bumped to 2.35.0. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds a multi-bone dope-sheet: a QML timeline UI ( ChangesDope Sheet Timeline Feature
Sequence Diagram(s)sequenceDiagram
actor User
participant QML as AnimationDopeSheet.qml
participant Controller as AnimationControlController
participant UndoMgr as UndoManager
participant Command as MoveKeyframeCommand
participant Ogre as Ogre Animation
User->>QML: Drag keyframe diamond
QML->>Controller: moveKeyframe(bone, oldTime, newTime)
Controller->>UndoMgr: Push MoveKeyframeCommand(...)
UndoMgr->>Command: redo()
Command->>Ogre: Resolve track, find keyframe at oldTime
Command->>Ogre: Check for collision at newTime
alt no collision
Command->>Ogre: Remove old keyframe, insert new at newTime
Command->>Ogre: _keyFrameDataChanged()
Command-->>UndoMgr: complete
Controller->>Controller: refreshSliderTicks()
Controller->>QML: boneRowsChanged()
QML->>QML: Re-query allBoneRows(), redraw diamonds
else collision
Command-->>UndoMgr: abort/return false
Controller->>QML: (no change)
end
sequenceDiagram
actor User
participant Main as MainWindow
participant QML as AnimationDopeSheet.qml
participant Controller as AnimationControlController
participant Ogre as Ogre Skeleton/Animation
User->>Main: Toggle "Dope Sheet" view
Main->>QML: QQuickWidget loads AnimationDopeSheet.qml
QML->>Controller: Connect boneRowsChanged()
User->>Main: Load rigged mesh + animation
Main->>Controller: Select animation
Controller->>Ogre: Query NodeAnimationTracks
Controller->>QML: boneRowsChanged()
QML->>Controller: allBoneRows()
Controller-->>QML: [{bone, keyTimes}, ...]
QML->>User: Render ListView rows and diamonds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
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 docstrings
🧪 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 60 minutes.Comment |
![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: fd535d9205
ℹ️ 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".
| if (target > len) target = len | ||
| if (Math.abs(target - parent.keyTime) > 0.001) { | ||
| if (AnimationControlController.moveKeyframe( | ||
| modelData.bone, parent.keyTime, target)) { |
There was a problem hiding this comment.
Within the Repeater delegate, modelData is the key time value (a number), not the outer row map, so modelData.bone is undefined here. That means diamond interactions call moveKeyframe/selectBone with an empty bone name, and AnimationControlController::moveKeyframe immediately rejects the operation; dragging/clicking keyframes on the dope sheet therefore fails in normal use.
Useful? React with 👍 / 👎.
Comment on lines
+629
to
+635
| UndoManager::getSingleton()->push(cmd); | ||
| // The command's redo() ran inside push(); refresh the slider ticks for | ||
| // the currently-edited bone and signal QML views to re-read rows. | ||
| refreshSliderTicks(); | ||
| emit boneRowsChanged(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
This method always returns true once a command is pushed, but MoveKeyframeCommand::redo() can be a no-op (for example when oldTime is missing or newTime collides with an existing keyframe). Because the result is ignored, callers receive a false success signal and the undo stack is polluted with ineffective commands, which breaks expected behavior such as reporting collision rejection.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (3)
src/commands/MoveKeyframeCommand_test.cpp (1)
48-48: ⚡ Quick winTrack lookup via
_getNodeTrackList().begin()->secondis fragile — may not be the "Child" bone's track.
_getNodeTrackList()is an internal Ogre API that returns amap<unsigned short, NodeAnimationTrack*>keyed by bone handle..begin()->secondyields the track for the bone with the numerically smallest handle, which is only the "Child" bone's track by coincidence. The correct Ogre idiom is to callanim->getNodeTrack(bone->getHandle()), wherebone = skel->getBone("Child"), as demonstrated throughout the Ogre ecosystem.If the test fixture introduces a bone whose handle is lower than "Child"'s, all three tests would verify keyframe times on the wrong track, producing misleading results.
♻️ Proposed fix (apply to lines 48, 78, and 116)
- auto* track = skel->getAnimation("TestAnim")->_getNodeTrackList().begin()->second; + Ogre::Bone* childBone = skel->getBone("Child"); + ASSERT_NE(childBone, nullptr); + auto* track = skel->getAnimation("TestAnim")->getNodeTrack(childBone->getHandle()); + ASSERT_NE(track, nullptr);Also applies to: 78-78, 116-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/MoveKeyframeCommand_test.cpp` at line 48, The test uses skel->getAnimation("TestAnim")->_getNodeTrackList().begin()->second which is fragile; instead fetch the bone and lookup its track explicitly: get the animation with getAnimation("TestAnim"), get the bone with skel->getBone("Child"), then call anim->getNodeTrack(bone->getHandle()) to obtain the correct NodeAnimationTrack; replace the three occurrences that use _getNodeTrackList().begin()->second with this getBone/getHandle/getNodeTrack sequence (preserve the variables anim, bone, track names used in the test).src/AnimationControlController_test.cpp (1)
686-709: 💤 Low valueMissing assertion guard before accessing
boneNames().first().Line 694 calls
ctrl->boneNames().first()without first asserting the list is non-empty. Other tests in this file (e.g., lines 665-666) includeASSERT_FALSE(ctrl->boneNames().isEmpty())before accessing the first element. WhileselectAnimationshould populate bones, adding the guard improves test robustness and consistency.♻️ Suggested fix
auto* ctrl = AnimationControlController::instance(); ctrl->updateAnimationTree(); ctrl->selectAnimation(QString::fromStdString(entity->getName()), "TestAnim"); + ASSERT_FALSE(ctrl->boneNames().isEmpty()); QString bone = ctrl->boneNames().first();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationControlController_test.cpp` around lines 686 - 709, The test MoveKeyframeRejectsCollision calls ctrl->boneNames().first() without ensuring the list is non-empty; add an assertion like ASSERT_FALSE(ctrl->boneNames().isEmpty()) immediately after ctrl->selectAnimation(...) (or before using ctrl->boneNames().first()) so the test fails fast and consistently if no bones were populated by AnimationControlController::selectAnimation; refer to AnimationControlControllerTest and the ctrl->boneNames() usage when adding this guard.src/AnimationControlController.cpp (1)
616-635: 💤 Low valueRedundant
boneRowsChanged()emission.
refreshSliderTicks()on line 632 already emitsboneRowsChanged()(line 434), so the explicit emit on line 633 is redundant and causes QML to re-queryallBoneRows()twice per move operation.♻️ Suggested fix
UndoManager::getSingleton()->push(cmd); // The command's redo() ran inside push(); refresh the slider ticks for // the currently-edited bone and signal QML views to re-read rows. refreshSliderTicks(); - emit boneRowsChanged(); return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationControlController.cpp` around lines 616 - 635, The duplicate emission of boneRowsChanged() in AnimationControlController::moveKeyframe causes QML to re-query allBoneRows() twice; remove the explicit emit boneRowsChanged() call at the end of moveKeyframe since refreshSliderTicks() already emits boneRowsChanged() (see refreshSliderTicks and its emission at its implementation), leaving the UndoManager::getSingleton()->push(cmd) and refreshSliderTicks() calls intact so the MoveKeyframeCommand redo still triggers the necessary UI update.
🤖 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 167-196: The drag handler currently calls
AnimationControlController.moveKeyframe on every onPositionChanged causing many
undo entries; change behavior so moveKeyframe is only invoked once in
onReleased: keep onPressed storing pressX and originalTime, in onPositionChanged
compute target using the existing dx/dt/bounds logic but do NOT call
moveKeyframe there — instead update a transient UI preview (e.g.
AnimationControlController.sliderValue or a preview time property) so the key
appears to move during drag; then in onReleased compute the final target the
same way and call AnimationControlController.moveKeyframe(modelData.bone,
originalTime, target) once, and set dragging = false. Use the existing symbols
onPressed, onPositionChanged, onReleased, pressX, originalTime, parent.keyTime,
AnimationControlController.moveKeyframe and
AnimationControlController.sliderValue to locate the code to change.
In `@src/commands/MoveKeyframeCommand.cpp`:
- Around line 89-97: The command is pushed even when the move is a no‑op because
MoveKeyframeCommand::redo() ignores moveKeyframeTo()'s bool result; fix by
validating the move before pushing: in
AnimationControlController::moveKeyframe(), check that a keyframe exists at
oldTime and no conflicting keyframe at newTime (i.e., the same checks performed
inside MoveKeyframeCommand::moveKeyframeTo()) and only call undoStack.push(new
MoveKeyframeCommand(...)) if those checks pass; alternatively add a
MoveKeyframeCommand::canMoveKeyframe(oldTime,newTime) helper that the controller
calls and only push when it returns true. Ensure you reference
MoveKeyframeCommand::moveKeyframeTo, MoveKeyframeCommand::redo/undo and
AnimationControlController::moveKeyframe when making the change.
In `@src/commands/MoveKeyframeCommand.h`:
- Around line 36-39: The doc comment for moveKeyframeTo incorrectly states
"Returns nullptr if no match" while the function returns a bool; update the
comment for moveKeyframeTo to accurately describe its return value (e.g.,
"Returns true if a keyframe at searchTime was found and moved to targetTime,
false if no matching keyframe was found") and keep the existing note that this
helper is used by undo/redo to re-resolve the keyframe after a previous move.
---
Nitpick comments:
In `@src/AnimationControlController_test.cpp`:
- Around line 686-709: The test MoveKeyframeRejectsCollision calls
ctrl->boneNames().first() without ensuring the list is non-empty; add an
assertion like ASSERT_FALSE(ctrl->boneNames().isEmpty()) immediately after
ctrl->selectAnimation(...) (or before using ctrl->boneNames().first()) so the
test fails fast and consistently if no bones were populated by
AnimationControlController::selectAnimation; refer to
AnimationControlControllerTest and the ctrl->boneNames() usage when adding this
guard.
In `@src/AnimationControlController.cpp`:
- Around line 616-635: The duplicate emission of boneRowsChanged() in
AnimationControlController::moveKeyframe causes QML to re-query allBoneRows()
twice; remove the explicit emit boneRowsChanged() call at the end of
moveKeyframe since refreshSliderTicks() already emits boneRowsChanged() (see
refreshSliderTicks and its emission at its implementation), leaving the
UndoManager::getSingleton()->push(cmd) and refreshSliderTicks() calls intact so
the MoveKeyframeCommand redo still triggers the necessary UI update.
In `@src/commands/MoveKeyframeCommand_test.cpp`:
- Line 48: The test uses
skel->getAnimation("TestAnim")->_getNodeTrackList().begin()->second which is
fragile; instead fetch the bone and lookup its track explicitly: get the
animation with getAnimation("TestAnim"), get the bone with
skel->getBone("Child"), then call anim->getNodeTrack(bone->getHandle()) to
obtain the correct NodeAnimationTrack; replace the three occurrences that use
_getNodeTrackList().begin()->second with this getBone/getHandle/getNodeTrack
sequence (preserve the variables anim, bone, track names used in the test).
🪄 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: 77096d94-5043-476e-a8fe-d75d52daaa4f
📒 Files selected for processing (13)
CMakeLists.txtqml/AnimationDopeSheet.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationControlController_test.cppsrc/CMakeLists.txtsrc/commands/MoveKeyframeCommand.cppsrc/commands/MoveKeyframeCommand.hsrc/commands/MoveKeyframeCommand_test.cppsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrctests/CMakeLists.txt
The MoveKeyframeRejectsCollision test failed: moveKeyframe() returned true even when the target time collided with another keyframe, because the controller was always pushing the MoveKeyframeCommand and the command's internal validation just silently no-op'd. Add the same find-source + collision-check up front in the controller. Returns false without pushing if the source keyframe doesn't exist or the target time would collide. The command's internal validation stays as a defensive backstop for the undo/redo path (in case the track changes between push and redo). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex P1 (qml): the inner Repeater shadows modelData with the keyTime number, so modelData.bone was always undefined. Capture row.bone + row.keyTimes at the row delegate scope as rowDelegate.boneName / rowDelegate.keyTimes and reach back through the explicit id. User feedback: clicking a diamond was de-selecting the row's bone. onPressed now calls selectBone(rowDelegate.boneName) before any drag motion, so a click always selects the diamond's bone + jumps the playhead to its time. CodeRabbit Major (qml): drag was pushing one MoveKeyframeCommand per onPositionChanged event, polluting the undo stack so Ctrl+Z only nudged the keyframe back one tick. Now the drag updates a visual dragPreviewTime only; onReleased pushes exactly one move command per gesture. CodeRabbit Minor (cmd.h): doc said moveKeyframeTo "returns nullptr" but it returns bool. Reworded to match. SonarCloud S5025 (critical x2): mark Qt parent-owned new for the dock widgets and the QUndoStack-owned MoveKeyframeCommand with NOSONAR. Standard Qt idiom — same pattern the rest of the file uses. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 180-202: The drag delta is computed in local coordinates causing
jitter as the marker moves; change the press and movement capture to use
scene-space coordinates: record pressX from mouse.sceneX in the onPressed
handler (where pressX, originalTime, parent.dragPreviewTime,
AnimationControlController.selectBone and sliderValue are set) and in
onPositionChanged compute dx = mouse.sceneX - pressX (leaving the rest of the
clamping and parent.dragPreviewTime assignment intact) so the delta is stable
regardless of item repositioning.
In `@src/AnimationControlController.cpp`:
- Around line 616-665: The moveKeyframe method performs a user-facing keyframe
move but does not log a Sentry breadcrumb; add a call to
SentryReporter::addBreadcrumb(...) immediately after the
UndoManager::getSingleton()->push(cmd) (or right after refreshSliderTicks()/emit
boneRowsChanged()) to record the action. Include a clear category like
"animation" and a message containing the moved bone (boneStd), the animation
name (m_selectedAnimation) and the times (oldTime -> newTime) so the breadcrumb
uniquely describes the operation and is added only on the successful path in
moveKeyframe.
- Around line 662-664: Remove the duplicate emission of boneRowsChanged(): since
refreshSliderTicks() already emits boneRowsChanged(), delete the explicit emit
boneRowsChanged() call (the line immediately after refreshSliderTicks()) so only
the single emission from refreshSliderTicks() remains and keep the return true;
as-is.
- Around line 598-612: allBoneRows() currently appends bone rows in the
iteration order of anim->_getNodeTrackList(), which is non-deterministic; fix by
building an explicit hierarchy traversal and emitting rows in that traversal
order: first create a map from Ogre::Node* (or node name) to its NodeTrack (from
anim->_getNodeTrackList()), then find root nodes (nodes whose parent is not in
the map) and perform a deterministic traversal (e.g., DFS or BFS) using
Ogre::Node parent/child relationships to visit nodes in hierarchy order, and for
each visited node that has a track build the QVariantMap row (same keys "bone"
and "keyTimes") and append to rows; update allBoneRows() to use this traversal
instead of direct for-each over _getNodeTrackList().
🪄 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: a922b8a5-2851-4af0-a955-ac92f0e8ca4b
📒 Files selected for processing (4)
qml/AnimationDopeSheet.qmlsrc/AnimationControlController.cppsrc/commands/MoveKeyframeCommand.hsrc/mainwindow.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mainwindow.cpp
Comment on lines
+180
to
+202
| onPressed: function(mouse) { | ||
| dragging = true | ||
| pressX = mouse.x | ||
| originalTime = parent.keyTime | ||
| parent.dragPreviewTime = parent.keyTime | ||
| // Selecting the diamond's bone + jumping the | ||
| // playhead is the natural "click" outcome — | ||
| // do it on press so it works even if the user | ||
| // drags slightly afterwards. | ||
| AnimationControlController.selectBone(rowDelegate.boneName) | ||
| AnimationControlController.sliderValue = | ||
| Math.round(parent.keyTime * 1000) | ||
| } | ||
| onPositionChanged: function(mouse) { | ||
| if (!dragging) return | ||
| var dx = mouse.x - pressX | ||
| var dt = dx / root.pxPerSec | ||
| var target = originalTime + dt | ||
| if (target < 0) target = 0 | ||
| var len = AnimationControlController.animationLength | ||
| if (target > len) target = len | ||
| parent.dragPreviewTime = target | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Qt Quick MouseArea, is mouse.xinonPositionChangedlocal to the MouseArea, and will moving that item during drag changemouse.x relative values?
💡 Result:
Yes, in Qt Quick MouseArea, mouse.x in onPositionChanged is local to the MouseArea item (relative to its top-left corner). Yes, moving the item (e.g., the parent of the MouseArea) during a drag will cause mouse.x relative values to change, because the mouse events are computed relative to the current position of the receiving item. Developers often store the initial drag offset (e.g., initialX = mouse.x) to compute absolute movement correctly, as shown in drag implementation examples.
Citations:
- 1: https://doc.qt.io/Qt-6/qml-qtquick-mousearea.html
- 2: https://doc.qt.io/QtForMCUs-2.2/qml-qtquick-mousearea.html
- 3: http://doc.qt.io/qt-5/qml-qtquick-mouseevent.html
- 4: https://doc-snapshots.qt.io/qt6-6.10/qml-qtquick-mouseevent.html
- 5: https://doc-snapshots.qt.io/qt6-dev/qml-qtquick-mousearea.html
- 6: https://stackoverflow.com/questions/49974790/qml-unable-to-change-position-of-item-while-being-dragged-by-mousearea
Use scene-space drag delta; local mouse.x causes unstable movement during drag.
At Line 195, dx = mouse.x - pressX is computed in the marker's local coordinate space. Since the marker moves during drag (Lines 160/201), the coordinate frame shifts as the item repositions. This causes mouse.x to report changing values even when the cursor position relative to the item hasn't changed—resulting in jitter or backtracking. Use mouse.sceneX instead to compute the delta in absolute screen coordinates, which remain stable regardless of item movement.
Suggested fix
property bool dragging: false
- property real pressX: 0
+ property real pressSceneX: 0
property real originalTime: 0
@@
dragging = true
- pressX = mouse.x
+ pressSceneX = mouse.sceneX
originalTime = parent.keyTime
parent.dragPreviewTime = parent.keyTime
@@
- var dx = mouse.x - pressX
+ var dx = mouse.sceneX - pressSceneX
var dt = dx / root.pxPerSec🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/AnimationDopeSheet.qml` around lines 180 - 202, The drag delta is
computed in local coordinates causing jitter as the marker moves; change the
press and movement capture to use scene-space coordinates: record pressX from
mouse.sceneX in the onPressed handler (where pressX, originalTime,
parent.dragPreviewTime, AnimationControlController.selectBone and sliderValue
are set) and in onPositionChanged compute dx = mouse.sceneX - pressX (leaving
the rest of the clamping and parent.dragPreviewTime assignment intact) so the
delta is stable regardless of item repositioning.
Comment on lines
+598
to
+612
| for (const auto& [handle, track] : anim->_getNodeTrackList()) { | ||
| Ogre::Node* node = track->getAssociatedNode(); | ||
| if (!node) continue; | ||
|
|
||
| QVariantList keyTimes; | ||
| keyTimes.reserve(static_cast<int>(track->getNumKeyFrames())); | ||
| for (unsigned short i = 0; i < track->getNumKeyFrames(); ++i) { | ||
| keyTimes.append(static_cast<double>(track->getKeyFrame(i)->getTime())); | ||
| } | ||
|
|
||
| QVariantMap row; | ||
| row[QStringLiteral("bone")] = QString::fromStdString(node->getName()); | ||
| row[QStringLiteral("keyTimes")] = keyTimes; | ||
| rows.append(row); | ||
| } |
There was a problem hiding this comment.
Return bone rows in explicit hierarchy order.
allBoneRows() currently relies on _getNodeTrackList() iteration order, which is not an explicit hierarchy sort. That can produce inconsistent row order across assets/runtimes and misses the slice objective.
💡 Suggested direction
QVariantList AnimationControlController::allBoneRows() const
{
QVariantList rows;
@@
- Ogre::Animation* anim = m_selectedSkeleton->getAnimation(m_selectedAnimation);
- for (const auto& [handle, track] : anim->_getNodeTrackList()) {
+ Ogre::Animation* anim = m_selectedSkeleton->getAnimation(m_selectedAnimation);
+ for (unsigned short i = 0; i < m_selectedSkeleton->getNumBones(); ++i) {
+ Ogre::Bone* bone = m_selectedSkeleton->getBone(i);
+ if (!bone) continue;
+ if (!anim->hasNodeTrack(bone->getHandle())) continue;
+ Ogre::NodeAnimationTrack* track = anim->getNodeTrack(bone->getHandle());
Ogre::Node* node = track->getAssociatedNode();
if (!node) continue;
@@
rows.append(row);
}
return rows;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AnimationControlController.cpp` around lines 598 - 612, allBoneRows()
currently appends bone rows in the iteration order of anim->_getNodeTrackList(),
which is non-deterministic; fix by building an explicit hierarchy traversal and
emitting rows in that traversal order: first create a map from Ogre::Node* (or
node name) to its NodeTrack (from anim->_getNodeTrackList()), then find root
nodes (nodes whose parent is not in the map) and perform a deterministic
traversal (e.g., DFS or BFS) using Ogre::Node parent/child relationships to
visit nodes in hierarchy order, and for each visited node that has a track build
the QVariantMap row (same keys "bone" and "keyTimes") and append to rows; update
allBoneRows() to use this traversal instead of direct for-each over
_getNodeTrackList().
Comment on lines
+616
to
+665
| bool AnimationControlController::moveKeyframe(const QString& boneName, | ||
| double oldTime, double newTime) | ||
| { | ||
| if (boneName.isEmpty()) return false; | ||
| if (!m_selectedSkeleton || m_selectedAnimation.empty()) return false; | ||
| if (!m_selectedSkeleton->hasAnimation(m_selectedAnimation)) return false; | ||
| if (qFuzzyCompare(oldTime + 1.0, newTime + 1.0)) return false; | ||
|
|
||
| // Validate up-front so we never push a no-op onto the undo stack. | ||
| // The command's internal validation is the source of truth, but | ||
| // duplicating the find-source-keyframe + collision-check here lets | ||
| // us return false without polluting the undo history. | ||
| Ogre::Animation* anim = m_selectedSkeleton->getAnimation(m_selectedAnimation); | ||
| const std::string boneStd = boneName.toStdString(); | ||
| if (!m_selectedSkeleton->hasBone(boneStd)) return false; | ||
| Ogre::Bone* bone = m_selectedSkeleton->getBone(boneStd); | ||
| if (!bone || !anim->hasNodeTrack(bone->getHandle())) return false; | ||
| Ogre::NodeAnimationTrack* track = anim->getNodeTrack(bone->getHandle()); | ||
|
|
||
| constexpr float kEpsilon = 0.001f; | ||
| int sourceIdx = -1; | ||
| for (unsigned short i = 0; i < track->getNumKeyFrames(); ++i) { | ||
| if (std::fabs(track->getKeyFrame(i)->getTime() - static_cast<float>(oldTime)) <= kEpsilon) { | ||
| sourceIdx = static_cast<int>(i); | ||
| break; | ||
| } | ||
| } | ||
| if (sourceIdx < 0) return false; // no keyframe at oldTime | ||
|
|
||
| for (unsigned short i = 0; i < track->getNumKeyFrames(); ++i) { | ||
| if (static_cast<int>(i) == sourceIdx) continue; | ||
| if (std::fabs(track->getKeyFrame(i)->getTime() - static_cast<float>(newTime)) <= kEpsilon) { | ||
| return false; // collision with another existing keyframe | ||
| } | ||
| } | ||
|
|
||
| // QUndoStack::push() takes ownership of the command — this raw new is | ||
| // the standard QUndoCommand idiom (mirrors TransformCommands callers). | ||
| auto* cmd = new MoveKeyframeCommand(m_selectedSkeleton, // NOSONAR — QUndoStack owns | ||
| m_selectedAnimation, | ||
| boneStd, | ||
| static_cast<float>(oldTime), | ||
| static_cast<float>(newTime)); | ||
| UndoManager::getSingleton()->push(cmd); | ||
| // The command's redo() ran inside push(); refresh the slider ticks for | ||
| // the currently-edited bone and signal QML views to re-read rows. | ||
| refreshSliderTicks(); | ||
| emit boneRowsChanged(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Add breadcrumb for keyframe-move user action.
A successful dope-sheet move is a significant user-facing operation, but this path currently has no SentryReporter::addBreadcrumb(...) call.
As per coding guidelines, "**/*.cpp: All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AnimationControlController.cpp` around lines 616 - 665, The moveKeyframe
method performs a user-facing keyframe move but does not log a Sentry
breadcrumb; add a call to SentryReporter::addBreadcrumb(...) immediately after
the UndoManager::getSingleton()->push(cmd) (or right after
refreshSliderTicks()/emit boneRowsChanged()) to record the action. Include a
clear category like "animation" and a message containing the moved bone
(boneStd), the animation name (m_selectedAnimation) and the times (oldTime ->
newTime) so the breadcrumb uniquely describes the operation and is added only on
the successful path in moveKeyframe.
Comment on lines
+662
to
+664
| refreshSliderTicks(); | ||
| emit boneRowsChanged(); | ||
| return true; |
There was a problem hiding this comment.
Avoid duplicate boneRowsChanged() emission.
refreshSliderTicks() already emits boneRowsChanged() (Line 434). Emitting it again here triggers redundant re-queries/repaints.
✂️ Minimal fix
refreshSliderTicks();
- emit boneRowsChanged();
return true;📝 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.
Suggested change
| refreshSliderTicks(); | |
| emit boneRowsChanged(); | |
| return true; | |
| refreshSliderTicks(); | |
| return true; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/AnimationControlController.cpp` around lines 662 - 664, Remove the
duplicate emission of boneRowsChanged(): since refreshSliderTicks() already
emits boneRowsChanged(), delete the explicit emit boneRowsChanged() call (the
line immediately after refreshSliderTicks()) so only the single emission from
refreshSliderTicks() remains and keep the return true; as-is.
|
This was referenced May 2, 2026
This was referenced May 4, 2026
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #372 (slice C of #260).
Adds a multi-bone dope sheet view at the bottom of the main window: one row per animated bone, diamond markers at every keyframe time, drag-to-move time, undoable. Wheel/middle-drag for zoom and pan.
AnimationDopeSheet.qml. Reuses the existing yellow-diamond / red-selected styling. Click a row's bone name to select it (shared state with the existing single-track panel). Click a diamond to move the playhead there. Drag a diamond to shift its keyframe time.AnimationControlControllergainsallBoneRows(),moveKeyframe(bone, oldT, newT), and aboneRowsChangedsignal. The signal is emitted from the existingrefreshSliderTicks()so any track-affecting op (add / delete / move / select) covers it.commands/MoveKeyframeCommand. Ogre'sKeyFramehas nosetTime(), so the command captures TRS, removes the source keyframe, creates one at the target time, and restores TRS. Refuses moves that would collide with another keyframe on the same track (1 ms tolerance).QDockWidgetat the bottom, toggleable via View menu (uses the dock's built-intoggleViewAction()somainwindow.uiis untouched). Sentry breadcrumbs on toggle.Version bumped to 2.35.0.
Out of scope (deferred)
Test plan
Animationtrack updatesAnimationControlController{,Playback}Testcovers row/move/collision;MoveKeyframeCommandTestcovers undo/redo + TRS preservation + missing-time no-op🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores