feat(skeleton): Slice B — bone hierarchy editing (#556) - #908
Conversation
…nect (#556) Add Slice B hierarchy ops through SkeletonEditor snapshot rebuild, undo commands, Inspector dialogs/context menu, viewport right-click, and clearer joint+link skeleton debug visuals. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds reparent, detach, split, connect/disconnect, and cross-entity bone attachment operations with undo support. New QML dialogs, tool buttons, context-menu actions, viewport picking, tests, and joint/link debug visuals expose and validate the functionality. ChangesSkeleton editing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: fc29ba7fb0
ℹ️ 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".
| splitBd.name = childName.toStdString(); | ||
| splitBd.handle = newHandle; | ||
| splitBd.parentName = boneStd; | ||
| splitBd.position = axis * (1.f - t); |
There was a problem hiding this comment.
When the split slider is set to any value other than 0.5, this places the inserted joint at (1 - t) along the old bone while the former children are also offset by axis * t. For example, splitting at 0.25 puts the new bone at 75% and leaves a former child another 75% away, stretching that chain to 150% of its original length; the UI allows 0.05–0.95, so non-default splits corrupt the hierarchy. The inserted bone should be positioned at axis * t to match the child offset logic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7afdd53 — the split joint is now placed at axis * t, and the test asserts tip length is preserved for t=0.25.
Non-0.5 splits were inserting the joint at (1-t) while shifting children by t, which stretched the chain. Position the joint at axis*t and assert the tip stays at the original length. Co-authored-by: Cursor <cursoragent@cursor.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: 10
🤖 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 3040-3076: Make the custom bone-editing controls
keyboard-accessible: in qml/PropertiesPanel.qml lines 3040-3076, update
SkelToolButton with activeFocusOnTab, Accessible.role/name, visible focus
styling, and Enter/Space activation; apply the same focusable, accessible,
keyboard-activation behavior to Cancel and Reparent in
qml/ReparentBoneDialog.qml lines 294-331, Cancel and Split in
qml/SplitBoneDialog.qml lines 73-114, and Cancel and Attach in
qml/AttachBoneDialog.qml lines 62-105. In qml/BoneContextMenu.qml lines 68-103,
update MenuRow with focus navigation, accessible semantics, visible focus
styling, and Enter/Space activation.
- Around line 3079-3093: Update the first action Row containing the “+ Bone”,
“Duplicate”, “Reparent”, “Detach”, and “Split” SkelToolButton controls to use a
wrapping layout, allowing its buttons to flow onto additional lines when the
Inspector dock is narrower than their combined minimum width. Keep the existing
spacing and button actions unchanged.
- Around line 3057-3064: The connect/disconnect labels currently call the
non-reactive SkeletonEditor.isSelectedBoneConnected() getter, so they do not
update when selection or structure changes. In qml/PropertiesPanel.qml lines
3057-3064, bind skelBtnLabel to a notified property or cached connected-state
value refreshed on those changes; in qml/BoneContextMenu.qml lines 121-125,
snapshot or refresh the connected state in openAt() before showing the menu.
In `@src/SkeletonDebug.cpp`:
- Around line 207-211: Update the entity-scale handling near entityScale and
invEntScale in SkeletonDebug to use clamped absolute magnitudes for each scale
component before both inversion and subsequent axis-scale calculations. Ensure
negative values contribute by magnitude and zero components are clamped to the
minimum threshold, preventing incorrect inversion and infinite axis scales.
- Around line 182-198: Update the skeleton debug link setup around childPos and
the per-frame onTimerTick flow to retain each link’s child-bone association
instead of using getInitialPosition() only once. Store the child bone with its
visual entity, then recompute each link’s direction, length, orientation, and
scale from the child’s current transform during onTimerTick, preserving the
existing zero-length and thickness behavior.
In `@src/SkeletonEditor_test.cpp`:
- Around line 372-381: Strengthen the collision assertions in the
attachBonesToEntity test: verify result.boneName is different from the
pre-existing "Child" name, and capture the destination skeleton bone count
before the call and assert it increases afterward. Keep the existing success and
returned-bone existence checks.
In `@src/SkeletonEditor.cpp`:
- Around line 1014-1036: Reorder the logic in the surrounding skeleton-copy
operation so every requested source bone is validated and descendants are
collected into toCopy before calling ensureEntitySkeleton or otherwise mutating
the destination. Preserve the existing error for missing source bones, then
proceed with destination creation and snapshot capture only after validation
succeeds.
- Around line 1044-1092: The copied-bone ordering in the loop over ordered must
be parent-first rather than relying on handle order. Traverse the source
hierarchy or sort by depth so every copied parent is entered in renameMap before
its children, and replace renameMap[parentName] lookup with a non-inserting
lookup that handles unresolved parents without assigning an empty parent.
- Around line 974-982: Update the disconnect handling around the
connected/target position assignment to persist connectivity beyond the
transient live pose. Store an explicit disconnected state or use a durable bind
offset, and ensure bind resets and animation evaluation preserve the
disconnected state instead of reconnecting the bone; keep connected bones’
existing position behavior unchanged.
- Around line 859-879: Update the inserted split bone initialization in the
split-bone logic so splitBd.position and splitBd.initialPosition use axis * t,
matching the child offset adjustment in the loop over snap.bones. Keep the
existing parent assignment and other bone properties unchanged.
🪄 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: 7cd6b961-38d2-4b24-bfa7-3fa63c9879b5
📒 Files selected for processing (15)
qml/AttachBoneDialog.qmlqml/BoneContextMenu.qmlqml/PropertiesPanel.qmlqml/ReparentBoneDialog.qmlqml/SplitBoneDialog.qmlqml/qmldirsrc/SkeletonDebug.cppsrc/SkeletonDebug.hsrc/SkeletonEditor.cppsrc/SkeletonEditor.hsrc/SkeletonEditor_test.cppsrc/TransformOperator.cppsrc/commands/SkeletonBoneCommands.cppsrc/commands/SkeletonBoneCommands.hsrc/qml_resources.qrc
| component SkelToolButton: Rectangle { | ||
| id: skelBtn | ||
| property string label: "" | ||
| property string action: "" | ||
| property bool needsBone: true | ||
| width: Math.max(56, skelBtnLabel.implicitWidth + 14) | ||
| height: 22 | ||
| radius: 3 | ||
| color: skelBtnMa.containsMouse | ||
| ? Qt.lighter(PropertiesPanelController.headerColor, 1.2) | ||
| : PropertiesPanelController.headerColor | ||
| border.color: PropertiesPanelController.borderColor | ||
| border.width: 1 | ||
| opacity: (SkeletonEditor.hasSkeletonSelection | ||
| && (!skelBtn.needsBone || AnimationControlController.selectedBone.length > 0)) | ||
| ? 1.0 : 0.45 | ||
|
|
||
| Text { | ||
| id: labelText | ||
| anchors.centerIn: parent | ||
| text: modelData.label | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 10 | ||
| } | ||
| MouseArea { | ||
| id: btnMa | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| cursorShape: Qt.PointingHandCursor | ||
| enabled: SkeletonEditor.hasSkeletonSelection | ||
| && (!modelData.needsBone || AnimationControlController.selectedBone.length > 0) | ||
| onClicked: { | ||
| root.boneEditError = false | ||
| root.boneEditStatus = "" | ||
| if (modelData.action === "create") { | ||
| if (SkeletonEditor.createBoneForSelected("")) { | ||
| root.boneEditStatus = "Bone created." | ||
| } else { | ||
| root.boneEditError = true | ||
| root.boneEditStatus = "Could not create bone." | ||
| } | ||
| } else if (modelData.action === "duplicate") { | ||
| if (SkeletonEditor.duplicateSelectedBone()) { | ||
| root.boneEditStatus = "Bone duplicated." | ||
| } else { | ||
| root.boneEditError = true | ||
| root.boneEditStatus = "Select a bone first." | ||
| } | ||
| } else if (modelData.action === "remove") { | ||
| root.openRemoveBoneDialog() | ||
| } else if (modelData.action === "rename") { | ||
| root.openRenameBoneDialog() | ||
| } | ||
| } | ||
| Text { | ||
| id: skelBtnLabel | ||
| anchors.centerIn: parent | ||
| text: { | ||
| if (skelBtn.action === "connect") | ||
| return SkeletonEditor.isSelectedBoneConnected() ? "Disconnect" : "Connect" | ||
| return skelBtn.label | ||
| } | ||
| color: PropertiesPanelController.textColor | ||
| font.pixelSize: 10 | ||
| } | ||
| MouseArea { | ||
| id: skelBtnMa | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| cursorShape: Qt.PointingHandCursor | ||
| enabled: SkeletonEditor.hasSkeletonSelection | ||
| && (!skelBtn.needsBone || AnimationControlController.selectedBone.length > 0) | ||
| onClicked: root.runSkeletonToolAction(skelBtn.action) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Custom bone-editing controls are mouse-only. Add activeFocusOnTab, Accessible.role/name, visible focus styling, and Enter/Space activation consistently.
qml/PropertiesPanel.qml#L3040-L3076: add button semantics and keyboard activation toSkelToolButton.qml/ReparentBoneDialog.qml#L294-L331: make Cancel and Reparent focusable and keyboard-activatable.qml/SplitBoneDialog.qml#L73-L114: make Cancel and Split focusable and keyboard-activatable.qml/AttachBoneDialog.qml#L62-L105: make Cancel and Attach focusable and keyboard-activatable.qml/BoneContextMenu.qml#L68-L103: add focus navigation and activation semantics toMenuRow.
📍 Affects 5 files
qml/PropertiesPanel.qml#L3040-L3076(this comment)qml/ReparentBoneDialog.qml#L294-L331qml/SplitBoneDialog.qml#L73-L114qml/AttachBoneDialog.qml#L62-L105qml/BoneContextMenu.qml#L68-L103
🤖 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 3040 - 3076, Make the custom
bone-editing controls keyboard-accessible: in qml/PropertiesPanel.qml lines
3040-3076, update SkelToolButton with activeFocusOnTab, Accessible.role/name,
visible focus styling, and Enter/Space activation; apply the same focusable,
accessible, keyboard-activation behavior to Cancel and Reparent in
qml/ReparentBoneDialog.qml lines 294-331, Cancel and Split in
qml/SplitBoneDialog.qml lines 73-114, and Cancel and Attach in
qml/AttachBoneDialog.qml lines 62-105. In qml/BoneContextMenu.qml lines 68-103,
update MenuRow with focus navigation, accessible semantics, visible focus
styling, and Enter/Space activation.
| const Ogre::Vector3 childPos = childNode->getInitialPosition(); | ||
| const float length = childPos.length(); | ||
| if (length < 0.00001f) | ||
| continue; | ||
|
|
||
| Ogre::Entity* link = mSceneMan->createEntity("SkeletonDebug/LinkMesh"); | ||
| auto* tp = mEntity->attachObjectToBone(pBone->getName(), (Ogre::MovableObject*)link); | ||
| mBoneEntities.push_back(link); | ||
|
|
||
| const Ogre::Vector3 dir = childPos / length; | ||
| tp->setOrientation(Ogre::Vector3::UNIT_Y.getRotationTo(dir)); | ||
| // Link mesh is unit length along +Y; keep thickness constant so | ||
| // multi-child fans don't look like a bundle of full bones. | ||
| const float thick = std::max(mBoneSize * 0.35f, 0.008f); | ||
| tp->setScale(thick, length, thick); | ||
|
|
||
| tagBoneVisual(link, pBone->getName(), mEntity->getName()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep outline src/SkeletonDebug.cpp \
--items all --type function --match 'onTimerTick|createChildLinks' --view expanded
rg -n -C3 'setTranslate\s*\(|getInitialPosition\s*\(|getPosition\s*\(' srcRepository: fernandotonon/QtMeshEditor
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' src/SkeletonDebug.cpp | cat -n | sed -n '1,260p'Repository: fernandotonon/QtMeshEditor
Length of output: 10850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,260p' src/SkeletonDebug.cpp | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 10850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'SkeletonDebug|createChildLinks|getInitialPosition|_getDerivedPosition|getDerivedPosition|update.*visual|bind pose|hierarchy edges' src/SkeletonDebug.* src/*.cpp src/*.hRepository: fernandotonon/QtMeshEditor
Length of output: 50383
Refresh hierarchy links from the child’s current transform. src/SkeletonDebug.cpp:182-196 builds each rod from getInitialPosition() once, but onTimerTick() never updates it. When a child bone translates during animation, the debug link can drift away from the child joint. Store the child bone and recompute the link orientation/length from the live pose.
🤖 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/SkeletonDebug.cpp` around lines 182 - 198, Update the skeleton debug link
setup around childPos and the per-frame onTimerTick flow to retain each link’s
child-bone association instead of using getInitialPosition() only once. Store
the child bone with its visual entity, then recompute each link’s direction,
length, orientation, and scale from the child’s current transform during
onTimerTick, preserving the existing zero-length and thickness behavior.
There was a problem hiding this comment.
Acknowledged / deferred. Links are bind-pose hierarchy edges today (same as the old octahedrons). Live per-frame retarget during animation is a reasonable follow-up but out of scope for hierarchy-edit Slice B.
There was a problem hiding this comment.
@fernandotonon, understood—this is intentional bind-pose hierarchy visualization and matches the previous behavior. The live-pose retargeting concern is non-blocking for Slice B.
Would you like me to open a follow-up GitHub issue for animated hierarchy-link updates?
✏️ Learnings added
Learnt from: fernandotonon
Repo: fernandotonon/QtMeshEditor PR: 908
File: src/SkeletonDebug.cpp:182-198
Timestamp: 2026-07-19T01:03:18.892Z
Learning: In `src/SkeletonDebug.cpp`, hierarchy link rods intentionally represent bind-pose hierarchy edges using child bones’ initial transforms, matching the old octahedron visualization. Updating links from live animated bone transforms is deferred and out of scope for the hierarchy-edit Slice B PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (connected) { | ||
| target->position = tip; | ||
| target->initialPosition = tip; | ||
| } else { | ||
| // Keep bind tip in initialPosition; offset only the live pose so | ||
| // isBoneConnected can recover the tip for sole children. | ||
| target->initialPosition = tip; | ||
| target->position = tip + dir * kDisconnectGap; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Persist disconnect state outside the transient live pose.
Disconnect leaves initialPosition at the tip and offsets only position. A bind reset or animation evaluation therefore reconnects the bone. Store connectivity explicitly, or encode a durable bind offset with tip calculation independent of the child’s initial position.
🤖 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/SkeletonEditor.cpp` around lines 974 - 982, Update the disconnect
handling around the connected/target position assignment to persist connectivity
beyond the transient live pose. Store an explicit disconnected state or use a
durable bind offset, and ensure bind resets and animation evaluation preserve
the disconnected state instead of reconnecting the bone; keep connected bones’
existing position behavior unchanged.
There was a problem hiding this comment.
Acknowledged / deferred for this slice. Slice B intentionally stores connect state via pose only (no new persistent flag). Encoding a durable sole-child disconnect without a flag is ambiguous; tracking as follow-up if needed.
There was a problem hiding this comment.
@fernandotonon, acknowledged. The limitation is understood and the finding remains deferred rather than blocking Slice B. Would you like me to open a follow-up GitHub issue for persistent disconnected-bone state across bind resets and animation evaluation?
✏️ Learnings added
Learnt from: fernandotonon
Repo: fernandotonon/QtMeshEditor PR: 908
File: src/SkeletonEditor.cpp:973-981
Timestamp: 2026-07-19T01:03:13.972Z
Learning: In `src/SkeletonEditor.cpp`, Slice B intentionally represents skeleton bone connection/disconnection through the live pose only and does not introduce a persistent connectivity flag. Persistence across bind resets or animation evaluation is deferred for a possible follow-up.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Validate attach sources before mutating the destination, order copied bones parent-before-child, sanitize entity scale for debug overlays, tighten attach collision assertions, and make Skeleton Tools buttons wrap with reactive Connect labels plus basic keyboard focus. Co-authored-by: Cursor <cursoragent@cursor.com>
|
1 participant
Summary
SkeletonEditorsnapshot rebuild + undo commandsCloses #556
Test plan
.001); source unchanged./build_local/bin/UnitTests --gtest_filter="SkeletonEditor*"Made with Cursor
Summary by CodeRabbit