feat(#517): Slice C — node-transform animation (GUI + MCP/CLI + export round-trip)

[fernandotonon]

Summary

Implements Slice C of epic #517 — non-skeletal node-transform (TRS) animation — plus the surrounding fixes that make skeletal + morph + node animation coexist and round-trip through every export format.

Node clips are Ogre::Animation + AnimationState owned by the SceneManager (not a mesh), driven by NodeAnimationTracks — for animated props, doors, spinning machinery, camera moves, animated lights, etc.

What's included

Authoring & playback (GUI)

• "Node Transform Animation" Inspector section: create/delete clips, key the selected node's TRS, edit-session model (Edit → key → Done).
• Node clips listed in the Animations list under their entity and driven by the main Play button.
• Combined dope sheet: skeletal + morph + node bands shown together; node band has interactive diamonds (drag to re-time, right-click delete, double-click to key).
• Rotation keying fixed (the rotate gizmo drives the SceneNode during a node-anim edit, not the bone).

Full MCP + CLI parity

• Every animation control exposed via MCP (node clip CRUD + keyframes, global playback, morph weight keyframing, skeletal keyframe editing/navigation).
• qtmesh nodeanim <file> --list.

Export / import round-trip (all formats)

• glTF/glb: node clips carried natively as aiNodeAnim channels.
• FBX + .mesh: node clips persisted to a <basename>.nodeanim.json sidecar (mirrors the .lights.json pattern) and rebuilt on import — those exporters have no native concept of SceneManager animation.
• Skeletal + morph coexistence: fixed the duplicate-MorphAnim bug that made a combined glb fail to re-import entirely (0 entities).

Reimport-hygiene fixes (from GUI testing)

• Phantom skeletal clip removed (a node-only aiAnimation no longer leaks a 0-track clip onto the skeleton).
• Node clip reconstructed even when the scene node is renamed after the file on reimport.
• Pose-shape (Shape_N, length 0) clips filtered from the animation list / no longer auto-selected.
• Reconstructed node clip surfaces in the list on load (deferred auto-select out of the render frame) and in the dope sheet (band resolves to any node clip animating the selected entity).

Crash fixes

• Deleting a scene node with an active node clip (dangling AnimationState/track cleanup).
• Adding a morph target (dangling skeleton pointer after _initialise(true)).

Testing

• scripts/anim-mcp-smoke.sh — 53 checks (MCP surface, no crash).
• scripts/anim-combined-roundtrip.sh + tests/fixtures/combined_skel_morph.glb — skeletal + morph + node round-trip incl. reimport hygiene (10 checks).
• scripts/anim-roundtrip.sh, scripts/anim-filemenu-roundtrip.sh — per-format + File-menu round-trips.
• Verified end-to-end in the GUI on a rigged Mixamo mesh with a node clip and morph shapes: export → reimport shows node clip in list + dope sheet with correct keyframes.

Known follow-ups (out of scope)

• Morph weight-over-time animation in FBX (shapes export; weight track needs the same sidecar treatment).
• Scene-level (multi-object) node animation.

Resolves #517 (Slice C).

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Added node-transform animation authoring with clip creation, editing, playback, scrubbing, and transform keyframing.
  • Added dope-sheet node tracks with keyframe creation, retiming, deletion, and undo/redo support.
  • Expanded animation controls for skeletal, morph, and node animations, including automation support.
  • Preserved combined animation data across GLB, FBX, and Ogre Mesh workflows.

• Bug Fixes

  • Prevented empty animation clips and improved cleanup when animated nodes are removed.
  • Improved layout and coexistence of skeletal, morph, and node animation tracks.

• Documentation

  • Documented node animation workflows, export behavior, integrations, and testing coverage.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 33d6a14f-d3e7-4d60-80b4-5970ab163bf9

📥 Commits

Reviewing files that changed from the base of the PR and between 78ebae0 and ac6b018.

📒 Files selected for processing (1)

• CLAUDE.md

🚧 Files skipped from review as they are similar to previous changes (1)

• CLAUDE.md

---

📝 Walkthrough

Walkthrough

This PR adds non-skinned node-transform animation authoring, playback, undo support, MCP controls, dope-sheet editing, multi-format import/export, animation-type coexistence, and regression harnesses.

Changes

Node animation system

Layer / File(s)	Summary
Node animation state and undo src/NodeAnimationManager.*, src/commands/NodeAnimCommands.*, src/NodeAnimationManager_test.cpp	Adds active and editing clips, inspection APIs, scene-node cleanup, undoable clip operations, and keyframe authoring, movement, and deletion.
Animation UI and playback integration qml/AnimationDopeSheet.qml, qml/NodeAnimationPanel.qml, qml/PropertiesPanel.qml, src/AnimationControlController.*, src/PropertiesPanelController.cpp, src/TransformOperator.cpp, src/mainwindow.cpp	Adds node animation controls, dope-sheet rows, transform editing, selection handling, playback advancement, and Properties Panel wiring.
MCP animation operations src/MCPServer.*	Adds MCP tools for node clips, playback state, animation and bone selection, morph-weight keyframes, and skeletal channel editing.
Animation import and export persistence src/MeshImporterExporter.cpp, src/Assimp/AnimationProcessor.cpp, src/*AnimationProcessor_test.cpp, src/MeshImporterExporter_test.cpp	Adds node TRS channels and .nodeanim.json sidecars, reconstructs node clips on import, injects morph animations into glTF/GLB, and removes empty skeletal clips.
Regression harness and documentation scripts/anim-*.sh, CLAUDE.md, .github/workflows/deploy.yml	Adds MCP smoke coverage, multi-format round-trip checks, documentation, and expanded headless crash handling.

Estimated code review effort: 5 (Critical) | ~90 minutes

Possibly related issues

• fernandotonon/QtMeshEditor#520 — Directly covers the node-transform animation scope implemented by this PR.

Possibly related PRs

• fernandotonon/QtMeshEditor#585 — Extends the same node-animation undo infrastructure with keyframe move and delete commands.
• fernandotonon/QtMeshEditor#587 — Shares MCP node-animation input handling and validation.
• fernandotonon/QtMeshEditor#576 — Shares dope-sheet and animation-controller integration for morph and node animation rows.

Sequence Diagram(s)

sequenceDiagram
  participant Editor
  participant NodeAnimationPanel
  participant NodeAnimationManager
  participant AnimationControlController
  participant MeshImporterExporter
  Editor->>NodeAnimationPanel: create and edit node clip
  NodeAnimationPanel->>NodeAnimationManager: key current transform
  AnimationControlController->>NodeAnimationManager: select, scrub, and advance clip
  MeshImporterExporter->>NodeAnimationManager: export or reconstruct node tracks
  NodeAnimationManager-->>Editor: emit clip and keyframe changes

Loading

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR covers node animation authoring, playback, dope-sheet support, MCP/CLI parity, undo, and round trips, but lacks Curve Editor and Sentry breadcrumb evidence.	Add node-TRS support to the Curve Editor and emit the required scene.anim.* Sentry breadcrumbs, or document these as approved Slice C deferrals.
Docstring Coverage	⚠️ Warning	Docstring coverage is 32.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies Slice C and its main node-transform animation, GUI, MCP/CLI, and export round-trip changes.
Description check	✅ Passed	The description provides a detailed summary, technical scope, testing results, known follow-ups, and linked issue context.
Out of Scope Changes check	✅ Passed	The changes remain related to Slice C, including animation coexistence fixes, MCP parity, import/export support, testing, documentation, and headless CI compatibility.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/anim-slice-c-node-transform-517

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 37d5ec7e6e

ℹ️ 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".

[chatgpt-codex-connector]

Keep node clips disabled during edit sessions

When an existing node clip is selected and then reopened for editing, moving the timeline calls setAnimationFrame, which unconditionally re-enables the clip after beginEdit disabled it. The next scene-animation application therefore drives the node again and prevents gizmo edits; merely selecting a disabled node clip also silently overrides its Enable checkbox. Route this through NodeAnimationManager::setClipEnabled or avoid enabling the state while isEditing is true.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Route inspector rotations through the node-edit path

This edit-session override only covers the quaternion overload. Changing the rotation fields in the Properties panel calls setSelectedOrientation, which dispatches entity selections to rotateSelected(const Vector3&); that overload still calls MeshTransform::rotateMesh, destructively baking the rotation into the mesh while leaving the SceneNode orientation unchanged, so the resulting node key records no rotation. Apply the same edit-session routing to that path.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Start the global transport when playing a node clip

set_node_animation_playing reports success after only enabling the SceneManager state, but MainWindow::frameRenderingQueued advances those states exclusively while the global isPlaying flag is true. For a node-only scene controlled through MCP, enabling the clip therefore leaves it frozen indefinitely because this handler never calls MainWindow::setPlaying(true), unlike the existing skeletal play_animation handler.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Interpolate staggered TRS channels during import

For imported node animations whose translation, rotation, and scale channels use different key times, sampling the nearest value produces incorrect motion. For example, translation keys at 0 and 2 seconds plus a rotation key at 1 second should yield the interpolated translation at 1 second, but this code selects one endpoint and writes it as a full TRS key, changing the animation after import and re-export. Sample vectors linearly and rotations with quaternion interpolation at each union time instead.

Useful? React with 👍 / 👎.

[coderabbitai]

Actionable comments posted: 13

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (11)

src/MCPServer.cpp-7615-7634 (1)

7615-7634: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Validate start and end for finiteness and ordering.

toolSetLoopRegion accepts any JSON number for start and end. The other new handlers reject non-finite values (for example Line 7517 and Line 7601). A non-finite or inverted loop region produces a loop that never triggers, and the response still reports ok: true.

♻️ Proposed validation

     auto* c = AnimationControlController::instance();
+    double newStart = c->loopStart();
+    double newEnd = c->loopEnd();
     if (args.contains("start")) {
         if (!args.value("start").isDouble())
             return makeErrorResult("Error: 'start' must be a number");
-        c->setLoopStart(args.value("start").toDouble());
+        newStart = args.value("start").toDouble();
+        if (!std::isfinite(newStart) || newStart < 0.0)
+            return makeErrorResult("Error: 'start' must be a non-negative finite number");
     }
     if (args.contains("end")) {
         if (!args.value("end").isDouble())
             return makeErrorResult("Error: 'end' must be a number");
-        c->setLoopEnd(args.value("end").toDouble());
+        newEnd = args.value("end").toDouble();
+        if (!std::isfinite(newEnd) || newEnd < 0.0)
+            return makeErrorResult("Error: 'end' must be a non-negative finite number");
     }
+    if (newEnd < newStart)
+        return makeErrorResult("Error: 'end' must be >= 'start'");
+    c->setLoopStart(newStart);
+    c->setLoopEnd(newEnd);

🤖 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 7615 - 7634, Update toolSetLoopRegion’s
start/end validation to reject non-finite numeric values and reject regions
where start is greater than end before calling setLoopStart or setLoopEnd.
Return the existing error result with a clear validation message and only report
ok=true for valid, ordered loop regions.

CLAUDE.md-269-269 (1)

269-269: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fix the harness path.

The text points at scratchpad/anim_mcp_test.sh. This PR adds the harness as scripts/anim-mcp-smoke.sh, together with scripts/anim-roundtrip.sh, scripts/anim-combined-roundtrip.sh and scripts/anim-filemenu-roundtrip.sh. Update the reference so the documentation points at a file that exists in the repository.

🤖 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 `@CLAUDE.md` at line 269, Update the harness reference in the animation MCP
documentation to point to the existing scripts/anim-mcp-smoke.sh file instead of
scratchpad/anim_mcp_test.sh, leaving the surrounding description unchanged.

scripts/anim-combined-roundtrip.sh-108-114 (1)

108-114: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

The four harness scripts print the summary before the crash check updates FAIL. Each epilogue echoes SUMMARY: PASS=... FAIL=..., then runs kill -0 $APP_PID and increments FAIL on a crash. A crashed app therefore prints FAIL=0 and exits 1. Move the crash check above the summary in each script.

• scripts/anim-combined-roundtrip.sh#L108-L114: run the kill -0 $APP_PID check and the FAIL=$((FAIL+1)) increment before the echo "===== SUMMARY..." line.
• scripts/anim-mcp-smoke.sh#L160-L169: move the kill -0 $APP_PID block at Line 164 above the summary echo at Line 161.
• scripts/anim-roundtrip.sh#L130-L137: move the kill -0 $APP_PID block at Line 133 above the summary echo at Line 131.
• scripts/anim-filemenu-roundtrip.sh#L76-L82: move the kill -0 $APP_PID block at Line 79 above the summary echo at Line 77.

🤖 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 `@scripts/anim-combined-roundtrip.sh` around lines 108 - 114, In
scripts/anim-combined-roundtrip.sh (lines 108-114), scripts/anim-mcp-smoke.sh
(lines 160-169), scripts/anim-roundtrip.sh (lines 130-137), and
scripts/anim-filemenu-roundtrip.sh (lines 76-82), move each kill -0 $APP_PID
crash check and its FAIL increment before the summary echo so the reported FAIL
count includes crashes; preserve the existing cleanup and exit behavior.

scripts/anim-filemenu-roundtrip.sh-14-15 (1)

14-15: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Fail fast when the fixture assets are missing.

BODY and FACE default to $ROOT/.mocap_work/out_body.glb and $ROOT/.mocap_work/out_face.glb. That directory is not part of the repository. If the files are absent, every load_mesh call still runs and the script reports a list of unrelated FAIL lines.

scripts/anim-combined-roundtrip.sh Line 25 already guards its fixture. Add the same check here.

🛡️ Proposed guard

 PASS=0; FAIL=0; fails=()
+
+for asset in "$BODY" "$FACE"; do
+  [ -f "$asset" ] || { echo "FATAL: fixture missing: $asset"; exit 2; }
+done

🤖 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 `@scripts/anim-filemenu-roundtrip.sh` around lines 14 - 15, In
scripts/anim-filemenu-roundtrip.sh, add an early guard immediately after the
BODY and FACE assignments that verifies both fixture files exist, matching the
existing fixture check in anim-combined-roundtrip.sh. If either asset is
missing, print a clear message and exit before any load_mesh calls run.

src/MeshImporterExporter_test.cpp-554-596 (1)

554-596: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Clean up the clip even when an assertion fails, and guard the manager pointer.

nam is dereferenced without a null check. Other code in this PR treats NodeAnimationManager::instance() as nullable (MeshImporterExporter.cpp Lines 2345-2346). A null pointer here crashes the whole suite instead of failing one test.

nam->deleteClip("SlideClip") at Line 595 only runs when every assertion passes. Any ASSERT_* failure returns early and leaves the clip registered on the SceneManager. Later tests in the same binary then export an extra animation.

Add the null guard and remove the clip from TearDown, or use a scope guard.

💚 Proposed fix

     auto* nam = NodeAnimationManager::instance();
+    ASSERT_NE(nam, nullptr);
+    // Remove the clip on every exit path so a failed assertion does not
+    // leak it into the next test in this binary.
+    struct ClipGuard {
+        NodeAnimationManager* m;
+        ~ClipGuard() { if (m) m->deleteClip(QStringLiteral("SlideClip")); }
+    } clipGuard{nam};
     ASSERT_TRUE(nam->createClip(QStringLiteral("SlideClip"), 2.0));
@@
-    nam->deleteClip(QStringLiteral("SlideClip"));
 }

🤖 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/MeshImporterExporter_test.cpp` around lines 554 - 596, Guard the
NodeAnimationManager pointer returned by NodeAnimationManager::instance() before
dereferencing it, failing the test cleanly when it is null. Ensure the
"SlideClip" created in this test is removed even when an ASSERT_* aborts
execution by moving cleanup into TearDown or adding a scope guard, and remove
the existing success-only deleteClip call.

src/NodeAnimationManager.h-132-137 (1)

132-137: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Correct two doc comments that contradict the implementation.

1. Lines 132-137 describe scrubClip as sampling the clip, pushing the pose to the scene, and enabling the driving AnimationState. The implementation is a deliberate no-op and explicitly rejects enabling the state, because an enabled state locks the node against the gizmo. A caller that trusts this header will expect a viewport update that never happens.
2. Lines 153-157 describe keyNodeCurrentTransform as capturing the "CURRENT world-relative transform". The implementation captures node->getPosition(), node->getOrientation(), and node->getScale(), which are parent-relative local values. The .cpp comment already says "CURRENT local transform".

📝 Proposed doc fixes

-    /// Sample `clipName` at `time` and push it onto the scene so the
-    /// viewport reflects the scrubbed pose immediately (used by the
-    /// timeline slider while playback is paused). No-op on a missing
-    /// clip. Enables the driving AnimationState if needed but leaves
-    /// playback paused — the caller owns play/pause.
+    /// Intentionally a no-op on the node: scrubbing only moves the
+    /// timeline playhead so "Key" lands at the right time. Enabling the
+    /// driving AnimationState would re-drive the node every frame and
+    /// lock it against the gizmo. Kept as a stable API point for a
+    /// future reset-then-apply preview. See the .cpp for the rationale.
     Q_INVOKABLE void scrubClip(const QString& clipName, double time);

-    /// Capture `nodeName`'s CURRENT world-relative transform (the live
-    /// SceneNode's position/orientation/scale) as a keyframe on
+    /// Capture `nodeName`'s CURRENT local (parent-relative) transform
+    /// (the live SceneNode's position/orientation/scale) as a keyframe on
     /// `clipName` at `time`, via SetNodeKeyframeCommand (undoable).

Also applies to: 153-157

🤖 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/NodeAnimationManager.h` around lines 132 - 137, Update the documentation
for scrubClip to state that it is currently a deliberate no-op and does not
sample, update the viewport, or enable AnimationState; preserve the note that
playback ownership remains with the caller only if accurate. Update
keyNodeCurrentTransform documentation to describe the captured position,
orientation, and scale as the node’s current parent-relative local transform
rather than a world-relative transform.

src/mainwindow.cpp-1195-1200 (1)

1195-1200: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The raise() on the Dope Sheet is overridden later in initToolBar().

Line 1200 raises m_dopeSheetDock. Lines 1239-1274 then run tabifyBottomToolDocks() and raise m_bottomContextDock or m_consoleDock based on QSettings. Line 1269-1274 adds a deferred singleShot raise for the Context Panel.

Both preferences default to true at Lines 1245-1248, so on a fresh profile the Context Panel becomes the front tab and the Dope Sheet does not. The stated intent at Lines 1195-1197 does not hold.

Move the Dope Sheet raise after tabifyBottomToolDocks(), or update the comment to describe the actual tab order.

🤖 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/mainwindow.cpp` around lines 1195 - 1200, Move the
m_dopeSheetDock->raise() call from the initial dock setup to after
tabifyBottomToolDocks() and its preference-based raises in initToolBar(), so the
Dope Sheet remains the front tab as described by the surrounding comment.
Preserve the existing lazy-QML initialization and other dock preference
behavior.

src/AnimationControlController.cpp-693-707 (1)

693-707: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

The node-clip branch sets m_selectedTick but does not emit keyframeTicksChanged.

selectedTick is declared with NOTIFY keyframeTicksChanged in src/AnimationControlController.h Line 56. The skeletal path at Lines 715-724 emits both keyframeTicksChanged and currentKeyframeChanged when it clears the keyframe. The node path emits only currentKeyframeChanged, so QML keeps the previous selectedTick value and the timeline keeps highlighting a stale tick.

🛠️ Proposed fix

         if (m_currentKeyframe) {
             m_currentKeyframe = nullptr;
             m_selectedTick = -1;
+            emit keyframeTicksChanged();
             emit currentKeyframeChanged();
         }

🤖 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 693 - 707, Update the
node-clip branch in the animation-selection flow to emit keyframeTicksChanged
immediately after resetting m_selectedTick, alongside the existing
currentKeyframeChanged notification. Keep the reset and notification behavior
consistent with the skeletal path.

src/PropertiesPanelController.cpp-1135-1145 (1)

1135-1145: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a Sentry breadcrumb for the node-clip enable toggle.

This branch handles a user action from the Inspector checkbox. NodeAnimationManager::setClipEnabled records no breadcrumb (see src/NodeAnimationManager.cpp Lines 292-310), unlike deleteClip, which records scene.anim.node. The toggle therefore leaves no trace in crash reports.

🛠️ Proposed fix

         nam->setClipEnabled(animName, enabled);
+        SentryReporter::addBreadcrumb(QStringLiteral("ui.action"),
+            QStringLiteral("node clip '%1' enabled=%2").arg(animName).arg(enabled));
         emit animationStateChanged();
         return;

As per coding guidelines: "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb; use ui.action for UI actions".

🤖 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 1135 - 1145, Add a Sentry
breadcrumb for the node-clip enable/disable user action in the shown branch,
using the `ui.action` category and including the relevant clip/entity context
before or alongside `NodeAnimationManager::setClipEnabled`. Keep the existing
state update, signal emission, and early return unchanged.

Source: Coding guidelines

qml/NodeAnimationPanel.qml-74-84 (1)

74-84: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

commitNewClip() closes the editor row even when creation fails.

NodeAnimationManager::createClip returns false when the name is empty, the length is not positive, or a clip with that name already exists (see src/NodeAnimationManager.cpp Lines 184-190). Line 83 sets creatingClip = false unconditionally, so a duplicate name discards the typed input with no message. The user sees the row close and no new clip.

Keep the row open and report the failure.

🛠️ Proposed fix

     function commitNewClip() {
         var nm = newNameInput.text.trim()
         var len = parseFloat(newLenInput.text)
-        if (nm.length > 0 && !isNaN(len) && len > 0) {
-            if (NodeAnimationManager.createClipUndoable(nm, len)) {
-                NodeAnimationManager.beginEdit(nm)
-                AnimationControlController.animationLength = len
-            }
-        }
-        nodeAnimSection.creatingClip = false
+        if (nm.length === 0 || isNaN(len) || len <= 0) {
+            nodeAnimSection.createError = "Enter a name and a length greater than 0."
+            return
+        }
+        if (!NodeAnimationManager.createClipUndoable(nm, len)) {
+            nodeAnimSection.createError = "A clip named '" + nm + "' already exists."
+            return
+        }
+        NodeAnimationManager.beginEdit(nm)
+        AnimationControlController.animationLength = len
+        nodeAnimSection.createError = ""
+        nodeAnimSection.creatingClip = false
     }

Add property string createError: "" next to creatingClip and show it in the editor row.

🤖 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/NodeAnimationPanel.qml` around lines 74 - 84, Update commitNewClip() so
creatingClip is set to false only after createClipUndoable succeeds; preserve
the editor row and typed input when creation fails. Add the requested
createError string property near creatingClip, set it when validation or clip
creation fails, clear it on a successful creation or new edit session, and
display it in the editor row.

qml/AnimationDopeSheet.qml-1367-1373 (1)

1367-1373: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Clamp the drag target to the clip length.

This handler clamps only the lower bound. The bone-diamond handler at Lines 776-780 also clamps against AnimationControlController.animationLength. A node keyframe can therefore be dragged past the end of the clip, where Ogre never evaluates it.

🛠️ Proposed fix

                                             var target = originalTime + dt
                                             if (target < 0) target = 0
+                                            var len = NodeAnimationManager.clipLength(root.nodeClip)
+                                            if (len > 0 && target > len) target = len
                                             nodeDiamond.dragPreviewTime = target

🤖 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/AnimationDopeSheet.qml` around lines 1367 - 1373, Update the
onPositionChanged handler to clamp target to
AnimationControlController.animationLength as well as zero, matching the
bone-diamond drag behavior. Ensure nodeDiamond.dragPreviewTime never exceeds the
clip length.

🧹 Nitpick comments (10)

src/MCPServer.cpp (2)

7684-7687: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Report failure when the bone selection does not take effect.

toolSelectBone always returns ok: true. If selectBone rejects an unknown bone name, the response reports success and returns the previously selected bone. toolSelectAnimation at Line 7667 already verifies the resulting state. Apply the same check here.

♻️ Proposed change

-    AnimationControlController::instance()->selectBone(bone);
+    auto* c = AnimationControlController::instance();
+    c->selectBone(bone);
+    if (c->selectedBone() != bone)
+        return makeErrorResult(
+            QString("Error: could not select bone '%1' (not found?)").arg(bone));
     QJsonObject content;
     content["ok"] = true;
-    content["bone"] = AnimationControlController::instance()->selectedBone();
+    content["bone"] = c->selectedBone();

🤖 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 7684 - 7687, Update toolSelectBone to verify
that AnimationControlController::instance()->selectedBone() matches the
requested bone after selectBone(bone) returns; set content["ok"] based on that
comparison and only report the selection as successful when it took effect,
mirroring the existing validation in toolSelectAnimation.

---

10794-10799: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add JSON Schema enum for the closed-set string arguments.

channel accepts exactly ten values and direction accepts two. The descriptions state the values, but the schemas do not constrain them. Other tools in this file already use enum for closed sets (for example up_axis at Line 9345 and match at Line 10242). An enum lets the MCP client reject a bad value before the call.

♻️ Proposed change

-        props["channel"] = QJsonObject{{"type", "string"}, {"description", "One of tx,ty,tz (translation), rw,rx,ry,rz (rotation quat), sx,sy,sz (scale)."}};
+        props["channel"] = QJsonObject{{"type", "string"},
+            {"enum", QJsonArray{"tx","ty","tz","rw","rx","ry","rz","sx","sy","sz"}},
+            {"description", "One of tx,ty,tz (translation), rw,rx,ry,rz (rotation quat), sx,sy,sz (scale)."}};

-        props["direction"] = QJsonObject{{"type", "string"}, {"description", "'next' or 'prev' — moves the playhead to the adjacent keyframe."}};
+        props["direction"] = QJsonObject{{"type", "string"},
+            {"enum", QJsonArray{"next", "prev"}},
+            {"description", "'next' or 'prev' — moves the playhead to the adjacent keyframe."}};

Also applies to: 10820-10823

🤖 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 10794 - 10799, Update the JSON schemas for
the closed-set string properties in the relevant tool definitions: add a channel
enum containing exactly tx, ty, tz, rw, rx, ry, rz, sx, sy, and sz, and add a
direction enum containing its two supported values. Keep the existing
descriptions and required fields unchanged, following the existing enum pattern
used by up_axis and match.

scripts/anim-filemenu-roundtrip.sh (1)

42-48: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Derive the node name instead of hardcoding out_body.

Line 43 passes the literal out_body to author_node. Manager::addSceneNode uniquifies a name that already exists, so a second import in the same session produces out_body_1. The keyframe calls then target a node that does not exist and the C1/C2 checks fail for a reason unrelated to the feature under test.

scripts/anim-roundtrip.sh Lines 62-64 already derive the node from get_scene_info. Use the same approach 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 `@scripts/anim-filemenu-roundtrip.sh` around lines 42 - 48, Replace the
hardcoded "out_body" argument in the C1 setup with the node name derived from
get_scene_info, matching the existing approach in anim-roundtrip.sh. Pass that
derived name to author_node so repeated imports target the uniquely created node
and the C1/C2 animation checks remain valid.

scripts/anim-combined-roundtrip.sh (1)

49-51: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Use mktemp for the export path.

/tmp/combined_out.glb is a fixed path. A local user can pre-create it, or two concurrent runs can clobber each other. Static analysis flags this as CWE-377. Allocate the path with mktemp and remove it in the cleanup block. The other three harness scripts use the same pattern; see the consolidated comment.

🤖 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 `@scripts/anim-combined-roundtrip.sh` around lines 49 - 51, Replace the fixed
/tmp/combined_out.glb path in the export flow with a unique path allocated by
mktemp, and use that variable in the export JSON and existence check. Ensure the
allocated temporary file is removed in the script’s cleanup block, following the
pattern used by the other harness scripts.

Source: Linters/SAST tools

src/MeshImporterExporter.cpp (1)

2451-2462: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Restrict the extra Assimp read to formats that can carry node channels.

reconstructNodeClipsFromFile performs a second full ReadFile of the source file on the main thread. The import path already read the same file once. The call runs for every suffix, including .mesh, .tmd, .rsd and Psy-Q .ply, where Assimp cannot produce node channels at all.

Skip the second read for formats handled by the sidecar path, or reuse the aiScene the importer already loaded.

Also applies to: 3235-3241

🤖 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/MeshImporterExporter.cpp` around lines 2451 - 2462, Restrict
reconstructNodeClipsFromFile and its corresponding call site around the later
import path to formats that Assimp can provide node channels for; skip the extra
ReadFile for sidecar-handled formats such as .mesh, .tmd, .rsd, and Psy-Q .ply.
Prefer reusing an already-loaded aiScene when available, otherwise guard the
existing reconstructNodeClipsFromFile call with the same supported-format check
while preserving reconstruction for eligible formats.

scripts/anim-roundtrip.sh (1)

111-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

RNODE is dead, and the discarded MCP call inside the loop is misleading.

Line 111 computes RNODE and nothing reads it. Shellcheck flags it (SC2034). Its regex alternative [A-Za-z0-9_]+ also matches almost any token, so the value would be unreliable if it were used.

Line 115 starts the command substitution with call list_node_animations '{}' >/dev/null;. That output is discarded and only the following Python produces the candidate list. Remove both.

FACE at Line 9 is also unused (SC2034); the script reads BODY and QTMESH_RT_MESH only.

🤖 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 `@scripts/anim-roundtrip.sh` around lines 111 - 118, Remove the unused RNODE
assignment and its unreliable broad-token extraction. In the MAXTX candidate
loop, remove the discarded call list_node_animations invocation so the command
substitution only generates candidates from the re-exported GLB via Python. Also
remove the unused FACE assignment near the script’s setup while preserving BODY
and QTMESH_RT_MESH usage.

Source: Linters/SAST tools

src/NodeAnimationManager_test.cpp (1)

570-595: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the captured TRS values, not only the keyframe time.

KeyNodeCurrentTransformCapturesLiveTransform sets the node position to (4,5,6) and the scale to (2,2,2), then asserts only that one key exists at t=0.5. A regression that keys an identity transform would still pass. The same gap exists in MoveNodeKeyframeUndoable (lines 597-626): retimeKeyframe removes and recreates the keyframe to preserve TRS, and no test asserts that the translation survives the move.

Read the key back off the Ogre track and compare the TRS.

💚 Sketch of the added assertions

// Helper near makeNamedNode:
Ogre::TransformKeyFrame* keyAt(const std::string& clip,
                               const std::string& node, double t)
{
    auto* scene = Manager::getSingleton()->getSceneMgr();
    if (!scene->hasAnimation(clip)) return nullptr;
    auto* anim = scene->getAnimation(clip);
    const auto& tracks = anim->_getNodeTrackList();
    for (auto it = tracks.begin(); it != tracks.end(); ++it) {
        auto* tr = it->second;
        if (!tr || !tr->getAssociatedNode()) continue;
        if (tr->getAssociatedNode()->getName() != node) continue;
        for (unsigned short i = 0; i < tr->getNumKeyFrames(); ++i) {
            auto* kf = static_cast<Ogre::TransformKeyFrame*>(tr->getKeyFrame(i));
            if (kf && std::abs(kf->getTime() - t) < 1e-3) return kf;
        }
    }
    return nullptr;
}

    // KeyNodeCurrentTransformCapturesLiveTransform, after the time check:
    auto* kf = keyAt("NA_KeyCurrent", "NA_KeyCurrent_Node", 0.5);
    ASSERT_NE(kf, nullptr);
    EXPECT_EQ(kf->getTranslate(), Ogre::Vector3(4, 5, 6));
    EXPECT_EQ(kf->getScale(), Ogre::Vector3(2, 2, 2));

    // MoveNodeKeyframeUndoable, after EXPECT_NEAR(keys[0], 1.5, 1e-4):
    auto* moved = keyAt("NA_MoveKf", "NA_MoveKf_Node", 1.5);
    ASSERT_NE(moved, nullptr);
    EXPECT_EQ(moved->getTranslate(), Ogre::Vector3(1, 2, 3));

🤖 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/NodeAnimationManager_test.cpp` around lines 570 - 595, Strengthen the
NodeAnimationManager tests by adding a helper near makeNamedNode that locates an
Ogre::TransformKeyFrame by clip, node, and time, then assert the captured
position and scale in KeyNodeCurrentTransformCapturesLiveTransform. Also update
MoveNodeKeyframeUndoable to read the retimed key and assert its translation
remains unchanged at the new time.

src/NodeAnimationManager.cpp (1)

392-416: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add ui.action breadcrumbs to beginEdit and endEdit.

Both methods are user-facing authoring actions. They change the edit session, force the clip state disabled, and change the visible clip list. Neither adds a breadcrumb. createClip, deleteClip, and addKeyframe already report through SentryReporter::addBreadcrumb.

As per coding guidelines: "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb; use ui.action for UI actions".

♻️ Proposed breadcrumbs

     setClipEnabled(name, false);
     setActiveClip(name);
     if (m_editingClip != name) {
         m_editingClip = name;
         emit editingClipChanged();
     }
+    SentryReporter::addBreadcrumb("ui.action",
+        QStringLiteral("node anim: begin edit '%1'").arg(name));
     // The set of listed clips changes (a draft is hidden from the main list).
     emit clipsChanged();
 }
 
 void NodeAnimationManager::endEdit()
 {
     assertMainThread();
     if (m_editingClip.isEmpty()) return;
+    SentryReporter::addBreadcrumb("ui.action",
+        QStringLiteral("node anim: end edit '%1'").arg(m_editingClip));
     m_editingClip.clear();

🤖 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/NodeAnimationManager.cpp` around lines 392 - 416, Add
SentryReporter::addBreadcrumb calls with category “ui.action” to
NodeAnimationManager::beginEdit and endEdit, recording the corresponding
edit-start and edit-end actions. Include the relevant clip name in beginEdit’s
breadcrumb, and preserve the existing early-return and edit-state behavior.

Source: Coding guidelines

qml/AnimationDopeSheet.qml (1)

43-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

These bindings are destroyed on the first refreshNodeRows() call.

Lines 225-226 assign root.nodeClip and root.nodeRows imperatively. QML drops the declarative bindings declared here at that moment. The initial values therefore follow only NodeAnimationManager.activeClip, which ignores priority cases 1 and 3 documented at Lines 203-210.

Declare plain defaults and run the resolver once at load so the initial state and the refreshed state use the same rules.

♻️ Proposed refactor

-    property string nodeClip: NodeAnimationManager.activeClip
-    property var nodeRows: NodeAnimationManager.activeClip.length > 0
-                           ? NodeAnimationManager.nodeRows(NodeAnimationManager.activeClip)
-                           : []
+    property string nodeClip: ""
+    property var nodeRows: []
+    Component.onCompleted: root.refreshNodeRows()

🤖 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/AnimationDopeSheet.qml` around lines 43 - 46, Update the nodeClip and
nodeRows properties in AnimationDopeSheet so they use plain default values
rather than bindings to NodeAnimationManager.activeClip. Invoke the existing
refreshNodeRows() resolver during component initialization, ensuring initial
state and later refreshes both apply the documented priority rules.

qml/NodeAnimationPanel.qml (1)

24-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Avoid shadowing enabled in ToolBtn.

property bool enabled, opacity: enabled, MouseArea.enabled: parent.enabled, enabled: NodeAnimationManager.activeClip.length > 0, and enabled: PropertiesPanelController.selectionName.length > 0 are in qml/NodeAnimationPanel.qml. Rename the custom property so the component no longer creates a second enabled property that hides the inherited Item.enabled.

🤖 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/NodeAnimationPanel.qml` around lines 24 - 36, The ToolBtn component
shadows the inherited Item.enabled property with its custom enabled declaration.
Rename the custom property and update its opacity binding, MouseArea.enabled
binding, and all ToolBtn usages that pass enabled values, while preserving the
existing enabled conditions based on NodeAnimationManager.activeClip and
PropertiesPanelController.selectionName.

🤖 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/AnimationDopeSheet.qml`:
- Around line 243-245: Update onKeyframesChanged to compare the signal’s clip
argument with root.nodeClip, matching the clip selected by refreshNodeRows,
instead of NodeAnimationManager.activeClip; keep refreshing node rows only when
the changed clip is the displayed clip.
- Around line 1250-1260: Extend scrollByPixels() to proxy wheel scrolling to
nodeList, preserving its existing morph-band behavior. When nodeList content
exceeds its viewport, update nodeList.contentY by the wheel delta and clamp it
to the valid scroll range so rows beyond the capped height remain accessible
despite interactive: false.

In `@qml/NodeAnimationPanel.qml`:
- Around line 86-95: Update Connections.onClipsChanged to also clear editingClip
when the currently edited clip no longer exists in nodeAnimSection.clipNames,
matching the existing activeClip reset. Preserve editingClip when its clip
remains available, and ensure deleting the active/editing clip hides the
authoring row and prevents actions targeting the removed clip.

In `@scripts/anim-mcp-smoke.sh`:
- Around line 84-95: Update the inline Python GLB channel check in
scripts/anim-mcp-smoke.sh to exit nonzero when no matching node animation
channel is found, then capture that status in the surrounding shell block and
increment PASS or FAIL accordingly. Preserve the existing output messages while
ensuring a missing channel affects the script’s final exit status.

In `@src/AnimationControlController.cpp`:
- Around line 438-461: Remove the cross-entity fallback in selectedBonePtr():
after resolving the currently selected entities, return nullptr unless
m_selectedEntity is still present, valid, and has a skeleton. Keep bone lookup
tied exclusively to m_selectedEntity so callers such as TransformOperator cannot
combine a bone from one entity with selectedEntity() from another.

In `@src/mainwindow.cpp`:
- Around line 979-982: In initToolBar(), add process-wide once-guards around the
NodeAnimationManager singleton registration and the neighboring
MorphAnimationManager and VertexAnimationManager registrations, matching the
existing MeshGenController guard pattern. Ensure repeated MainWindow
construction registers each PropertiesPanel singleton only once while preserving
any test-binary exception required by main.cpp registration behavior.

In `@src/MCPServer.cpp`:
- Around line 3158-3170: Update the node-clip time handling before
setTimePosition and the slider conversion: reject non-finite or negative values
using the same contract as toolSetNodeKeyframe and toolDeleteNodeKeyframe, then
clamp the millisecond slider value to the valid int range before converting.
Preserve the existing animation-state validation and controller synchronization
for valid times.

In `@src/MeshImporterExporter.cpp`:
- Around line 5076-5090: Update injectMorphWeightAnimations and its callers to
accept the exported scene-node name, and have findMorphNode prefer an exact
nodes[i].name match before applying the existing target-count fallback. In the
SCENE export loop, pass each entity’s corresponding exported node name so
morph-weight channels bind to the correct entity, and consolidate processing to
avoid re-reading and rewriting the exported file once per entity.
- Around line 2398-2447: Replace the nearest-key implementations in sampleVec
and sampleQuat with forward-indexed interpolation over sorted key times:
linearly interpolate vectors between bracketing keys and use
Ogre::Quaternion::Slerp for rotations, clamping to endpoint values outside each
channel’s range. Advance each sampler’s key index across the ordered times so
sampling avoids rescanning all keys while preserving the existing defaults for
empty channels.

In `@src/NodeAnimationManager.cpp`:
- Around line 85-131: Update deleteClip to clear m_activeClip and m_editingClip
when either references the deleted clip, and emit the corresponding changed
signals. In the undo path that rebuilds a clip, also clear m_editingClip so no
stale editing state survives reconstruction; preserve existing track-handle and
Ogre animation cleanup.

In `@src/PropertiesPanelController.cpp`:
- Around line 1324-1331: Update the node-transform clip deletion branch in the
surrounding animation deletion method to call
NodeAnimationManager::deleteClipUndoable instead of deleteClip, preserving the
existing validation, animationStateChanged emission, and return behavior so
Inspector deletions use the same undoable command as NodeAnimationPanel.

In `@src/TransformOperator.cpp`:
- Line 1754: Guard NodeAnimationManager::instance() before calling editingClip()
in both src/TransformOperator.cpp#L1754-L1754 within mouseMoveEvent’s bone-gizmo
rotate branch and src/TransformOperator.cpp#L2526-L2526 at the start of
rotateSelected. Preserve the existing rotation behavior when the singleton
exists, while safely skipping the editing-clip check when it is null.
- Around line 2526-2537: Update the animation-edit routing block around
NodeAnimationManager::instance()->editingClip() to collect unique target
SceneNode objects from both selected nodes and entity parent nodes before
applying rotation, preventing duplicate rotations. Reuse the existing hasNodes()
pivot math and gizmo-center offset/reposition logic so all routed targets orbit
the gizmo pivot rather than rotating around their individual origins, while
preserving updateGizmoPosition() and the early return when targets exist.

---

Minor comments:
In `@CLAUDE.md`:
- Line 269: Update the harness reference in the animation MCP documentation to
point to the existing scripts/anim-mcp-smoke.sh file instead of
scratchpad/anim_mcp_test.sh, leaving the surrounding description unchanged.

In `@qml/AnimationDopeSheet.qml`:
- Around line 1367-1373: Update the onPositionChanged handler to clamp target to
AnimationControlController.animationLength as well as zero, matching the
bone-diamond drag behavior. Ensure nodeDiamond.dragPreviewTime never exceeds the
clip length.

In `@qml/NodeAnimationPanel.qml`:
- Around line 74-84: Update commitNewClip() so creatingClip is set to false only
after createClipUndoable succeeds; preserve the editor row and typed input when
creation fails. Add the requested createError string property near creatingClip,
set it when validation or clip creation fails, clear it on a successful creation
or new edit session, and display it in the editor row.

In `@scripts/anim-combined-roundtrip.sh`:
- Around line 108-114: In scripts/anim-combined-roundtrip.sh (lines 108-114),
scripts/anim-mcp-smoke.sh (lines 160-169), scripts/anim-roundtrip.sh (lines
130-137), and scripts/anim-filemenu-roundtrip.sh (lines 76-82), move each kill
-0 $APP_PID crash check and its FAIL increment before the summary echo so the
reported FAIL count includes crashes; preserve the existing cleanup and exit
behavior.

In `@scripts/anim-filemenu-roundtrip.sh`:
- Around line 14-15: In scripts/anim-filemenu-roundtrip.sh, add an early guard
immediately after the BODY and FACE assignments that verifies both fixture files
exist, matching the existing fixture check in anim-combined-roundtrip.sh. If
either asset is missing, print a clear message and exit before any load_mesh
calls run.

In `@src/AnimationControlController.cpp`:
- Around line 693-707: Update the node-clip branch in the animation-selection
flow to emit keyframeTicksChanged immediately after resetting m_selectedTick,
alongside the existing currentKeyframeChanged notification. Keep the reset and
notification behavior consistent with the skeletal path.

In `@src/mainwindow.cpp`:
- Around line 1195-1200: Move the m_dopeSheetDock->raise() call from the initial
dock setup to after tabifyBottomToolDocks() and its preference-based raises in
initToolBar(), so the Dope Sheet remains the front tab as described by the
surrounding comment. Preserve the existing lazy-QML initialization and other
dock preference behavior.

In `@src/MCPServer.cpp`:
- Around line 7615-7634: Update toolSetLoopRegion’s start/end validation to
reject non-finite numeric values and reject regions where start is greater than
end before calling setLoopStart or setLoopEnd. Return the existing error result
with a clear validation message and only report ok=true for valid, ordered loop
regions.

In `@src/MeshImporterExporter_test.cpp`:
- Around line 554-596: Guard the NodeAnimationManager pointer returned by
NodeAnimationManager::instance() before dereferencing it, failing the test
cleanly when it is null. Ensure the "SlideClip" created in this test is removed
even when an ASSERT_* aborts execution by moving cleanup into TearDown or adding
a scope guard, and remove the existing success-only deleteClip call.

In `@src/NodeAnimationManager.h`:
- Around line 132-137: Update the documentation for scrubClip to state that it
is currently a deliberate no-op and does not sample, update the viewport, or
enable AnimationState; preserve the note that playback ownership remains with
the caller only if accurate. Update keyNodeCurrentTransform documentation to
describe the captured position, orientation, and scale as the node’s current
parent-relative local transform rather than a world-relative transform.

In `@src/PropertiesPanelController.cpp`:
- Around line 1135-1145: Add a Sentry breadcrumb for the node-clip
enable/disable user action in the shown branch, using the `ui.action` category
and including the relevant clip/entity context before or alongside
`NodeAnimationManager::setClipEnabled`. Keep the existing state update, signal
emission, and early return unchanged.

---

Nitpick comments:
In `@qml/AnimationDopeSheet.qml`:
- Around line 43-46: Update the nodeClip and nodeRows properties in
AnimationDopeSheet so they use plain default values rather than bindings to
NodeAnimationManager.activeClip. Invoke the existing refreshNodeRows() resolver
during component initialization, ensuring initial state and later refreshes both
apply the documented priority rules.

In `@qml/NodeAnimationPanel.qml`:
- Around line 24-36: The ToolBtn component shadows the inherited Item.enabled
property with its custom enabled declaration. Rename the custom property and
update its opacity binding, MouseArea.enabled binding, and all ToolBtn usages
that pass enabled values, while preserving the existing enabled conditions based
on NodeAnimationManager.activeClip and PropertiesPanelController.selectionName.

In `@scripts/anim-combined-roundtrip.sh`:
- Around line 49-51: Replace the fixed /tmp/combined_out.glb path in the export
flow with a unique path allocated by mktemp, and use that variable in the export
JSON and existence check. Ensure the allocated temporary file is removed in the
script’s cleanup block, following the pattern used by the other harness scripts.

In `@scripts/anim-filemenu-roundtrip.sh`:
- Around line 42-48: Replace the hardcoded "out_body" argument in the C1 setup
with the node name derived from get_scene_info, matching the existing approach
in anim-roundtrip.sh. Pass that derived name to author_node so repeated imports
target the uniquely created node and the C1/C2 animation checks remain valid.

In `@scripts/anim-roundtrip.sh`:
- Around line 111-118: Remove the unused RNODE assignment and its unreliable
broad-token extraction. In the MAXTX candidate loop, remove the discarded call
list_node_animations invocation so the command substitution only generates
candidates from the re-exported GLB via Python. Also remove the unused FACE
assignment near the script’s setup while preserving BODY and QTMESH_RT_MESH
usage.

In `@src/MCPServer.cpp`:
- Around line 7684-7687: Update toolSelectBone to verify that
AnimationControlController::instance()->selectedBone() matches the requested
bone after selectBone(bone) returns; set content["ok"] based on that comparison
and only report the selection as successful when it took effect, mirroring the
existing validation in toolSelectAnimation.
- Around line 10794-10799: Update the JSON schemas for the closed-set string
properties in the relevant tool definitions: add a channel enum containing
exactly tx, ty, tz, rw, rx, ry, rz, sx, sy, and sz, and add a direction enum
containing its two supported values. Keep the existing descriptions and required
fields unchanged, following the existing enum pattern used by up_axis and match.

In `@src/MeshImporterExporter.cpp`:
- Around line 2451-2462: Restrict reconstructNodeClipsFromFile and its
corresponding call site around the later import path to formats that Assimp can
provide node channels for; skip the extra ReadFile for sidecar-handled formats
such as .mesh, .tmd, .rsd, and Psy-Q .ply. Prefer reusing an already-loaded
aiScene when available, otherwise guard the existing
reconstructNodeClipsFromFile call with the same supported-format check while
preserving reconstruction for eligible formats.

In `@src/NodeAnimationManager_test.cpp`:
- Around line 570-595: Strengthen the NodeAnimationManager tests by adding a
helper near makeNamedNode that locates an Ogre::TransformKeyFrame by clip, node,
and time, then assert the captured position and scale in
KeyNodeCurrentTransformCapturesLiveTransform. Also update
MoveNodeKeyframeUndoable to read the retimed key and assert its translation
remains unchanged at the new time.

In `@src/NodeAnimationManager.cpp`:
- Around line 392-416: Add SentryReporter::addBreadcrumb calls with category
“ui.action” to NodeAnimationManager::beginEdit and endEdit, recording the
corresponding edit-start and edit-end actions. Include the relevant clip name in
beginEdit’s breadcrumb, and preserve the existing early-return and edit-state
behavior.

🪄 Autofix

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 Plus

Run ID: 4ec74b50-ce5b-47c2-9323-ebfea072e1fa

📥 Commits

Reviewing files that changed from the base of the PR and between aaf75fe and 37d5ec7.

📒 Files selected for processing (27)

• .github/workflows/deploy.yml
• CLAUDE.md
• qml/AnimationControlPanel.qml
• qml/AnimationDopeSheet.qml
• qml/NodeAnimationPanel.qml
• qml/PropertiesPanel.qml
• scripts/anim-combined-roundtrip.sh
• scripts/anim-filemenu-roundtrip.sh
• scripts/anim-mcp-smoke.sh
• scripts/anim-roundtrip.sh
• src/AnimationControlController.cpp
• src/AnimationControlController.h
• src/Assimp/AnimationProcessor.cpp
• src/MCPServer.cpp
• src/MCPServer.h
• src/MeshImporterExporter.cpp
• src/MeshImporterExporter_test.cpp
• src/NodeAnimationManager.cpp
• src/NodeAnimationManager.h
• src/NodeAnimationManager_test.cpp
• src/PropertiesPanelController.cpp
• src/TransformOperator.cpp
• src/commands/NodeAnimCommands.cpp
• src/commands/NodeAnimCommands.h
• src/mainwindow.cpp
• src/qml_resources.qrc
• tests/fixtures/combined_skel_morph.glb

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Morph-weight channels can bind to the wrong node when more than one entity has morph targets.

injectMorphWeightAnimations resolves its target with findMorphNode(root, poseCount, &numTargets) (Line 3561). That helper matches only on the primitive target count and otherwise falls back to the first node that has any targets. It does not know which entity the call is for.

If the scene has two morph-bearing entities with the same target count, both loop iterations resolve the same glTF node. The second entity's weight clips are then written against the first entity's node.

Each iteration also re-reads, re-parses and rewrites the whole exported file, so the write cost scales with the number of entities.

Pass the exported scene-node name into injectMorphWeightAnimations and match nodes[i].name against it before falling back to the target-count heuristic.

🤖 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/MeshImporterExporter.cpp` around lines 5076 - 5090, Update
injectMorphWeightAnimations and its callers to accept the exported scene-node
name, and have findMorphNode prefer an exact nodes[i].name match before applying
the existing target-count fallback. In the SCENE export loop, pass each entity’s
corresponding exported node name so morph-weight channels bind to the correct
entity, and consolidate processing to avoid re-reading and rewriting the
exported file once per entity.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/Assimp/AnimationProcessor_test.cpp (1)

64-91: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Make the scale test detect ignored scaling keys.

The only input scale equals the bind scale. The expected result is identity. A processor that creates a keyframe but does not apply mScalingKeys can pass this test.

Add a second scale key that differs from the bind scale. Assert its normalized value. For example, use scale 200 at time 1.0 and assert scale 2.

Proposed test update

-    channel->mNumScalingKeys = 1;
-    channel->mScalingKeys = new aiVectorKey[1];
+    channel->mNumScalingKeys = 2;
+    channel->mScalingKeys = new aiVectorKey[2];
     channel->mScalingKeys[0] = aiVectorKey(0.0, aiVector3D(100.f, 100.f, 100.f));
+    channel->mScalingKeys[1] = aiVectorKey(1.0, aiVector3D(200.f, 200.f, 200.f));
@@
-    ASSERT_GE(track->getNumKeyFrames(), 1);
+    ASSERT_GE(track->getNumKeyFrames(), 2);
     auto* kf = static_cast<Ogre::TransformKeyFrame*>(track->getKeyFrame(0));
+    auto* secondKf = static_cast<Ogre::TransformKeyFrame*>(track->getKeyFrame(1));
@@
     EXPECT_NEAR(kf->getScale().z, 1.0f, 1e-4f);
+    EXPECT_NEAR(secondKf->getScale().x, 2.0f, 1e-4f);
+    EXPECT_NEAR(secondKf->getScale().y, 2.0f, 1e-4f);
+    EXPECT_NEAR(secondKf->getScale().z, 2.0f, 1e-4f);

#!/bin/bash
set -euo pipefail
ast-grep outline src/Assimp/AnimationProcessor.cpp --items all
rg -n -C 5 'mScalingKeys|createNodeKeyFrame|setScale' src/Assimp/AnimationProcessor.cpp

🤖 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/Assimp/AnimationProcessor_test.cpp` around lines 64 - 91, Strengthen the
scaling-key test in the animation setup around
AnimationProcessor::processAnimations by adding a second mScalingKeys entry with
scale 200 at time 1.0. Keep the existing bind-scale key and assertions, then
retrieve the second keyframe from the node track and assert its normalized scale
is 2.0 on all axes.

src/MCPServer.h (1)

224-226: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use ai.tool_call for MCP breadcrumbs.

The implementations of toolSplitMeshBySegments, toolExplodeMeshParts, and toolJoinMeshParts use mesh.parts.* as the first SentryReporter::addBreadcrumb argument. Use ai.tool_call for all MCP operations. This keeps Sentry filtering consistent with the animation MCP handlers.

The affected implementations are in src/MCPServer.cpp, Lines 4899-4959, 4961-5009, and 5011-5078.

Proposed fix

-SentryReporter::addBreadcrumb(QStringLiteral("mesh.parts.split_segments"),
+SentryReporter::addBreadcrumb(QStringLiteral("ai.tool_call"),
                               QStringLiteral("MCP split_mesh_by_segments"));

-SentryReporter::addBreadcrumb(QStringLiteral("mesh.parts.explode"),
+SentryReporter::addBreadcrumb(QStringLiteral("ai.tool_call"),
                               QStringLiteral("MCP explode_mesh_parts"));

-SentryReporter::addBreadcrumb(QStringLiteral("mesh.parts.join"),
+SentryReporter::addBreadcrumb(QStringLiteral("ai.tool_call"),
                               QStringLiteral("MCP join_mesh_parts"));

As per coding guidelines, MCP operations must use SentryReporter::addBreadcrumb with ai.tool_call.

🤖 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.h` around lines 224 - 226, Update the
SentryReporter::addBreadcrumb calls within toolSplitMeshBySegments,
toolExplodeMeshParts, and toolJoinMeshParts to use ai.tool_call as the first
argument instead of mesh.parts.*. Preserve each operation’s existing breadcrumb
details and behavior.

Source: Coding guidelines

🤖 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/Assimp/AnimationProcessor_test.cpp`:
- Around line 12-15: Replace the direct Ogre setup in
src/Assimp/AnimationProcessor_test.cpp lines 12-15 and lines 50-53 with a shared
Ogre test fixture whose SetUp asserts tryInitOgre() and canLoadMeshFiles(), then
convert the affected tests to TEST_F while preserving their existing test logic.
Apply the same fixture and TEST_F conversion to the zero-ticks and related
Ogre-dependent tests in src/AnimationProcessor_test.cpp lines 89-92; ensure all
named sites use the fixture and fail loudly when either prerequisite is
unavailable.

---

Outside diff comments:
In `@src/Assimp/AnimationProcessor_test.cpp`:
- Around line 64-91: Strengthen the scaling-key test in the animation setup
around AnimationProcessor::processAnimations by adding a second mScalingKeys
entry with scale 200 at time 1.0. Keep the existing bind-scale key and
assertions, then retrieve the second keyframe from the node track and assert its
normalized scale is 2.0 on all axes.

In `@src/MCPServer.h`:
- Around line 224-226: Update the SentryReporter::addBreadcrumb calls within
toolSplitMeshBySegments, toolExplodeMeshParts, and toolJoinMeshParts to use
ai.tool_call as the first argument instead of mesh.parts.*. Preserve each
operation’s existing breadcrumb details and behavior.

🪄 Autofix

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 Plus

Run ID: 781e2df1-5672-4f07-951d-b54ff63d716a

📥 Commits

Reviewing files that changed from the base of the PR and between 37d5ec7 and 3b2f093.

📒 Files selected for processing (12)

• CLAUDE.md
• qml/PropertiesPanel.qml
• src/AnimationControlController.cpp
• src/AnimationControlController.h
• src/AnimationProcessor_test.cpp
• src/Assimp/AnimationProcessor.cpp
• src/Assimp/AnimationProcessor_test.cpp
• src/MCPServer.cpp
• src/MCPServer.h
• src/MeshImporterExporter.cpp
• src/mainwindow.cpp
• src/qml_resources.qrc

🚧 Files skipped from review as they are similar to previous changes (9)

• src/qml_resources.qrc
• src/Assimp/AnimationProcessor.cpp
• src/AnimationControlController.h
• src/AnimationControlController.cpp
• qml/PropertiesPanel.qml
• src/mainwindow.cpp
• src/MeshImporterExporter.cpp
• CLAUDE.md
• src/MCPServer.cpp

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Use the required Ogre test fixture.

These tests construct Ogre::Root directly. They do not fail clearly when Ogre initialization or mesh-file loading is unavailable in CI. Add a shared fixture with ASSERT_TRUE(tryInitOgre()) and ASSERT_TRUE(canLoadMeshFiles()) in SetUp, then use TEST_F.

• src/Assimp/AnimationProcessor_test.cpp#L12-L15: replace direct Ogre setup with the shared fixture.
• src/Assimp/AnimationProcessor_test.cpp#L50-L53: use the same fixture for the scale test.
• src/AnimationProcessor_test.cpp#L89-L92: use the fixture for the zero-ticks test and the related Ogre-dependent tests.

As per coding guidelines, src/**/*_test.cpp tests must work under Xvfb. Based on learnings, Ogre-dependent fixtures must fail loudly with both prerequisite assertions.

#!/bin/bash
set -euo pipefail
fd -a '^TestHelpers\.h$' .
rg -n -C 3 'tryInitOgre|canLoadMeshFiles|TEST_F|::testing::Test' src --glob '*_test.cpp'

📍 Affects 2 files

• src/Assimp/AnimationProcessor_test.cpp#L12-L15 (this comment)
• src/Assimp/AnimationProcessor_test.cpp#L50-L53
• src/AnimationProcessor_test.cpp#L89-L92

🤖 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/Assimp/AnimationProcessor_test.cpp` around lines 12 - 15, Replace the
direct Ogre setup in src/Assimp/AnimationProcessor_test.cpp lines 12-15 and
lines 50-53 with a shared Ogre test fixture whose SetUp asserts tryInitOgre()
and canLoadMeshFiles(), then convert the affected tests to TEST_F while
preserving their existing test logic. Apply the same fixture and TEST_F
conversion to the zero-ticks and related Ogre-dependent tests in
src/AnimationProcessor_test.cpp lines 89-92; ensure all named sites use the
fixture and fail loudly when either prerequisite is unavailable.

[fernandotonon]

Thanks for the thorough review — addressed in bc12ca2d (plus the earlier 3b2f0933 for the unit-test updates and Sonar null-deref).

Fixed

• Null-deref (Sonar MAJOR + TransformOperator.cpp:1754 Critical): guarded NodeAnimationManager::instance() before deref in updateAnimationTree, the bone-gizmo rotate path, and rotateSelected.
• deleteClip leaves stale active/edit state (NodeAnimationManager.cpp:131): now clears m_activeClip/m_editingClip + emits signals — this is the central fix; the NodeAnimationPanel onClipsChanged reset backs it up.
• Node scrub re-enables during edit (AnimationControlController.cpp:699): setAnimationFrame no longer enables a node clip while it's being edited.
• set_node_animation_playing didn't start transport (MCPServer.cpp:7862): now calls setPlaying(true).
• Inspector rotation fields bypassed node routing (TransformOperator.cpp:2526): Euler overload now routes to the SceneNode during a node edit; quaternion overload dedups target nodes (no double-rotate) and preserves the pivot orbit (2537).
• Node-clip delete not undoable (PropertiesPanelController.cpp:1331): routed through deleteClipUndoable.
• selectedBonePtr could mix entities (AnimationControlController.cpp:461): removed the divergent fallback.
• MCP validation: set_loop_region (finiteness/ordering) and set_animation_time node branch (finiteness/negative + clamped slider cast).
• Staggered TRS on import (MeshImporterExporter.cpp:2418): now lerp (vec) / slerp (quat) instead of nearest-key.
• QML: dope-sheet onKeyframesChanged compares against the displayed clip (root.nodeClip); node band now scrolls; panel ends the edit session if the edited clip vanishes.
• Smoke harness glb-channel check now drives PASS/FAIL + exit code.
• Unit tests updated for the zero-track-clip drop.

Deferred (with reason)

• injectMorphWeightAnimations multi-entity node binding (5100): real but a rare 2-morph-entity/identical-target-count edge case; needs threading the node name through the signature — tracked as follow-up rather than expand this PR.
• AnimationProcessor_test GL fixture (test.cpp:15): those tests intentionally use a bare Ogre::Root for pure keyframe math (no display); adding tryInitOgre() would make them require a GL context.
• Per-singleton QML registration once-guards (mainwindow.cpp:1272): pre-existing pattern shared by ~12 sibling registrations; not introduced by this PR and out of scope to change piecemeal.

smoke 54/54, combined round-trip 10/10, full build green on all three platforms.

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/TransformOperator.cpp (1)

1748-1760: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add ui.action breadcrumbs for node-edit rotations.

The gizmo paths record ui.transform, not ui.action. The inspector rotation path records neither. Add one ui.action breadcrumb at gesture start or inspector dispatch. Do not log from mouseMoveEvent.

🤖 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/TransformOperator.cpp` around lines 1748 - 1760, Add a single ui.action
breadcrumb for node-edit rotations at gesture start or inspector dispatch,
covering the node rotation path near the nodeAnimMgr branch and the inspector
rotation paths at src/TransformOperator.cpp lines 2521-2548 and 2613-2638; do
not add logging from mouseMoveEvent, and preserve the existing ui.transform
breadcrumbs.

Source: Coding guidelines

🤖 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/TransformOperator.cpp`:
- Around line 2623-2636: Update the rotation logic around the selected-entity
loop to collect unique parent SceneNodes and apply each inspector rotation delta
only once per node. Preserve mesh rotation behavior for non-editing nodes, and
update SelectionSet::setEntityRotation for every entity associated with each
rotated parent node.
- Around line 2527-2548: Keep entity rotation tracking synchronized in both
node-edit rotation paths: in src/TransformOperator.cpp lines 2527-2548, after
rotating the deduplicated targets, add the quaternion’s Euler delta to each
selected entity whose parent node was rotated; in src/TransformOperator.cpp
lines 1754-1760, apply the same tracked-rotation update immediately after
rotating ent->getParentSceneNode(). Use the existing entity rotation tracking
mechanism so later getEntityRotation calculations remain accurate.
- Around line 2529-2538: Update the target collection near the existing nodes
and entities loops to also iterate the selected Ogre::SubEntity objects, resolve
each sub-entity’s parent entity and its parent SceneNode, and insert that node
into targets when available. Preserve the existing deduplication behavior so
node rotation and keyframe capture occur once per scene node.

---

Outside diff comments:
In `@src/TransformOperator.cpp`:
- Around line 1748-1760: Add a single ui.action breadcrumb for node-edit
rotations at gesture start or inspector dispatch, covering the node rotation
path near the nodeAnimMgr branch and the inspector rotation paths at
src/TransformOperator.cpp lines 2521-2548 and 2613-2638; do not add logging from
mouseMoveEvent, and preserve the existing ui.transform breadcrumbs.

🪄 Autofix

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 Plus

Run ID: 02a92630-44d8-4eea-a646-d7017e164129

📥 Commits

Reviewing files that changed from the base of the PR and between 3b2f093 and c583b02.

📒 Files selected for processing (11)

• CLAUDE.md
• qml/AnimationDopeSheet.qml
• qml/NodeAnimationPanel.qml
• scripts/anim-mcp-smoke.sh
• src/AnimationControlController.cpp
• src/MCPServer.cpp
• src/MeshImporterExporter.cpp
• src/NodeAnimationManager.cpp
• src/PropertiesPanelController.cpp
• src/TransformOperator.cpp
• src/mainwindow.cpp

🚧 Files skipped from review as they are similar to previous changes (10)

• scripts/anim-mcp-smoke.sh
• qml/NodeAnimationPanel.qml
• src/AnimationControlController.cpp
• qml/AnimationDopeSheet.qml
• src/MeshImporterExporter.cpp
• CLAUDE.md
• src/PropertiesPanelController.cpp
• src/MCPServer.cpp
• src/mainwindow.cpp
• src/NodeAnimationManager.cpp

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Keep entity rotation tracking synchronized with node-edit rotations.

Both paths rotate an entity parent SceneNode and return before the existing tracking update at Lines 2587-2588. Line 2625 later uses getEntityRotation(obj) to calculate an inspector delta. A gizmo rotation therefore leaves stale state and a later inspector rotation applies an incorrect delta.

• src/TransformOperator.cpp#L2527-L2548: after rotating targets, add the quaternion Euler delta to every selected entity whose parent node was rotated.
• src/TransformOperator.cpp#L1754-L1760: after rotating ent->getParentSceneNode(), update that entity's tracked rotation with the same Euler delta.

📍 Affects 1 file

• src/TransformOperator.cpp#L2527-L2548 (this comment)
• src/TransformOperator.cpp#L1754-L1760

🤖 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/TransformOperator.cpp` around lines 2527 - 2548, Keep entity rotation
tracking synchronized in both node-edit rotation paths: in
src/TransformOperator.cpp lines 2527-2548, after rotating the deduplicated
targets, add the quaternion’s Euler delta to each selected entity whose parent
node was rotated; in src/TransformOperator.cpp lines 1754-1760, apply the same
tracked-rotation update immediately after rotating ent->getParentSceneNode().
Use the existing entity rotation tracking mechanism so later getEntityRotation
calculations remain accurate.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Route selected sub-entities to their parent SceneNode.

If the selection contains only Ogre::SubEntity objects, targets remains empty. The code then reaches Lines 2591-2605 and rotates mesh geometry instead of the scene node. Node-keyframe capture and playback then miss the rotation.

Add each selected sub-entity parent entity's scene node to targets.

Proposed fix

         for (Ogre::Entity* ent : SelectionSet::getSingleton()->getEntitiesSelectionList())
             if (Ogre::SceneNode* sn = ent ? ent->getParentSceneNode() : nullptr)
                 targets.insert(sn);
+        for (Ogre::SubEntity* sub : SelectionSet::getSingleton()->getSubEntitiesSelectionList())
+            if (Ogre::Entity* ent = sub ? sub->getParent() : nullptr)
+                if (Ogre::SceneNode* sn = ent->getParentSceneNode())
+                    targets.insert(sn);

📝 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.

Suggested change

	// Collect the DISTINCT target scene nodes from both selection lists: a
	// selected SceneNode can also be the parent node of a selected Entity,
	// and rotating the same node twice would double the rotation. Dedup via
	// a set, then rotate each once (pivot-relative, like the hasNodes path).
	std::set<Ogre::SceneNode*> targets;
	for (Ogre::SceneNode* node : SelectionSet::getSingleton()->getNodesSelectionList())
	if (node) targets.insert(node);
	for (Ogre::Entity* ent : SelectionSet::getSingleton()->getEntitiesSelectionList())
	if (Ogre::SceneNode* sn = ent ? ent->getParentSceneNode() : nullptr)
	targets.insert(sn);
	// Collect the DISTINCT target scene nodes from both selection lists: a
	// selected SceneNode can also be the parent node of a selected Entity,
	// and rotating the same node twice would double the rotation. Dedup via
	// a set, then rotate each once (pivot-relative, like the hasNodes path).
	std::set<Ogre::SceneNode*> targets;
	for (Ogre::SceneNode* node : SelectionSet::getSingleton()->getNodesSelectionList())
	if (node) targets.insert(node);
	for (Ogre::Entity* ent : SelectionSet::getSingleton()->getEntitiesSelectionList())
	if (Ogre::SceneNode* sn = ent ? ent->getParentSceneNode() : nullptr)
	targets.insert(sn);
	for (Ogre::SubEntity* sub : SelectionSet::getSingleton()->getSubEntitiesSelectionList())
	if (Ogre::Entity* ent = sub ? sub->getParent() : nullptr)
	if (Ogre::SceneNode* sn = ent->getParentSceneNode())
	targets.insert(sn);

🤖 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/TransformOperator.cpp` around lines 2529 - 2538, Update the target
collection near the existing nodes and entities loops to also iterate the
selected Ogre::SubEntity objects, resolve each sub-entity’s parent entity and
its parent SceneNode, and insert that node into targets when available. Preserve
the existing deduplication behavior so node rotation and keyframe capture occur
once per scene node.

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Apply one inspector rotation per parent scene node.

If two selected entities share one parent SceneNode, Line 2629 rotates that node once for each entity. The node receives the requested delta multiple times.

Collect unique parent nodes before rotation. Update SelectionSet rotation tracking for every entity that belongs to each rotated node.

🤖 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/TransformOperator.cpp` around lines 2623 - 2636, Update the rotation
logic around the selected-entity loop to collect unique parent SceneNodes and
apply each inspector rotation delta only once per node. Preserve mesh rotation
behavior for non-editing nodes, and update SelectionSet::setEntityRotation for
every entity associated with each rotated parent node.

[coderabbitai]

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 @.github/workflows/deploy.yml:
- Line 1352: Replace the broad suite-name GL_CRASH_ALLOWLIST exemption with
captured suite output validation in the later crash handler: allow a crash only
when the exact known GL3Plus initialization signature appears before any “[ RUN
]” marker and the failure is the expected startup crash. Reject node-animation
or post-test crashes, other signals, and explicit exit codes in the >=128 range
as failures.

🪄 Autofix

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 Plus

Run ID: 2f0d717d-3df2-4727-bec3-c941fe968e1e

📥 Commits

Reviewing files that changed from the base of the PR and between c583b02 and 78ebae0.

📒 Files selected for processing (1)

• .github/workflows/deploy.yml

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Restrict the allowlist to verified pre-test GL failures.

Line [1352] exempts entire test suites. The later crash handler accepts any exit code >= 128 and counts every expected test as executed. It does not verify SIGSEGV, the known GL3Plus initialization signature, or the absence of a [ RUN ] marker. A node-animation regression that crashes after test execution can therefore pass CI. Other signals and explicit exit codes in this range are also accepted.

Capture the suite output and apply the exemption only when the exact startup GL signature is present before any test starts. Treat all other crashes as failures.

🤖 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 @.github/workflows/deploy.yml at line 1352, Replace the broad suite-name
GL_CRASH_ALLOWLIST exemption with captured suite output validation in the later
crash handler: allow a crash only when the exact known GL3Plus initialization
signature appears before any “[ RUN ]” marker and the failure is the expected
startup crash. Reject node-animation or post-test crashes, other signals, and
explicit exit codes in the >=128 range as failures.

[sonarqubecloud]

Quality Gate passed

Issues
162 New issues
0 Accepted issues

Measures
0 Security Hotspots
44.5% Coverage on New Code
5.9% Duplication on New Code

See analysis details on SonarQube Cloud