feat(vat): slice 2 — RGBA16 encoding + normal texture - #566
Merged
Conversation
Closes more of #371. EXR is deferred (TinyEXR vendoring needs explicit authorization); follow-up PR will add it. ### What's new in slice 2 - **`Encoding::RGBA16`** — 16-bit per channel, written as a Qt `Format_RGBA64` PNG. Same channel layout as RGBA8, alpha reserved at 65535. Round-trip error drops from ~3 mm (RGBA8 over ±1 m bounds) to ~50 µm. - **`Options::bakeNormals`** — when true, samples post-skin normals via `VES_NORMAL` and writes `<basename>_nrm.png` next to the position texture. Same row-per-frame / column-per-vertex layout. Normals are mapped from [-1, 1] → [0, MAX] per channel; the unpacker just runs the inverse map (no re-normalisation, leaves that choice to the runtime shader). - **Sidecar additions** — `encoding` field now reflects the chosen encoding; `nrmTexture` filename appears when normals were baked. - **CLI flags** — `--encoding rgba8|rgba16` (default rgba8) and `--normals` (no-arg flag). The output banner reports the chosen encoding plus the normal-texture path when applicable. ### Tests (9 new) - `EncodeRGBA16RoundTrip` — clean round-trip at 1e-3 tolerance. - `EncodeRGBA16BeatsRGBA8OnSamePoints` — quantitative precision comparison; 16-bit must be >10× better than 8-bit on the same inputs. - `EncodeNormalsRGBA8RoundTrip` — basis vectors round-trip within ~2/255. - `EncodeNormalsRGBA8ClampsOutOfRange` — out-of-[-1,1] inputs clamp to 0/255 rather than wrap. - `EncodeNormalsRGBA16RoundTrip` — same at 16-bit precision. - `EncodeNormalsReturnsEmptyOnMismatchedSize` — defensive sizing. - `SidecarMentionsEncodingAndOptionalNrmTexture` — sidecar snapshot. - `BakeWithRGBA16WritesSixteenBitPng` — e2e check that the PNG comes back as `QImage::Format_RGBA64`. - `BakeWithNormalsWritesNormalTexture` — e2e check on size + sidecar. ### Manual smoke ``` $ ./build_local/bin/qtmesh vat media/models/robot.mesh \ --anim Idle --fps 20 --encoding rgba16 --normals -o /tmp/vat Baked VAT for 'Idle' (57 frames × 295 vertices, rgba16) position texture: /tmp/vat/Idle_pos.png normal texture: /tmp/vat/Idle_nrm.png sidecar: /tmp/vat/Idle.json ``` 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 (2)
📝 WalkthroughWalkthroughThis PR adds RGBA16 texture encoding and optional normal-map baking to the VAT baker system. It extends the API with encoding selection and normal-baking options, implements 16-bit position and normal encoders/decoders, integrates these features into the baking pipeline with conditional texture output, exposes the new capabilities through CLI arguments, and validates all changes with comprehensive unit and end-to-end tests. ChangesVAT Texture Encoding and Normal-Map Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline.cpp (1)
4826-4829: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd the standard import/export breadcrumbs to
cmdVat.This new CLI path records
cli.vat, but it also performs an asset import and a multi-file export. Please addfile.importbeforeMeshImporterExporter::importer(...)andfile.exportbeforeVATBaker::bake(...)so VAT runs show up in the same telemetry flow as the other commands here.As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use 'ui.action' for toolbar/menu clicks, 'ai.tool_call' for MCP tool invocations, 'file.import' / 'file.export' for I/O operations."Suggested patch
SentryReporter::addBreadcrumb("cli.vat", QString("VAT bake .%1 anim=%2 fps=%3").arg(fi.suffix(), animName).arg(fps)); + SentryReporter::addBreadcrumb("file.import", + QString("Importing %1").arg(fi.absoluteFilePath())); MeshImporterExporter::importer({fi.absoluteFilePath()}); @@ opts.encoding = encoding; opts.bakeNormals = bakeNormals; + SentryReporter::addBreadcrumb("file.export", + QString("Writing VAT outputs to %1").arg(QDir(outDir).absolutePath())); VATBaker::BakeResult result = VATBaker::bake(entity, opts);Also applies to: 4850-4858
🤖 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/CLIPipeline.cpp` around lines 4826 - 4829, Add Sentry breadcrumbs for import/export around the VAT CLI flow: insert SentryReporter::addBreadcrumb("file.import", ...) immediately before the MeshImporterExporter::importer({fi.absoluteFilePath()}) call (include the filename/path in the message), and insert SentryReporter::addBreadcrumb("file.export", ...) immediately before the VATBaker::bake(...) call (include target VAT filename/anim/fps context in the message); repeat the same pattern for the related VAT bake/export block around the VATBaker::bake invocation found elsewhere in this file so both importer and baker steps are tracked in the same telemetry flow.
🤖 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/VATBaker.cpp`:
- Around line 144-153: The current branch silently pads missing normals (when
normElem is null) by emplacing up-vectors (out.emplace_back(0.0f, 1.0f, 0.0f))
for animData->vertexCount and marking sharedAppended/sub->useSharedVertices,
which yields incorrect results when --normals is requested; change this to
either 1) abort the bake with a clear error when normElem is missing
(throw/return an error or log and exit) so callers know normals are absent, or
2) explicitly recompute normals for the submesh before writing (call your
normal-recalculation routine and append those values instead of placeholders);
update the code paths that reference normElem, out.emplace_back, appended,
sub->useSharedVertices and sharedAppended so they no longer write fabricated
up-vectors when --normals is enabled.
- Around line 538-569: The file VATBaker.cpp uses std::memcpy in the functions
that write the QImage rows after encodeRGBA8 and encodeRGBA16 (see usages near
the loops that copy into img.scanLine(y)), but it doesn't include <cstring>; add
the header `#include` <cstring> at the top of VATBaker.cpp so std::memcpy is
declared (this ensures portability across strict toolchains while leaving the
encodeRGBA8/encodeRGBA16, QImage, and related logic unchanged).
---
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 4826-4829: Add Sentry breadcrumbs for import/export around the VAT
CLI flow: insert SentryReporter::addBreadcrumb("file.import", ...) immediately
before the MeshImporterExporter::importer({fi.absoluteFilePath()}) call (include
the filename/path in the message), and insert
SentryReporter::addBreadcrumb("file.export", ...) immediately before the
VATBaker::bake(...) call (include target VAT filename/anim/fps context in the
message); repeat the same pattern for the related VAT bake/export block around
the VATBaker::bake invocation found elsewhere in this file so both importer and
baker steps are tracked in the same telemetry flow.
🪄 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: 21694506-2bd0-43db-89f8-8102071cb958
📒 Files selected for processing (4)
src/CLIPipeline.cppsrc/VATBaker.cppsrc/VATBaker.hsrc/VATBaker_test.cpp
Two findings from CodeRabbit on PR #566: 1. **Critical** — `std::memcpy` was used without an explicit `#include <cstring>`. Works today through transitive Qt includes but fails on stricter toolchains. Add the header explicitly. 2. **Major** — `collectPostSkinNormals` silently padded missing `VES_NORMAL` submeshes with (0, 1, 0) up-vectors, producing a plausible-looking normal texture that lights the mesh wrong while reporting bake success. That's strictly worse than refusing to bake. Change the helper to return a sentinel (`SIZE_MAX`) when a submesh lacks `VES_NORMAL`. `bake()` catches the sentinel and surfaces a clear error: "frame N has a submesh without VES_NORMAL — cannot bake normals (regenerate normals on the source mesh or omit --normals)". Meshes that already have normals are unaffected — `robot.mesh` + `--normals` still bakes successfully in the smoke test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CodeRabbit follow-up: per the project's coding guideline that "file.import / file.export breadcrumbs are required for I/O operations", surface the VAT CLI's import + export steps so VAT runs appear in the same Sentry telemetry flow as the other subcommands (cmdConvert, cmdFix, etc.). - `file.import` before MeshImporterExporter::importer() with the absolute input path. - `file.export` before VATBaker::bake() with the output dir, anim name, encoding, and normals flag for grep-ability in Sentry. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
This was referenced May 17, 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 more of #371. Slice 2 of 4. EXR (
Encoding::RGBAF) is deferred — vendoring TinyEXR needs explicit authorization, so it'll come in its own follow-up PR.What ships in slice 2
Encoding::RGBA16— 16-bit per channel, written as a QtFormat_RGBA64PNG. Same channel layout as RGBA8, alpha reserved at 65535. Round-trip error drops from ~3 mm (RGBA8 over ±1 m bounds) to ~50 µm.Options::bakeNormals— when true, samples post-skin normals viaVES_NORMALand writes<basename>_nrm.pngnext to the position texture. Same row-per-frame layout. Normals are mapped from [-1, 1] → [0, MAX] per channel; the unpacker just runs the inverse map (no re-normalisation, leaves that choice to the runtime shader).encodingfield reflects the chosen encoding;nrmTexturefilename appears when normals were baked.--encoding rgba8|rgba16(defaultrgba8) and--normals.9 new tests
QImage::Format_RGBA64Manual smoke
```
$ ./build_local/bin/qtmesh vat media/models/robot.mesh \
--anim Idle --fps 20 --encoding rgba16 --normals -o /tmp/vat
Baked VAT for 'Idle' (57 frames × 295 vertices, rgba16)
position texture: /tmp/vat/Idle_pos.png
normal texture: /tmp/vat/Idle_nrm.png
sidecar: /tmp/vat/Idle.json
```
Test plan
VATBakerStandalone.*andVATBakerEndToEndTest.*pass (10 → 19 tests).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
--encodingoption to select output texture format (RGBA8 or RGBA16)--normalsflag to enable normal texture generationTests