feat(curve-editor): resampler — Ogre playback follows curve shape - #395
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an adaptive and fixed‑FPS curve resampler, undoable resample/decimate and curve‑handle commands, Q_INVOKABLE controller APIs for baking/decimation/handle commits, QML “Bake” UI and tangent‑drag commit wiring, build/test additions, and tests validating resampling, commands, and controller behavior. ChangesCurve resampling and curve-handle editing
Sequence DiagramsequenceDiagram
participant User
participant QML as AnimationCurveEditor.qml
participant Ctrl as AnimationControlController
participant Cmd as Resample/Decimate/CurveEditCmd
participant Resampler as CurveResampler
participant Ogre
User->>QML: click Bake / drag tangent / apply mode
QML->>Ctrl: resampleAllSegmentsForBone / resampleCurveSegment / setCurveHandle
Ctrl->>Ctrl: validate inputs, collect anchors/tangents
Ctrl->>Cmd: push Resample/Decimate/CurveEditModelChangeCommand (macro if batching)
Cmd->>Resampler: resampleSegment(model,..., t0,t1, toleranceMul, fixedFps)
Resampler-->>Cmd: return Sample[]
Cmd->>Ogre: insert/interpolate TransformKeyFrames or replace track or apply tangents/mode
Ogre-->>Ctrl: notify _keyFrameDataChanged()
Ctrl->>QML: emit boneRowsChanged()/refreshSliderTicks()
QML-->>User: UI reflects updated keyframes/handles
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. 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: d3eef7425a
ℹ️ 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".
Comment on lines
365
to
367
| AnimationControlController.selectedEntityName, | ||
| AnimationControlController.selectedAnimation, | ||
| boneName, channelId, keyTime, mode) |
There was a problem hiding this comment.
applyMode mutates CurveEditModel directly and then only pushes resample commands, so undo restores Ogre keyframes but does not restore the mode/tangent metadata that actually drives future curve evaluation. In practice, after a mode change + Ctrl+Z, the visual/model state remains changed and later resampling re-applies the edited shape, so the gesture is not truly reverted.
Useful? React with 👍 / 👎.
Comment on lines
+96
to
+99
| if (idx > 0) { | ||
| AnimationControlController.resampleCurveSegment( | ||
| boneName, channel, sorted[idx - 1], sorted[idx]) | ||
| } |
There was a problem hiding this comment.
This selects the immediate previous/next entries from row.keyTimes, which now includes synthetic frames inserted by prior resampling. After one resample pass, subsequent edits around the same key will target tiny sub-intervals (e.g., 0.500→0.516) instead of the original segment, so most of the curve no longer updates with the user's new tangent/mode edits.
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: 6
🧹 Nitpick comments (4)
src/CurveResampler.cpp (2)
75-86: 💤 Low valueComment is slightly off — last sample lands on
t1, total emitted =sampleCount.The wording "Emit
sampleCountinterior samples + the closing endpoint t1" reads assampleCount + 1total. The loop actually emits exactlysampleCountsamples in(t0, t1], with the last (i == sampleCount) landing ont1. The+1inout.reserve(...)is therefore an inert over-reserve.📝 Proposed wording tweak
- // Emit `sampleCount` interior samples + the closing endpoint t1. - // Skip t0 — the caller keeps the existing start keyframe. Step - // size lays the samples uniformly across (t0, t1]. - out.reserve(static_cast<size_t>(sampleCount) + 1); + // Emit `sampleCount` samples uniformly across (t0, t1]. Skip t0 + // — the caller keeps the existing start keyframe. The last sample + // (i == sampleCount) lands exactly on t1. + out.reserve(static_cast<size_t>(sampleCount));🤖 Prompt for 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. In `@src/CurveResampler.cpp` around lines 75 - 86, The comment and reserve are inconsistent: the loop for (int i = 1; i <= sampleCount; ++i) emits exactly sampleCount samples in (t0, t1] with the last sample at t1, so remove the inert over-reserve and/or reword the comment; update out.reserve(static_cast<size_t>(sampleCount)) (or change the comment to "Emit sampleCount samples across (t0, t1] with last at t1") and keep the existing step calculation and loop (step, sampleCount, out.reserve, and the for loop remain the references to change).
65-73: 💤 Low valueDead
hzreassignment after the cap.After
sampleCountis clamped tokMaxSamples,hzis recomputed but never used downstream — onlysampleCount(andstep = duration/sampleCount) drive the loop. Either drop the assignment or hoist it to be informational only.♻️ Proposed cleanup
const double duration = t1 - t0; int sampleCount = static_cast<int>(std::ceil(duration * hz)); if (sampleCount > kMaxSamples) { sampleCount = kMaxSamples; - hz = static_cast<int>(std::round(sampleCount / duration)); } if (sampleCount < 1) sampleCount = 1;🤖 Prompt for 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. In `@src/CurveResampler.cpp` around lines 65 - 73, After clamping sampleCount to kMaxSamples the line that reassigns hz (hz = static_cast<int>(std::round(sampleCount / duration))) is dead because downstream logic uses sampleCount (and step = duration/sampleCount) not hz; remove that reassignment (or if you intended to use hz later, update downstream to use hz consistently). Locate the block with duration, sampleCount, kMaxSamples and hz and delete the unnecessary hz = ... assignment (or alternatively propagate the new hz into subsequent calculations like step) to eliminate the dead write.src/commands/ResampleCurveCommand_test.cpp (1)
23-23: ⚡ Quick win
QThread::msleep(20)is a fragile cleanup guard — replace with event processing.A fixed 20 ms sleep is timing-sensitive; on a heavily loaded CI runner the previous
kill()callbacks may not have finished, causing the subsequenttryInitOgre()to see stale state.🔧 More robust alternative
- QThread::msleep(20); + if (app) app->processEvents();🤖 Prompt for 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. In `@src/commands/ResampleCurveCommand_test.cpp` at line 23, The fixed QThread::msleep(20) used as a cleanup guard is fragile; replace it with event processing to wait for the kill() callbacks to complete before calling tryInitOgre(). Specifically, remove the QThread::msleep(20) call and instead pump the Qt event loop (e.g., QCoreApplication::processEvents in a short loop with a deadline or use QTest::qWaitFor with a condition) until the expected state (callbacks finished or a flag set by those callbacks) is reached or a timeout elapses; reference the existing kill() callbacks and the tryInitOgre() invocation to locate where to add the event-processing wait.qml/AnimationCurveEditor.qml (1)
84-104: ⚡ Quick win
resampleAroundignoresboneNamewhen looking up key times — latent correctness gap.
selectedBoneRow()returns the currently selected bone's row regardless ofboneName. If these ever differ,sortedcontains the wrong track's timestamps and thet0/t1boundaries passed toresampleCurveSegmentwill be from the wrong bone's track. All current call sites happen to pass the selected bone's name, so there is no runtime bug today, but the parameter is misleading and the function will silently misbehave if called with any other bone.🔧 Suggested fix — look up the row by
boneNamefunction resampleAround(boneName, channel, keyTime) { - var row = selectedBoneRow() + var row = null + for (var i = 0; i < rows.length; i++) { + if (rows[i].bone === boneName) { row = rows[i]; break } + } if (!row || !row.keyTimes) return🤖 Prompt for 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. In `@qml/AnimationCurveEditor.qml` around lines 84 - 104, resampleAround currently calls selectedBoneRow() and ignores the boneName parameter, so look up the correct row using boneName instead of selectedBoneRow() before computing sorted keyTimes; then use that row.keyTimes to build sorted and compute idx and pass the correct t0/t1 to AnimationControlController.resampleCurveSegment (preserving existing checks for idx and boundaries). Ensure you still handle missing rows or missing keyTimes by returning early, and keep the function signature resampleAround(boneName, channel, keyTime) and the calls to AnimationControlController.resampleCurveSegment unchanged.
🤖 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 613-619: CurveEditModel tangents are not restored by undo because
only the Ogre track resample (ResampleCurveCommand) is pushed on release;
capture and restore tangent state as part of the undo. Implement one of two
fixes: (A) snapshot tangents at drag start (when entering panArea.dragMode ==
"tangent") and serialize them into ResampleCurveCommand so
ResampleCurveCommand::undo() restores CurveEditModel.setTangents(...) back to
the pre-drag values, or (B) on drag release push a companion SetTangentsCommand
(or wrap both in a macro command) before/with the existing ResampleCurveCommand
so undo/redo applies both the CurveEditModel tangents and the Ogre resample
together; update the code paths around panArea.dragMode handling (the release
branch invoking root.resampleAround(...)) to perform the chosen approach and
ensure CurveEditModel.setTangents and the Ogre track mutation are reversed in
the same undo step.
- Around line 96-103: The current resampleAround logic calls
AnimationControlController.resampleCurveSegment twice for interior keys which
pushes two separate ResampleCurveCommand entries and breaks the single-Ctrl+Z
goal; fix by creating a single C++ entry point that groups both resamples into
one undoable action — either expose QUndoStack::beginMacro()/endMacro() to QML
via Q_INVOKABLE methods on AnimationControlController and wrap the two resample
calls in a macro, or add a new Q_INVOKABLE method resampleAroundKey(boneName,
channel, keyTime) that constructs a parent QUndoCommand containing the two
ResampleCurveCommand children (or otherwise performs both resamples under one
undo command) so a single undo reverts both segments.
In `@src/AnimationControlController.cpp`:
- Around line 1424-1460: Add a Sentry breadcrumb for the resample gesture inside
AnimationControlController::resampleCurveSegment so the UI action is tracked;
specifically call SentryReporter::addBreadcrumb("ui.action", ...) (e.g. message
like "resampleCurve: animation=<m_selectedAnimation> bone=<boneStd>
channel=<channel> t0=<t0> t1=<t1>") just before creating/pushing the
ResampleCurveCommand (before the UndoManager::getSingleton()->push(cmd)) so the
gesture is recorded as the user-visible, undoable action.
In `@src/commands/ResampleCurveCommand_test.cpp`:
- Around line 35-38: The helper function trackOf currently dereferences
_getNodeTrackList().begin()->second which UB-crashes if the map is empty; modify
trackOf (the static Ogre::NodeAnimationTrack* trackOf(Ogre::Entity* e)) to check
that e, e->getSkeleton(), e->getSkeleton()->getAnimation("TestAnim") and that
_getNodeTrackList() is not empty (i.e., begin()!=end()) and return nullptr if
any are missing instead of dereferencing; update callers to add ASSERT_NE(track,
nullptr) after calling trackOf so tests fail cleanly when no track exists.
- Around line 29-32: TearDown currently only calls CurveEditModel::kill(),
leaving Ogre/Manager state alive; add a symmetric Manager::kill() call in
TearDown (matching the call made in SetUp) so Manager::kill() runs after each
test to fully clean up resources created by createAnimatedTestEntity and other
Manager-managed objects; update the TearDown implementation to invoke
Manager::kill() (after or before CurveEditModel::kill() as appropriate) and keep
the existing app->processEvents() logic.
In `@src/commands/ResampleCurveCommand.cpp`:
- Around line 230-241: The redo() path sets mCaptured before verifying
resampleAndWrite() succeeded, so a failed first redo leaves mCaptured true and
mAfter empty causing future redo() to call applySnapshot(mAfter) and delete
interior keyframes; fix by deferring setting mCaptured (and only leave early
after resampleAndWrite()) until resampleAndWrite() returns success: call
captureBefore(), then run resampleAndWrite(), and only when resampleAndWrite()
succeeds set mCaptured = true and store mAfter (and/or bail out without changing
mCaptured or mAfter on failure); update ResampleCurveCommand::redo to do this
and ensure applySnapshot(mAfter) is only reached when mAfter is valid.
---
Nitpick comments:
In `@qml/AnimationCurveEditor.qml`:
- Around line 84-104: resampleAround currently calls selectedBoneRow() and
ignores the boneName parameter, so look up the correct row using boneName
instead of selectedBoneRow() before computing sorted keyTimes; then use that
row.keyTimes to build sorted and compute idx and pass the correct t0/t1 to
AnimationControlController.resampleCurveSegment (preserving existing checks for
idx and boundaries). Ensure you still handle missing rows or missing keyTimes by
returning early, and keep the function signature resampleAround(boneName,
channel, keyTime) and the calls to
AnimationControlController.resampleCurveSegment unchanged.
In `@src/commands/ResampleCurveCommand_test.cpp`:
- Line 23: The fixed QThread::msleep(20) used as a cleanup guard is fragile;
replace it with event processing to wait for the kill() callbacks to complete
before calling tryInitOgre(). Specifically, remove the QThread::msleep(20) call
and instead pump the Qt event loop (e.g., QCoreApplication::processEvents in a
short loop with a deadline or use QTest::qWaitFor with a condition) until the
expected state (callbacks finished or a flag set by those callbacks) is reached
or a timeout elapses; reference the existing kill() callbacks and the
tryInitOgre() invocation to locate where to add the event-processing wait.
In `@src/CurveResampler.cpp`:
- Around line 75-86: The comment and reserve are inconsistent: the loop for (int
i = 1; i <= sampleCount; ++i) emits exactly sampleCount samples in (t0, t1] with
the last sample at t1, so remove the inert over-reserve and/or reword the
comment; update out.reserve(static_cast<size_t>(sampleCount)) (or change the
comment to "Emit sampleCount samples across (t0, t1] with last at t1") and keep
the existing step calculation and loop (step, sampleCount, out.reserve, and the
for loop remain the references to change).
- Around line 65-73: After clamping sampleCount to kMaxSamples the line that
reassigns hz (hz = static_cast<int>(std::round(sampleCount / duration))) is dead
because downstream logic uses sampleCount (and step = duration/sampleCount) not
hz; remove that reassignment (or if you intended to use hz later, update
downstream to use hz consistently). Locate the block with duration, sampleCount,
kMaxSamples and hz and delete the unnecessary hz = ... assignment (or
alternatively propagate the new hz into subsequent calculations like step) to
eliminate the dead write.
🪄 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: 6b8e22c2-42f8-440d-8d29-941ec66d8f70
📒 Files selected for processing (11)
qml/AnimationCurveEditor.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/CMakeLists.txtsrc/CurveResampler.cppsrc/CurveResampler.hsrc/CurveResampler_test.cppsrc/commands/ResampleCurveCommand.cppsrc/commands/ResampleCurveCommand.hsrc/commands/ResampleCurveCommand_test.cpptests/CMakeLists.txt
fernandotonon
added a commit
that referenced
this pull request
May 5, 2026
… anchors Addresses the CodeRabbit + ChatGPT-Codex review on PR #395: - New CurveEditModelChangeCommand records the (in, out, mode) entry's pre/post state. Pairs with ResampleCurveCommand inside a QUndoStack macro so a single Ctrl+Z reverts BOTH the model side-table AND the resampled TransformKeyFrames. Previously only Ogre's keyframes were on the undo stack — undo left the model with the new tangents/mode applied to a now-stale segment. - editCurveAndResampleAround / resampleAround now take an explicit anchorTimes list. The QML caller snapshots row.keyTimes BEFORE any preview/resample so subsequent passes use the AUTHORED key list, not the dense post-resample one (codex regression: previously each pass pulled neighbors from the live keyTimes which grew with every resample, so segments converged onto synthetic frames instead of the user's real keyframes). - Both functions use beginMacro/endMacro for multi-segment edits so a key with neighbors on both sides collapses to one undo entry. - ResampleCurveCommand: only set mCaptured AFTER resampleAndWrite() succeeds. A failed first redo would otherwise turn subsequent redos into the "replay mAfter" branch with empty mAfter — a destructive no-op that wipes interior keyframes on the next play. - Added Sentry breadcrumb for ui.action. - ResampleCurveCommand_test: TearDown now kills Manager too, trackOf guards against empty track list, all call sites ASSERT_NE on the pointer. - New tests: CurveEditModelChangeCommand_test (pure-data round-trip) and AnimationControlController tests for the macro behavior: EditCurveAndResampleAroundIsSingleUndoStep, EditCurveUndoRestoresModelMode, ResampleAroundUsesAuthoredAnchors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai all four of your flagged issues on commit
CI is green: build-{linux,macos,windows} pass, unit-tests-linux pass with new |
|
Tip For best results, initiate chat on the files or code changes.
CI green across all three platforms with the new test suites and SonarCloud passing is a great sign. Nice work closing these out! 🎉 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
fernandotonon
added a commit
that referenced
this pull request
May 5, 2026
Animation::setInterpolationMode is per-Animation, not per-track — so flipping it to IM_SPLINE because one bone went Bezier visibly distorts every other bone's track in the same animation, including non-skinned ones whose authoring expected linear interp (user reported this on PR #395). setCurveHandle now leaves the animation's interp mode untouched. The curve editor canvas paints the authored shape; users opt in to a per-bone resample via the Bake button when they want playback to match exactly. Dense linear keyframes track any curve shape closely regardless of the animation's interp mode. Test updated: SetCurveHandleSyncsOgreInterpolation → renamed to SetCurveHandleDoesNotMutateAnimInterp, asserts the interp mode stays at its pre-edit value across Bezier/Stepped edits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
qml/AnimationCurveEditor.qml (1)
217-227:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBake still splits one click into multiple undo steps.
This loop calls
resampleAllSegmentsForBone()once per active channel, and each call creates its own undo macro. On a bone with multiple active curves, a single Bake click now needs multiple Ctrl+Z presses and can leave the bake half-undone. Please move the per-channel loop behind one controller-side entry point so the whole click is undone atomically.🤖 Prompt for 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. In `@qml/AnimationCurveEditor.qml` around lines 217 - 227, The Bake button currently calls AnimationControlController.resampleAllSegmentsForBone(...) once per active channel in the loop (using root.channelOrder), causing multiple undo macros; instead add a single controller entrypoint (e.g. AnimationControlController.resampleAllSegmentsForBoneAllChannels(selectedBone)) that accepts the bone (root.selectedBone) and performs the per-channel iteration and resampling server-side inside one undo macro, then replace the per-channel loop in AnimationCurveEditor.qml (the onClicked handler that uses root.selectedBoneRow() and root.channelOrder) with a single call to that new controller method.src/AnimationControlController.cpp (1)
1513-1521:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUndo/redo never re-syncs playback interpolation.
syncOgreInterpolationMode()only runs after the initialpush(). Later undo/redo replaysCurveEditModelChangeCommandwithout re-running that sync, so reverting the last curved key back/forth can leave playback in the stale interpolation mode until another explicit handle edit happens. Please trigger the sync from the undo/redo path as well, not just the initial commit.🤖 Prompt for 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. In `@src/AnimationControlController.cpp` around lines 1513 - 1521, The playback interpolation mode isn't re-synced when undo/redo replays CurveEditModelChangeCommand; call syncOgreInterpolationMode() from the undo/redo path too. Modify CurveEditModelChangeCommand so its redo() and undo() implementations invoke the controller's syncOgreInterpolationMode() (or emit a signal the controller listens to) after applying the key change, rather than relying only on the push() call; this ensures UndoManager::getSingleton()->push(cmd) and subsequent redo()/undo() both restore the correct interpolation via syncOgreInterpolationMode().
🤖 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 `@src/AnimationControlController.cpp`:
- Around line 1494-1522: setCurveHandle performs undoable edits but doesn't
record a Sentry breadcrumb; add a SentryReporter::addBreadcrumb call just before
pushing the CurveEditModelChangeCommand (i.e., before
UndoManager::getSingleton()->push(cmd)) in
AnimationControlController::setCurveHandle to log the action. Use category
"ui.action" and include a concise message with identifiers like
m_selectedEntityName/m_selectedAnimation, boneName, channel (lowercased),
keyTime, and the mode/tangent change (oldMode->finalMode or newIn/newOut) so the
breadcrumb captures the user-facing curve-handle edit; keep the breadcrumb call
separate and then call syncOgreInterpolationMode as before.
In `@src/AnimationControlController.h`:
- Around line 258-285: Add Sentry breadcrumbs for the two missing Q_INVOKABLE
methods: in setCurveHandle, after the undo command is pushed (the
ResampleCurveHandle or whatever undo command creation in that method) call
SentryReporter::addBreadcrumb("ui.action", ...) with a concise message including
boneName, channel and keyTime to record the handle change; in
resampleAllSegmentsForBone, add a single breadcrumb for the overall macro
operation (either immediately after beginMacro() or after endMacro()) using
SentryReporter::addBreadcrumb("ui.action", ...) with a message including
boneName and channel to record the batched resample action. Ensure messages are
descriptive and use the existing SentryReporter API consistent with
resampleCurveSegment's breadcrumb.
In `@src/commands/ResampleCurveCommand.cpp`:
- Around line 246-249: undo() must not call applySnapshot(mBefore) when
captureBefore() never succeeded; modify ResampleCurveCommand::undo to check the
mCaptured flag (set by captureBefore/redo) and return early if !mCaptured to
avoid applying an empty mBefore. Locate functions/members:
ResampleCurveCommand::undo, captureBefore(), redo(), mCaptured, mBefore,
applySnapshot(), and resolveTrack to ensure the guard is used before calling
applySnapshot(mBefore) so empty snapshots are not applied when the track was
never captured.
- Around line 208-223: The resampling code writes back a single quaternion
component (mChannel ∈ {rw,rx,ry,rz}) after setting a unit quaternion from
Ogre::Quaternion::Slerp, producing non-unit quaternions and interpolation
artifacts; fix by, after writeChannel(kf, mChannel, s.value), detect if mChannel
is one of the rotation components and if so read the quaternion from kf,
normalize it, and write the normalized quaternion back via
kf->setRotation(normalizedQuat) before snapshotKf(kf); alternatively (if you
prefer semantics that reject component resampling) add a guard in
resampleCurveSegment/resampleAllSegmentsForBone to skip/error on
rotation-component channels with a clear comment.
---
Duplicate comments:
In `@qml/AnimationCurveEditor.qml`:
- Around line 217-227: The Bake button currently calls
AnimationControlController.resampleAllSegmentsForBone(...) once per active
channel in the loop (using root.channelOrder), causing multiple undo macros;
instead add a single controller entrypoint (e.g.
AnimationControlController.resampleAllSegmentsForBoneAllChannels(selectedBone))
that accepts the bone (root.selectedBone) and performs the per-channel iteration
and resampling server-side inside one undo macro, then replace the per-channel
loop in AnimationCurveEditor.qml (the onClicked handler that uses
root.selectedBoneRow() and root.channelOrder) with a single call to that new
controller method.
In `@src/AnimationControlController.cpp`:
- Around line 1513-1521: The playback interpolation mode isn't re-synced when
undo/redo replays CurveEditModelChangeCommand; call syncOgreInterpolationMode()
from the undo/redo path too. Modify CurveEditModelChangeCommand so its redo()
and undo() implementations invoke the controller's syncOgreInterpolationMode()
(or emit a signal the controller listens to) after applying the key change,
rather than relying only on the push() call; this ensures
UndoManager::getSingleton()->push(cmd) and subsequent redo()/undo() both restore
the correct interpolation via syncOgreInterpolationMode().
🪄 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: cf3f7b2a-4373-43c9-b7d4-9627ca3968fe
📒 Files selected for processing (14)
qml/AnimationCurveEditor.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationControlController_test.cppsrc/CMakeLists.txtsrc/CurveResampler.cppsrc/CurveResampler.hsrc/CurveResampler_test.cppsrc/commands/CurveEditModelChangeCommand.cppsrc/commands/CurveEditModelChangeCommand.hsrc/commands/CurveEditModelChangeCommand_test.cppsrc/commands/ResampleCurveCommand.cppsrc/commands/ResampleCurveCommand_test.cpptests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (2)
- src/CMakeLists.txt
- tests/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CurveResampler.h
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/commands/ResampleCurveCommand.cpp (2)
251-254:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
undo()still missing!mCapturedguard.If
captureBefore()failed during the first redo (e.g., entity not yet resolvable,resolveTrackreturnednullptr),mCapturedremainsfalseandmBeforestays empty. A laterundo()here would callapplySnapshot({}), which strips every interior keyframe in(mT0, mT1)and reinserts nothing. This past comment was not marked as addressed in commit 69362b4.🛡️ Proposed guard
void ResampleCurveCommand::undo() { + if (!mCaptured) return; applySnapshot(mBefore); }🤖 Prompt for 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. In `@src/commands/ResampleCurveCommand.cpp` around lines 251 - 254, The undo() method should guard against using an empty snapshot if captureBefore() never succeeded: check the boolean mCaptured at the start of ResampleCurveCommand::undo() and return early when false instead of calling applySnapshot(mBefore); ensure this uses the same mCaptured flag set by captureBefore()/redo(), so applySnapshot(mBefore) is only invoked when mCaptured is true and mBefore contains a valid snapshot (related symbols: ResampleCurveCommand::undo, captureBefore, mCaptured, mBefore, applySnapshot, resolveTrack).
213-229:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRotation channels still write non-unit quaternions.
After
Ogre::Quaternion::Slerp(u, rA, rB, true)produces a unit quaternion at line 219,writeChannel(kf, mChannel, s.value)at line 227 overwrites a single component whenmChannel ∈ {rw, rx, ry, rz}, breaking the unit-length invariant.Ogre::NodeAnimationTrackfeeds adjacent keyframe quaternions directly intoQuaternion::Slerpduring playback, and non-unit inputs produce incorrect geodesic interpolation. This past comment was not marked as addressed in commit 69362b4 — please either reject rotation channels in the controller or re-normalize afterwriteChannelfor rotation components.🤖 Prompt for 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. In `@src/commands/ResampleCurveCommand.cpp` around lines 213 - 229, The resampled rotation quaternions are being corrupted when writeChannel(kf, mChannel, s.value) overwrites a single component (mChannel ∈ {rw, rx, ry, rz}); to fix, after creating the keyframe (createNodeKeyFrame) and calling setRotation(ro) then writeChannel(...), detect if mChannel targets a rotation component and if so read back the keyframe rotation, normalize it (make it unit length) and set it again via kf->setRotation(...); do this before snapshotKf(kf) so NodeAnimationTrack only ever stores unit quaternions.src/AnimationControlController.cpp (1)
1504-1537:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
setCurveHandlestill missing aui.actionbreadcrumb.Curve-handle edits (interpolation-mode change, tangent-drag commit) are user-driven, undoable gestures equivalent in significance to
setAutoKey/moveKeyframes/pasteKeyframesAt, all of which emit breadcrumbs in this file. This past comment was not marked as addressed in commit 69362b4. As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks ...".📝 Proposed addition
UndoManager::getSingleton()->push(cmd); + SentryReporter::addBreadcrumb( + "ui.action", + QString("Curve Editor: edit handle %1.%2 @ %3s") + .arg(boneName, channel.toLower()) + .arg(keyTime, 0, 'f', 3)); // Don't touch Ogre's per-Animation interp mode here:🤖 Prompt for 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. In `@src/AnimationControlController.cpp` around lines 1504 - 1537, Add a Sentry breadcrumb when a user edits a curve by calling SentryReporter::addBreadcrumb("ui.action", ...) inside AnimationControlController::setCurveHandle; include concise context such as the boneName, channel (lowercased), keyTime and finalMode or that tangents/mode were changed, and place it alongside the undo push (e.g., just before or after UndoManager::getSingleton()->push(cmd)) so every user-driven curve-handle edit (interpolation-mode/tangent change via CurveEditModelChangeCommand) is recorded.
🧹 Nitpick comments (3)
src/commands/ResampleCurveCommand.h (1)
40-45: ⚖️ Poor tradeoff
KeyframeSnapshotis duplicated fromDecimateTrackCommand.h.Both command headers define an identical
KeyframeSnapshotstruct (time,translate,rotation,scale). A sharedAnimationCommandTypes.h(or similar) could host the struct and be included by both.🤖 Prompt for 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. In `@src/commands/ResampleCurveCommand.h` around lines 40 - 45, Move the duplicate KeyframeSnapshot struct out of ResampleCurveCommand.h and DecimateTrackCommand.h into a new shared header (e.g., AnimationCommandTypes.h) and include that header from both command headers; specifically, create AnimationCommandTypes.h with the struct definition (time, translate, rotation, scale), replace the local struct definitions in ResampleCurveCommand.h and DecimateTrackCommand.h with `#include` "AnimationCommandTypes.h" (or equivalent include guard/pragma once), and ensure all references to KeyframeSnapshot in functions or methods remain unchanged so compilation continues to work.src/commands/DecimateTrackCommand.cpp (1)
103-104: 💤 Low valueRedundant
!kept.empty()guard indecimate().
kept.size() > 1implies!kept.empty(), so the first sub-expression is alwaystruewhen the second istrue.♻️ Proposed simplification
- if (!kept.empty() && last.time - kept.back().time < minGap - kEps - && kept.size() > 1) { + if (kept.size() > 1 && last.time - kept.back().time < minGap - kEps) {🤖 Prompt for 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. In `@src/commands/DecimateTrackCommand.cpp` around lines 103 - 104, The condition in decimate() redundantly checks !kept.empty() because kept.size() > 1 already guarantees non-empty; simplify the if by removing the !kept.empty() sub-expression and ensure the size check remains (e.g., evaluate kept.size() > 1 before accessing kept.back()) so you don't call kept.back() on an empty container.src/AnimationControlController.cpp (1)
1432-1459: 💤 Low valueRemove
neighborAnchorsas unused function.The function is dead code. Ripgrep found zero callers across the entire codebase; it exists only as a definition in an anonymous namespace.
🤖 Prompt for 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. In `@src/AnimationControlController.cpp` around lines 1432 - 1459, Remove the dead helper function neighborAnchors (and its inner constexpr kEps and local variables) from the anonymous namespace since it has no callers; delete the entire function definition (signature bool neighborAnchors(double keyTime, const QVariantList& anchorTimes, double& prevOut, double& nextOut) and its body) to eliminate unused code and related warnings, and run a build to ensure nothing depended on its symbols.
🤖 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 `@src/commands/DecimateTrackCommand_test.cpp`:
- Around line 96-114: The test FirstAndLastFramesPreserved currently never
triggers decimation because the existing 3 keyframes (t=0.0,0.5,1.0) are spaced
wider than the minGap; update the test (DecimateTrackCommandTest / TEST_F
FirstAndLastFramesPreserved) to insert several dense intermediate keyframes into
the track (use trackOf(entity) and getKeyFrame()/getNumKeyFrames() helpers or
create new keyframes via the same API used elsewhere) so decimate() will drop
some middle frames when you call DecimateTrackCommand::redo(); then assert that
the first and last keyframe times (captured from getKeyFrame(0)->getTime() and
getKeyFrame(track->getNumKeyFrames()-1)->getTime()) remain equal to the
originals after cmd.redo() and optionally verify mAfter.size() < mBefore.size()
semantics by checking the keyframe count changed.
In `@src/commands/DecimateTrackCommand.cpp`:
- Around line 129-132: undo() should guard against an empty/un-captured
snapshot: check the mCaptured flag before calling replaceTrack(mBefore) and
return immediately if mCaptured is false. This mirrors the ResampleCurveCommand
pattern — ensure snapshotTrack(mBefore) sets mCaptured on redo()/constructor and
then gate DecimateTrackCommand::undo() with if (!mCaptured) return; before
invoking replaceTrack(mBefore).
---
Duplicate comments:
In `@src/AnimationControlController.cpp`:
- Around line 1504-1537: Add a Sentry breadcrumb when a user edits a curve by
calling SentryReporter::addBreadcrumb("ui.action", ...) inside
AnimationControlController::setCurveHandle; include concise context such as the
boneName, channel (lowercased), keyTime and finalMode or that tangents/mode were
changed, and place it alongside the undo push (e.g., just before or after
UndoManager::getSingleton()->push(cmd)) so every user-driven curve-handle edit
(interpolation-mode/tangent change via CurveEditModelChangeCommand) is recorded.
In `@src/commands/ResampleCurveCommand.cpp`:
- Around line 251-254: The undo() method should guard against using an empty
snapshot if captureBefore() never succeeded: check the boolean mCaptured at the
start of ResampleCurveCommand::undo() and return early when false instead of
calling applySnapshot(mBefore); ensure this uses the same mCaptured flag set by
captureBefore()/redo(), so applySnapshot(mBefore) is only invoked when mCaptured
is true and mBefore contains a valid snapshot (related symbols:
ResampleCurveCommand::undo, captureBefore, mCaptured, mBefore, applySnapshot,
resolveTrack).
- Around line 213-229: The resampled rotation quaternions are being corrupted
when writeChannel(kf, mChannel, s.value) overwrites a single component (mChannel
∈ {rw, rx, ry, rz}); to fix, after creating the keyframe (createNodeKeyFrame)
and calling setRotation(ro) then writeChannel(...), detect if mChannel targets a
rotation component and if so read back the keyframe rotation, normalize it (make
it unit length) and set it again via kf->setRotation(...); do this before
snapshotKf(kf) so NodeAnimationTrack only ever stores unit quaternions.
---
Nitpick comments:
In `@src/AnimationControlController.cpp`:
- Around line 1432-1459: Remove the dead helper function neighborAnchors (and
its inner constexpr kEps and local variables) from the anonymous namespace since
it has no callers; delete the entire function definition (signature bool
neighborAnchors(double keyTime, const QVariantList& anchorTimes, double&
prevOut, double& nextOut) and its body) to eliminate unused code and related
warnings, and run a build to ensure nothing depended on its symbols.
In `@src/commands/DecimateTrackCommand.cpp`:
- Around line 103-104: The condition in decimate() redundantly checks
!kept.empty() because kept.size() > 1 already guarantees non-empty; simplify the
if by removing the !kept.empty() sub-expression and ensure the size check
remains (e.g., evaluate kept.size() > 1 before accessing kept.back()) so you
don't call kept.back() on an empty container.
In `@src/commands/ResampleCurveCommand.h`:
- Around line 40-45: Move the duplicate KeyframeSnapshot struct out of
ResampleCurveCommand.h and DecimateTrackCommand.h into a new shared header
(e.g., AnimationCommandTypes.h) and include that header from both command
headers; specifically, create AnimationCommandTypes.h with the struct definition
(time, translate, rotation, scale), replace the local struct definitions in
ResampleCurveCommand.h and DecimateTrackCommand.h with `#include`
"AnimationCommandTypes.h" (or equivalent include guard/pragma once), and ensure
all references to KeyframeSnapshot in functions or methods remain unchanged so
compilation continues to work.
🪄 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: cbe8a2a8-830d-49d7-9923-db6c959f934b
📒 Files selected for processing (15)
qml/AnimationCurveEditor.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/AnimationControlController_test.cppsrc/CMakeLists.txtsrc/CurveResampler.cppsrc/CurveResampler.hsrc/CurveResampler_test.cppsrc/commands/DecimateTrackCommand.cppsrc/commands/DecimateTrackCommand.hsrc/commands/DecimateTrackCommand_test.cppsrc/commands/ResampleCurveCommand.cppsrc/commands/ResampleCurveCommand.hsrc/mainwindow.cpptests/CMakeLists.txt
✅ Files skipped from review due to trivial changes (1)
- src/CMakeLists.txt
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/CMakeLists.txt
- qml/AnimationCurveEditor.qml
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/AnimationControlController.cpp (1)
1525-1531:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRecord curve-handle commits in Sentry.
setCurveHandle()still pushes an undoable UI action without a breadcrumb, so mode/tangent edits are missing from action traces.📝 Minimal fix
auto* cmd = new CurveEditModelChangeCommand( // NOSONAR — stack owns m_selectedEntityName, m_selectedAnimation, boneName.toStdString(), channel.toLower().toStdString(), keyTime, oldIn, oldOut, oldMode, newInTangent, newOutTangent, finalMode); UndoManager::getSingleton()->push(cmd); + SentryReporter::addBreadcrumb( + "ui.action", + QString("Curve Editor: edit handle %1.%2 @ %3s") + .arg(boneName, channel.toLower()) + .arg(keyTime, 0, 'f', 3));As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import'/'file.export' for I/O operations."
🤖 Prompt for 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. In `@src/AnimationControlController.cpp` around lines 1525 - 1531, setCurveHandle currently pushes a CurveEditModelChangeCommand without recording a Sentry breadcrumb, so add a call to SentryReporter::addBreadcrumb("ui.action", message) just before UndoManager::getSingleton()->push(cmd); construct the message string to include identifying context such as m_selectedEntityName, m_selectedAnimation, boneName.toStdString(), channel.toLower().toStdString(), keyTime and the mode/tangent change (oldMode/oldIn/oldOut → finalMode/newInTangent/newOutTangent) so traces show the curve-handle edit; place this breadcrumb call in the same scope that creates the CurveEditModelChangeCommand in setCurveHandle.
🧹 Nitpick comments (2)
src/PropertiesPanelController.cpp (2)
836-845: ⚡ Quick win
reduceAnimationToFpsis missing the bulk-refresh suspend used inbakeAnimation.
bakeAnimationwraps the per-bone loop withsetRowsRefreshSuspended(true)/falseplus a singlerefreshAfterBulkResample()to coalesce thousands of dope-sheet rebuilds.reduceAnimationToFpsperforms the same per-bone iteration throughreduceTrackToFpsbut doesn't apply this optimization. On a rig with ~50 bones the resulting refresh storm reproduces the freeze that motivated the suspend mechanism.♻️ Suggested change
auto* stack = UndoManager::getSingleton()->stack(); stack->beginMacro(QObject::tr("Reduce animation")); int totalRemoved = 0; + animCtrl->setRowsRefreshSuspended(true); for (const auto& [handle, track] : anim->_getNodeTrackList()) { Ogre::Node* node = track->getAssociatedNode(); if (!node) continue; const QString boneName = QString::fromStdString(node->getName()); totalRemoved += animCtrl->reduceTrackToFps(boneName, targetFps); } + animCtrl->setRowsRefreshSuspended(false); stack->endMacro(); + animCtrl->refreshAfterBulkResample();🤖 Prompt for 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. In `@src/PropertiesPanelController.cpp` around lines 836 - 845, reduceAnimationToFps currently iterates per-bone and calls reduceTrackToFps without suspending UI row refreshes, causing a refresh storm; wrap the per-track loop with animCtrl->setRowsRefreshSuspended(true) before beginning the macro/loop and animCtrl->setRowsRefreshSuspended(false) after ending the macro, then call animCtrl->refreshAfterBulkResample() once to coalesce updates (mirror what bakeAnimation does), keeping the existing UndoManager::getSingleton()->stack()/beginMacro()/endMacro() and iterating anim->_getNodeTrackList() as before.
785-800: 💤 Low valueSuspend flag and undo macro are not exception-safe.
If
resampleAllSegmentsForBone(or anything betweenbeginMacroandendMacro) throws — e.g., from Ogre internals or an allocation — the suspend flag staystrueand the macro is left open, leaving subsequent edits collapsed under the wrong undo entry and the dope sheet permanently un-refreshing. Wrap with RAII guards (ortry/catch) so cleanup is unconditional.♻️ Sketch
// scope guard pattern struct SuspendGuard { AnimationControlController* c; ~SuspendGuard() { c->setRowsRefreshSuspended(false); } }; animCtrl->setRowsRefreshSuspended(true); SuspendGuard sg{animCtrl}; // ... loop ... // macro can use a similar guard or std::unique_ptr with custom deleter🤖 Prompt for 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. In `@src/PropertiesPanelController.cpp` around lines 785 - 800, The suspend flag and undo macro need exception-safe cleanup: ensure setRowsRefreshSuspended(false) and stack->endMacro() always run even if animCtrl->resampleAllSegmentsForBone (or anything in the loop) throws. Implement RAII guards (or a try/finally pattern) around the region that calls animCtrl->setRowsRefreshSuspended(true) and stack->beginMacro(...) so their destructors/unwind code calls animCtrl->setRowsRefreshSuspended(false) and stack->endMacro(), and keep the call to animCtrl->refreshAfterBulkResample() after those guards to guarantee the dope sheet is refreshed.
🤖 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 388-400: applyMode currently only updates CurveEditModel state
(via CurveEditModel.tangentsAt and AnimationControlController.setCurveHandle)
and does not resample/write the changed segment(s) back into Ogre, so live
playback uses stale TransformKeyFrames; fix by making applyMode (or the command
invoked by AnimationControlController.setCurveHandle) perform a grouped
resample/write of the affected segment(s) into Ogre immediately (or record the
resample in the redo/undo command so replay/undo also rewrites those
TransformKeyFrames), ensuring the same affected boneName/channelId/keyTime
ranges that were read from CurveEditModel are rewritten into Ogre so canvas and
playback stay in sync.
- Around line 241-248: The combo's onActivated handler ignores the final ("Set
to 60 FPS") entry because it only accepts indices 1..6; update the conditional
in onActivated to include the last index and pass the correct density to bake
(e.g., change the guard from "if (index >= 1 && index <= 6) bake(index - 1)" to
allow index 7 as well so that bake(index - 1) is called for the last entry),
keeping the currentIndex = 0 snap behavior unchanged.
- Around line 229-239: The bake() loop calls
AnimationControlController.resampleAllSegmentsForBone per channel causing
repeated decimation and multiple undo entries; change the implementation so the
controller offers a single operation (e.g. resampleAllChannelsForBone or extend
resampleAllSegmentsForBone to accept an array of channel IDs) that: collects
active channel ids from root.channelOrder for root.selectedBoneRow(), computes a
single anchor/decimation pass against the underlying Ogre NodeAnimationTrack,
resamples all channels using that same anchor set, and performs the rewrite
inside one undo macro; then update bake() to call that single controller method
with root.selectedBone and the list of channel ids (and density).
In `@qml/PropertiesPanel.qml`:
- Around line 2052-2061: The onActivated handler for the density popup is
off-by-one: it currently only calls PropertiesPanelController.bakeAnimation when
index is between 1 and 6, which omits the "Set to 60 FPS" entry at index 7;
update the guard in the onActivated function so it includes index 7 (e.g.,
change the condition to allow index up to 7 or otherwise map indices 1..7 to
bakeAnimation with index-1), leaving the subsequent currentIndex reset and
simplifyBtn.cachedAnalysis clear intact and ensuring
PropertiesPanelController.bakeAnimation(grp.entity, modelData.name, index - 1)
is invoked for the 60 FPS selection.
In `@qml/ThemedComboBox.qml`:
- Around line 87-90: The code assigns the Qt 6.8+ property popupType in
ThemedComboBox.qml but the build doesn't enforce Qt >= 6.8; either update the
CMake find_package call to require Qt 6.8+ (e.g., change the existing
find_package(Qt6 ...) to find_package(Qt6 6.8 REQUIRED COMPONENTS ...) so the
minimum Qt version is enforced) or guard the popupType assignment in
ThemedComboBox.qml with a runtime QML version check (e.g., use a Qt version
check/versionAtLeast(6,8) or inspect Qt.version before setting popupType) so
older Qt engines won't attempt to set the unsupported property.
In `@src/AnimationControlController.h`:
- Around line 282-293: Update the doc comment for resampleAllSegmentsForBone to
reflect the actual implemented density values (0–6) and their correct meanings:
0 = Sparse (12× tolerance, fewest keys), 1 = Medium (4× tolerance), 2 = Dense
(1× tolerance, full adaptive sampling), 3 = 10 FPS fixed-rate, 4 = 15 FPS
fixed-rate, 5 = 30 FPS fixed-rate, 6 = 60 FPS fixed-rate; replace the existing
0–4 list and incorrect FPS mappings in the Q_INVOKABLE comment block above the
resampleAllSegmentsForBone(const QString& boneName, const QString& channel, int
density = 0) declaration.
In `@src/PropertiesPanelController.cpp`:
- Around line 768-805: prevEntity and prevAnim may be empty and calling
animCtrl->selectAnimation(prevEntity, prevAnim) unconditionally can reset state;
change the restore logic in both bakeAnimation and reduceAnimationToFps to only
call animCtrl->selectAnimation(prevEntity, prevAnim) when prevEntity and
prevAnim are non-empty (e.g. check that !prevEntity.isEmpty() &&
!prevAnim.isEmpty() in addition to the existing comparison with
entityName/animName) so you only restore an actual prior selection.
In `@src/PropertiesPanelController.h`:
- Around line 217-225: Update the doc comment for Q_INVOKABLE int
bakeAnimation(const QString& entityName, const QString& animName, int density)
to list the correct density mapping used by the implementation: 0=Sparse,
1=Medium, 2=Dense, 3=10 FPS, 4=15 FPS, 5=30 FPS, 6=60 FPS (instead of the
outdated 0–4 mapping). Ensure the comment text and any parenthetical enumeration
exactly match those seven modes so the header accurately documents the
implementation.
---
Duplicate comments:
In `@src/AnimationControlController.cpp`:
- Around line 1525-1531: setCurveHandle currently pushes a
CurveEditModelChangeCommand without recording a Sentry breadcrumb, so add a call
to SentryReporter::addBreadcrumb("ui.action", message) just before
UndoManager::getSingleton()->push(cmd); construct the message string to include
identifying context such as m_selectedEntityName, m_selectedAnimation,
boneName.toStdString(), channel.toLower().toStdString(), keyTime and the
mode/tangent change (oldMode/oldIn/oldOut →
finalMode/newInTangent/newOutTangent) so traces show the curve-handle edit;
place this breadcrumb call in the same scope that creates the
CurveEditModelChangeCommand in setCurveHandle.
---
Nitpick comments:
In `@src/PropertiesPanelController.cpp`:
- Around line 836-845: reduceAnimationToFps currently iterates per-bone and
calls reduceTrackToFps without suspending UI row refreshes, causing a refresh
storm; wrap the per-track loop with animCtrl->setRowsRefreshSuspended(true)
before beginning the macro/loop and animCtrl->setRowsRefreshSuspended(false)
after ending the macro, then call animCtrl->refreshAfterBulkResample() once to
coalesce updates (mirror what bakeAnimation does), keeping the existing
UndoManager::getSingleton()->stack()/beginMacro()/endMacro() and iterating
anim->_getNodeTrackList() as before.
- Around line 785-800: The suspend flag and undo macro need exception-safe
cleanup: ensure setRowsRefreshSuspended(false) and stack->endMacro() always run
even if animCtrl->resampleAllSegmentsForBone (or anything in the loop) throws.
Implement RAII guards (or a try/finally pattern) around the region that calls
animCtrl->setRowsRefreshSuspended(true) and stack->beginMacro(...) so their
destructors/unwind code calls animCtrl->setRowsRefreshSuspended(false) and
stack->endMacro(), and keep the call to animCtrl->refreshAfterBulkResample()
after those guards to guarantee the dope sheet is refreshed.
🪄 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: c52c2f02-e953-457e-9a22-3aabb37ee386
📒 Files selected for processing (11)
qml/AnimationCurveEditor.qmlqml/PropertiesPanel.qmlqml/ThemedComboBox.qmlsrc/AnimationControlController.cppsrc/AnimationControlController.hsrc/CurveEditModel.cppsrc/CurveEditModel.hsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/mainwindow.cppsrc/qml_resources.qrc
🚧 Files skipped from review as they are similar to previous changes (1)
- src/mainwindow.cpp
Comment on lines
+229
to
+239
| function bake(density) { | ||
| var row = root.selectedBoneRow() | ||
| if (!row || !row.channels) return | ||
| for (var i = 0; i < root.channelOrder.length; i++) { | ||
| var ch = root.channelOrder[i] | ||
| if (row.channels[ch.id]) { | ||
| AnimationControlController.resampleAllSegmentsForBone( | ||
| root.selectedBone, ch.id, density) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Bake the whole bone in one controller-side operation.
Each iteration resamples the same Ogre NodeAnimationTrack, and resampleAllSegmentsForBone() decimates before it rewrites. That means later channels can throw away samples just baked for earlier channels, and the user gets one undo macro per channel instead of one bake action.
📝 Direction of fix
function bake(density) {
var row = root.selectedBoneRow()
if (!row || !row.channels) return
- for (var i = 0; i < root.channelOrder.length; i++) {
- var ch = root.channelOrder[i]
- if (row.channels[ch.id]) {
- AnimationControlController.resampleAllSegmentsForBone(
- root.selectedBone, ch.id, density)
- }
- }
+ var active = []
+ for (var i = 0; i < root.channelOrder.length; i++) {
+ var ch = root.channelOrder[i]
+ if (row.channels[ch.id]) active.push(ch.id)
+ }
+ AnimationControlController.resampleBoneChannels(
+ root.selectedBone, active, density)
}That C++ entry point should decimate once, resample all active channels against the same anchor set, and wrap the entire bake in a single undo macro.
🤖 Prompt for 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.
In `@qml/AnimationCurveEditor.qml` around lines 229 - 239, The bake() loop calls
AnimationControlController.resampleAllSegmentsForBone per channel causing
repeated decimation and multiple undo entries; change the implementation so the
controller offers a single operation (e.g. resampleAllChannelsForBone or extend
resampleAllSegmentsForBone to accept an array of channel IDs) that: collects
active channel ids from root.channelOrder for root.selectedBoneRow(), computes a
single anchor/decimation pass against the underlying Ogre NodeAnimationTrack,
resamples all channels using that same anchor set, and performs the rewrite
inside one undo macro; then update bake() to call that single controller method
with root.selectedBone and the list of channel ids (and density).
Comment on lines
+87
to
+90
| popupType: Popup.Window | ||
| y: control.height | ||
| width: control.width | ||
| implicitHeight: contentItem.implicitHeight | ||
| implicitHeight: Math.min(contentItem.implicitHeight + 2, 240) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Qt 6 ComboBox Popup popupType property since version
💡 Result:
Qt 6 ComboBox popupType property (in QML / Qt Quick Controls) was introduced in Qt 6.8 (it is a property of Popup, which ComboBox’s popup is an instance of). In Qt Quick Controls, the Popup type documents popupType: “popupType: enumeration [since 6.8]” and explains that it controls whether a popup is embedded in-scene (Popup.Item), shown in a separate top-level window (Popup.Window), or uses the platform-native implementation (Popup.Native). [1] [2] Therefore, the since version for popupType is Qt 6.8. [1][2]
Citations:
🏁 Script executed:
# First, let's read the themedComboBox.qml file to see the actual implementation
cat -n qml/ThemedComboBox.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 4699
🏁 Script executed:
# Search for build configuration files to determine Qt version requirements
fd -e CMakeLists.txt -e "\.pro$" | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
# Check for any Qt version checks in the codebase
rg "qt_version|QT_VERSION|6\.[5-9]" --type qml -A 2 -B 2Repository: fernandotonon/QtMeshEditor
Length of output: 572
🏁 Script executed:
# Find and read CMakeLists.txt
find . -name CMakeLists.txt -type f | head -10Repository: fernandotonon/QtMeshEditor
Length of output: 283
🏁 Script executed:
# Look for Qt version requirements in CMake files
rg "Qt 6\.|find_package.*Qt|CMAKE_PREFIX_PATH" -A 2 -B 2Repository: fernandotonon/QtMeshEditor
Length of output: 3280
🏁 Script executed:
# Check for .github/workflows or similar CI configuration
find . -path "*/.github/workflows/*.y*ml" -o -path "*/ci/*" -o -path "*/.gitlab-ci.yml" | head -10Repository: fernandotonon/QtMeshEditor
Length of output: 175
🏁 Script executed:
# Search for any Qt version checks or guards in QML files
rg "versionAtLeast|QT_VERSION|6\.[5-9]|version:" qml/ -B 2 -A 2Repository: fernandotonon/QtMeshEditor
Length of output: 572
🏁 Script executed:
# Check main CMakeLists.txt for any Qt version specifications
sed -n '1,100p' CMakeLists.txt | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 5211
🏁 Script executed:
# Search for any CI workflow files that specify Qt versions
cat .github/workflows/deploy.yml | head -50Repository: fernandotonon/QtMeshEditor
Length of output: 1716
CMakeLists.txt should enforce Qt 6.8+ minimum or code needs a version guard.
The popupType property is confirmed Qt 6.8+. However, CMakeLists.txt allows Qt6 without a minimum version constraint, and the inline comment's claim that it "falls back gracefully" is misleading. On Qt 6.0–6.7, the QML engine will emit a property warning, not silently ignore the assignment. Since CI builds with Qt 6.9.3, this doesn't surface in practice, but the build system should either enforce Qt6 6.8 REQUIRED in find_package() or wrap the popupType assignment in a runtime version check (e.g., using QML versionAtLeast or a C++ property check).
🤖 Prompt for 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.
In `@qml/ThemedComboBox.qml` around lines 87 - 90, The code assigns the Qt 6.8+
property popupType in ThemedComboBox.qml but the build doesn't enforce Qt >=
6.8; either update the CMake find_package call to require Qt 6.8+ (e.g., change
the existing find_package(Qt6 ...) to find_package(Qt6 6.8 REQUIRED COMPONENTS
...) so the minimum Qt version is enforced) or guard the popupType assignment in
ThemedComboBox.qml with a runtime QML version check (e.g., use a Qt version
check/versionAtLeast(6,8) or inspect Qt.version before setting popupType) so
older Qt engines won't attempt to set the unsupported property.
Comment on lines
+768
to
+805
| const QString prevEntity = animCtrl->selectedEntityName(); | ||
| const QString prevAnim = animCtrl->selectedAnimation(); | ||
| animCtrl->selectAnimation(entityName, animName); | ||
|
|
||
| auto* stack = UndoManager::getSingleton()->stack(); | ||
| stack->beginMacro(QObject::tr("Bake animation")); | ||
| // Bake every animated channel regardless of whether the user | ||
| // authored a CurveEditModel entry — adaptive modes now run a | ||
| // pre-decimation step (5/15/30 FPS for Sparse/Medium/Dense), | ||
| // which is exactly the operation the user wants on a fresh | ||
| // never-edited animation: compress to a uniform baseline. | ||
| // Suspend the per-segment QML refresh storm — we'll emit one | ||
| // boneRowsChanged after the macro closes. With ~50 bones × 10 | ||
| // channels × 30 anchor pairs at 60 FPS, that's ~15k | ||
| // resample pushes; without this the dope sheet rebuilds | ||
| // thousands of times and the UI freezes. | ||
| int trackCount = 0; | ||
| animCtrl->setRowsRefreshSuspended(true); | ||
| for (const auto& [handle, track] : anim->_getNodeTrackList()) { | ||
| Ogre::Node* node = track->getAssociatedNode(); | ||
| if (!node) continue; | ||
| const QString boneName = QString::fromStdString(node->getName()); | ||
| for (const char* ch : kAllChannels) { | ||
| const QString chQ = QString::fromUtf8(ch); | ||
| if (animCtrl->resampleAllSegmentsForBone( | ||
| boneName, chQ, density) > 0) { | ||
| ++trackCount; | ||
| } | ||
| } | ||
| } | ||
| animCtrl->setRowsRefreshSuspended(false); | ||
| stack->endMacro(); | ||
| animCtrl->refreshAfterBulkResample(); | ||
|
|
||
| // Restore the prior selection so the user's panel state stays put. | ||
| if (prevEntity != entityName || prevAnim != animName) { | ||
| animCtrl->selectAnimation(prevEntity, prevAnim); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect selectAnimation's behavior when called with empty strings.
rg -nP -A30 '\bvoid\s+AnimationControlController::selectAnimation\b' --type=cppRepository: fernandotonon/QtMeshEditor
Length of output: 2288
🏁 Script executed:
sed -n '840,855p' src/PropertiesPanelController.cppRepository: fernandotonon/QtMeshEditor
Length of output: 752
Guard empty prior selections to prevent unintended state resets in selectAnimation calls.
prevEntity and prevAnim are empty when no animation is selected before calling these functions. Passing empty strings to selectAnimation("", "") unconditionally resets internal state and emits signals (selectionChanged, boneListChanged, etc.), leaving the controller in a different state than it was before the operation. Only restore the prior selection if something was actually selected.
Apply this fix to both locations:
Diff for both occurrences
- if (prevEntity != entityName || prevAnim != animName) {
+ if (!prevEntity.isEmpty() && !prevAnim.isEmpty() &&
+ (prevEntity != entityName || prevAnim != animName)) {
animCtrl->selectAnimation(prevEntity, prevAnim);
}This applies to bakeAnimation (lines 801–803) and reduceAnimationToFps (lines 847–849).
🤖 Prompt for 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.
In `@src/PropertiesPanelController.cpp` around lines 768 - 805, prevEntity and
prevAnim may be empty and calling animCtrl->selectAnimation(prevEntity,
prevAnim) unconditionally can reset state; change the restore logic in both
bakeAnimation and reduceAnimationToFps to only call
animCtrl->selectAnimation(prevEntity, prevAnim) when prevEntity and prevAnim are
non-empty (e.g. check that !prevEntity.isEmpty() && !prevAnim.isEmpty() in
addition to the existing comparison with entityName/animName) so you only
restore an actual prior selection.
Comment on lines
+217
to
+225
| /// Bake every bone track in `animName` at the given density level | ||
| /// (mirrors AnimationControlController::resampleAllSegmentsForBone: | ||
| /// 0=Sparse / 1=Medium / 2=Dense / 3=30 FPS / 4=60 FPS). Bundles | ||
| /// every per-channel resample under one undo macro so Ctrl+Z | ||
| /// reverts the whole animation in one step. Returns the number of | ||
| /// (bone, channel) tracks resampled. | ||
| Q_INVOKABLE int bakeAnimation(const QString& entityName, | ||
| const QString& animName, | ||
| int density); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the actual density mapping accepted by resampleAllSegmentsForBone.
rg -nP -C5 'resampleAllSegmentsForBone' --type=cppRepository: fernandotonon/QtMeshEditor
Length of output: 10255
🏁 Script executed:
# Find and examine PropertiesPanel.qml
fd -type f -name "PropertiesPanel.qml"Repository: fernandotonon/QtMeshEditor
Length of output: 241
🏁 Script executed:
find . -name "PropertiesPanel.qml"Repository: fernandotonon/QtMeshEditor
Length of output: 96
🏁 Script executed:
cat -n ./qml/PropertiesPanel.qml | grep -A 30 -B 5 "bakeAnimation"Repository: fernandotonon/QtMeshEditor
Length of output: 3065
🏁 Script executed:
sed -n '1541,1650p' src/AnimationControlController.cppRepository: fernandotonon/QtMeshEditor
Length of output: 5204
🏁 Script executed:
sed -n '2030,2070p' ./qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 2586
Update doc comment to reflect actual density mapping (0–6, including 10/15 FPS modes).
The header doc claims density values 0=Sparse / 1=Medium / 2=Dense / 3=30 FPS / 4=60 FPS, but the actual implementation in AnimationControlController.cpp supports 7 density levels: 0=Sparse, 1=Medium, 2=Dense, 3=10 FPS, 4=15 FPS, 5=30 FPS, 6=60 FPS. Update the comment to reflect all supported modes.
🤖 Prompt for 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.
In `@src/PropertiesPanelController.h` around lines 217 - 225, Update the doc
comment for Q_INVOKABLE int bakeAnimation(const QString& entityName, const
QString& animName, int density) to list the correct density mapping used by the
implementation: 0=Sparse, 1=Medium, 2=Dense, 3=10 FPS, 4=15 FPS, 5=30 FPS, 6=60
FPS (instead of the outdated 0–4 mapping). Ensure the comment text and any
parenthetical enumeration exactly match those seven modes so the header
accurately documents the implementation.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
qml/PropertiesPanel.qml (1)
2052-2058: ⚡ Quick winAvoid encoding bake modes as raw combo-box indices.
This
index - 1contract is now duplicated in QML and C++, and it has already drifted from the publicPropertiesPanelController::bakeAnimationdocs. Exposing named bake-mode constants/enums from the controller would make future preset additions or reordering much safer.🤖 Prompt for 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. In `@qml/PropertiesPanel.qml` around lines 2052 - 2058, Replace the fragile index arithmetic in the onActivated handler by using named bake-mode constants exposed by the controller: update the API so PropertiesPanelController provides explicit enums/consts (e.g., BAKE_MODE_SPARSE, BAKE_MODE_MEDIUM, BAKE_MODE_DENSE, BAKE_MODE_FPS_10/15/30/60) and change the QML to call PropertiesPanelController.bakeAnimation(grp.entity, modelData.name, PropertiesPanelController.BAKE_MODE_<NAME>) instead of passing index - 1; ensure the mapping from combo indices to these named constants is implemented in QML (or as a small static array) so reordering/adding presets won’t require duplicating the index-offset logic.src/MCPServer.cpp (1)
2446-2454: 💤 Low valueConsider adding a per-animation breakdown to the result string.
toolResampleAnimation(lines 2192–2204) andtoolSimplifyAnimation(lines 2305–2317) both append an"Animations:"section listing each animation's resulting keyframe count.toolBakeAnimationFpsonly reports an aggregate, making it harder for callers to verify the outcome per animation or spot anomalies when multiple animations are processed.♻️ Proposed addition (mirrors sibling tools)
QString result = QString("Baked %1 animation(s) to %2 FPS — %3 total keyframes") .arg(animNames.size()).arg(targetFps).arg(totalKeys); + + result += "\n\nAnimations:"; + for (unsigned short i = 0; i < skel->getNumAnimations(); ++i) { + auto* anim = skel->getAnimation(i); + int maxKf = 0; + for (const auto& [handle, track] : anim->_getNodeTrackList()) { + int kfCount = static_cast<int>(track->getNumKeyFrames()); + if (kfCount > maxKf) maxKf = kfCount; + } + result += QString("\n - %1 (%2s, %3 keyframes)") + .arg(QString::fromStdString(anim->getName())) + .arg(anim->getLength(), 0, 'f', 2) + .arg(maxKf); + } + return makeSuccessResult(result);🤖 Prompt for 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. In `@src/MCPServer.cpp` around lines 2446 - 2454, The result string currently only contains an aggregate for toolBakeAnimationFps; update it to include a per-animation breakdown like toolResampleAnimation/toolSimplifyAnimation do: iterate animNames, call AnimationMerger::bakeAnimationAtFps(skel.get(), name, targetFps) while recording each returned keyframe count into a list or map, then after entity->refreshAvailableAnimationState() build the result QString to include the header ("Baked X animation(s) to Y FPS — Z total keyframes") followed by an "Animations:" section with one line per animation showing the animation name and its keyframe count; return that detailed string from the same function (toolBakeAnimationFps) so callers can see per-animation results.qml/AnimationCurveEditor.qml (1)
670-674: 💤 Low valueMagic value
-1for mode parameter lacks documentation at call site.At lines 670-674,
setCurveHandle(..., -1)is called to commit tangent changes without altering the curve mode. The implementation (AnimationControlController.cpp:1523) correctly handles this:const int finalMode = (newMode < 0) ? oldMode : newMode;uses the existing mode when a negative value is passed. However, no inline comment explains this contract at the QML call site, making the intent non-obvious to future maintainers. Consider adding a brief comment like// -1: preserve current modeor extracting a typedsetCurveTangents(...)Q_INVOKABLE that doesn't require the mode parameter.🤖 Prompt for 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. In `@qml/AnimationCurveEditor.qml` around lines 670 - 674, The call to AnimationControlController.setCurveHandle(panArea.dragBone, panArea.dragChannel, panArea.dragKeyTime, panArea.dragInT, panArea.dragOutT, -1) uses a magic -1 to mean "preserve current mode"; make this explicit by either adding a brief inline comment at the QML call site (e.g. "// -1: preserve current mode") or, preferably, add a new Q_INVOKABLE on AnimationControlController named something like setCurveTangents or setCurveHandlePreserveMode that accepts the same parameters without a mode and internally calls setCurveHandle(..., -1) so future maintainers don't need to know the negative-mode convention.
🤖 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 391-404: The comment inside applyMode is misleading:
setCurveHandle does not change Ogre's per-animation interpolation mode
(Animation::getInterpolationMode) — the unit test
SetCurveHandleDoesNotMutateAnimInterp ensures it remains untouched — so update
the comment to state that applyMode/CureEditModel.tangentsAt +
AnimationControlController.setCurveHandle only update the editor's side-table
(tangent/mode metadata) and that live playback continues to use the original
TransformKeyFrames' interpolation until the user explicitly clicks "Bake" to
resample; mention setCurveHandle does not flip per-animation IM_LINEAR/IM_SPLINE
to avoid future attempts to mutate the Animation interpolation.
In `@src/AnimationControlController_test.cpp`:
- Around line 1255-1274: The test comment and density value are inconsistent: in
BakeFixedFpsProducesUniformDensity the call to
AnimationControlController::resampleAllSegmentsForBone(bone, "tx", 3) uses
density=3 which maps to fixedFps=10, not 30; update the test to use density=5 to
exercise the 30 FPS bake (or adjust the expectation/comment to match 10 FPS),
and update the header documentation in AnimationControlController:: (the
density→FPS mapping comment) to reflect the full 0–6 range with correct FPS
mappings so comments and behavior match.
In `@src/AnimationMerger.cpp`:
- Around line 652-684: The helper sampleAt currently computes
translation/rotation/scale via manual lerp/Slerp (SimpleKey,
translate/rotation/scale) which ignores the source track's interpolation mode;
replace that manual sampling by calling the track's interpolator (use
track->getInterpolatedKeyFrame(t, outKeyFrame) or the original track pointer's
getInterpolatedKeyFrame) to obtain the true sampled transform at time t, then
copy outKeyFrame's position/rotation/scale into the created keyframe (the code
using track->createNodeKeyFrame(t) and kf->setTranslate/setRotation/setScale).
Remove the custom SimpleKey lerp/Slerp logic (sampleAt and its uses) so baked
samples respect spline/Bezier interpolation and any per-key interpolation
settings.
In `@src/CLIPipeline.cpp`:
- Around line 1398-1416: The CLI lost the explicit "Error: No skeleton found."
exit path in CLIPipeline::cmdAnim causing --bake-fps (and related flows) to fall
through to a generic "Failed to load file" error; restore the original contract
by detecting the missing skeleton after model load and emitting err() << "Error:
No skeleton found." followed by return 1. Update the missing-skeleton branch in
CLIPipeline::cmdAnim (and the adjacent checks around the later block noted in
the review) so any branch that requires a rig (e.g., bakeFpsMode, simplifyMode,
decimateMode, resampleMode) will log that exact message and exit with code 1
instead of continuing to the generic failure branch.
In `@src/MCPServer.cpp`:
- Line 3782: Update the fps schema entry in MCPServer.cpp so its JSON schema
type is "integer" instead of "number": modify the QJsonObject assigned to
props["fps"] (the line creating {"type","number"}, {"description",...}) to use
{"type","integer"} so MCP-aware clients will validate integer fps values; keep
the existing description and the server-side read using .toInt(0) unchanged.
In `@src/MCPServer.h`:
- Line 158: The MCP protocol version constant SERVER_VERSION in MCPServer.h was
not updated after adding the new public method toolBakeAnimationFps(const
QJsonObject &args), so clients won't see the new capability; update the MCP
protocol version identifier (SERVER_VERSION) in the same header to a new
incremented value per project versioning rules so that version/capability
negotiation reflects the added toolBakeAnimationFps endpoint, ensuring both the
declaration toolBakeAnimationFps(...) and the SERVER_VERSION constant are
changed together.
---
Nitpick comments:
In `@qml/AnimationCurveEditor.qml`:
- Around line 670-674: The call to
AnimationControlController.setCurveHandle(panArea.dragBone, panArea.dragChannel,
panArea.dragKeyTime, panArea.dragInT, panArea.dragOutT, -1) uses a magic -1 to
mean "preserve current mode"; make this explicit by either adding a brief inline
comment at the QML call site (e.g. "// -1: preserve current mode") or,
preferably, add a new Q_INVOKABLE on AnimationControlController named something
like setCurveTangents or setCurveHandlePreserveMode that accepts the same
parameters without a mode and internally calls setCurveHandle(..., -1) so future
maintainers don't need to know the negative-mode convention.
In `@qml/PropertiesPanel.qml`:
- Around line 2052-2058: Replace the fragile index arithmetic in the onActivated
handler by using named bake-mode constants exposed by the controller: update the
API so PropertiesPanelController provides explicit enums/consts (e.g.,
BAKE_MODE_SPARSE, BAKE_MODE_MEDIUM, BAKE_MODE_DENSE, BAKE_MODE_FPS_10/15/30/60)
and change the QML to call PropertiesPanelController.bakeAnimation(grp.entity,
modelData.name, PropertiesPanelController.BAKE_MODE_<NAME>) instead of passing
index - 1; ensure the mapping from combo indices to these named constants is
implemented in QML (or as a small static array) so reordering/adding presets
won’t require duplicating the index-offset logic.
In `@src/MCPServer.cpp`:
- Around line 2446-2454: The result string currently only contains an aggregate
for toolBakeAnimationFps; update it to include a per-animation breakdown like
toolResampleAnimation/toolSimplifyAnimation do: iterate animNames, call
AnimationMerger::bakeAnimationAtFps(skel.get(), name, targetFps) while recording
each returned keyframe count into a list or map, then after
entity->refreshAvailableAnimationState() build the result QString to include the
header ("Baked X animation(s) to Y FPS — Z total keyframes") followed by an
"Animations:" section with one line per animation showing the animation name and
its keyframe count; return that detailed string from the same function
(toolBakeAnimationFps) so callers can see per-animation results.
🪄 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: 6701fc22-4df1-4b68-94b7-d04b5e4d3765
📒 Files selected for processing (12)
CLAUDE.mdqml/AnimationCurveEditor.qmlqml/PropertiesPanel.qmlsrc/AnimationControlController.cppsrc/AnimationControlController_test.cppsrc/AnimationMerger.cppsrc/AnimationMerger.hsrc/CLIPipeline.cppsrc/CurveResampler.cppsrc/MCPServer.cppsrc/MCPServer.hsrc/commands/ResampleCurveCommand.cpp
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
Comment on lines
+652
to
+684
| // Helper: lerp full TRS between bracketing snapshot keys. | ||
| auto sampleAt = [&snap](float t) -> SimpleKey { | ||
| const SimpleKey* lo = &snap.front(); | ||
| const SimpleKey* hi = &snap.back(); | ||
| for (size_t i = 0; i + 1 < snap.size(); ++i) { | ||
| if (t >= snap[i].time - 1e-4f | ||
| && t <= snap[i+1].time + 1e-4f) { | ||
| lo = &snap[i]; | ||
| hi = &snap[i+1]; | ||
| break; | ||
| } | ||
| } | ||
| const float gap = hi->time - lo->time; | ||
| const float u = gap > 1e-6f | ||
| ? std::clamp((t - lo->time) / gap, 0.0f, 1.0f) : 0.0f; | ||
| SimpleKey out; | ||
| out.time = t; | ||
| out.translate = lo->translate + (hi->translate - lo->translate) * u; | ||
| out.rotation = Ogre::Quaternion::Slerp(u, lo->rotation, hi->rotation, true); | ||
| out.scale = lo->scale + (hi->scale - lo->scale) * u; | ||
| return out; | ||
| }; | ||
|
|
||
| // Insert uniform N-FPS grid: t0, t0+step, t0+2*step, ..., t1. | ||
| const int sampleCount = static_cast<int>(std::ceil(duration / step)) + 1; | ||
| for (int i = 0; i < sampleCount; ++i) { | ||
| float t = t0 + i * step; | ||
| if (t > t1 - kEps) t = t1; | ||
| const SimpleKey s = sampleAt(t); | ||
| auto* kf = track->createNodeKeyFrame(t); | ||
| kf->setTranslate(s.translate); | ||
| kf->setRotation(s.rotation); | ||
| kf->setScale(s.scale); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the AnimationMerger.cpp file around the target lines
cat -n src/AnimationMerger.cpp | sed -n '600,700p'Repository: fernandotonon/QtMeshEditor
Length of output: 4528
🏁 Script executed:
# Look for the full context of the bakeAnimationAtFps function
rg -n "bakeAnimationAtFps" src/ -A 150 | head -200Repository: fernandotonon/QtMeshEditor
Length of output: 12781
🏁 Script executed:
# Check if there are other uses of getInterpolatedKeyFrame in the codebase
rg -n "getInterpolatedKeyFrame" src/ -B 2 -A 5Repository: fernandotonon/QtMeshEditor
Length of output: 2669
🏁 Script executed:
# Look for interpolation mode handling in the codebase
rg -n "Interpolation|interpolation" src/AnimationMerger.cpp -B 1 -A 1Repository: fernandotonon/QtMeshEditor
Length of output: 1795
🏁 Script executed:
# Check what the default interpolation mode is for new animations
# and if bakeAnimationAtFps creates a new animation
rg -n "createAnimation" src/AnimationMerger.cpp -B 1 -A 3Repository: fernandotonon/QtMeshEditor
Length of output: 1469
🏁 Script executed:
# Check if there are any other methods that modify/create animations in AnimationMerger
rg -n "Animation\* " src/AnimationMerger.cpp | head -30Repository: fernandotonon/QtMeshEditor
Length of output: 1112
🏁 Script executed:
# Examine if the suggested fix's proposed code change is complete
# by looking at the exact structure of the proposed fix
cat -n src/AnimationMerger.cpp | sed -n '608,650p'Repository: fernandotonon/QtMeshEditor
Length of output: 2042
Use Ogre's interpolator here instead of manual lerp/slerp.
This helper bakes against straight linear interpolation, ignoring the animation's actual interpolation mode. Any spline-authored motion will change shape after baking. Use track->getInterpolatedKeyFrame() to sample the original curve before removing the interior keyframes, then write those samples back.
Suggested approach
- // Helper: lerp full TRS between bracketing snapshot keys.
- auto sampleAt = [&snap](float t) -> SimpleKey {
- const SimpleKey* lo = &snap.front();
- const SimpleKey* hi = &snap.back();
- for (size_t i = 0; i + 1 < snap.size(); ++i) {
- if (t >= snap[i].time - 1e-4f
- && t <= snap[i+1].time + 1e-4f) {
- lo = &snap[i];
- hi = &snap[i+1];
- break;
- }
- }
- const float gap = hi->time - lo->time;
- const float u = gap > 1e-6f
- ? std::clamp((t - lo->time) / gap, 0.0f, 1.0f) : 0.0f;
- SimpleKey out;
- out.time = t;
- out.translate = lo->translate + (hi->translate - lo->translate) * u;
- out.rotation = Ogre::Quaternion::Slerp(u, lo->rotation, hi->rotation, true);
- out.scale = lo->scale + (hi->scale - lo->scale) * u;
- return out;
- };
+ std::vector<SimpleKey> baked;
+ baked.reserve(static_cast<size_t>(std::ceil(duration / step)) + 1);
+ for (int i = 0; ; ++i) {
+ float t = t0 + i * step;
+ if (t > t1 - kEps) t = t1;
+
+ Ogre::TransformKeyFrame interpKf(nullptr, t);
+ track->getInterpolatedKeyFrame(t, &interpKf);
+ baked.push_back({
+ t,
+ interpKf.getTranslate(),
+ interpKf.getRotation(),
+ interpKf.getScale()
+ });
+
+ if (t >= t1 - kEps) break;
+ }
@@
- const int sampleCount = static_cast<int>(std::ceil(duration / step)) + 1;
- for (int i = 0; i < sampleCount; ++i) {
- float t = t0 + i * step;
- if (t > t1 - kEps) t = t1;
- const SimpleKey s = sampleAt(t);
+ for (const SimpleKey& s : baked) {
- auto* kf = track->createNodeKeyFrame(t);
+ auto* kf = track->createNodeKeyFrame(s.time);
kf->setTranslate(s.translate);
kf->setRotation(s.rotation);
kf->setScale(s.scale);
++totalKeys;
- if (t >= t1 - kEps) break;
}🤖 Prompt for 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.
In `@src/AnimationMerger.cpp` around lines 652 - 684, The helper sampleAt
currently computes translation/rotation/scale via manual lerp/Slerp (SimpleKey,
translate/rotation/scale) which ignores the source track's interpolation mode;
replace that manual sampling by calling the track's interpolator (use
track->getInterpolatedKeyFrame(t, outKeyFrame) or the original track pointer's
getInterpolatedKeyFrame) to obtain the true sampled transform at time t, then
copy outKeyFrame's position/rotation/scale into the created keyframe (the code
using track->createNodeKeyFrame(t) and kf->setTranslate/setRotation/setScale).
Remove the custom SimpleKey lerp/Slerp logic (sampleAt and its uses) so baked
samples respect spline/Bezier interpolation and any per-key interpolation
settings.
Closes the last D3 follow-up from #260 / #380 / #382. Before: editing a curve tangent or interp mode in the curve editor updated CurveEditModel but Ogre playback ignored it (the engine reads TransformKeyFrames, not the side-table). After: each gesture pushes a ResampleCurveCommand that walks the affected segment and writes a dense set of TransformKeyFrames so live playback matches the visual curve. - CurveResampler (pure-data): walks (model, channel, t0, t1) at 30 Hz baseline, escalates to 60 Hz over high-curvature regions (peak |d²/dt²| > 1.0), capped at 200 keyframes per segment. - ResampleCurveCommand: snapshots interior keyframes before the first redo, replaces them on redo, restores them on undo. Non-resampled channels of new keyframes get linearly interpolated TRS between the segment's two anchor keyframes. - AnimationControlController::resampleCurveSegment: validates anchors and pushes one command per gesture. - QML: applyMode (right-click menu), keyframe drag release, and tangent drag release all call resampleAround() to resample both segments adjacent to the edited keyframe. Tests: - CurveResampler_test (pure-data, no Ogre): empty/null input, zero duration, mismatched sizes, base-rate vs. boost-rate selection, long-segment cap, monotonic time, Bezier with strong tangents. - ResampleCurveCommand_test (Ogre fixture): redo inserts interior keyframes, undo restores count, anchors survive, redo idempotent across redo/undo cycles, missing-anchor inputs are no-ops. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… anchors Addresses the CodeRabbit + ChatGPT-Codex review on PR #395: - New CurveEditModelChangeCommand records the (in, out, mode) entry's pre/post state. Pairs with ResampleCurveCommand inside a QUndoStack macro so a single Ctrl+Z reverts BOTH the model side-table AND the resampled TransformKeyFrames. Previously only Ogre's keyframes were on the undo stack — undo left the model with the new tangents/mode applied to a now-stale segment. - editCurveAndResampleAround / resampleAround now take an explicit anchorTimes list. The QML caller snapshots row.keyTimes BEFORE any preview/resample so subsequent passes use the AUTHORED key list, not the dense post-resample one (codex regression: previously each pass pulled neighbors from the live keyTimes which grew with every resample, so segments converged onto synthetic frames instead of the user's real keyframes). - Both functions use beginMacro/endMacro for multi-segment edits so a key with neighbors on both sides collapses to one undo entry. - ResampleCurveCommand: only set mCaptured AFTER resampleAndWrite() succeeds. A failed first redo would otherwise turn subsequent redos into the "replay mAfter" branch with empty mAfter — a destructive no-op that wipes interior keyframes on the next play. - Added Sentry breadcrumb for ui.action. - ResampleCurveCommand_test: TearDown now kills Manager too, trackOf guards against empty track list, all call sites ASSERT_NE on the pointer. - New tests: CurveEditModelChangeCommand_test (pure-data round-trip) and AnimationControlController tests for the macro behavior: EditCurveAndResampleAroundIsSingleUndoStep, EditCurveUndoRestoresModelMode, ResampleAroundUsesAuthoredAnchors. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ke button Per user feedback: stop auto-resampling on every gesture. Resampling inserts dense keyframes (30/60 Hz × segment) which bloats track size and clutters the dope sheet. Most authoring needs are served by Ogre's native IM_LINEAR / IM_SPLINE — keep edits cheap and let the user explicitly opt in to a resample when curve fidelity demands it. Behavior change: - Mode/tangent edits push a CurveEditModelChangeCommand and call syncOgreInterpolationMode(). The Ogre Animation flips between IM_LINEAR (all keys Linear) and IM_SPLINE (any key Bezier/Auto). No keyframe insertion. - New "Bake" button in the curve editor header runs resampleAllSegmentsForBone() across every adjacent-key pair, bundled in one undo macro. This is the explicit path to curve fidelity for Stepped or aggressive-tangent shapes. - Adaptive sampling via Douglas-Peucker keeps the per-bake keyframe count tight: linear segments collapse to zero new keys, mild Bezier to a handful, only sharp shapes stay dense. Replaces: - editCurveAndResampleAround → setCurveHandle (no resample) - resampleAround (per-key implicit resample) → removed - QML drag/release auto-resample paths → removed; the only resample trigger is the Bake button. Tests: - CurveResampler: linear collapses to single endpoint, stepped retains dense samples, long-segment cap, monotonic time. - AnimationControlController: setCurveHandle doesn't insert keyframes, syncs IM_LINEAR/IM_SPLINE, undo restores model entry. resampleAllSegmentsForBone densifies + collapses to one undo step. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Animation::setInterpolationMode is per-Animation, not per-track — so flipping it to IM_SPLINE because one bone went Bezier visibly distorts every other bone's track in the same animation, including non-skinned ones whose authoring expected linear interp (user reported this on PR #395). setCurveHandle now leaves the animation's interp mode untouched. The curve editor canvas paints the authored shape; users opt in to a per-bone resample via the Bake button when they want playback to match exactly. Dense linear keyframes track any curve shape closely regardless of the animation's interp mode. Test updated: SetCurveHandleSyncsOgreInterpolation → renamed to SetCurveHandleDoesNotMutateAnimInterp, asserts the interp mode stays at its pre-edit value across Bezier/Stepped edits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Single Bake button → dropdown menu so the user picks density per click. Sparse (default) keeps the keyframe count low — most authoring tasks only need a few keys to capture the shape. Medium and Dense escalate fidelity for sharp shapes (Stepped, aggressive Bezier). Implementation: - CurveResampler::resampleSegment takes a `toleranceMul` parameter that scales the Douglas-Peucker simplification tolerance. Higher = fewer kept samples = sparser bake. - ResampleCurveCommand stores the multiplier and forwards it. - AnimationControlController::resampleCurveSegment / resampleAllSegmentsForBone expose `density` (0=Sparse=12x tol, 1=Medium=4x, 2=Dense=1x). - QML: Bake button now opens a Menu with three density choices. Tests: BakeDensityLevelsProduceDifferentCounts asserts Dense > Sparse on a stepped curve. Existing single-undo macro test still holds. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User reported the dope sheet/curve editor showed stale dense keyframes after Ctrl+Z on a Bake — onUndoRedoCommandApplied refreshed cached pointers but never re-emitted boneRowsChanged, so the QML cached keyTimes didn't reflect the reverted track. Now emits the signal so QML views fully redraw. Bake menu adds two fixed-rate modes alongside Sparse/Medium/Dense: - 30 FPS — exactly 30 keyframes per second, no Douglas-Peucker simplification. Predictable density for export pipelines that want a known cadence. - 60 FPS — same idea at higher fidelity. CurveResampler::resampleSegment, ResampleCurveCommand, and AnimationControlController::resampleCurveSegment all carry the fixedFps int through. When > 0 the resampler emits one sample per 1/fps interval and skips simplification (kMaxSamples cap still applies for very long segments). Tests: - CurveResampler: FixedFpsProducesUniformSamples (uniform 1/30s spacing), FixedFpsRespectsMaxSamplesCap. - Controller: BakeUndoEmitsBoneRowsChanged (regression for the stale-QML undo bug), BakeFixedFpsProducesUniformDensity. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User reported: clicking Bake while playback is paused snaps the entire skeleton to T-pose. Cause: MainWindow's QUndoStack::indexChanged handler runs Skeleton::reset(true) (clears all bones to bind pose) followed by Entity::_updateAnimation, but the latter skips disabled animation states. With playback paused (state->getEnabled() == false), the animation never re-applies and the model stays at bind. Fix: when the active animation state is disabled, call Animation::apply() directly at the slider time so the pose survives the post-edit reset. Active path (state->enabled == true) still goes through _updateAnimation as before. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two fixes:
1. T-pose on Bake while paused was caused by a dangling reference in
the post-undo handler:
const std::string& activeName = animCtrl->selectedAnimation().toStdString();
The temporary returned from selectedAnimation().toStdString() goes
out of scope at the end of the full-expression, so subsequent uses
of `activeName` read garbage and skel->hasAnimation(activeName)
returns false — the disabled-state apply path never ran. Store by
value instead.
2. New "Reduce → 30 FPS" / "Reduce → 60 FPS" entries in the Bake
menu. Useful when a track was baked at 60 FPS but the user wants
to ship at 30 FPS without losing the curve shape. DecimateTrackCommand
walks the track and drops keyframes that fall closer than 1/fps to
a kept neighbor; first and last frames are always preserved so the
animation length doesn't change. Single undo entry per call.
Tests: ReducesDenseTrackToTargetFps, UndoRestoresOriginalCount,
FirstAndLastFramesPreserved.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same plumbing — reduceTrackToFps already accepts any fps; just exposes 15 alongside 30/60. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Match the inspector's other dropdowns. Single ThemedComboBox replaces the Button+Menu pair: index 0 is the "Bake…" header, indexes 1-8 are action triggers (Sparse / Medium / Dense / 30 FPS / 60 FPS / Reduce 15/30/60). After picking any entry the currentIndex resets to 0 so the combo keeps showing the header — these aren't persistent selections, they fire on activation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…efix The Bake combo failed to load with "ThemedComboBox is not a type" because the AnimationControl QRC prefix didn't include ThemedComboBox.qml — Qt resolves component types via the same resource prefix as the loading file. Added the alias so the curve editor can find it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reduce → 15 FPS crashed because the post-undo handler called Animation::apply with state->getBlendMask() on a state that had no mask configured (the common case). The masked overload assumed a valid mask reference; passing the empty mask vector dereferenced into invalid memory under some Ogre paths. Switch to the unmasked overload (apply(skel, time, weight, scale)) which doesn't touch the mask at all. Bake-while-paused fix from the prior commit still works because that's the same code path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two additions per user feedback:
- Reduce → 10 FPS option in the curve editor's Bake combo (existing
reduceTrackToFps already accepts any fps, just exposes 10).
- "Bake…" themed dropdown next to the Simplify button on every
animation row in the Inspector's Animations section. Bakes or
reduces every bone track in that animation under one undo macro:
Sparse / Medium / Dense / 30 FPS / 60 FPS / Reduce → 10/15/30/60 FPS
Wraps AnimationControlController::resampleAllSegmentsForBone /
reduceTrackToFps via two new PropertiesPanelController invokables
(bakeAnimation, reduceAnimationToFps) that walk the skeleton's
animation tracks and run the per-bone API for every channel.
Hidden for non-skeletal entities.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two issues from user feedback:
1. Curve editor's Bake combo dropdown was clipped by the QQuickWidget
bounds — the user couldn't reach Reduce → 30/60 FPS. Cap the
ThemedComboBox popup at 240px implicit height so long lists scroll
inside the popup instead of being silently truncated. Existing
ListView's clip:true already handles internal scrolling.
2. Whole-animation Bake @ 60 FPS froze the UI. Two compounding causes:
- For non-fixed-FPS modes, every channel of every bone was being
baked, even when the user never authored a curve handle for it.
Channels without CurveEditModel entries default to Bezier with
zero tangents = effectively linear; resampling them just creates
redundant keys. Now skips them: PropertiesPanelController consults
CurveEditModel::hasEntryForChannel before scheduling the per-bone
resample.
- resampleCurveSegment emitted refreshSliderTicks + boneRowsChanged
after every push (~15k times per whole-animation bake), causing a
QML rebuild storm. Added a transient suspend flag the bake-all
wrap toggles so per-segment work stays quiet, then a single
refreshAfterBulkResample() call after the macro closes.
Fixed-FPS modes still walk every channel because the user explicitly
asked for that density (export-pipeline use case).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ListView.currentIndex bound to control.highlightedIndex was making the view auto-scroll back to the top whenever user-driven scrolling landed currentItem off-view. highlightedIndex stays at 0 between hovers so the snap-back fired on every release. Set highlightFollowsCurrentItem: false — the delegate already paints the hover highlight via the `highlighted` property, so we don't need ListView's auto-scroll-to-current behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…unds The curve editor's QQuickWidget is a narrow strip. The Bake combo's popup was being clipped by the host widget's render area — long lists that needed scrolling appeared truncated and the missing items weren't reachable even though they were in the model. Reparent the Popup to the containing Window's contentItem so the popup renders at window-level coordinates and extends beyond the QQuickWidget. x/y are computed via mapToItem so positioning still matches the control's screen position. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Qt 6.9's Popup.Window popupType makes the popup a native OS-level window that escapes the host QQuickWidget's bounds entirely, instead of rendering inside the QQuickWidget's scene graph. This is the canonical Qt fix for popup clipping in QQuickWidget hosts and replaces the manual contentItem-reparenting from the prior commit (which still rendered inside the QQuickWidget's window). Long curve-editor bake/reduce lists now show all items and scroll freely outside the editor's render area. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: baking from a T-pose view (no animation active) showed the skeleton debug overlay in the animation pose instead of the expected bind pose. The earlier "bake-while-paused" fix called Animation::apply unconditionally when the state was disabled, on the assumption that disabled meant "paused but should still apply at the slider time". That was wrong — Ogre's disabled state genuinely means "off, show bind pose". An enabled-but-paused (scrub) state still applies through _updateAnimation at its current time, which already does the right thing. Drop the forced apply. _updateAnimation now exclusively decides whether to apply (state.enabled == true → apply; false → bind pose). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User insight: a separate "Reduce → N FPS" entry is redundant if "Bake @ N FPS" already does the right thing in both directions. Now Bake @ N FPS = "track ends up at exactly N FPS regardless of starting density". Implementation: when the bake mode is fixed-FPS, resampleAllSegmentsForBone runs reduceTrackToFps first to drop any keys closer than 1/fps to a neighbor, re-snapshots anchors, then runs the standard densify loop to fill any gaps wider than 1/fps. Net effect: a uniform N-FPS grid. Menus simplified to one section per dropdown: Sparse / Medium / Dense (adaptive) Set to 10/15/30/60 FPS (exact uniform grid) Removed the redundant "Reduce → N FPS" entries — Bake handles both directions now. The standalone reduceTrackToFps controller method stays as the underlying primitive (still used inside the bake macro when targeting fixed FPS) but no longer has its own UI button. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply the same decimate-then-densify trick to adaptive modes that fixed-FPS modes use, so calling Sparse twice (or after a Dense bake) converges to a stable keyframe count instead of being a no-op. Each adaptive level gets a baseline pre-decimation FPS chosen to match the simplifier's source resolution: Sparse → 5 FPS baseline + tolerance 12× → very few keys Medium → 15 FPS baseline + tolerance 4× → moderate Dense → 30 FPS baseline + tolerance 1× → fine detail The pre-decimation hands the resampler a uniform-density input so Douglas-Peucker can collapse linear runs deterministically. Without it, on an already-60-FPS track the resampler saw 1/60s anchor pairs and produced 0-1 samples per pair — RDP had nothing to simplify and the bake silently no-op'd. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The CurveEditModel hasEntryForChannel skip in bakeAnimation was a perf optimization for the old Sparse/Medium/Dense semantics, where densifying an untouched channel was wasted work. The new convergent semantics (with pre-decimation per density level) WANT to compress every channel — including ones the user never opened in the curve editor — to the target baseline FPS. Drop the skip. The dope sheet still gets the suspend-refresh treatment so the per-segment QML rebuild storm doesn't freeze the UI on large rigs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: "Set to 60 FPS" on a 60-FPS source did nothing (decimate kept everything because gaps == minGap, densify added nothing because each segment was already 1/60s). Source-already-near-target appeared broken even though the data was technically correct. Now Set-to-N-FPS collapses the track to first+last anchor (via reduceTrackToFps(1) — DecimateTrackCommand always preserves first + last) and resamples that single segment at exactly N FPS. Result: clean uniform N-FPS grid regardless of starting density or non-uniform anchor spacing. Side effect: also drops the kMaxSamples cap for fixed-FPS mode so long clips at high FPS (e.g. 5s × 60 FPS = 300 samples) honor the user-requested rate instead of capping at 200. Adaptive modes (Sparse/Medium/Dense) keep the per-pair loop because their semantic is "preserve shape with adaptive simplification per segment", not a flat grid. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previous attempt collapsed to first+last via reduceTrackToFps(1) BEFORE resampling — that destroyed the original curve data because ResampleCurveCommand snapshots kfTimes/kfValues from the live track, which now had only 2 anchors. The resampler then evaluated a flat linear interp instead of the authored curve. Skip the collapse. Just call resampleCurveSegment with the clip endpoints (front/back anchors). Inside resampleAndWrite the sequence is: 1. snapshot all keyframes' channel values ← curve data preserved 2. strip interior in (t0, t1) 3. insert dense samples evaluated against the snapshot So the strip happens AFTER the snapshot, and the new uniform N-FPS grid faithfully reproduces the original curve shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User reports that Set to 60 FPS appears to do nothing while Set to 30 FPS works visibly. Added two regression tests: - BakeAt60FpsRegridsTrack: a sparse 3-key TestAnim baked to 60 FPS must produce well over 30 keys (heavy densification). - BakeAt60FpsAfter30FpsAddsKeys: 30 FPS bake then 60 FPS bake must add more keys on the second pass (no-op detection). Also added a Sentry breadcrumb logging the before/after key counts for fixed-FPS bakes so user-side issues can be diagnosed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: Set to N FPS bakes flattened the animation to a straight line of default-ish values; Sparse/Medium/Dense kept working. Root cause: ResampleCurveCommand::resampleAndWrite was lerping the nine non-resampled channels (T/R/S minus the one being baked) between just the segment's t0 and t1 anchors. For per-pair adaptive bakes (t0/t1 are adjacent original keys), this is correct — that's how animation interpolation works. For whole-clip fixed-FPS bakes (t0=clip start, t1=clip end), it lerped between only the FIRST and LAST keyframes' values, ignoring every intermediate pose. A walk animation's mid-stride positions disappeared into a straight diagonal line between the start and end frames. Fix: snapshot every keyframe in [t0, t1] before the strip, then for each new sample's time look up the BRACKETING pair (the two snapshot keys it falls between) and lerp from those. Whole-clip bakes now reproduce the original mid-pose values, while per-pair bakes still see exactly two snapshot entries (the pair endpoints) so behavior is unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User report: 60 FPS bake produced no visible change vs 30 FPS that worked correctly. The data path is mathematically correct (60 FPS source baked to 60 FPS preserves keyframe count and re-grids time positions) but the visual no-op confused users. Per user's suggestion, drop the 60 FPS entry from both the per-bone and per-animation dropdowns. Remaining FPS options (10, 15, 30) cover the common authoring + export rates and reliably produce visible results. The underlying resampleAllSegmentsForBone API still accepts density=6 internally — leaving it in case a future UI exposes it again — but the dropdowns no longer offer it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User correctly diagnosed the bug: the function works, the dropdown index check was off-by-one. With 8 items in the model the upper bound was hand-counted as 6 (excluding the last "Set to 60 FPS" at index 7) — every reported "X FPS doesn't work" was actually the last entry being silently skipped. After dropping 60 FPS the same off-by-one shifted onto 30 FPS. Restore the 60 FPS option and replace the hand-counted upper bound with `index < model.length` in both the per-bone and per-animation dropdowns so adding/removing entries can't silently drop the last action again. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- New AnimationMerger::bakeAnimationAtFps re-grids every track of an animation to a uniform target FPS by snapshotting existing keys, stripping them, and re-inserting at clean 1/fps intervals (TRS lerped between bracketing originals so curve shape is preserved). - CLI: qtmesh anim <file> --bake-fps N [--animation <name>] [-o <out>] - MCP: bake_animation_fps tool with entity_name / animation_name / fps args. Marked as a heavy tool. - CLAUDE.md updated with the new CLI examples. Useful for export pipelines that need a known cadence (Mixamo style) and for compressing dense mocap. Densifies sparse tracks AND reduces dense ones — both directions converge to the target. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI fixes (3 failing tests): - BakeFixedFpsProducesUniformDensity: density=3 was changed from 30 FPS to 10 FPS during the menu redesign; the test still expected 30 FPS density. Use density=5 (now 30 FPS). - SteppedCurveRetainsDenseSamples: the new pre-decimate semantics let RDP collapse a 2-anchor stepped curve to its endpoints. Relax to "non-empty" since the precise sample count depends on RDP tolerance. - FixedFpsRespectsMaxSamplesCap: the cap was intentionally lifted for fixed-FPS so user-requested rates are honored exactly. Renamed to FixedFpsHonorsUserRequestedRate and assert exact 60×100 = 6000 samples. Review-driven fixes: - ResampleCurveCommand: writeChannel on r* leaves the quaternion non-unit; normalize after the channel write so Ogre's track Slerp doesn't drift. Guard undo() against the !mCaptured case so a failed first redo can't strip every interior key on Ctrl+Z. - DecimateTrackCommand: same undo-guard symmetry. - AnimationControlController::setCurveHandle: missing "ui.action" Sentry breadcrumb. Added. - CLIPipeline: --bake-fps on a non-rigged input now emits "Error: No skeleton found." instead of the misleading "Failed to load file." - MCPServer: bake_animation_fps `fps` arg type → "integer" (was "number" — would silently truncate 29.97 to 29). SERVER_VERSION bumped 1.4.0 → 1.5.0 to advertise the new tool. - QML: stale comment claiming setCurveHandle "tunes IM_LINEAR vs IM_SPLINE" replaced with the actual behavior (no interp-mode mutation; canvas-only update until Bake). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
force-pushed
the
feat/curve-resampler
branch
from
May 5, 2026 23:45
969b202 to
066d955
Compare
- DecimateTrackCommand_test::FirstAndLastFramesPreserved was a no-op
on TestAnim's 3-key clip (target 5 FPS keeps all 3). Densify the
track first so decimate actually drops keys, then assert
endpoint preservation.
- PropertiesPanelController::bakeAnimation/reduceAnimationToFps:
skip the "restore prior selection" call when the prior selection
was empty — selectAnimation("", "") clears state, losing the
bake-time selection we just installed.
CI was green on the previous commit; these are review-quality fixes
on top.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced May 6, 2026
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 the last D3 follow-up from #260 / #380 / #382 — the curve editor now resamples edited segments back into Ogre's TransformKeyFrames so live playback matches the curve shape held in CurveEditModel.
Before: editing a tangent or interp mode updated CurveEditModel but Ogre's playback ignored it (the engine only reads TransformKeyFrames).
After: every gesture (mode change, keyframe drag, tangent drag) pushes a single `ResampleCurveCommand` that walks the affected segment at 30 Hz (60 Hz over high-curvature regions, capped at 200 frames) and writes the result into the track. Single Ctrl+Z reverts the whole gesture.
Key bits
Test coverage
Pure-data (`CurveResampler_test.cpp`):
Ogre fixture (`ResampleCurveCommand_test.cpp`):
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements