feat(morph): A3b — Inspector add / rename / delete for morph targets#579
Conversation
Follow-up to #578. Wires the C++ authoring API from A3 into the existing Morph Targets subgroup in qml/PropertiesPanel.qml: - **"+ Add…" button** next to "Reset all" — opens a built-in Qt Quick Controls Popup with a name field, calls `addMorphTargetFromCurrentEdit`. The Popup deliberately avoids custom dialog components: A5's QML changes hit cross-engine initialization issues we never fully isolated, so this slice sticks to primitives with no singleton dependencies beyond the already-imported MorphAnimationManager. - **Inline rename** — double-click a target name to edit in place, matching the per-animation rename UX a few sections above. Empty / whitespace-only / unchanged names are filtered client-side before the call. Server-side collision rejection still applies. - **Per-row "×" delete** — pushes DeleteMorphTargetCommand through UndoManager so Ctrl+Z restores the pose. The existing `Connections` block on the manager already refreshes the target list and weight readouts on `morphTargetsChanged` / `morphWeightChanged`, so add / rename / delete all flow back into the UI for free. After this slice the morph epic (#518) is end-to-end: import → list → set weight → save current edit as new target → rename → delete → export — all with undo. Closes the authoring acceptance criterion in the issue. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughQML UI layer adds morph-target creation, renaming, and deletion controls to the Animations panel. A header "+ Add…" button opens a name-entry popup, inline row edits use double-click TextInput renaming, and each target gains a delete button—all delegating state changes to MorphAnimationManager. ChangesMorph Target CRUD Operations
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 860eccd918
ℹ️ 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".
| MorphAnimationManager.addMorphTargetFromCurrentEdit(n) | ||
| addNamePopup.close() |
There was a problem hiding this comment.
Handle the return value from MorphAnimationManager.addMorphTargetFromCurrentEdit(n) before closing the popup. That method returns false for common failure cases (for example duplicate names or not being in edit mode), but this code closes the dialog unconditionally, so users lose their input and get no feedback while nothing is created.
Useful? React with 👍 / 👎.
| if (trimmed.length > 0 && trimmed !== modelData) | ||
| MorphAnimationManager.renameMorphTarget(modelData, trimmed) | ||
| visible = false | ||
| } | ||
| Keys.onEscapePressed: visible = false |
There was a problem hiding this comment.
Pressing Escape is intended to cancel inline rename, but Keys.onEscapePressed only hides the TextInput; hiding causes focus loss, and onEditingFinished then still executes the rename path when text changed. This means users can accidentally rename a target even when they hit Escape to cancel.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/PropertiesPanel.qml`:
- Around line 4372-4395: The "+ Add…" control must be disabled when
add-from-edit is unavailable: use a controller boolean (e.g.
PropertiesPanelController.addFromEditAvailable) to drive the enabled state and
visual disabled styling on the Rectangle (addBtn) and MouseArea (addMa), e.g.
set addBtn.enabled: PropertiesPanelController.addFromEditAvailable (or
addMa.enabled likewise), change color/opacity and cursorShape when disabled, and
remove/guard the addNamePopup.open() call so it only runs when the flag is true;
apply the same change to the other "+ Add…" block referenced (lines 4462-4467)
so both instances respect PropertiesPanelController.addFromEditAvailable.
- Around line 4550-4557: Escape currently hides the TextInput via
Keys.onEscapePressed which causes onEditingFinished to run and trigger
MorphAnimationManager.renameMorphTarget; fix by introducing a cancel flag on the
TextInput (e.g., skipRename) and set skipRename = true inside
Keys.onEscapePressed before setting visible = false, then change the
onEditingFinished handler to return early if skipRename is true (clear
skipRename after checking) so Escape cancels without renaming; reference the
existing onEditingFinished, Keys.onEscapePressed,
MorphAnimationManager.renameMorphTarget, modelData and trimmed identifiers when
making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Addresses three findings from Codex + CodeRabbit on PR #579: ## Codex P2 — keep Add popup open on failure `MorphAnimationManager.addMorphTargetFromCurrentEdit` returns false for several legitimate failure cases (duplicate name, not in edit mode, no vertex moved vs bind baseline). The popup was closing unconditionally, so users lost their typed name with no feedback about why nothing was created. Fix: branch on the return value. On true → close. On false → leave the popup open and surface an inline red error message ("Couldn't save: name already in use, or no vertex was edited."). Empty-name and edit-mode-required failures get their own explicit messages. ## Codex P2 — Escape must not commit rename Pressing Escape on the inline morph-rename input previously fired `Keys.onEscapePressed: visible = false`, but the hide path causes focus loss which then re-fires `onEditingFinished` — committing the rename the user tried to cancel. Fix: add a `cancelled` boolean property set by Escape and checked at the top of `onEditingFinished`; the cancel path resets the flag and returns without calling renameMorphTarget. ## CodeRabbit minor — visibly disable Add outside edit mode The "+ Add…" button stayed enabled even when not in edit mode, where `addMorphTargetFromCurrentEdit` always returns false. Fix: bind a `canAddFromEdit: EditModeController.editModeActive` property on the button; control opacity, hover highlight, MouseArea enabled state, cursor shape (PointingHand vs Forbidden), and add a tooltip that says "Enter Edit Mode (Tab) to add morph targets from current edit." The Save handler in the popup also re-checks `editModeActive` defensively — protects against the edge case where the user opens the popup mid-edit, exits edit mode, then clicks Save. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Final sub-slice of #518. The C++ data API (`allMorphRows`) shipped in #576 as a Q_INVOKABLE on AnimationControlController; this PR wires it into the dope-sheet view. ## What ships - New `morphRows` property on the dope-sheet root, bound to `AnimationControlController.allMorphRows()`. Refreshed via the existing `onSelectionChanged` handler so a clip change rebuilds both bone and morph rows in lockstep. - `rowsView` (bone ListView) bottom anchor switches to `morphBand.top` when the morph band is visible. Bone-only entities → band collapses to `height=0` → bone list draws full height as before. - New `morphBand` Rectangle at the bottom: header row showing "Morph Targets (N)", then one row per pose with the target name on the left and `#88ccff` diamond markers at each keyframe time on the right. Diamonds share the bone-row timeline math (`pxPerSec`, `viewStart`) so they line up vertically. ## Why this is small and safe The original A5 (PR #576's first commit) deterministically crashed unrelated MainWindow / MCPServer visibility-toggle suites in CI. Root cause was never fully isolated but the suspicion was an `import PropertiesPanel 1.0` triggering a different QML singleton chain during MainWindow construction. This PR keeps the discipline that worked for A3b: - **No new QML imports.** `AnimationControl 1.0` was already imported. `allMorphRows()` was deliberately put on AnimationControlController (not MorphAnimationManager) for exactly this reason. - **No new `Connections` blocks.** The existing one already fires on the signals we care about. - **No MouseAreas, no selection, no drag** on morph diamonds — this is strictly read-only display. Interactive morph keyframes (selection, multi-select, drag, copy/paste) need their own controller surface and land in a separate PR. ## #518 status After this slice the issue is feature-complete and ready to close: | Sub-slice | Status | |-|-| | A1 — Importer + manager + CLI list | shipped (#569) | | A2 — Inspector subgroup | shipped (#570) | | A3 — Authoring data layer | shipped (#578) | | A3b — Inspector authoring UI | shipped (#579) | | A4a — glTF export | shipped (#573) | | A4b — FBX export | shipped (#575) | | A5 — Controller API | shipped (#576) | | A5b — Dope-sheet UI | **this PR** | | A6 — MCP tools | shipped (#571) | Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Follow-up to #578. Surfaces the C++ authoring API from morph slice A3 in the existing Morph Targets subgroup in
qml/PropertiesPanel.qml. After this PR the morph epic (#518) is end-to-end: import → list → set weight → save current edit as new target → rename → delete → export, all with undo.What ships
"+ Add…" button (top row, next to "Reset all")
Opens a built-in Qt Quick Controls
Popupwith a name field, callsMorphAnimationManager.addMorphTargetFromCurrentEdit. Deliberately uses primitives — no custom dialog component, no new imports — because A5's QML changes hit cross-engine initialization issues we never fully isolated and I want to keep this slice's blast radius minimal.Inline rename (double-click name)
Mirrors the per-animation rename UX a few sections above. Double-click → hidden
TextInputbecomes visible →onEditingFinishedcallsrenameMorphTarget. Empty / whitespace-only / unchanged names are filtered client-side before the call. Server-side collision rejection still applies (no shadowing).Per-row "×" delete
Pushes
DeleteMorphTargetCommandthrough the sharedUndoManager, so Ctrl+Z restores the pose + animation.Why this is small and safe
MorphAnimationManagerwas already imported by the existing subgroup.Connectionsblocks — the existing one already refreshes target list and weight readouts onmorphTargetsChanged/morphWeightChanged, so add / rename / delete flow back into the UI without new wiring.Q_INVOKABLEmethods onMorphAnimationManager. The C++ surface was already covered by the 16 tests merged in feat(morph): sub-slice A3 — authoring (add from edit, rename, delete) #578.#518 status
Once A3b lands the epic is feature-complete except for the dope-sheet morph band (the deferred UI from A5).
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes