feat(skeleton): rest pose authoring — Slice C (#557) - #911
Conversation
…#557) Add bind-pose capture with animation re-bake so world motion is preserved, imported-rest reset/cache, per-bone snap, edit-rest mode (gizmo commits rest + rebake), and a translucent rest-pose ghost overlay. Inspector controls and SetRestPoseCommand undo cover the Slice C acceptance path; A/T-pose helpers deferred until Slice E naming. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds rest-pose capture, reset, selected-bone snapping, edit mode, undo/redo, animation rebaking, bone-drag integration, and a translucent rest-pose ghost overlay to the skeleton tools UI. ChangesRest pose authoring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PropertiesPanel
participant SkeletonEditor
participant SetRestPoseCommand
participant SkeletonEntity
PropertiesPanel->>SkeletonEditor: captureRestPoseForSelected()
SkeletonEditor->>SetRestPoseCommand: create and push command
SetRestPoseCommand->>SkeletonEditor: apply rest-pose operation
SkeletonEditor->>SkeletonEntity: update rest TRS and rebake animation tracks
SetRestPoseCommand->>SkeletonEditor: emit restPoseChanged()
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. 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: 1b053f1850
ℹ️ 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".
| if (m_editRestPoseMode == on) return; | ||
| m_editRestPoseMode = on; | ||
| applyEditRestAnimMute(on); |
There was a problem hiding this comment.
When Edit rest pose is left enabled and the user selects a different animated entity, this setter is not invoked again and no selectionChanged path calls applyEditRestAnimMute(true) for the new selection. TransformOperator still treats every later drag as edit-rest mode, so the new entity's active animation states remain enabled and the drag can commit an animated frame into the bind pose instead of the reset bind pose; the previously selected entity also stays muted until the mode is turned off. Re-run the mute/reset when the selected skinned entity changes, or scope edit-rest mode to the entity that was muted.
Useful? React with 👍 / 👎.
| for (const auto& [name, newRest] : newRests) { | ||
| if (!skel->hasBone(name)) { | ||
| if (error) *error = QStringLiteral("Bone not found: %1").arg(QString::fromStdString(name)); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If the imported-rest cache was captured before a bone was renamed or deleted, Reset Rest passes stale bone names into this loop. Because the loop rebakes and updates each existing bone as it goes and only returns false when it eventually hits the missing cached bone, a reset that reports failure can still partially mutate earlier bones and animation tracks depending on the unordered iteration order. Validate all cached names before applying, or filter out missing bones before starting the rebake.
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: 3
🧹 Nitpick comments (3)
src/SkeletonEditor.cpp (1)
1428-1442: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider breadcrumbs for the edit-rest and ghost toggles.
Capture/snap/reset already emit
scene.skel.rest_pose.*breadcrumbs, but entering/exiting edit-rest mode and toggling the ghost overlay are user-facing actions that currently record nothing. Addingui.action(orscene.skel.rest_pose.*) breadcrumbs here keeps the operation trail complete.
As per coding guidelines: "Track all user-facing actions and significant operations withSentryReporter::addBreadcrumb; useui.actionfor toolbar/menu clicks".🤖 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 1428 - 1442, Add SentryReporter::addBreadcrumb calls to SkeletonEditor::setEditRestPoseMode and setShowRestPoseGhost after each state change, using ui.action or the established scene.skel.rest_pose category with descriptive data for entering/exiting edit-rest mode and enabling/disabling the ghost overlay. Preserve the existing early returns and signal/update ordering.Source: Coding guidelines
qml/PropertiesPanel.qml (1)
3145-3179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the file's existing themed checkbox instead of a stock
CheckBox.This file already defines
InspectorCheckBox(lines 25-64) specifically to match the Inspector's dark palette (16px box + checkmark), and the Skeleton section a few lines above hand-rolls themed Rectangle-based toggles for the same reason.editRestCheck/ghostRestCheckinstead use a stock Qt QuickCheckBoxwith only thecontentItemrestyled, leaving the default indicator — this will visually clash with the rest of the panel.♻️ Suggested fix
- CheckBox { - id: editRestCheck - text: "Edit rest pose" - checked: SkeletonEditor.editRestPoseMode - onCheckedChanged: { - if (checked !== SkeletonEditor.editRestPoseMode) - SkeletonEditor.editRestPoseMode = checked - } - contentItem: Text { - text: editRestCheck.text - color: PropertiesPanelController.textColor - font.pixelSize: 11 - leftPadding: editRestCheck.indicator.width + 4 - verticalAlignment: Text.AlignVCenter - } - } + InspectorCheckBox { + id: editRestCheck + text: "Edit rest pose" + checked: SkeletonEditor.editRestPoseMode + onCheckedChanged: { + if (checked !== SkeletonEditor.editRestPoseMode) + SkeletonEditor.editRestPoseMode = checked + } + }(same for
ghostRestCheck)🤖 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 3145 - 3179, Replace the stock CheckBox controls editRestCheck and ghostRestCheck with the file’s existing InspectorCheckBox component, preserving their labels, checked bindings, and onCheckedChanged synchronization with SkeletonEditor.editRestPoseMode and SkeletonEditor.showRestPoseGhost. Remove the custom contentItem styling that is only needed for the stock controls.Source: Coding guidelines
src/SkeletonDebug.cpp (1)
120-122: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGhost transform recompute runs every timer tick, not only when the rest pose changes.
updateGhostVisuals()allocates anstd::unordered_mapand walks the full bone hierarchy recursively on everyonTimerTick()call while the ghost is visible — this timer fires continuously (mTimer.start(0)). The rest pose it visualizes is static except right after a capture/reset/snap/commit, so this work is wasted on nearly every frame. Consider only callingupdateGhostVisuals()when the rest pose actually changes (e.g. afterSetRestPoseCommandapplies, or fromrebuildVisuals()/showRestGhost(true)), instead of unconditionally each tick.♻️ Suggested direction
- if (mShowRestGhost) - updateGhostVisuals(); + // updateGhostVisuals() is now invoked only when the rest pose changes + // (showRestGhost(true), rebuildVisuals(), and rest-pose commit/undo), + // not on every tick.Also applies to: 389-418
🤖 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 120 - 122, Remove the unconditional updateGhostVisuals() call from onTimerTick when mShowRestGhost is true, and invoke it only at rest-pose change points such as SetRestPoseCommand application, rebuildVisuals(), or showRestGhost(true). Preserve ghost visibility updates while avoiding repeated hierarchy traversal on unchanged poses.
🤖 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 3129-3135: Update the hint Text containing “Capture the current
pose as bind...” to use PropertiesPanelController.textColor instead of
borderColor, and apply the file’s established reduced opacity for description
text so it matches the other hints.
In `@src/PropertiesPanelController.cpp`:
- Around line 1208-1226: Update setRestPoseGhostVisible so enabling the
rest-pose ghost does not activate skeleton debugging for every skinned entity.
Restrict the behavior to the currently active skeleton-debug instances, or track
and restore each entity’s prior debug state when the ghost is disabled; preserve
the existing ghost visibility update.
In `@src/SkeletonEditor.cpp`:
- Around line 1444-1460: Update the unmute branch of
SkeletonEditor::applyEditRestAnimMute so restoration depends only on
m_editRestMutedEntity and the resolved entity, not on the unused
AnimationControlController::instance() result. Keep re-enabling each recorded
animation state before clearing m_editRestMutedEntity and m_editRestMutedAnims.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 3145-3179: Replace the stock CheckBox controls editRestCheck and
ghostRestCheck with the file’s existing InspectorCheckBox component, preserving
their labels, checked bindings, and onCheckedChanged synchronization with
SkeletonEditor.editRestPoseMode and SkeletonEditor.showRestPoseGhost. Remove the
custom contentItem styling that is only needed for the stock controls.
In `@src/SkeletonDebug.cpp`:
- Around line 120-122: Remove the unconditional updateGhostVisuals() call from
onTimerTick when mShowRestGhost is true, and invoke it only at rest-pose change
points such as SetRestPoseCommand application, rebuildVisuals(), or
showRestGhost(true). Preserve ghost visibility updates while avoiding repeated
hierarchy traversal on unchanged poses.
In `@src/SkeletonEditor.cpp`:
- Around line 1428-1442: Add SentryReporter::addBreadcrumb calls to
SkeletonEditor::setEditRestPoseMode and setShowRestPoseGhost after each state
change, using ui.action or the established scene.skel.rest_pose category with
descriptive data for entering/exiting edit-rest mode and enabling/disabling the
ghost overlay. Preserve the existing early returns and signal/update ordering.
🪄 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: 41024155-d996-4dd2-b521-7c9e83fb170c
📒 Files selected for processing (16)
qml/PropertiesPanel.qmlsrc/AnimationWidget.cppsrc/AnimationWidget.hsrc/BoneDragRelease.cppsrc/BoneDragRelease.hsrc/BoneDragRelease_test.cppsrc/PropertiesPanelController.cppsrc/PropertiesPanelController.hsrc/SkeletonDebug.cppsrc/SkeletonDebug.hsrc/SkeletonEditor.cppsrc/SkeletonEditor.hsrc/SkeletonEditor_test.cppsrc/TransformOperator.cppsrc/commands/SkeletonBoneCommands.cppsrc/commands/SkeletonBoneCommands.h
… reviews Rest ghost no longer force-enables the Skeleton overlay; ghost-only hosts stay bone-hidden and tear down when unchecked. Inspector-themed checkboxes, edit-rest remute on selection change, safer rest-cache reset, and unmute without depending on AnimationControlController. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed review + UX feedback:
|
Rest-pose gizmo/snap commits now bake skinned verts into bind buffers so the mesh stays posed after release. The rest ghost clones mesh at enable time so it keeps the old rest while live geometry updates. Avoid mesh->_dirtyState() so skeleton TagPoints survive commit; keep skeleton debug visible when the overlay checkbox is checked. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Follow-up after manual QA (rest gizmo + ghost): Rest commit / mesh bake
Rest ghost
Tests
Merged latest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/SkeletonEditor.cpp`:
- Around line 322-326: Update the bounding-sphere radius calculation in the mesh
bounds block to use Ogre::Math::boundingRadiusFromAABB(box) instead of the
manual std::max expression, while preserving the existing mesh->_setBounds and
conditional any flow.
🪄 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: 0346b810-1f6e-4ef3-a9bc-d5f920c6563c
📒 Files selected for processing (13)
src/AnimationWidget.cppsrc/BoneDragRelease.cppsrc/BoneDragRelease.hsrc/BoneDragRelease_test.cppsrc/Manager.cppsrc/SkeletonDebug.cppsrc/SkeletonDebug.hsrc/SkeletonEditor.cppsrc/SkeletonEditor.hsrc/SkeletonEditor_test.cppsrc/TransformOperator.cppsrc/commands/SkeletonBoneCommands.cppsrc/commands/SkeletonBoneCommands.h
🚧 Files skipped from review as they are similar to previous changes (5)
- src/commands/SkeletonBoneCommands.h
- src/AnimationWidget.cpp
- src/SkeletonEditor.h
- src/BoneDragRelease.h
- src/SkeletonDebug.h
| if (any) { | ||
| mesh->_setBounds(box, false); | ||
| mesh->_setBoundingSphereRadius( | ||
| std::max(box.getMaximum().length(), box.getMinimum().length())); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Ogre::Math::boundingRadiusFromAABB signature
💡 Result:
The signature for the Ogre::Math::boundingRadiusFromAABB function in the OGRE engine is as follows [1][2][3]: static Real boundingRadiusFromAABB(const AxisAlignedBox& aabb); This static method returns a Real value and accepts a constant reference to an AxisAlignedBox object as its only parameter [1][4][3]. Its purpose is to calculate the radius of a bounding sphere that is centered at the origin, based on the provided axis-aligned bounding box [1][3]. A related function, boundingRadiusFromAABBCentered, exists to calculate the radius of a sphere centered on the bounding box itself [3][5].
Citations:
- 1: https://ogrecave.github.io/ogre/api/latest/class_ogre_1_1_math.html
- 2: https://ogrecave.github.io/ogre-next/api/latest/class_ogre_1_1_math.html
- 3: https://github.com/OGRECave/ogre/blob/master/OgreMain/include/OgreMath.h
- 4: https://ogrecave.github.io/ogre/api/14/class_ogre_1_1_math.html
- 5: https://ogrecave.github.io/ogre/api/13/class_ogre_1_1_math.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/SkeletonEditor.cpp excerpt ---'
sed -n '300,335p' src/SkeletonEditor.cpp
printf '\n%s\n' '--- geometric probe ---'
python3 - <<'PY'
import itertools, math
box_min = (-3, -3, 0)
box_max = (0, 0, 3)
current = max(
math.sqrt(sum(c*c for c in box_max)),
math.sqrt(sum(c*c for c in box_min)),
)
true_max = max(
math.sqrt(sum(c*c for c in corner))
for corner in itertools.product(
[box_min[0], box_max[0]],
[box_min[1], box_max[1]],
[box_min[2], box_max[2]],
)
)
print("current:", current)
print("true_max:", true_max)
PYRepository: fernandotonon/QtMeshEditor
Length of output: 1818
Use Ogre::Math::boundingRadiusFromAABB(box) here std::max(box.getMaximum().length(), box.getMinimum().length()) can under-estimate the true radius when the AABB straddles the origin asymmetrically, which can cause culling glitches. src/SkeletonEditor.cpp:322-326
🤖 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 322 - 326, Update the bounding-sphere
radius calculation in the mesh bounds block to use
Ogre::Math::boundingRadiusFromAABB(box) instead of the manual std::max
expression, while preserving the existing mesh->_setBounds and conditional any
flow.
|
1 participant
Summary
SkeletonEditor+ undoableSetRestPoseCommand)SkeletonDebugscene.skel.rest_pose.*Deferred: A-pose / T-pose helpers (need Slice E L/R naming).
Closes #557
Test plan
SkeletonEditorTest.*+BoneDragReleaseTest.*(incl. CaptureRestPoseRebasesAnimation, Reset, Snap, UndoRedo, EditRestMode)Made with Cursor
Summary by CodeRabbit
New Features
Bug Fixes
Tests