feat(vat): slice 4 — Inspector subgroup + MCP bake_vat tool - #568
Merged
Conversation
Closes the final piece of #371 (parent epic: #517 / Slice E). Surfaces the existing VATBaker via the two user-facing channels QtMeshEditor expects: the QML Inspector and the MCP server. ### `VATBakerController` (QML_SINGLETON) QObject wrapper around `VATBaker::bake()`: - `availableAnimations` — read-only list of clip names on the currently-selected entity. Refreshes on `selectionChanged` and on the QML mount. - `isBaking` — true between `bake()` call and `bakeFinished`. QML uses it to disable the button. - `bake(animationName, fps, encoding, target, bakeNormals, outputDir, basename)` — validates args, builds `VATBaker::Options`, calls `VATBaker::bake`, emits `bakeFinished(bool ok, QString posTexture, QString error)`. Synchronous for slice 4; the Ogre animation state can't safely be sampled off-thread (it races with Manager's per-frame updates), so a future slice would split the sampling out into a producer/consumer pair. The QML surface already exposes the `isBaking` flag so adding the off-thread sampler is a behavior change, not an API one. ### Inspector UI New "Bake VAT" subgroup inside the Animations section (`qml/PropertiesPanel.qml`). Fields: - Anim picker (auto-populated from `VATBakerController.availableAnimations`) - FPS spinbox (1..120, default 30) - Format dropdown (`rgba8` / `rgba16`) - Target dropdown (`agnostic` / `unity` / `unreal` / `godot`) - Normals checkbox - Output dir text field (no native picker for slice 4 — Inspector text-fields are standard here; a follow-up can wire in QFileDialog for parity with the Save dialogs elsewhere) - Bake button + last-result status line (green ✓ on success, red ✗ + error on failure) Bake button disables itself while `isBaking` is true. ### MCP `bake_vat` tool `MCPServer::toolBakeVat(args)`: - Args mirror the CLI subcommand: `file`, `anim`, `fps`, `encoding`, `target`, `normals`, `output_dir`, `basename`. - Required: `file`, `anim`, `output_dir`. - Loads via `MeshImporterExporter::importer` (same path as CLI), bakes via `VATBaker::bake`, returns a JSON content result with the texture paths, frame/vertex counts, bounds, target, and encoding. - Registered in `toolHandlers()` and listed in `buildToolsList()` with per-arg descriptions. - Listed in `isHeavyTool()` so it runs through the deferred execution path (Qt event loop, not re-entrant from MCP-stdin processing). - Sentry breadcrumbs: `ai.tool_call` on entry, then `file.import` before the mesh load and `file.export` before the bake. ### Tests (11 new, in `VATBakerController_test.cpp`) Standalone (no Ogre): - `InstanceIsSingleton` - `BakeRefusedWhenNothingSelected` (error path) - `BakeRefusesEmptyAnimName` - `BakeRefusesEmptyOutputDir` - `AvailableAnimationsEmptyWithoutSelection` Scene fixture: - `RefreshAnimationsFindsSelectionAnim` - `BakeKicksOffAndEmitsFinished` — happy path + isBaking transitions - `BakeReportsErrorForMissingAnim` - `EncodingAndTargetStringsRoutedThrough` — confirms the rgba16 + godot path produces both the 16-bit PNG and the `.gdshader` template - `AvailableAnimationsRefreshesOnSelectionChange` 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 (6)
📝 WalkthroughWalkthroughThis PR introduces a complete VAT baking workflow by adding a QML singleton controller ( ChangesVAT Baking Integration
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 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: 104682ea5c
ℹ️ 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
+4123
to
+4128
| auto& entities = Manager::getSingleton()->getEntities(); | ||
| Ogre::Entity* entity = nullptr; | ||
| for (auto* obj : entities) { | ||
| if (obj && obj->getMovableType() == "Entity") { | ||
| entity = static_cast<Ogre::Entity*>(obj); | ||
| break; |
There was a problem hiding this comment.
After importing file, the tool iterates Manager::getEntities() and bails out on the first entity in the scene graph rather than the entity that was just imported. If the editor already has entities loaded (or the import creates multiple entities), bake_vat can bake the wrong mesh or fail with an animation-not-found error even though the requested file/clip is valid.
Useful? React with 👍 / 👎.
| // Load via MeshImporterExporter (same path the CLI subcommand uses). | ||
| SentryReporter::addBreadcrumb("file.import", | ||
| QString("Importing %1 for VAT bake").arg(filePath)); | ||
| MeshImporterExporter::importer({filePath}); |
There was a problem hiding this comment.
This import path adds scene nodes/entities to the live Manager state, but toolBakeVat has no cleanup on success or failure. In a long-lived MCP session, repeated calls will accumulate imported meshes, increasing memory usage and contaminating subsequent tool behavior (including which entity is selected by later scans).
Useful? React with 👍 / 👎.
Two findings from Codex on PR #568, both real: P1 — `toolBakeVat` picked the first entity from `Manager::getEntities()` after import, but the editor may already have entities loaded when MCP runs alongside the GUI. That meant `bake_vat` could bake the wrong mesh or fail with "animation not found" even though the requested file/clip was valid. Snapshot `getEntities()` *before* the import and pick only the entities that newly appeared. Same pattern `toolOptimizeMesh` already uses. P2 — `toolBakeVat` didn't clean up the entities it imported, so in a long-lived MCP session every bake accumulated scene state (memory + selection contamination). Add an RAII `ImportCleanup` struct that destroys the imported scene nodes on every exit path. Again same pattern as `toolOptimizeMesh`. Also wraps the importer call in try/catch so a malformed input file surfaces as a clean MCP error instead of propagating an exception out of the tool handler. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/VATBakerController.h (1)
31-71: ⚡ Quick winAlign the header contract with the actual synchronous bake flow.
The public comments still describe worker-thread orchestration and “no
bakeFinishedon start failure”, but the current implementation is synchronous and emitsbakeFinishedfor most validation failures. Please update the contract text so QML callers don’t build the wrong assumptions.💡 Suggested comment update
- * - `bake(...)` kicks off the bake on a worker QThread so the UI - * doesn't freeze on long anims. Progress is emitted as + * - `bake(...)` runs bake synchronously in this slice. Progress is emitted as * `bakeProgress(int done, int total)` after each frame; the * terminal signal is `bakeFinished(bool ok, QString posTexture, * QString error)`. @@ - /// Start a bake on a worker thread. Returns false synchronously if - /// the bake can't even start (no selection, no animation, bake - /// already in progress); in that case `bakeFinished` is NOT - /// emitted. Returns true if the worker was kicked off; the result - /// arrives via `bakeFinished`. + /// Start a bake synchronously. Returns false if validation fails. + /// Validation failures may emit `bakeFinished(false, ...)` depending + /// on the failure path; successful start emits final result via + /// `bakeFinished(...)` before returning.🤖 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/VATBakerController.h` around lines 31 - 71, The header comment for VATBakerController is out of sync: it says bake(...) runs on a worker QThread and that bakeFinished is not emitted on synchronous validation failures, but the implementation runs synchronously and does emit bakeFinished for most validation failures. Update the class-level documentation (around VATBakerController, and the Q_INVOKABLE refreshAnimations / bake contract text) to state that the current bake flow is synchronous for validation/error cases, that bakeFinished(bool ok, QString posTexture, QString error) may be emitted even when the bake fails to start, and clarify the true meaning of isBaking, availableAnimations, progressDone/progressTotal and when bakeProgress/bakeFinished are emitted so QML callers handle both sync and async outcomes correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 549-759: The VAT subgroup Rectangle (containing id vatCol and
bakeMa) is currently nested inside a fixed-height Row (the small 26px toolbar
Row) which causes horizontal overflow and vertical overlap; move the entire
Rectangle (the VAT block starting with "Rectangle { width: parent.width - 16 ...
}" that contains property vatCol and MouseArea bakeMa) out of that fixed-height
Row and place it as a sibling (e.g., directly inside the parent
Column/container) so it can size vertically normally and not be constrained by
the toolbar Row; ensure any anchors referencing the old Row are updated to use
the new sibling parent so enabled/disabled logic (VATBakerController.isBaking,
vatCol.outputDir, animPicker, bakeMa) continues to work.
In `@src/MCPServer.cpp`:
- Around line 4124-4168: Delta tracking and subsequent bake target selection are
fragile because Manager::getEntities() can return non-Entity movables and you
unconditionally cast and use imported.first() as the bake target; change
discovery to filter safely by checking obj->getMovableType()=="Entity" before
casting to Ogre::Entity*, build the imported list only from those safe-cast
entities (avoid relying on raw getEntities() entries that might be ManualObject
or other types), then choose the bake target by scanning that filtered list for
an entity that hasSkeleton() (and preferably also has animation clips if
available) instead of using imported.first(); update any references around
MeshImporterExporter::importer, the imported list construction, and the bake
site that currently uses entity/hasSkeleton() so the code skips non-Entity
objects and selects a valid skeletoned entity or returns an explicit error if
none found.
In `@src/MCPServer.h`:
- Around line 206-210: The MCP interface was extended by adding the
toolBakeVat(const QJsonObject &args) method, so update the MCP protocol version
constant SERVER_VERSION in MCPServer.h to a new appropriate semantic version
(e.g., bump patch or minor such as "1.7.1" or "1.8.0") so clients can detect the
capability change; find the SERVER_VERSION declaration in the same header
(symbol SERVER_VERSION) and update its string literal and any related
comments/documentation mentioning the MCP protocol version to match the new
value.
In `@src/VATBakerController_test.cpp`:
- Around line 89-93: The SetUp method in VATBakerControllerSceneTest currently
asserts tryInitOgre() but doesn't assert mesh-loading capability; add an
assertion ASSERT_TRUE(canLoadMeshFiles()) inside
VATBakerControllerSceneTest::SetUp() (alongside the existing
ASSERT_TRUE(tryInitOgre()) and before clearSelection()) so failures to load mesh
fixtures fail loudly in CI; locate the SetUp implementation to insert the call
to canLoadMeshFiles().
In `@src/VATBakerController.cpp`:
- Around line 100-123: The validation branches in bake() (checks for
animationName, outputDir, SelectionSet::getSingleton(), empty entities, and
entity->hasSkeleton()) currently emit bakeFinished and return without telemetry;
add a Sentry breadcrumb before each early exit by calling
SentryReporter::addBreadcrumb with a consistent category (e.g.,
"bake.validation") and a clear message describing the failure (e.g., "missing
animationName", "missing outputDir", "no SelectionSet", "no entity selected",
"selected entity has no skeleton") immediately before each emit
bakeFinished(false, ...) so all user-facing validation failures are tracked.
- Around line 154-157: Reset m_progressDone and m_progressTotal before emitting
the initial bakeProgress signal: set m_progressDone = 0 and m_progressTotal = 0
prior to calling emit bakeProgress(0, /*totalSentinel*/0) so QML property-bound
listeners never observe stale values; update the sequence in VATBakerController
(referencing m_progressDone, m_progressTotal, and bakeProgress) accordingly.
---
Nitpick comments:
In `@src/VATBakerController.h`:
- Around line 31-71: The header comment for VATBakerController is out of sync:
it says bake(...) runs on a worker QThread and that bakeFinished is not emitted
on synchronous validation failures, but the implementation runs synchronously
and does emit bakeFinished for most validation failures. Update the class-level
documentation (around VATBakerController, and the Q_INVOKABLE refreshAnimations
/ bake contract text) to state that the current bake flow is synchronous for
validation/error cases, that bakeFinished(bool ok, QString posTexture, QString
error) may be emitted even when the bake fails to start, and clarify the true
meaning of isBaking, availableAnimations, progressDone/progressTotal and when
bakeProgress/bakeFinished are emitted so QML callers handle both sync and async
outcomes correctly.
🪄 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: fb7ffb3d-bbc9-4a64-bf0b-9cdb141dd5b6
📒 Files selected for processing (9)
qml/PropertiesPanel.qmlsrc/CMakeLists.txtsrc/MCPServer.cppsrc/MCPServer.hsrc/VATBakerController.cppsrc/VATBakerController.hsrc/VATBakerController_test.cppsrc/mainwindow.cpptests/CMakeLists.txt
Six findings from CodeRabbit on PR #568 (against commit c9b1c97). All real: **Critical (MCP)** — `toolBakeVat` used `Manager::getEntities()` for both delta tracking and bake-target selection. That getter returns raw `MovableObject*` which would crash if non-Entity attachments (gizmos, manual objects, mask overlays, ring objects) were in the scene, and bailing on `imported.first()` could return a false "no skeleton" when one of the *other* imported entities had a valid one. Replace with a `collectEntitiesSafe()` helper that walks scene nodes and filters by `getMovableType() == "Entity"`, then pick the bake target by scanning for `hasSkeleton()` rather than list order. **Major (QML)** — the VAT subgroup was a child of the 26 px horizontal `Row` that hosts the Merge Animations button, so the panel overflowed horizontally and overlapped the next scene-tree row vertically. Wrap the merge `Row` + the VAT `Rectangle` in a shared `Column` so each lays out vertically. **Major (test fixture)** — `VATBakerControllerSceneTest::SetUp()` only asserted `tryInitOgre()`; add `canLoadMeshFiles()` so an incomplete CI environment fails loudly at setup rather than later in the first test step. **Major (telemetry)** — `bake()`'s validation-failure exit paths emitted `bakeFinished(false, …)` but no Sentry breadcrumb, leaving gaps in the `ui.action` flow. Add a `"VAT bake refused: <reason>"` breadcrumb at each refuse site (empty anim name, empty outputDir, missing SelectionSet, no entity selected, no skeleton). **Minor (MCP version)** — `bake_vat` extends the MCP tool surface; bump `SERVER_VERSION` from 1.7.0 → 1.8.0 so clients can detect the new capability. **Minor (signal race)** — `bake()` emitted the initial `bakeProgress(0, 0)` *before* resetting the bound member variables. QML listeners bound to the `progressDone` / `progressTotal` properties could read stale values. Reset members first, then emit. **Nit (header docs)** — the class docstring still described the bake as running on a worker QThread and said `bakeFinished` is not emitted on validation failures. Both are false in the slice 4 implementation. Rewrite the contract block + the per-method comment to match the actual synchronous flow so QML callers don't build wrong assumptions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced May 19, 2026
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.
Closes the final piece of #371 (parent epic: #517 / Slice E). Surfaces the existing
VATBakervia the two user-facing channels QtMeshEditor expects: the QML Inspector and the MCP server.What ships
VATBakerController(QML_SINGLETON)QObject wrapper around
VATBaker::bake():Inspector UI
New "Bake VAT" subgroup inside the Animations section in `qml/PropertiesPanel.qml`. Anim picker, FPS, encoding/target dropdowns, normals checkbox, output dir field, Bake button + status line.
MCP `bake_vat` tool
`MCPServer::toolBakeVat(args)` mirrors the CLI subcommand: `file`, `anim`, `fps`, `encoding`, `target`, `normals`, `output_dir`, `basename`. Returns texture paths + bounds + frame/vertex counts. Registered in `toolHandlers()`, listed in `buildToolsList()` with full descriptions, marked as `isHeavyTool()` so it runs through the deferred execution path. Sentry breadcrumbs (`ai.tool_call` + `file.import` + `file.export`).
11 new tests
Manual smoke
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit