feat(pose-lib): sub-slice D-Project — .poselib sidecar persistence#602
Merged
Conversation
Fifth sub-slice of #521 (named-pose library). Persists each entity's pose library to a `.poselib` sidecar JSON file so libraries survive across editor sessions and can be version-controlled alongside the asset they describe. Unblocks meaningful CLI surface (`qtmesh pose --library list/apply`) and any future "save project" workflow that wants pose data folded in. ## File format — `qtmesheditor.poselib.v1` ```json { "schema": "qtmesheditor.poselib.v1", "poses": [ { "name": "JawOpen", "bones": { "JawBone": { "t": [x,y,z], "r": [w,x,y,z], "s": [x,y,z] } } } ] } ``` Insertion order is preserved (the library walks `entIt->order` when writing). Bone names are the stable identifier — matches the in-memory representation, so the LOD/skeleton-swap resilience carries over to disk. ## What ships - `PoseLibrary::savePoseLibrary(entity, filePath)` — `QSaveFile` atomic write. Rejects empty libraries (don't write a `poses: []` file that loadPoseLibrary would reject), empty paths, null entities. - `PoseLibrary::loadPoseLibrary(entity, filePath)` — strict schema check, replaces existing per-entity library 1:1 (partial overlay would be confusing UX on name collision). Emits `posesChanged`. - `*ForSelection` Q_INVOKABLE wrappers for Inspector / MCP use. ## 4 tests - `SaveAndLoadLibraryRoundTripsViaSidecar` — write, forget, load, verify pose list + TRS values survive. - `SaveLibraryRejectsEmptyLibraryOrInvalidPath` — empty library (no poses saved), empty path, null entity → false. - `LoadLibraryRejectsMissingFileAndBadSchema` — three error paths: missing file, bad JSON, valid JSON with wrong schema string. - `LoadLibraryWipesExistingPosesFirst` — load replaces, doesn't merge: in-memory pose that's not in the file is dropped. ## #521 status | Sub-slice | Status | |-|-| | D1 — Singleton data layer | shipped (#592) | | D-MCP — MCP tools (5: list/save/apply/delete/mirror) | shipped (#593, #599) | | D3 — Undo commands | shipped (#595) | | D4 — Mirror pose | shipped (#597) | | D-Project — `.poselib` sidecar | **this PR** | | D2 — Inspector subgroup | follow-up | | D5 — Apply-with-mask | follow-up | | D6 — Time-blended apply | follow-up | | D-Thumbnail | follow-up | | D-CLI | now unblocked — follow-up | Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe PoseLibrary singleton gains JSON-backed persistence for full pose libraries via ChangesPose Library JSON Sidecar Persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/PoseLibrary.cpp`:
- Around line 364-366: The breadcrumb category used in
SentryReporter::addBreadcrumb for file save/load is incorrect: replace the
current "scene.anim.pose" category in the SentryReporter::addBreadcrumb call
(the one that builds the message with filePath and entIt->order.size()) with
"file.export" for save operations, and similarly ensure the corresponding
load-side SentryReporter::addBreadcrumb calls use "file.import" (also update the
other occurrences around the load code path where addBreadcrumb is called); keep
the existing message text and arguments (filePath and entIt->order.size()) but
only change the first argument (category) to the appropriate "file.export" or
"file.import".
- Around line 385-392: The code currently calls m_byEntity.remove(entity) and
then obtains auto& store = m_byEntity[entity] before validating the QJsonArray
poses, which can clear in-memory poses on malformed payloads; in
loadPoseLibrary, first fully validate the top-level payload and every entry in
the poses array (required keys, types, ranges, and any schema invariants) and
build the new pose data into a temporary container, and only if validation
succeeds perform the removal and assign/swap the temporary container into
m_byEntity[entity]; apply the same pattern to the other load path referenced
around the 407-426 region so no in-memory data is wiped until validation passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a42dba0e-c98d-41ee-b6f9-265cedd8d3a2
📒 Files selected for processing (3)
src/PoseLibrary.cppsrc/PoseLibrary.hsrc/PoseLibrary_test.cpp
Comment on lines
+364
to
+366
| SentryReporter::addBreadcrumb("scene.anim.pose", | ||
| QStringLiteral("save library to '%1' (%2 poses)") | ||
| .arg(filePath).arg(entIt->order.size())); |
There was a problem hiding this comment.
Use file I/O breadcrumb categories required by policy.
For save/load sidecar operations, breadcrumb categories should be file.export and file.import instead of scene.anim.pose.
Proposed fix
- SentryReporter::addBreadcrumb("scene.anim.pose",
+ SentryReporter::addBreadcrumb("file.export",
QStringLiteral("save library to '%1' (%2 poses)")
.arg(filePath).arg(entIt->order.size()));
@@
- SentryReporter::addBreadcrumb("scene.anim.pose",
+ SentryReporter::addBreadcrumb("file.import",
QStringLiteral("load library from '%1' (%2 poses)")
.arg(filePath).arg(store.order.size()));As per coding guidelines: "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb() using appropriate categories ... 'file.import'/'file.export' for I/O operations".
Also applies to: 428-430
🤖 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/PoseLibrary.cpp` around lines 364 - 366, The breadcrumb category used in
SentryReporter::addBreadcrumb for file save/load is incorrect: replace the
current "scene.anim.pose" category in the SentryReporter::addBreadcrumb call
(the one that builds the message with filePath and entIt->order.size()) with
"file.export" for save operations, and similarly ensure the corresponding
load-side SentryReporter::addBreadcrumb calls use "file.import" (also update the
other occurrences around the load code path where addBreadcrumb is called); keep
the existing message text and arguments (filePath and entIt->order.size()) but
only change the first argument (category) to the appropriate "file.export" or
"file.import".
Comment on lines
+385
to
+392
| const QJsonArray poses = root.value("poses").toArray(); | ||
|
|
||
| // Wipe the existing per-entity entry — the file is canonical | ||
| // for this load. Partial overlay would be confusing UX (which | ||
| // pose wins on name collision?), so we go with replacement. | ||
| m_byEntity.remove(entity); | ||
| auto& store = m_byEntity[entity]; | ||
|
|
There was a problem hiding this comment.
Validate full payload before wiping existing poses.
loadPoseLibrary removes current poses at Line 390 before it strictly validates the poses payload/entries, so a schema-matching but malformed sidecar can still clear in-memory data.
Proposed fix
- const QJsonArray poses = root.value("poses").toArray();
+ if (!root.contains("poses") || !root.value("poses").isArray()) return false;
+ const QJsonArray poses = root.value("poses").toArray();
- m_byEntity.remove(entity);
- auto& store = m_byEntity[entity];
+ EntityPoses parsed;
+ QSet<QString> seenNames;
@@
- for (const QJsonValue& p : poses) {
+ for (const QJsonValue& p : poses) {
+ if (!p.isObject()) return false;
const QJsonObject pObj = p.toObject();
const QString name = pObj.value("name").toString();
- if (name.isEmpty()) continue;
+ if (name.isEmpty() || seenNames.contains(name)) return false;
+ if (!pObj.value("bones").isObject()) return false;
+ seenNames.insert(name);
PoseSnapshot snapshot;
const QJsonObject bones = pObj.value("bones").toObject();
@@
- store.byName.insert(name, snapshot);
- store.order.append(name);
+ parsed.byName.insert(name, snapshot);
+ parsed.order.append(name);
}
+ m_byEntity.insert(entity, parsed);Also applies to: 407-426
🤖 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/PoseLibrary.cpp` around lines 385 - 392, The code currently calls
m_byEntity.remove(entity) and then obtains auto& store = m_byEntity[entity]
before validating the QJsonArray poses, which can clear in-memory poses on
malformed payloads; in loadPoseLibrary, first fully validate the top-level
payload and every entry in the poses array (required keys, types, ranges, and
any schema invariants) and build the new pose data into a temporary container,
and only if validation succeeds perform the removal and assign/swap the
temporary container into m_byEntity[entity]; apply the same pattern to the other
load path referenced around the 407-426 region so no in-memory data is wiped
until validation passes.
![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: 04254a24c5
ℹ️ 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".
Comment on lines
+385
to
+390
| const QJsonArray poses = root.value("poses").toArray(); | ||
|
|
||
| // Wipe the existing per-entity entry — the file is canonical | ||
| // for this load. Partial overlay would be confusing UX (which | ||
| // pose wins on name collision?), so we go with replacement. | ||
| m_byEntity.remove(entity); |
There was a problem hiding this comment.
poses payload before replacing library
When the file has a valid schema but poses is missing or not an array (for example after a bad manual edit or merge), toArray() yields an empty array and the function still returns true after wiping the current in-memory library. That silently drops existing poses while signaling success, which is data loss for callers that trust the return value. Validate that root["poses"] is actually an array (and fail early) before m_byEntity.remove(entity).
Useful? React with 👍 / 👎.
Comment on lines
+424
to
+425
| store.byName.insert(name, snapshot); | ||
| store.order.append(name); |
There was a problem hiding this comment.
Loading appends every parsed pose name into order even when the same name appears multiple times in the sidecar. Because byName keeps only the last value, order can contain duplicates, which later makes list/delete behavior inconsistent (e.g., a phantom name can remain in listPoses after deletePose removes the hash entry). Guard order.append(name) with an overwrite check (or reject duplicate names) to preserve internal invariants.
Useful? React with 👍 / 👎.
|
fernandotonon
added a commit
that referenced
this pull request
May 18, 2026
…eadcrumb category (#603) Three review findings on PR #602 (merged). ## Codex P1 — schema-matching file with bad `poses` field wiped library `loadPoseLibrary` removed the in-memory per-entity entry before checking that the `poses` field was actually an array. A file with matching schema but a malformed `poses` value (string, object, missing) would silently drop the user's existing poses and return success — data loss masked as success. Fix: stage parse output in a local `EntityPoses` so the in-memory store stays untouched until the file is fully accepted. Validate `posesV.isArray()` before any mutation. The all-or-nothing replacement (`m_byEntity.insert(entity, staging)`) only fires after a clean parse. ## Codex P2 — duplicate names in file left phantoms in `order` Loading appended every parsed pose name into `order` even on name re-occurrences. `byName` keeps only the last value (hash overwrite), so `order` could grow longer than the actual pose count. A `deletePose("A")` then removed the hash entry but left "A" in `order`, leaving `listPoses` and `hasPose` disagreeing. Fix: guard the `order.append(name)` with a `byName.contains(name)` check so later entries with the same name overwrite the snapshot (last-write-wins, same as a regular `savePose`) without growing `order`. ## CodeRabbit Major — file.import / file.export breadcrumb categories Per the project's Sentry breadcrumb policy in `CLAUDE.md`, file I/O operations use `file.import` / `file.export` not the scene-side category. Switched both `savePoseLibrary` and `loadPoseLibrary` to the right categories. ## 2 new tests - `LoadLibraryWithMissingPosesArrayPreservesInMemoryLibrary` — schema-matching file with `"poses": "oops"` (string instead of array) returns false AND keeps existing in-memory poses intact. - `LoadLibraryDeduplicatesDuplicateNamesInFile` — file with two entries under name "A" produces `listPoses = ["A", "B"]` not `["A", "A", "B"]`; after `deletePose("A")` no phantom remains. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
May 18, 2026
Closes the GUI / MCP / CLI triad for the pose library now that D-Project (#602) provides the .poselib sidecar format. ## What ships ### MCP tools (2 new) - `save_pose_library` — required `path`. Wraps `PoseLibrary::savePoseLibraryForSelection`. Returns ok+path on success; error on no selection / empty library / unwritable path. - `load_pose_library` — required `path`. Wraps `loadPoseLibraryForSelection`. Returns ok+path+count on success; error on no selection / missing file / malformed JSON or schema (in-memory library is preserved on parse failure thanks to the D-Project staging fix). ### CLI mode (new branch in `cmdPose`) `qtmesh pose <library.poselib> --library list [--json]` — reads the sidecar JSON directly (no mesh load) and prints the pose names. JSON shape mirrors the other CLI tools: `{ file, count, poses: [name…] }`. Doesn't go through `PoseLibrary` itself — there's no entity to key against in a standalone CLI invocation. Just parses the JSON. This intentionally bypasses the strict schema-replacement semantics that `loadPoseLibrary` enforces on a live entity; for read-only listing, schema-checking the file before walking is enough. `--library apply <name> -o out.fbx` is deliberately not in this PR — it needs the round-trip exporter to write pose-driven bone states back into the mesh. Documented in the help text as a follow-up. ## 3 new MCP tests - `SavePoseLibrary_MissingPathRejected` — empty args → error mentions 'path'. - `LoadPoseLibrary_MissingPathRejected` — same for load. - `LoadPoseLibrary_MissingFileRejected` — nonexistent file → error. ## Manual CLI verification - `qtmesh pose test.poselib --library list` → "Poses (N): …" - `qtmesh pose test.poselib --library list --json` → JSON shape. - `qtmesh pose /nonexistent.poselib --library list` → exit 1 with "File not found:". ## #521 status | Sub-slice | Status | |-|-| | D1 — Singleton data layer | shipped (#592) | | D-MCP — 5 + 2 = 7 tools | shipped (#593, #599, **#604**) | | D3 — Undo commands | shipped (#595) | | D4 — Mirror pose | shipped (#597) | | D-Project — .poselib sidecar | shipped (#602) | | D-CLI — `qtmesh pose --library list` | shipped (**this PR**) | | D2 — Inspector subgroup | follow-up | | D5 — Apply-with-mask | follow-up | | D6 — Time-blended apply | follow-up | | D-Thumbnail | follow-up | | D-CLI apply | follow-up (needs exporter round-trip) | Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
May 18, 2026
Codex P2 on PR #604 (merged). The CLI's `--library list` path checked the `schema` string but then did `root.value("poses").toArray()` unconditionally — same silent-empty-on-bad-type bug class as loadPoseLibrary had on PR #602 (fixed in #603), just in the duplicated parse-in-the-CLI path. A schema-matching `.poselib` file with `poses` missing or non-array would report "No poses in library" with exit 0 instead of surfacing the corruption. CI/audit workflows relying on non-zero exit for invalid libraries would miss the failure. Fix: explicit `isArray()` check after the schema match, emit "malformed 'poses' field in <path>" to stderr and return 1. Manual verification: a `{"schema":"qtmesheditor.poselib.v1","poses":"oops"}` file now exits 1 with the expected message instead of 0 with a vacuous result. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
added a commit
that referenced
this pull request
May 18, 2026
…ply <name> -o <out> (#606) Adds the apply side of `--library` mode (the list side shipped in #604). Loads a mesh, loads a .poselib sidecar, applies the named pose to the skeleton, exports the posed mesh. ## CLI shape qtmesh pose <mesh.fbx> --library apply --lib <library.poselib> --apply <name> -o <out.fbx> `filePath` (positional) is the MESH in apply mode, contrasting with list mode where it's the library file. The library path arrives via `--lib` so the positional convention stays consistent across all `pose` modes (positional = primary input). ## Implementation - Reuses `MeshImporterExporter::importer` + `exportCurrentPose` — same path the `--animation --time` mode takes, just with the pose source being `PoseLibrary::applyPose` instead of an `AnimationState`. - Strict input validation: - mesh file exists - library file exists - mesh has a skeleton (apply needs one) - pose name exists in the library (otherwise lists available names on stderr so the user can fix their command line) - Sentry breadcrumbs `cli.pose` + `file.import` for telemetry. Help text updated next to the existing `--library list` line. No new tests in this PR — the existing PoseLibrary + CLI tests cover the loadPoseLibrary / applyPose paths in isolation, and the new code is just glue between them. ## #521 status | Sub-slice | Status | |-|-| | D1 — Singleton data layer | shipped (#592) | | D-MCP — 7 tools | shipped (#593, #599, #604) | | D3 — Undo commands | shipped (#595) | | D4 — Mirror pose | shipped (#597) | | D-Project — .poselib sidecar | shipped (#602) | | D-CLI — `pose --library list` | shipped (#604) | | D-CLI — `pose --library apply` | **this PR** | | D2 — Inspector subgroup | follow-up | | D5 — Apply-with-mask | follow-up | | D6 — Time-blended apply | follow-up | | D-Thumbnail | follow-up | Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fifth sub-slice of #521 (named-pose library). Persists each entity's pose library to a
.poselibsidecar JSON file so libraries survive across editor sessions and can be version-controlled alongside the asset they describe. Unblocks meaningful CLI surface (qtmesh pose --library list/apply) and any future "save project" workflow.File format —
qtmesheditor.poselib.v1{ "schema": "qtmesheditor.poselib.v1", "poses": [ { "name": "JawOpen", "bones": { "JawBone": { "t": [x, y, z], "r": [w, x, y, z], "s": [x, y, z] } } } ] }Insertion order is preserved (writer walks
entIt->order). Bone names are the stable identifier — matches the in-memory representation, so the LOD/skeleton-swap resilience carries over to disk.What ships
savePoseLibrary(entity, filePath)—QSaveFileatomic write. Rejects empty libraries (don't writeposes: []), empty paths, null entities.loadPoseLibrary(entity, filePath)— strict schema check, replaces existing per-entity library 1:1. EmitsposesChanged.*ForSelectionQ_INVOKABLEwrappers.4 tests
SaveAndLoadLibraryRoundTripsViaSidecarSaveLibraryRejectsEmptyLibraryOrInvalidPathLoadLibraryRejectsMissingFileAndBadSchema(missing file, bad JSON, wrong schema)LoadLibraryWipesExistingPosesFirst(no partial merge)#521 status
.poselibsidecar🤖 Generated with Claude Code
Summary by CodeRabbit