fix(animation): addKeyframe captures live bone pose + auto-key toggle - #384
Conversation
Closes #383. Two related fixes that build toward #358's full bone gizmo without requiring it first: 1. addKeyframe captures the live bone pose - Was using getInterpolatedKeyFrame to sample the existing animation curve at the scrub time. With a flat or empty curve, the new keyframe came out identity TRS — the user-visible "blank registry" bug. Now reads the bone's current local TRS via bone->getPosition() - bone->getInitialPosition() etc., the same math the blender's bake (slice B) and the dope sheet's keyframe TRS preservation already use. - Means: drag the entity in the viewport, scrub to a fresh time, hit +KF, and the keyframe captures the actual pose under the cursor. 2. Auto-key toggle wired to TransformOperator end-of-drag - Re-add the autoKey property + signal + setter (slice A originally shipped this; we ripped it out in #356's cleanup commit because bone manipulation didn't exist yet). - New "Auto Key" button next to the loop toggle in the Animation Control panel. - TransformOperator::mouseReleaseEvent calls AnimationControlController::autoKeyOnTransform() after every scene-node transform commit. With autoKey enabled and a bone-track selected, that pushes a keyframe at the current scrub time capturing the bone's live pose. This isn't a bone-direct gizmo — that's still in #358. But combined with the addKeyframe pose-capture fix, users can now record drag-and- snapshot animations using the existing scene-node gizmo without manually hitting +KF after every move. Tests - AddKeyframeCapturesBonePose: manually offset a bone, scrub, +KF; the new keyframe's translate.x matches the manual offset. - AutoKeyOnTransformPushesKeyframeWhenEnabled: autoKey off → no-op; autoKey on → autoKeyOnTransform pushes a new keyframe. - AutoKeyDefaultsOff + AutoKeyToggleEmitsSignal pure-data. - AutoKeyOnTransformNoOpWithoutSelection: never crashes when called before any selection is made. 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)
📝 WalkthroughWalkthroughAuto-key animation recording is added to the animation controller. The feature captures bone poses on user transform commits. A new QML toggle controls ChangesAuto-Key Animation Recording
Sequence DiagramsequenceDiagram
participant User
participant TransformOperator
participant AnimationControlController
participant SkeletonBone
User->>TransformOperator: Drag scene node (transform)
TransformOperator->>TransformOperator: Apply transform + record undo
TransformOperator->>AnimationControlController: autoKeyOnTransform()
alt autoKey is enabled
AnimationControlController->>SkeletonBone: Get current local transform
SkeletonBone-->>AnimationControlController: position, orientation, scale
AnimationControlController->>AnimationControlController: Compute offsets from bind pose
AnimationControlController->>AnimationControlController: addKeyframe() with captured pose
AnimationControlController->>AnimationControlController: Emit keyframe added signal
else autoKey is disabled
AnimationControlController->>AnimationControlController: No-op return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 36 minutes and 43 seconds.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: 267291f3d0
ℹ️ 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".
| // captures the bone pose at the moment of any scene-node transform | ||
| // commit, which is enough for users to record drag-and-snapshot | ||
| // animations without the dedicated bone gizmo. | ||
| AnimationControlController::instance()->autoKeyOnTransform(); |
There was a problem hiding this comment.
autoKeyOnTransform() is invoked unconditionally for every left-button release whenever there is any selected node/entity, even if no transform delta was applied. In this path, enabling Auto Key will add keyframes during plain clicks or box-selection releases (not just drag commits), which quickly pollutes tracks with unintended duplicate keys at the current scrub time. This call should be conditioned on the same changed state used to decide whether a transform command was actually committed.
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/TransformOperator.cpp (1)
1552-1639:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly auto-key after a real transform commit.
This call currently sits under
hasNodes() || hasEntities(), so it also runs on plain left-button releases while something is already selected, including selection clicks / box-select completions. With Auto Key enabled, that will insert keys even when nothing moved. Please gate it on an actual non-identity translate/rotate/scale commit, or move it into the same branches that push the transform undo command.Suggested direction
+ bool committedTransform = false; + if (SelectionSet::getSingleton()->hasNodes() && !mUndoStartPositions.isEmpty()) { auto nodes = SelectionSet::getSingleton()->getNodesSelectionList(); bool changed = false; @@ if (changed) { // Revert to start, then push command (which will redo) for (int i = 0; i < nodes.size() && i < mUndoStartPositions.size(); ++i) nodes[i]->setPosition(mUndoStartPositions[i]); UndoManager::getSingleton()->push(new TranslateCommand(nodes, totalDelta)); + committedTransform = true; } } @@ - AnimationControlController::instance()->autoKeyOnTransform(); + if (committedTransform) + AnimationControlController::instance()->autoKeyOnTransform();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/TransformOperator.cpp` around lines 1552 - 1639, The auto-key call runs even when no transform occurred; only invoke AnimationControlController::instance()->autoKeyOnTransform() when a real transform commit happened (i.e. when the code detects changed == true and you actually push an undo command). Move or duplicate the autoKey call into the same branches that push new TranslateCommand / RotateCommand / ScaleCommand (or wrap the existing call in a check that any of the translate/rotate/scale "changed" conditions were true and an undo was pushed), using mTransformState, the local changed flag, and presence of the pushed command (TranslateCommand, RotateCommand, ScaleCommand) to gate the call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/AnimationControlController.cpp`:
- Around line 351-365: Add Sentry breadcrumbs for the new Auto Key UI flow: in
AnimationControlController::setAutoKey, after changing m_autoKey and emitting
autoKeyChanged(), call SentryReporter::addBreadcrumb("ui.action",
QString("AutoKey toggled %1").arg(m_autoKey ? "on" : "off")) to record the
toggle; in AnimationControlController::autoKeyOnTransform, before calling
addKeyframe() (and only when m_autoKey is true), call
SentryReporter::addBreadcrumb("ui.action", "AutoKey applied keyframe
(autoKeyOnTransform)") so automatic key insertions are recorded for tracing.
- Around line 520-533: The code always calls
m_selectedTrack->createNodeKeyFrame(time) which unconditionally inserts a new
key and causes duplicate same-time keys; instead check for an existing
TransformKeyFrame at that time on m_selectedTrack (via the track's API or by
iterating its keyframes and comparing key->getTime()) and if found reuse that
keyframe (assign to newKf) rather than creating a new one; update the existing
keyframe's translate/rotation/scale (the same setTranslate/setRotation/setScale
calls) and only call createNodeKeyFrame(time) when no same-time keyframe exists.
---
Outside diff comments:
In `@src/TransformOperator.cpp`:
- Around line 1552-1639: The auto-key call runs even when no transform occurred;
only invoke AnimationControlController::instance()->autoKeyOnTransform() when a
real transform commit happened (i.e. when the code detects changed == true and
you actually push an undo command). Move or duplicate the autoKey call into the
same branches that push new TranslateCommand / RotateCommand / ScaleCommand (or
wrap the existing call in a check that any of the translate/rotate/scale
"changed" conditions were true and an undo was pushed), using mTransformState,
the local changed flag, and presence of the pushed command (TranslateCommand,
RotateCommand, ScaleCommand) to gate the call.
🪄 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: fbc449b1-3c80-4fc2-aa2d-3facf4456d37
📒 Files selected for processing (5)
qml/AnimationControlPanel.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationControlController_test.cppsrc/TransformOperator.cpp
The 3 AutoKey TEST_F blocks using AnimationControlControllerPlaybackTest were inserted before the fixture's class declaration, which caused the unit-tests-linux job to fail with "marked 'override', but does not override" errors. Move them after the existing PasteRejectsMalformedJson test where the fixture is in scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Gate auto-key on actual transform commits (Codex P1): plain clicks and zero-delta releases no longer push duplicate keys. Hoist TransformOperator's `changed` flag into `nodeTransformCommitted` and call autoKeyOnTransform only when true. - Reuse same-time keyframes (CodeRabbit Major): addKeyframe now finds any existing key within 1 ms of the scrub time and updates it in place instead of stacking duplicates the rest of the controller treats as invalid. - Add Sentry breadcrumbs (CodeRabbit Major): "ui.action" on Auto Key toggle and on every auto-key write so the new flow is traceable. - New test covers the dedup path (autoKeyOnTransform fired twice at the same scrub time → keyframe count grows by exactly 1). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed all three review findings in 3b50a89:
|
|
1 participant
Summary
Closes #383. Two related fixes that work today (without requiring #358's full bone-manipulation gizmo).
1. addKeyframe captures the live bone pose
Previously,
addKeyframecalledgetInterpolatedKeyFrameto sample the existing curve. With a flat/empty curve, every new keyframe was identity TRS — the "blank registry" the user reported. Now readsbone->getPosition() - bone->getInitialPosition()etc., same math the blender's bake (slice B) uses.2. Auto-key toggle wired to scene-node end-of-drag
Restores the autoKey property + Auto Key button (originally in slice A, removed when bone manipulation wasn't ready).
TransformOperator::mouseReleaseEventnow callsAnimationControlController::autoKeyOnTransform()after every transform commit. With autoKey enabled + a bone-track selected, that pushes a keyframe at the current scrub time with the bone's live pose.Combined: drag the entity → release → keyframe with the actual pose lands automatically. No need to remember +KF.
Out of scope (still in #358)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit