feat(node-anim): sub-slice C6 — MCP tools - #586
Conversation
Third sub-slice of #520. Surfaces the C1 (manager) and C3 (commands) data layer through three new MCP tools so an AI agent can author node animations end-to-end via JSON-RPC. ## What ships ### Three new MCP tools - **`list_node_animations`** — no args. Returns `{ count, clips: [name…] }`. Light; pure read off `NodeAnimationManager::listClips()`. - **`add_node_animation_clip`** — required: `name`, `length` (seconds). Wraps `createClip`. Errors on missing args, non-positive length, name collision. - **`set_node_keyframe`** — required: `clip`, `node`, `time`. Optional: `translate`, `rotation`, `scale` (defaulting to ZERO, IDENTITY, ONE if omitted, so the agent can author "snapshot pose at T" in one call). Each TRS array is strictly parsed — malformed arrays return an error rather than silently falling back to defaults, so agent bugs surface immediately. All three operate on the **live scene** (not a transient import), matching `set_morph_weight`'s surface. Agents are expected to drive `load_mesh` / `save_scene` around them for persistence. ## 4 new tests - `AddNodeAnimationClip_MissingArgsRejected` — empty / name-only / zero-length all rejected with descriptive errors. - `AddNodeAnimationClipThenList` — round-trip: create one, list shows it. - `SetNodeKeyframe_RejectsMissingClip` — clip-not-found surfaces. - `SetNodeKeyframe_RejectsMalformedTransformArray` — wrong-arity translate array → error message names the bad field. ## #520 status | Sub-slice | Status | |-|-| | C1 — Manager data layer | shipped (#584) | | C3 — Undo commands | shipped (#585) | | C6 — MCP tools | **this PR** | | C2 — Inspector subgroup | follow-up | | C4 — Dope-sheet integration | follow-up | | C5 — glTF + FBX exporter round-trip | follow-up | Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13e7aa5e58
ℹ️ 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".
| const double time = args.value("time").toDouble(); | ||
| if (!std::isfinite(time) || time < 0.0) |
There was a problem hiding this comment.
set_node_keyframe treats any non-Double JSON time as 0.0 because QJsonValue::toDouble() falls back to 0, and the subsequent guard only rejects negative/NaN values. In practice, inputs like "time": "abc" or "time": false will silently create/update a keyframe at t=0 instead of returning a validation error, which can corrupt authored animation data without surfacing the caller bug.
Useful? React with 👍 / 👎.
| return Ogre::Vector3(static_cast<Ogre::Real>(a[0].toDouble()), | ||
| static_cast<Ogre::Real>(a[1].toDouble()), | ||
| static_cast<Ogre::Real>(a[2].toDouble())); |
There was a problem hiding this comment.
The new strict TRS parsing only checks array length, but each component is still read via toDouble() without checking the JSON type. That means malformed values (for example "translate": ["x", 1, 2] or non-numeric quaternion entries) are silently coerced to 0 instead of being rejected, which contradicts the tool’s strict-parse contract and can write incorrect transforms while appearing successful.
Useful? React with 👍 / 👎.
|
* review(mcp): node-anim — strict JSON type validation Two Codex P1 findings on PR #586 (merged): ## Bug — silent coercion to 0.0 on non-numeric JSON `QJsonValue::toDouble()` returns 0.0 (or the supplied default) on any non-Double type — string, bool, null. So a caller bug like `"time": "0.5"` or `"translate": ["x", 1, 2]` would silently create a keyframe at t=0 with zeroed components instead of surfacing the problem. That contradicts the strict-parse contract documented in the tool descriptions and can quietly corrupt authored animation data. ## Fix - `set_node_keyframe` — explicit `isDouble()` guard on `time` before `toDouble()`. Inner Vec3 / Quat parse helpers loop over every component and reject any non-numeric element with a descriptive error naming the failing index (e.g. `"Error: 'translate'[0] must be a number"`). - `add_node_animation_clip` — same `isDouble()` guard on `length`. ## 3 new tests - `SetNodeKeyframe_RejectsNonNumericTime` — `"time": "0.5"` → error message contains "time" + "number". - `SetNodeKeyframe_RejectsNonNumericTRSComponent` — string in a translate slot → error message names the field. - `AddNodeAnimationClip_RejectsNonNumericLength` — string-typed length → error message contains "length" + "number". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review(mcp): refactor to fit SonarCloud cognitive-complexity threshold SonarCloud failed PR #587 with three issues on toolSetNodeKeyframe: - cognitive complexity 28 > 25 threshold - two inline lambdas each > 20 lines Fix: extract the per-array validation into a file-scope template helper `readNumericArray<N>` shared by translate/scale (N=3) and rotation (N=4). Removes both lambdas entirely and shrinks the dispatcher to short straight-line validation calls. Behaviour preserved — same wrong-arity and non-numeric guards, same error message format. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fourth sub-slice of #520. Adds a CLI subcommand to enumerate node-animation clips on a scene file from the shell. Mirrors the existing `qtmesh morph --list` precedent: one file in, names + count out (text or `--json`). Useful for CI scripts that audit Mixamo- style assets imported via the editor's GUI, and the round-trip target for the future C5 glTF/FBX exporter work. ## What ships - `cmdNodeAnim(argc, argv)` in `CLIPipeline.{h,cpp}` — parses `nodeanim <file> --list [--json]`, runs `MeshImporterExporter::importer`, reads `NodeAnimationManager::instance()->listClips()`, prints text or JSON. JSON shape mirrors `qtmesh morph --list --json`: `{ file, count, clips: [name…] }`. - Subcommand registration in `main.cpp` (CLI-mode detection by first non-flag arg) and `CLIPipeline::run` dispatcher. - Help text in `printUsage` next to the `morph` line. ## What's deferred - Authoring on the CLI side (`--add-clip`, `--add-keyframe`, `-o out.gltf`) needs the C5 exporter round-trip first — otherwise edits don't persist anywhere outside the in-process scene. ## #520 status | Sub-slice | Status | |-|-| | C1 — Manager data layer | shipped (#584) | | C3 — Undo commands | shipped (#585) | | C6 — MCP tools | shipped (#586, hardened #587 + #588) | | C-CLI — `qtmesh nodeanim --list` | **this PR** | | C2 — Inspector subgroup | follow-up | | C4 — Dope-sheet integration | follow-up | | C5 — glTF + FBX exporter round-trip | follow-up | Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Third sub-slice of #520 (node-transform animation). Surfaces the C1 (manager) and C3 (commands) data layers through three new MCP tools so an AI agent can author node animations end-to-end via JSON-RPC.
What ships
Three new MCP tools (all operate on the live scene, like
set_morph_weight)list_node_animations— no args. Returns{ count, clips: [name…] }. Light; pure read offNodeAnimationManager::listClips().add_node_animation_clip— required:name,length(seconds). WrapscreateClip. Errors on missing args, non-positive length, name collision.set_node_keyframe— required:clip,node,time. Optional:translate,rotation,scale(defaulting to ZERO, IDENTITY, ONE if omitted, so the agent can author "snapshot pose at T" in one call). Each TRS array is strictly parsed — malformed arrays return an error rather than silently falling back to defaults, so agent bugs surface immediately.Agents are expected to drive
load_mesh/save_scenearound them for persistence.4 new tests
AddNodeAnimationClip_MissingArgsRejected— empty / name-only / zero-length all rejected with descriptive errors.AddNodeAnimationClipThenList— round-trip: create one, list shows it.SetNodeKeyframe_RejectsMissingClip— clip-not-found surfaces.SetNodeKeyframe_RejectsMalformedTransformArray— wrong-arity translate array → error message names the bad field.#520 status
Test plan
🤖 Generated with Claude Code