feat(animation): playback speed, loop region, auto-key (Phase 5 slice A) - #356
Merged
Conversation
|
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 (6)
📝 WalkthroughWalkthroughAnimation playback now supports variable speed control and configurable loop regions. The QML interface adds a speed selector and loop toggle with interactive handle overlays. The controller manages playback speed, loop start/end boundaries, and a new time advancement helper that applies speed scaling and wraps time within active loop regions. Frame updates integrate this logic to advance animations accordingly. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant QML as QML UI
participant Ctrl as AnimationControlController
participant MainWin as MainWindow
participant AnimState as AnimationState
User->>QML: Adjust playback speed/loop toggle
QML->>Ctrl: setPlaybackSpeed() / setLoopRegionActive()
Ctrl->>Ctrl: Clamp values, emit signal
QML->>QML: Update UI display
MainWin->>MainWin: frameRenderingQueued()
MainWin->>Ctrl: Get current playback speed
MainWin->>Ctrl: advanceTime(currentTime, dt)
Ctrl->>Ctrl: Apply speed scaling
Ctrl->>Ctrl: Conditionally wrap into loop region
Ctrl->>MainWin: Return advanced time
MainWin->>AnimState: setTimePosition(advancedTime)
AnimState->>AnimState: Update animation frame
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 46 minutes and 21 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: bb6eeef50c
ℹ️ 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".
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| cursorShape: Qt.SizeHorCursor | ||
| drag.target: parent |
There was a problem hiding this comment.
Using drag.target: parent on the loop marker rectangles mutates their x property directly, which breaks the declarative x bindings to loopStart/loopEnd after the first drag. Once that happens, later controller-driven updates (for example selecting a new clip, changing animation length, or external loop value changes) can leave the handles visually out of sync with the actual loop region values. Consider driving loop values from pointer deltas (without dragging the visual item itself) so the handle positions stay purely bound.
Useful? React with 👍 / 👎.
5 tasks
![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
🧹 Nitpick comments (1)
src/AnimationControlController.cpp (1)
307-323: 💤 Low valueMinor: Signal may emit when value unchanged after clamping.
The early-return check
qFuzzyCompare(s, m_loopStart)happens before the constraint enforcement on line 312. If the caller setsloopStartto a value larger thanloopEnd, andm_loopStartwas already clamped tom_loopEnd, the function will emitloopRegionChanged()even though the final value is unchanged.Same pattern applies to
setLoopEnd.This is functionally harmless (just an extra signal) but could be tightened by comparing before emission:
♻️ Optional: avoid redundant signal emission
void AnimationControlController::setLoopStart(double s) { if (s < 0.0) s = 0.0; - if (qFuzzyCompare(s, m_loopStart)) return; - m_loopStart = s; + double newVal = s; if (m_loopEnd > 0.0 && m_loopStart > m_loopEnd) m_loopStart = m_loopEnd; + if (newVal > m_loopEnd && m_loopEnd > 0.0) newVal = m_loopEnd; + if (qFuzzyCompare(newVal, m_loopStart)) return; + m_loopStart = newVal; emit loopRegionChanged(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/AnimationControlController.cpp` around lines 307 - 323, The early qFuzzyCompare check in setLoopStart and setLoopEnd happens before clamping against the opposite boundary, so callers that pass out-of-range values can trigger emit loopRegionChanged() even when the stored m_loopStart/m_loopEnd remains unchanged; fix by applying the clamping logic (the lines that enforce s >= 0 and clamp s to m_loopEnd/m_loopStart) first, then perform qFuzzyCompare(s, m_loopStart) / qFuzzyCompare(s, m_loopEnd) and only update the member and emit loopRegionChanged() if the post-clamp value differs; update both setLoopStart and setLoopEnd accordingly.
🤖 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/AnimationControlPanel.qml`:
- Around line 490-503: The loop shading and handle positions currently use a
hard-coded pad/track offset (pad = 13) and avail value, causing misalignment;
update the painting and drag math to use the timeSlider's actual geometry (e.g.,
timeSlider.leftPadding for pad and timeSlider.availableWidth for avail) when
computing lx/rx and any handle positions so the loop range matches the slider
groove; apply the same change wherever pad/avail is used for
loopRegionActive/loopStart/loopEnd (including the other blocks mentioned around
lines 540–549 and 551–597) and keep references to
AnimationControlController.loopRegionActive, loopStart, loopEnd, and the
timeSlider properties.
In `@qml/PropertiesPanel.qml`:
- Around line 1623-1629: The currentIndex binding in PropertiesPanel.qml can
incorrectly return a hardcoded fallback index (2) causing the ComboBox to
display "1x" for unsupported playback speeds; update the currentIndex logic that
references AnimationControlController.playbackSpeed and values to compute the
nearest preset instead of returning 2—iterate values, track the minimum absolute
difference and its index, and return that index; alternatively you can snap the
incoming speed in AnimationControlController.setPlaybackSpeed(), but the quick
QML-side fix is to replace the fallback with the nearest-index selection so the
UI always reflects the closest preset.
---
Nitpick comments:
In `@src/AnimationControlController.cpp`:
- Around line 307-323: The early qFuzzyCompare check in setLoopStart and
setLoopEnd happens before clamping against the opposite boundary, so callers
that pass out-of-range values can trigger emit loopRegionChanged() even when the
stored m_loopStart/m_loopEnd remains unchanged; fix by applying the clamping
logic (the lines that enforce s >= 0 and clamp s to m_loopEnd/m_loopStart)
first, then perform qFuzzyCompare(s, m_loopStart) / qFuzzyCompare(s, m_loopEnd)
and only update the member and emit loopRegionChanged() if the post-clamp value
differs; update both setLoopStart and setLoopEnd accordingly.
🪄 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: e30d7da7-7b5e-4e8f-b185-47de663fa8a8
📒 Files selected for processing (6)
qml/AnimationControlPanel.qmlqml/PropertiesPanel.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationControlController_test.cppsrc/mainwindow.cpp
Adds the timeline polish from #260: - playbackSpeed (0.25–4×) global multiplier; ComboBox sits next to the Play button in the Inspector's Animations section. In-app only — scales dt before setTimePosition; keyframes/length untouched. - loopStart/loopEnd/loopRegionActive scoped to the entity+animation selected in the Animation Control panel. Other entities advance at native timing. Wraps via fmod so large overshoots fold back inside the region. Resets to [0, length] on each new selection. - autoKey toggle: end-of-drag in TransformOperator pushes a keyframe on the active bone via AnimationControlController::addKeyframe(). - QML: timeline canvas shades the loop region and renders blue in/out markers; two transparent drag handles let you reposition them. Tests: 14 new pure-data tests in a separate fixture (no Ogre needed) covering speed scaling, signal emission, loop wrap (basic, large overshoot, degenerate, inactive passthrough, clamping), and auto-key safety. 2 Ogre-fixture tests for the auto-key add-keyframe path and loop-region reset on selection (run on Linux CI). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Codex flagged that drag.target: parent on the loop-marker rectangles would mutate their x property directly, breaking the declarative bind to loopStart/loopEnd. After the first drag, controller-driven updates (new clip selected, length changed, external value change) could leave the handles visually out of sync. Drop drag.target and compute the new time from mouseX inside the MouseArea instead. The handle's x binding stays intact; the controller remains the single source of truth. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Auto-key only makes sense once bones are directly manipulable in the viewport (click a bone → drag the gizmo). The original wiring fired on scene-node end-of-drag and sampled an interpolated pose, which is not what the toggle's name implies. Without a bone-gizmo there is no honest behavior to ship. Splitting it out into #358 (bone manipulation gizmo + auto-keyframe). Slice A keeps speed scaling + per-entity loop region, which work on their own. Removes: - autoKey property/setter/signal + autoKeyOnTransform() from controller - "Auto Key" toggle from AnimationControlPanel.qml - TransformOperator end-of-drag hook (and its include) - 5 auto-key tests (4 pure-data + 1 Ogre-fixture) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- mainwindow.cpp (Sonar S5350): animCtrl is now const auto*. - mainwindow.cpp (Sonar S5827 ×2): use auto for static_cast results where the type is already on the RHS. - AnimationControlController.cpp (Sonar S2681 ×2 / CodeRabbit nitpick): brace single-line ifs in setLoopStart / setLoopEnd, and clamp before the qFuzzyCompare bail-out so we don't emit loopRegionChanged when the request collapses to the existing value. - AnimationControlPanel.qml (CodeRabbit major): drop the hard-coded pad=13 / avail=width-26 in the timeline canvas and loop-handles layer; bind to timeSlider.leftPadding and timeSlider.availableWidth so the loop shading and drag handles track the slider groove across Qt styles, DPI settings, and platforms. - PropertiesPanel.qml (CodeRabbit minor): speed combobox now picks the nearest preset rather than silently showing 1× when the controller's value isn't an exact match. - AnimationControlController_test.cpp: align with master PR #355's new pattern — ASSERT_TRUE(canLoadMeshFiles()) instead of GTEST_SKIP() (CI now requires headless GL to work). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/phase5-slice-a-anim-timeline
branch
from
May 2, 2026 02:52
ed14531 to
6bcc2e1
Compare
|
5 tasks
7 tasks
9 tasks
4 tasks
fernandotonon
added a commit
that referenced
this pull request
May 3, 2026
…#384) * fix(animation): addKeyframe captures live bone pose + auto-key toggle 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> * fix(animation): place auto-key playback tests after fixture class 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> * fix(animation): address PR #384 review feedback - 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> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
First slice of #260 — animation timeline polish.
dtbeforesetTimePosition; keyframes and clip length untouched, so saved/exported meshes play at native speed elsewhere.loopStart/loopEnd/loopRegionActive): scoped to the entity + animation selected in the Animation Control panel. Other entities advance normally. Wraps viafmodso large overshoots fold back into the region. Resets to[0, length]whenever a new clip is selected. Two transparent drag handles let you reposition the in/out points while playing; the timeline canvas shades the active region.Auto-keyframe was removed from this slice — it's meaningless without a way to directly manipulate bones in the viewport, and the original wiring would have written the wrong pose anyway. Split into #358 (bone manipulation gizmo + auto-keyframe) so it can land on top of real bone editing.
Test plan
AnimationControlControllerPlaybackTest.*(10 cases) +SelectAnimationResetsLoopRegion🤖 Generated with Claude Code