feat(vat): --include-shaders flag + Inspector checkboxes - #649
Merged
Conversation
Adds an opt-in step at the end of a VAT bake that drops the
matching drop-in shader templates next to the bake — so a
downstream consumer has the bake (PNG, sidecar, glTF, bind file)
AND the engine glue code together in one folder.
CLI surface:
qtmesh vat <file> --anim <name> --include-shaders <list>
where <list> is a comma-separated subset of {godot, unity, unreal},
or "all". Unknown tokens are silently dropped with a warning.
Inspector surface:
A new "Include shader" checkbox in the VAT panel, followed by
three per-engine sub-checkboxes (Godot / Unity / Unreal) that
only appear when the master toggle is on. Godot defaults to
on; the others off. The state lives on the QML side and is
passed to VATBakerController.bake() as a QStringList.
Mechanics:
- `tools/vat-shaders/vat_shaders.qrc` registers the three
engine shaders and the bundled README as Qt resources under
`:/vat-shaders/`. Linked into the executable so the templates
ship inside the binary — no filesystem lookup needed when
invoked from an installed `.deb` / `.app` / `.exe`.
- `VATShaderEmitter` (pure-data helper): parses the CLI list
string, copies the requested resources to `outputDir`, and
drops a small `OpenVAT_README.md` alongside when at least
one engine was emitted. Idempotent overwrite.
- Both the CLI (`CLIPipeline::cmdVat`) and GUI controller
(`VATBakerController::bake`) route through the same emitter,
so the JSON report's new `shaders` array, the text report's
`shader:` lines, and the Inspector's bake output stay in
sync.
Docs:
- `qtmesh vat` reference section gains `--include-shaders <list>`
with two new example invocations.
- Home-page CLI panel "VAT" tab demonstrates the flag.
|
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 (5)
📝 WalkthroughWalkthroughThe PR extends the VAT (Vertex Animation Texture) exporter to optionally generate engine-specific shader template files during bake. A new ChangesShader Emission for VAT Exporter
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 190365fa72
ℹ️ 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
+5948
to
+5951
| if (engines.isEmpty()) { | ||
| err() << "Warning: --include-shaders=\"" << includeShadersArg | ||
| << "\" did not match any known engine " | ||
| "(accepted: godot, unity, unreal, all)." << Qt::endl; |
There was a problem hiding this comment.
Only the engines.isEmpty() path emits a warning, so inputs like --include-shaders godot,blender silently drop blender and still succeed. In that case users can believe all requested templates were emitted, but one engine is missing and the problem is only discovered later during integration. Please detect and warn when any token is invalid, not just when all tokens are invalid.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/CLIPipeline.cpp`:
- Around line 5704-5710: The argument parsing currently skips
`--include-shaders` if no following token exists; update the parsing loop where
`arg`, `i`, and `argc` are used (the block that sets includeShadersArg) to
detect a missing value for `--include-shaders` and handle it as an error: when
`arg == "--include-shaders"` and `i + 1 >= argc`, emit a clear error message
(including the flag name) and abort/exit with non-zero status (or return an
error code) instead of silently continuing; otherwise keep the existing behavior
of assigning includeShadersArg = QString(argv[++i]).
- Around line 5943-5964: The current branch handling for includeShadersArg only
adds a Sentry breadcrumb on success; add breadcrumbs for the two failure
branches so invalid-engine and write-failure outcomes are tracked: when
VATShaderEmitter::parseEngineList(includeShadersArg) returns empty, call
SentryReporter::addBreadcrumb("file.export", QStringLiteral("VAT shaders:
invalid engine list: %1").arg(includeShadersArg)) alongside the existing err()
warning; when VATShaderEmitter::writeShaders(outDir, engines) returns an empty
list, add SentryReporter::addBreadcrumb("file.export", QStringLiteral("VAT
shaders: no files written for engines:
%1").arg(engines.join(QStringLiteral(",")))) alongside the existing err()
warning so both failure paths are recorded.
In `@src/VATBakerController.cpp`:
- Around line 179-187: When users requested shaders (includeShadersFor not
empty) we currently only breadcrumb the success case; update VATBakerController
so after calling VATShaderEmitter::writeShaders(outputDir, includeShadersFor)
you always emit a SentryReporter::addBreadcrumb: if shadersWritten.isEmpty() add
a "file.export" breadcrumb indicating shader write failed/no files written and
include the original includeShadersFor list; if shadersWritten.size() <
includeShadersFor.size() add a "file.export" breadcrumb indicating a partial
write that lists shadersWritten and the missing entries (requested minus
written); also ensure you add a breadcrumb when result.ok is false to record the
overall bake/export failure using SentryReporter::addBreadcrumb so all
failure/partial paths are tracked.
In `@src/VATShaderEmitter.cpp`:
- Around line 66-94: parseEngineList currently appends valid explicit tokens in
input order but must return a stable canonical order defined by kSpecs; change
the function to record valid engines into the seen set (use findSpec to validate
and still handle the "all" branch by inserting all kSpecs into seen) and defer
building out until the end by iterating kSpecs and calling pushIfFresh/append
for each spec whose engine is present in seen, so the final returned QStringList
follows the canonical kSpecs order while keeping uniqueness (references:
parseEngineList, kSpecs, findSpec, pushIfFresh, tokens).
🪄 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: b8858717-0b22-42cc-9e16-a119284fdaa9
📒 Files selected for processing (10)
qml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/CMakeLists.txtsrc/VATBakerController.cppsrc/VATBakerController.hsrc/VATShaderEmitter.cppsrc/VATShaderEmitter.htools/vat-shaders/vat_shaders.qrcwebsite/src/DocsApp.jsxwebsite/src/data/content.js
1) parseEngineList — surface rejected unknown tokens (Codex P2) parseEngineList now takes an optional `QStringList* rejectedOut` that the CLI fills and reports separately from the "no valid engines at all" branch. Previously `--include-shaders godot,blender` silently dropped "blender" and succeeded with just the Godot template, leaving the user to discover the missing engine during integration. 2) Missing value for --include-shaders is now an error (CodeRabbit Major) Bare `qtmesh vat ... --include-shaders` used to skip the flag silently if no value followed. Now it exits 2 with an error message naming the accepted values. 3) Breadcrumbs on every shader-emission outcome (CodeRabbit Major + Minor) Both the CLI (CLIPipeline::cmdVat) and the GUI controller (VATBakerController::bake) now emit a `file.export` breadcrumb on the no-valid-engines, write-failed-for-all-requested, AND success branches. The previous code only logged success, which left Sentry blind to "user asked for shaders, got nothing." 4) Canonical output order in parseEngineList (CodeRabbit Minor) The API contract said output order was stable as "godot, unity, unreal" but the implementation preserved input order (so `"unity,godot"` returned `["unity", "godot"]`). Fixed by collecting valid tokens into a set, then emitting them by walking the canonical engine spec list. Verified end-to-end with the actual binary: --include-shaders → exit 2, error message --include-shaders godot,blender → warning + writes godot only --include-shaders blender,maya,godot → warning + writes godot only --include-shaders blender,maya → 2 warnings, no shaders written --include-shaders unity,godot → writes in canonical order
CI broke on the previous commit because test binaries link against CLIPipeline.cpp and VATBakerController.cpp, both of which now reference VATShaderEmitter::parseEngineList / writeShaders. The src/CMakeLists.txt already added the new translation unit to TEST_SRC_FILES's sibling list for the main executable, but the test binaries pull from tests/CMakeLists.txt's TEST_SRC_FILES — that list was missed. Adding the .cpp there makes every test executable that links the common test-source bundle find the symbols.
|
5 tasks
This was referenced May 20, 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.
Summary
Adds an opt-in step at the end of a VAT bake that drops the matching drop-in shader templates next to the bake — so a downstream consumer has the bake (PNG, sidecar, glTF, bind file) AND the engine glue code together in one folder, no chasing `tools/vat-shaders/` in the repo.
CLI
```bash
qtmesh vat character.fbx --anim "Dance" --include-shaders godot,unity
```
`` is a comma-separated subset of `{godot, unity, unreal}` (case-insensitive), or `all`. Unknown tokens are silently dropped with a warning.
Inspector
A new "Include shader" master checkbox in the VAT panel, followed by three per-engine sub-checkboxes (Godot / Unity / Unreal) that only appear when the master toggle is on. Godot defaults to on (the engine the website demo + most early users target); the others off. The state lives on the QML side and is passed to `VATBakerController.bake()` as a `QStringList`.
Mechanics
Docs
Test plan
Related
Follows the v3.3.0 VAT MVP release (PR #648).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--include-shaders {godot,unity,unreal,all}for command-line VAT export.Documentation