feat(animation): curve editor data model + read-only view (Phase 5 slice D3a) - #379
Conversation
Closes part of #378 (D3 of slice D from #260). This PR is the first half of the curve editor work — the side-table data model and the QML curve view. Handle dragging + resample-into-TransformKeyFrame land in a follow-up (D3b) so this PR stays reviewable. CurveEditModel (C++) - New singleton storing per-keyframe Bezier tangent handles + an interpolation mode (Bezier / Linear / Stepped / Auto) keyed by (skeleton, animation, bone, channel, time). Q_INVOKABLE getters / setters; in-memory only — sidecar persistence is a follow-up. - evaluate(skel, anim, bone, ch, time, keyTimes, keyValues) computes the channel value at any time. Stepped/Linear are explicit; Bezier uses stored tangents (cubic Hermite); Auto derives Catmull-Rom-style tangents from neighbors. Pure-data — keeps Ogre out of the model for unit-testability. - Editing tangents on a Linear/Stepped keyframe auto-promotes the mode to Bezier (the user's intent is implicit in the drag). - Sign-agnostic checks aren't relevant here (the model deals in scalar channels), unlike collectActiveChannels in slice D2. QML (AnimationCurveEditor.qml) - New view, hosted in a QDockWidget tabified next to the Dope Sheet. View → Curve Editor toggle in the menu (uses toggleViewAction so mainwindow.ui doesn't need a new action). - Renders the selected bone's active channels as colored curves (matches D2 sub-row colors). Per-keyframe squares + tangent stubs for visual reference. Empty-state placeholder when no bone is selected. - Time-axis ruler + value-axis grid lines. - Cmd/Ctrl+wheel zooms timeline; middle-drag pans. Plain wheel is reserved for future vertical scroll. - Read-only this slice: handles draw but don't drag. D3b implements drag-to-edit + resample-into-track. Tests - CurveEditModelTest: 12 pure-data cases covering tangent round-trip, mode persistence + invalid-mode rejection, auto-promotion to Bezier, per-key scoping (skeleton/anim/bone/channel), clearAnimation pruning, and evaluate() across all modes (empty / single key / Linear / Stepped / exact-key / past-end / Auto-Catmull-Rom-midpoint). Out of scope (D3b follow-up) - Handle dragging (mouse interaction on tangents/keys) - Resample-into-TransformKeyFrame on edit - Per-channel value plumbing — D3a uses CurveEditModel.evaluate as a display-only sample; D3b will pipe per-channel keyframe values through from AnimationControlController. - Sidecar JSON persistence Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughIntroduces a complete animation curve editor system consisting of a C++ model ( ChangesAnimation Curve Editor System
Sequence DiagramsequenceDiagram
participant User
participant AnimationCurveEditor as AnimationCurveEditor.qml
participant Controller as AnimationControlController
participant Model as CurveEditModel
participant Canvas
User->>AnimationCurveEditor: Select bone
AnimationCurveEditor->>Controller: Get allBoneRows()
Controller-->>AnimationCurveEditor: Row data + selection state
AnimationCurveEditor->>AnimationCurveEditor: Compute activeChannelsForSelected()
AnimationCurveEditor->>Canvas: requestPaint()
Canvas->>Canvas: For each active channel:
Canvas->>Model: evaluate(time, keyframeTimes, keyframeValues)
Model-->>Canvas: Interpolated value
Canvas->>Model: tangentsAt(bone, channel, time)
Model-->>Canvas: [inTangent, outTangent, mode]
Canvas->>Canvas: Draw curve polyline + keyframe squares + tangent stubs
User->>AnimationCurveEditor: Drag middle-mouse / Ctrl-wheel
AnimationCurveEditor->>AnimationCurveEditor: Update viewStart / pxPerSec
AnimationCurveEditor->>Canvas: requestPaint()
Canvas-->>Canvas: Re-render with new view transform
User->>AnimationCurveEditor: Click tangent handle (future interaction)
AnimationCurveEditor->>Model: setTangents(bone, channel, time, inT, outT)
Model->>Model: Store tangent + promote mode to Bezier
Model-->>AnimationCurveEditor: modelChanged signal
AnimationCurveEditor->>Canvas: requestPaint()
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e1b8d28a4
ℹ️ 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".
| int lo = 0; | ||
| while (lo + 1 < n && keyframeTimes[lo + 1].toDouble() <= time) ++lo; | ||
| if (lo == n - 1) return keyframeValues[n - 1].toDouble(); |
There was a problem hiding this comment.
When time is earlier than the first keyframe, the bracketing logic leaves lo = 0 and proceeds to interpolate with a negative u, so Linear/Bezier/Auto modes extrapolate instead of holding the first value. This produces incorrect curve values whenever a track’s first key is not at t=0 (for example after shifting keys right), and the editor will render pre-roll segments with spurious values.
Useful? React with 👍 / 👎.
| tangentOut = (vHi - neighborPrev) * 0.5; | ||
| tangentIn = (neighborNext - vLo) * 0.5; |
There was a problem hiding this comment.
The Auto mode tangent estimates use only neighboring values and ignore neighboring keyframe times, but these tangents are later multiplied by dt in Hermite evaluation. On non-uniformly spaced keys this inflates or shrinks slopes dramatically (e.g., wide intervals overshoot heavily), so Auto interpolation is mathematically wrong unless keys are uniformly spaced.
Useful? React with 👍 / 👎.
Codex P1 (clamp before first keyframe):
- evaluate() with time < firstTime used to keep lo = 0, then compute
u < 0, then evaluate the cubic Hermite with negative u —
extrapolating the curve past the first keyframe instead of holding
it. Hold the first value explicitly when time is at or before the
first keyframe time. Matches Ogre playback semantics.
- New EvaluateBeforeFirstKeyClampsToFirst test covers Bezier and
Linear paths.
Codex P1 (Auto tangent ignores spacing):
- Auto-mode tangent estimates used (vNeighbor - vSelf) * 0.5, which
silently assumes uniform keyframe spacing. With non-uniform
spacing, Hermite's term produced wrong slopes —
e.g. on a perfect linear ramp at times {0, 1, 5}, the bracket
[1, 5] would compute a tangent against the unit-spacing assumption
and ring above/below the line.
- Switch to standard Catmull-Rom-style time-normalized tangents:
tangent[i] = (v[i+1] - v[i-1]) / (t[i+1] - t[i-1])
Collapses to the half-difference for uniform spacing (existing test
still passes); now correct for non-uniform.
- New EvaluateAutoModeNonUniformSpacing test guards against
regression.
Sonar S5276 (cpp:S5276):
- explicit static_cast<int>(keyframeTimes.size()) so the qsizetype
→ int conversion is intentional.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![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 (2)
qml/AnimationCurveEditor.qml (2)
173-177: ⚡ Quick winHardcoded value-axis range [-2, 2] clips most real-world channel data.
Translation channels routinely carry values in the tens or hundreds of world-units. With the fixed grid loop
for (var v = -2; v <= 2; v += 0.5)and no adaptiveyCenter/yScale, curves for those channels are entirely off-screen with no visual feedback to the user. Consider computingvMin/vMaxfrom the sampled keyframe values and centering the grid around the actual data range, or at minimum let the user know their data exceeds the visible range.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/AnimationCurveEditor.qml` around lines 173 - 177, The horizontal grid loop uses a hardcoded value range (for (var v = -2; v <= 2; v += 0.5)) which causes curves to clip; update the logic that draws horizontal ticks to compute vMin/vMax from the actual sampled keyframe values (or from root.yCenter/root.yScale inputs) and derive tick step and range from those values so ticks are centered and scaled to the data; modify the loop to iterate from computed vMin to vMax (or clamp and render overflow indicators) and ensure midY, root.yCenter and root.yScale are used consistently to map value->pixel so large translation channels remain visible.
162-167: ⚡ Quick winTime-axis ruler draws off-screen ticks unconditionally.
For a 60-second animation at
pxPerSec = 200withstep = 0.25, the ruler loop runs 240 iterations, most of which producefillTextcalls well outside[0, width]. A simple bounds guard keeps the work proportional to what's actually visible:♻️ Proposed guard
for (var t = 0; t <= maxT; t += step) { var x = (t - root.viewStart) * root.pxPerSec + if (x < -20 || x > width + 20) continue ctx.beginPath(); ctx.moveTo(x, height - 12); ctx.lineTo(x, height); ctx.stroke() ctx.fillText(t.toFixed(2) + "s", x + 2, height - 14) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@qml/AnimationCurveEditor.qml` around lines 162 - 167, The time-axis loop in AnimationCurveEditor.qml (calculating step from root.pxPerSec and iterating t up to maxT) draws ticks and calls ctx.fillText/ctx.stroke for values off-screen; add a bounds check using the computed x (x = (t - root.viewStart) * root.pxPerSec) against the visible range [0, width] (or a small margin) and skip drawing when x is outside that range so work is proportional to visible pixels; update the for loop that uses step, t, x, ctx.beginPath()/ctx.moveTo()/ctx.lineTo(), ctx.stroke() and ctx.fillText() to only perform drawing when x >= -margin and x <= width + margin (or equivalent).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@qml/AnimationCurveEditor.qml`:
- Around line 170-179: The grid and labels are drawn with ctx.globalAlpha set to
0.25, which also dims the text; restore the alpha before drawing labels so grid
lines remain dim but labels are fully opaque. In AnimationCurveEditor.qml adjust
the loop that draws horizontal grid lines and labels (uses midY, root.yCenter,
root.yScale, ctx.strokeStyle) so you set ctx.globalAlpha = 0.25 for the
stroke(), then reset ctx.globalAlpha = 1.0 immediately before calling
ctx.fillText(v.toFixed(1), ...) for each label (or after stroke and before any
fillText calls) to ensure labels render at full opacity.
- Around line 222-231: The tangent handle drawing uses raw slope values
(inT/outT) as pixel offsets, which ignores axis scaling; convert each tangent
(from CurveEditModel.tangentsAt()) into a screen-space dy using dy = tangent *
(handlePx / pxPerSec) * yScale and use that dy for the handle endpoints (replace
occurrences of inT * handlePx * 0.5 and outT * handlePx * 0.5 with the scaled
dy, keeping the existing 0.5 factor if intended), referencing variables kx, ky,
handlePx, pxPerSec, yScale, inT, outT and the drawing calls
(ctx.moveTo/ctx.lineTo) in AnimationCurveEditor.qml.
- Around line 260-272: The WheelHandler currently uses acceptedModifiers:
Qt.ControlModifier | Qt.MetaModifier which requires both modifiers together;
change acceptedModifiers to Qt.ControlModifier (remove the bitwise OR with
Qt.MetaModifier) in the WheelHandler block (the instance named WheelHandler with
the onWheel handler) so the wheel zoom triggers when the platform’s primary
control key is pressed; keep the rest of the onWheel logic (root.pxPerSec,
viewStart, curveCanvas.requestPaint()) unchanged.
In `@src/CurveEditModel.cpp`:
- Around line 34-48: makeKey currently joins raw fields with '|' which allows
ambiguity when any skeleton/anim/bone/channel contains '|' and also breaks
clearAnimation's prefix matching; change makeKey to produce an unambiguous
structured key (for example, encode each field as a length-prefixed segment like
"<len>:<data>" or apply a reversible escape on '|' and backslashes) so
collisions are impossible, and update clearAnimation to use the same
encoding/decoding logic or to parse the structured key instead of naive prefix
matching; ensure both makeKey and clearAnimation share the exact same encoding
routine so lookups and prefix-removals remain correct.
- Around line 71-87: In setTangents(...) update the promotion logic so editing
tangents on an auto key actually takes effect: when you modify
entry.inTangent/entry.outTangent in CurveEditModel::setTangents, treat ModeAuto
the same as ModeLinear/ModeStepped and set entry.mode = ModeBezier so evaluate()
won't recompute Catmull‑Rom handles; i.e. include entry.mode == ModeAuto in the
condition that promotes the mode to ModeBezier.
- Around line 105-113: CurveEditModel::clearAnimation currently erases matching
entries from m_entries but doesn't notify QML; after the loop that erases
entries in CurveEditModel::clearAnimation, emit the modelChanged() signal so
views (e.g., AnimationCurveEditor.qml) update. Locate the clearAnimation method
and add a call to emit modelChanged() (or the appropriate signal/method used by
this model) immediately after the removal loop completes.
---
Nitpick comments:
In `@qml/AnimationCurveEditor.qml`:
- Around line 173-177: The horizontal grid loop uses a hardcoded value range
(for (var v = -2; v <= 2; v += 0.5)) which causes curves to clip; update the
logic that draws horizontal ticks to compute vMin/vMax from the actual sampled
keyframe values (or from root.yCenter/root.yScale inputs) and derive tick step
and range from those values so ticks are centered and scaled to the data; modify
the loop to iterate from computed vMin to vMax (or clamp and render overflow
indicators) and ensure midY, root.yCenter and root.yScale are used consistently
to map value->pixel so large translation channels remain visible.
- Around line 162-167: The time-axis loop in AnimationCurveEditor.qml
(calculating step from root.pxPerSec and iterating t up to maxT) draws ticks and
calls ctx.fillText/ctx.stroke for values off-screen; add a bounds check using
the computed x (x = (t - root.viewStart) * root.pxPerSec) against the visible
range [0, width] (or a small margin) and skip drawing when x is outside that
range so work is proportional to visible pixels; update the for loop that uses
step, t, x, ctx.beginPath()/ctx.moveTo()/ctx.lineTo(), ctx.stroke() and
ctx.fillText() to only perform drawing when x >= -margin and x <= width + margin
(or equivalent).
🪄 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: c7d23ae4-c0d6-481b-896c-cdf6a414423a
📒 Files selected for processing (9)
qml/AnimationCurveEditor.qmlsrc/CMakeLists.txtsrc/CurveEditModel.cppsrc/CurveEditModel.hsrc/CurveEditModel_test.cppsrc/mainwindow.cppsrc/mainwindow.hsrc/qml_resources.qrctests/CMakeLists.txt
| var midY = (height - 16) / 2 | ||
| ctx.strokeStyle = AnimationControlController.borderColor | ||
| ctx.globalAlpha = 0.25 | ||
| for (var v = -2; v <= 2; v += 0.5) { | ||
| var y = midY - (v - root.yCenter) * root.yScale | ||
| if (y < 0 || y > height - 16) continue | ||
| ctx.beginPath(); ctx.moveTo(0, y); ctx.lineTo(width, y); ctx.stroke() | ||
| ctx.fillText(v.toFixed(1), 4, y - 2) | ||
| } | ||
| ctx.globalAlpha = 1.0 |
There was a problem hiding this comment.
Value-axis grid labels are rendered at 25% opacity — nearly invisible.
ctx.globalAlpha = 0.25 is set at line 172 to dim the grid lines, but the ctx.fillText calls on line 177 are inside the same alpha scope. The alpha is not restored until line 179 (ctx.globalAlpha = 1.0), after all labels have been drawn.
🐛 Proposed fix — reset alpha before drawing text
ctx.globalAlpha = 0.25
for (var v = -2; v <= 2; v += 0.5) {
var y = midY - (v - root.yCenter) * root.yScale
if (y < 0 || y > height - 16) continue
ctx.beginPath(); ctx.moveTo(0, y); ctx.lineTo(width, y); ctx.stroke()
+ ctx.globalAlpha = 1.0
ctx.fillText(v.toFixed(1), 4, y - 2)
+ ctx.globalAlpha = 0.25
}
ctx.globalAlpha = 1.0📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var midY = (height - 16) / 2 | |
| ctx.strokeStyle = AnimationControlController.borderColor | |
| ctx.globalAlpha = 0.25 | |
| for (var v = -2; v <= 2; v += 0.5) { | |
| var y = midY - (v - root.yCenter) * root.yScale | |
| if (y < 0 || y > height - 16) continue | |
| ctx.beginPath(); ctx.moveTo(0, y); ctx.lineTo(width, y); ctx.stroke() | |
| ctx.fillText(v.toFixed(1), 4, y - 2) | |
| } | |
| ctx.globalAlpha = 1.0 | |
| var midY = (height - 16) / 2 | |
| ctx.strokeStyle = AnimationControlController.borderColor | |
| ctx.globalAlpha = 0.25 | |
| for (var v = -2; v <= 2; v += 0.5) { | |
| var y = midY - (v - root.yCenter) * root.yScale | |
| if (y < 0 || y > height - 16) continue | |
| ctx.beginPath(); ctx.moveTo(0, y); ctx.lineTo(width, y); ctx.stroke() | |
| ctx.globalAlpha = 1.0 | |
| ctx.fillText(v.toFixed(1), 4, y - 2) | |
| ctx.globalAlpha = 0.25 | |
| } | |
| ctx.globalAlpha = 1.0 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/AnimationCurveEditor.qml` around lines 170 - 179, The grid and labels are
drawn with ctx.globalAlpha set to 0.25, which also dims the text; restore the
alpha before drawing labels so grid lines remain dim but labels are fully
opaque. In AnimationCurveEditor.qml adjust the loop that draws horizontal grid
lines and labels (uses midY, root.yCenter, root.yScale, ctx.strokeStyle) so you
set ctx.globalAlpha = 0.25 for the stroke(), then reset ctx.globalAlpha = 1.0
immediately before calling ctx.fillText(v.toFixed(1), ...) for each label (or
after stroke and before any fillText calls) to ensure labels render at full
opacity.
| ctx.globalAlpha = 0.6 | ||
| // Draw a short handle in each direction | ||
| var handlePx = 30 | ||
| ctx.beginPath() | ||
| ctx.moveTo(kx - handlePx, ky + inT * handlePx * 0.5) | ||
| ctx.lineTo(kx, ky) | ||
| ctx.lineTo(kx + handlePx, ky - outT * handlePx * 0.5) | ||
| ctx.stroke() | ||
| ctx.globalAlpha = 1.0 | ||
| } |
There was a problem hiding this comment.
Tangent handle pixel-space formula ignores axis scaling.
The in/out tangent values returned by CurveEditModel.tangentsAt() are slopes in value/time units (dv/dt). Applying them directly to pixel offsets (inT * handlePx * 0.5) yields an angle that scales with pxPerSec and yScale in opposite ways; the handles won't visually match the curve's actual slope. The correct screen-space offset is:
dy = tangent * (handlePx / pxPerSec) * yScale
🐛 Proposed fix
- ctx.moveTo(kx - handlePx, ky + inT * handlePx * 0.5)
- ctx.lineTo(kx, ky)
- ctx.lineTo(kx + handlePx, ky - outT * handlePx * 0.5)
+ var dtPx = handlePx / root.pxPerSec
+ ctx.moveTo(kx - handlePx, ky + inT * dtPx * root.yScale)
+ ctx.lineTo(kx, ky)
+ ctx.lineTo(kx + handlePx, ky - outT * dtPx * root.yScale)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/AnimationCurveEditor.qml` around lines 222 - 231, The tangent handle
drawing uses raw slope values (inT/outT) as pixel offsets, which ignores axis
scaling; convert each tangent (from CurveEditModel.tangentsAt()) into a
screen-space dy using dy = tangent * (handlePx / pxPerSec) * yScale and use that
dy for the handle endpoints (replace occurrences of inT * handlePx * 0.5 and
outT * handlePx * 0.5 with the scaled dy, keeping the existing 0.5 factor if
intended), referencing variables kx, ky, handlePx, pxPerSec, yScale, inT, outT
and the drawing calls (ctx.moveTo/ctx.lineTo) in AnimationCurveEditor.qml.
| WheelHandler { | ||
| target: null | ||
| acceptedModifiers: Qt.ControlModifier | Qt.MetaModifier | ||
| onWheel: function(event) { | ||
| var factor = event.angleDelta.y > 0 ? 1.15 : (1.0 / 1.15) | ||
| var newPx = Math.max(20, Math.min(2000, root.pxPerSec * factor)) | ||
| if (newPx === root.pxPerSec) return | ||
| var tCursor = root.viewStart + event.point.position.x / root.pxPerSec | ||
| root.pxPerSec = newPx | ||
| root.viewStart = tCursor - event.point.position.x / newPx | ||
| if (root.viewStart < 0) root.viewStart = 0 | ||
| curveCanvas.requestPaint() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && find . -name "AnimationCurveEditor.qml" 2>/dev/null | head -5Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
cat -n qml/AnimationCurveEditor.qml | sed -n '260,272p'Repository: fernandotonon/QtMeshEditor
Length of output: 785
🌐 Web query:
Qt WheelHandler acceptedModifiers keyboard modifiers semantics documentation
💡 Result:
In Qt Quick QML, WheelHandler has an acceptedModifiers property of type flags (Qt::KeyboardModifiers). It specifies which keyboard modifiers must be pressed for the handler to react to wheel events; otherwise, it ignores them. Default is Qt.KeyboardModifierMask, ignoring modifiers. Semantics: Setting acceptedModifiers requires exactly those modifiers (via bitwise OR) to be active. Use multiple handlers for different combinations, e.g., one with Qt.ControlModifier for rotation, another with Qt.NoModifier for scaling. Available modifiers: - Qt.NoModifier: No modifiers allowed. - Qt.ShiftModifier: Shift pressed. - Qt.ControlModifier: Ctrl pressed. - Qt.AltModifier: Alt pressed. - Qt.MetaModifier: Meta pressed. - Qt.KeypadModifier: Keypad button pressed. - Qt.GroupSwitchModifier: X11 Mode_switch (optional on Windows). - Qt.KeyboardModifierMask: Ignore modifiers. Example from docs: WheelHandler { property: "rotation" acceptedModifiers: Qt.ControlModifier } WheelHandler { property: "scale" acceptedModifiers: Qt.NoModifier } For complex checks, use point.modifiers or wheel.modifiers in onWheel with bitwise AND (&), e.g., if (wheel.modifiers & Qt.ControlModifier). This matches PointerHandler behavior and WheelEvent.modifiers (bitwise Qt::KeyboardModifiers at event time). Official Qt 6 docs consistent across versions.
Citations:
- 1: https://doc.qt.io/qt-6.4/qml-qtquick-wheelhandler.html
- 2: https://doc.qt.io/qt-6/qml-qtquick-wheelhandler.html
- 3: https://doc.qt.io/qt-5.15/qml-qtquick-wheelhandler.html
- 4: https://doc.qt.io/qt-6/qml-qtquick-wheelhandler-members.html
- 5: http://doc.qt.io/qt-6/qml-qtquick-wheelevent.html
🌐 Web query:
Qt 6 acceptedModifiers combined flags Control Meta bitwise AND matching logic
💡 Result:
In Qt 6 QML input handlers like MouseArea, TapHandler, HoverHandler, and Keys attached property, the acceptedModifiers property uses Qt::KeyboardModifiers flags combined with bitwise OR (|). The matching logic between the event's modifiers (mouse.modifiers or event.modifiers, also Qt::KeyboardModifiers) and acceptedModifiers is a bitwise AND (&) check: the handler accepts the event if (event.modifiers & acceptedModifiers) == acceptedModifiers. This means the event modifiers must have exactly the bits set that are set in acceptedModifiers—no more, no less among those bits. For example: - acceptedModifiers: Qt::ControlModifier accepts only Ctrl (no other modifiers from the checked set). - acceptedModifiers: Qt::ControlModifier | Qt::MetaModifier accepts exactly Ctrl+Meta. - acceptedModifiers: Qt::KeyboardModifierMask accepts any combination. To check if specific modifiers are present (allowing others), use (event.modifiers & Qt::ControlModifier) inside the handler signal, with acceptedModifiers: Qt::KeyboardModifierMask. This behavior is unchanged in Qt 6 from previous versions, as confirmed by code examples and explanations in technical resources.
Citations:
- 1: https://runebook.dev/en/docs/qt/qml-qtquick-hoverhandler/acceptedModifiers
- 2: https://runebook.dev/en/docs/qt/qt3dinput-qkeyevent/modifiers-prop
- 3: https://doc.qt.io/qt-6/qml-qtquick-keyevent.html
🌐 Web query:
Qt WheelHandler "acceptedModifiers" "Qt.ControlModifier | Qt.MetaModifier" requires both
💡 Result:
Yes, setting acceptedModifiers: Qt.ControlModifier | Qt.MetaModifier in Qt WheelHandler requires both the Control and Meta modifiers to be pressed simultaneously for the handler to react to wheel events. If either is missing, it ignores the event. This behavior is by design in Qt's PointerHandler system (including WheelHandler). The documentation explicitly states: "If you set acceptedModifiers to an OR combination of modifier keys, it means all of those modifiers must be pressed to activate the handler." The bitwise OR (|) creates a bitmask where the handler checks that point.modifiers exactly matches the set bits for those modifiers (using a bitwise AND check internally). Example from docs: Item { TapHandler { // Same logic applies to WheelHandler acceptedModifiers: Qt.ControlModifier | Qt.AltModifier | Qt.ShiftModifier onTapped: console.log("control-alt-shift-tapped") } } For WheelHandler specifically, the Qt 6.11 docs confirm acceptedModifiers works the same way, requiring the specified modifiers.
Citations:
- 1: https://doc.qt.io/qt-6/qml-qtquick-wheelhandler.html
- 2: https://doc.qt.io/qt-6.4/qml-qtquick-wheelhandler.html
- 3: https://doc.qt.io/qt-5.15/qml-qtquick-wheelhandler.html
Zoom gesture is broken: acceptedModifiers requires BOTH modifiers simultaneously.
acceptedModifiers: Qt.ControlModifier | Qt.MetaModifier will fire only when both modifiers are pressed at once. Qt's matching logic is (event.modifiers & acceptedModifiers) == acceptedModifiers, so all bits must match exactly. This requires Ctrl+Win on Windows/Linux and ⌘+physical Ctrl on macOS—effectively unreachable in practice.
Qt.ControlModifier alone correctly handles zoom across platforms (Cmd on macOS, Ctrl on Windows/Linux).
Fix
- acceptedModifiers: Qt.ControlModifier | Qt.MetaModifier
+ acceptedModifiers: Qt.ControlModifier📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| WheelHandler { | |
| target: null | |
| acceptedModifiers: Qt.ControlModifier | Qt.MetaModifier | |
| onWheel: function(event) { | |
| var factor = event.angleDelta.y > 0 ? 1.15 : (1.0 / 1.15) | |
| var newPx = Math.max(20, Math.min(2000, root.pxPerSec * factor)) | |
| if (newPx === root.pxPerSec) return | |
| var tCursor = root.viewStart + event.point.position.x / root.pxPerSec | |
| root.pxPerSec = newPx | |
| root.viewStart = tCursor - event.point.position.x / newPx | |
| if (root.viewStart < 0) root.viewStart = 0 | |
| curveCanvas.requestPaint() | |
| } | |
| WheelHandler { | |
| target: null | |
| acceptedModifiers: Qt.ControlModifier | |
| onWheel: function(event) { | |
| var factor = event.angleDelta.y > 0 ? 1.15 : (1.0 / 1.15) | |
| var newPx = Math.max(20, Math.min(2000, root.pxPerSec * factor)) | |
| if (newPx === root.pxPerSec) return | |
| var tCursor = root.viewStart + event.point.position.x / root.pxPerSec | |
| root.pxPerSec = newPx | |
| root.viewStart = tCursor - event.point.position.x / newPx | |
| if (root.viewStart < 0) root.viewStart = 0 | |
| curveCanvas.requestPaint() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@qml/AnimationCurveEditor.qml` around lines 260 - 272, The WheelHandler
currently uses acceptedModifiers: Qt.ControlModifier | Qt.MetaModifier which
requires both modifiers together; change acceptedModifiers to Qt.ControlModifier
(remove the bitwise OR with Qt.MetaModifier) in the WheelHandler block (the
instance named WheelHandler with the onWheel handler) so the wheel zoom triggers
when the platform’s primary control key is pressed; keep the rest of the onWheel
logic (root.pxPerSec, viewStart, curveCanvas.requestPaint()) unchanged.
| std::string CurveEditModel::makeKey(const QString& skeleton, | ||
| const QString& anim, | ||
| const QString& bone, | ||
| const QString& channel, | ||
| double time) | ||
| { | ||
| // Quantize time to ms so floating-point round-trip doesn't break the key. | ||
| // Same precision as the dope sheet's keyframe-tick comparisons. | ||
| std::ostringstream ss; | ||
| ss << skeleton.toStdString() << '|' | ||
| << anim.toStdString() << '|' | ||
| << bone.toStdString() << '|' | ||
| << channel.toStdString() << '|' | ||
| << static_cast<long long>(std::llround(time * 1000.0)); | ||
| return ss.str(); |
There was a problem hiding this comment.
Make the composite key unambiguous.
makeKey() joins raw names with |, so any imported skeleton/animation/bone name containing that character can alias a different (skeleton, anim, bone, channel, time) tuple. clearAnimation() inherits the same ambiguity through its prefix match, so one asset can overwrite or prune another's curve data. Use a structured key or length-prefix/escape each field before concatenation.
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 42-42: There is an unknown macro here somewhere. Configuration is required. If Q_ENUM is a macro then please configure it.
(unknownMacro)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CurveEditModel.cpp` around lines 34 - 48, makeKey currently joins raw
fields with '|' which allows ambiguity when any skeleton/anim/bone/channel
contains '|' and also breaks clearAnimation's prefix matching; change makeKey to
produce an unambiguous structured key (for example, encode each field as a
length-prefixed segment like "<len>:<data>" or apply a reversible escape on '|'
and backslashes) so collisions are impossible, and update clearAnimation to use
the same encoding/decoding logic or to parse the structured key instead of naive
prefix matching; ensure both makeKey and clearAnimation share the exact same
encoding routine so lookups and prefix-removals remain correct.
| void CurveEditModel::setTangents(const QString& skeleton, | ||
| const QString& anim, | ||
| const QString& bone, | ||
| const QString& channel, | ||
| double time, | ||
| double inTangent, double outTangent) | ||
| { | ||
| const auto key = makeKey(skeleton, anim, bone, channel, time); | ||
| auto& entry = m_entries[key]; | ||
| entry.inTangent = inTangent; | ||
| entry.outTangent = outTangent; | ||
| if (entry.mode == ModeLinear || entry.mode == ModeStepped) { | ||
| // Editing a tangent implies the user wants curve control — | ||
| // promote the mode to Bezier so the tangents take effect. | ||
| entry.mode = ModeBezier; | ||
| } | ||
| emit modelChanged(skeleton, anim, bone, channel); |
There was a problem hiding this comment.
Promote ModeAuto to ModeBezier when tangents are edited.
Right now a key in ModeAuto keeps that mode after setTangents(), and evaluate() recomputes Catmull-Rom tangents instead of using the stored handles. That makes manual tangent edits on auto keys a no-op.
Suggested fix
- if (entry.mode == ModeLinear || entry.mode == ModeStepped) {
+ if (entry.mode == ModeLinear || entry.mode == ModeStepped || entry.mode == ModeAuto) {
// Editing a tangent implies the user wants curve control —
// promote the mode to Bezier so the tangents take effect.
entry.mode = ModeBezier;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CurveEditModel.cpp` around lines 71 - 87, In setTangents(...) update the
promotion logic so editing tangents on an auto key actually takes effect: when
you modify entry.inTangent/entry.outTangent in CurveEditModel::setTangents,
treat ModeAuto the same as ModeLinear/ModeStepped and set entry.mode =
ModeBezier so evaluate() won't recompute Catmull‑Rom handles; i.e. include
entry.mode == ModeAuto in the condition that promotes the mode to ModeBezier.
| void CurveEditModel::clearAnimation(const QString& skeleton, const QString& anim) | ||
| { | ||
| const std::string prefix = | ||
| skeleton.toStdString() + "|" + anim.toStdString() + "|"; | ||
| for (auto it = m_entries.begin(); it != m_entries.end(); ) { | ||
| if (it->first.rfind(prefix, 0) == 0) it = m_entries.erase(it); | ||
| else ++it; | ||
| } | ||
| } |
There was a problem hiding this comment.
Emit modelChanged() after clearAnimation().
This erases entries without notifying QML. AnimationCurveEditor.qml repaints off CurveEditModel.onModelChanged, so cleared handles/modes can remain visible until some unrelated repaint happens.
Suggested fix
void CurveEditModel::clearAnimation(const QString& skeleton, const QString& anim)
{
const std::string prefix =
skeleton.toStdString() + "|" + anim.toStdString() + "|";
+ bool removedAny = false;
for (auto it = m_entries.begin(); it != m_entries.end(); ) {
- if (it->first.rfind(prefix, 0) == 0) it = m_entries.erase(it);
+ if (it->first.rfind(prefix, 0) == 0) {
+ it = m_entries.erase(it);
+ removedAny = true;
+ }
else ++it;
}
+ if (removedAny) emit modelChanged(skeleton, anim, QString(), QString());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CurveEditModel.cpp` around lines 105 - 113,
CurveEditModel::clearAnimation currently erases matching entries from m_entries
but doesn't notify QML; after the loop that erases entries in
CurveEditModel::clearAnimation, emit the modelChanged() signal so views (e.g.,
AnimationCurveEditor.qml) update. Locate the clearAnimation method and add a
call to emit modelChanged() (or the appropriate signal/method used by this
model) immediately after the removal loop completes.
|
1 participant
Summary
First half of slice D3 from #260 / #378 — the curve editor's data model + read-only QML view. Splitting D3 in two so the PR stays reviewable; D3b lands handle dragging + resample-into-track.
What's in this PR
CurveEditModel(C++ singleton) — per-keyframe Bezier tangent handles + interpolation mode (Bezier / Linear / Stepped / Auto), keyed by (skeleton, animation, bone, channel, time). Pure-data so it tests cleanly without Ogre.evaluate()— cubic Hermite spline for Bezier, explicit Linear and Stepped, Catmull-Rom-style auto-tangents for Auto. Returns the curve value at any time given the channel's keyframe times + values.AnimationCurveEditor.qml— Curve Editor view, tabified next to the Dope Sheet. Renders the selected bone's active channels with color coding matching the D2 sub-rows. Time-axis ruler + value-axis grid; per-keyframe squares + tangent handle stubs (non-interactive in D3a).Test plan
Deferred to D3b
🤖 Generated with Claude Code
Summary by CodeRabbit