feat(curve-editor): live drag preview + trackpad pan + scroll/zoom controls - #392
Conversation
…rols The curve editor's keyframe drag pushed an undo command per move event, firing MainWindow's QUndoStack::indexChanged handler — which calls Skeleton::reset(true) — so the bone visibly snapped to T-pose between events. Add non-undoable preview setters (moveKeyframePreview, setKeyframeValuePreview) that retime/rewrite in place and skip the undo stack; the QML drag handler commits one MoveKeyframeCommand + SetKeyframeValueCommand on release so a single Ctrl+Z reverts the whole gesture. Also wires up: - Two-finger trackpad pan (horizontal swipe = time, vertical = value) via WheelHandler with pixelDelta tracking, scoped inside the MouseArea so pointer handlers actually receive trackpad events. - Cmd/Ctrl+wheel = horizontal zoom; Shift+wheel = vertical zoom. - Toolbar +/− zoom buttons + fit-to-view, anchored to a cursor pixel. - Horizontal ScrollBar.AlwaysOn that tracks viewStart in both directions with a `syncing` guard against the binding loop. - Inline rows + channelValues refresh during preview so the keyframe square + curve track the cursor in real time. Tests cover preview methods: no undo push, TRS preserved on retime, collision rejection, missing-time rejection, no-op on identical time, no-op without selection, unknown-channel rejection. 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 (1)
📝 WalkthroughWalkthroughThis PR transforms the AnimationCurveEditor into an interactive tool by adding horizontal/vertical zoom controls with fit-to-view, keyframe and tangent-handle dragging via live preview APIs, axis-locking via Shift, and viewport synchronization with a horizontal scrollbar. The C++ controller gains ChangesInteractive Curve Editor with Live Preview
Sequence DiagramsequenceDiagram
participant User as User Interaction
participant QML as AnimationCurveEditor.qml
participant Controller as AnimationControlController
participant Ogre as Ogre Rendering
participant Visual as Visual Feedback
User->>QML: Left-drag keyframe/tangent
QML->>QML: Hit-test & track drag state
QML->>Controller: moveKeyframePreview/setKeyframeValuePreview
Controller->>Controller: Validate & update keyframe
Controller->>Ogre: notifyOgreUpdate()
Ogre->>Ogre: Refresh transform state
Ogre->>Visual: Render updated curves
Visual->>User: Live preview on canvas
User->>User: Release drag
QML->>Controller: moveKeyframe/setKeyframeValue (commit)
Controller->>Controller: Push undo command & apply
Controller->>Ogre: notifyOgreUpdate()
Visual->>User: Final rendered state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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. 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: 84917f6d47
ℹ️ 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".
| panArea.dragLastValue = newValue | ||
| if (!lockX) { | ||
| AnimationControlController.setKeyframeValuePreview( | ||
| panArea.dragBone, panArea.dragChannel, | ||
| panArea.dragKeyTime, newValue) | ||
| } |
There was a problem hiding this comment.
dragLastValue is updated unconditionally on every mouse move, even when Y is axis-locked (lockY) and no preview value write occurs, and it is never initialized on press. On release, valueChanged compares this stale value to dragOriginalValue and can push setKeyframeValue(...) with an unintended value (e.g., Shift-horizontal drags with slight vertical jitter, or click-and-release without movement using a previous drag's value/default 0). This causes silent keyframe value edits during operations that should only retime or select.
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
🤖 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.
Inline comments:
In `@qml/AnimationCurveEditor.qml`:
- Around line 467-475: When starting a keyframe drag, seed the preview state by
initializing panArea.dragLastValue to the keyframe's current value so onReleased
doesn't treat an untouched click as an edit; update the first drag-start block
(where panArea.dragMode, dragBone, dragChannel, dragKeyTime,
dragOriginalKeyTime, dragOriginalValue, dragPressX/Y are set) to also set
panArea.dragLastValue = khit.value, and make the identical change in the other
drag-start block referenced around the later section (the block that sets the
same panArea.drag* properties at lines ~551-573) so both drag paths initialize
dragLastValue consistently.
- Around line 84-90: clampViewStart currently allows viewStart up to maxT -
visibleSecs * 0.5 which lets the canvas drift past the scrollbar's end; change
the clamp to use the same range as the scrollbar by computing visibleSecs =
curveCanvas.width / root.pxPerSec and clamping root.viewStart to Math.max(0,
AnimationControlController.animationLength - visibleSecs). Update the clamp
logic in the clampViewStart function (and the other similar clamp blocks noted
around the file) to use this exact maxStart formula so panning and middle-button
movement cannot move the canvas into blank space beyond the scrollbar limit.
- Around line 503-523: The axis-lock logic is inverted when applying previews:
swap the two condition checks so value updates are skipped when lockY is true
and time moves are skipped when lockX is true. Concretely, in the drag handler
use if (!lockY) around AnimationControlController.setKeyframeValuePreview(...)
(and update panArea.dragLastValue as now), and use if (!lockX) around
AnimationControlController.moveKeyframePreview(...) (and update
panArea.dragKeyTime when ok); keep the existing lockX/lockY calculations and the
referenced symbols (lockX, lockY, panArea.dragLastValue, panArea.dragBone,
panArea.dragChannel, panArea.dragKeyTime,
AnimationControlController.setKeyframeValuePreview,
AnimationControlController.moveKeyframePreview).
🪄 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: 6c3453c3-4d78-4d83-8fd3-92b981f14e3f
📒 Files selected for processing (4)
qml/AnimationCurveEditor.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationControlController_test.cpp
…range Addresses CodeRabbit + ChatGPT-Codex review on PR #392: - Initialize dragLastValue at press time so a click without drag doesn't commit a stale 0 back to the curve. - clampViewStart() now clamps to (animationLength - visibleSecs), matching the scrollbar's scrollable range; middle-button pan routes through it instead of clamping only to >= 0, so the canvas can't drift past the thumb's end position. - Replace the inverted lockX/lockY flags with writeValue/writeTime gating: a Shift-constrained drag now writes only the dominant axis instead of skipping the wrong one. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
|
Thanks @coderabbitai @chatgpt-codex-connector — the three issues you flagged on commit
CI is green, SonarCloud quality gate passes, unit tests pass with new |
|
To use Codex here, create an environment for this repo. |
|
Tip For best results, initiate chat on the files or code changes.
Great to hear CI, SonarCloud, and the new preview unit test coverage are all green. 🎉 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
Addresses the two follow-up issues from D3 (#380, #382) and the user-reported "T-pose blink while dragging keyframes":
moveKeyframePreview/setKeyframeValuePreviewnon-undoable setters used during keyframe drag. Single Ctrl+Z still reverts the whole gesture because the realMoveKeyframeCommand+SetKeyframeValueCommandare pushed once on release.WheelHandlerwithpixelDeltafor high-resolution input.ScrollBar.AlwaysOnfor long animations, +/− zoom buttons for both axes, fit-to-view button, Cmd/Ctrl+wheel for horizontal zoom, Shift+wheel for vertical zoom.Why the preview API
Before: every keyframe drag move pushed a
SetKeyframeValueCommandANDMoveKeyframeCommandonto the undo stack — once per pixel of cursor motion.MainWindow'sQUndoStack::indexChangedhandler callsskel->reset(true)after every push, so the bone visibly snapped to T-pose between events.After: drag previews retime/rewrite in place and skip the undo stack. On release, the QML handler restores the originals first, then pushes the real commands —
redo()captures the correct old-state snapshot, so Ctrl+Z reverts the whole gesture as a single edit.Trackpad gesture summary
Pinch-to-zoom isn't wired up: Qt 6 routes macOS native magnify gestures (
NSEventTypeMagnify) through paths thatPinchHandlerdoesn't observe when a coveringMouseAreaowns the pointer. Cmd+wheel and the +/− buttons cover the use case consistently across platforms.Test plan
MoveKeyframePreview*andSetKeyframeValuePreview*cover undo-stack behavior, TRS preservation, collision/missing-time rejection, no-op cases🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests