vat: collapse to OpenVAT, drop per-engine targets - #620
Merged
Conversation
|
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 ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR converts VAT baking to an OpenVAT-only pipeline: it simplifies VATBaker public types, implements a 16-bit packed position+normal PNG and an os-remap sidecar, enforces normals and outward-rounded bounds, updates CLI/MCP/controller/QML integrations to the reduced contract, expands tests, and adapts Godot and Unity test harnesses to load the OpenVAT outputs. ChangesOpenVAT-only VAT API and encoder core
Integration layers (CLI, MCP, QML, Controller)
Test coverage updates
Godot 4 OpenVAT test harness
Unity 2022 OpenVAT test harness
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 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: 14e6deae8b
ℹ️ 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".
| .arg(result.posTexPath); | ||
| return result; | ||
| } | ||
| auto packed = packOpenVAT16(flat, normals, frameCount, vertexCount, lo, hi); |
There was a problem hiding this comment.
For any animation whose sampled min/max is not already exactly on the 0.1 OpenVAT grid, this packs the PNG using the exact lo/hi while the JSON sidecar later reports rounded-outward bounds. Consumers decode the texture with the sidecar os-remap values, so a vertex at the true max (for example 1.03) is written as 65535 and then decoded as the rounded max (1.10), scaling/offsetting the whole animation. The packer and sidecar need to use the same rounded bounds.
Useful? React with 👍 / 👎.
Comment on lines
+140
to
+144
| # Required fields per QtMeshEditor VAT v1 sidecar contract. | ||
| for required_key in ["frameCount", "vertexCount", "fps", "bounds", "posTexture"]: | ||
| if not sidecar.has(required_key): | ||
| push_error("VATPlayer: sidecar missing '%s' key" % required_key) | ||
| return false |
There was a problem hiding this comment.
The staging script now calls qtmesh vat, which emits <basename>-remap_info.json containing only os-remap plus _origin, but this loader still requires the old QtMeshEditor keys (frameCount, vertexCount, fps, bounds, posTexture). As a result, every freshly staged OpenVAT bake fails here with “sidecar missing 'frameCount'” before the texture is loaded, so the Godot verification project cannot replay the output this commit generates.
Useful? React with 👍 / 👎.
Comment on lines
+117
to
+119
| _sidecar = JsonUtility.FromJson<VATSidecar>(sidecarJson.text); | ||
| if (_sidecar == null || _sidecar.frameCount <= 0 || _sidecar.vertexCount <= 0) { | ||
| Debug.LogError($"[VATPlayer] {name}: malformed sidecar JSON"); |
There was a problem hiding this comment.
Fresh bakes staged by tools/unity-vat-test/bake_and_stage.sh now contain the OpenVAT os-remap JSON, not the legacy fields modeled by VATSidecar. JsonUtility will therefore leave frameCount/vertexCount at 0 and bounds null, causing RebuildMaterials() to reject the bake as malformed (or fail later if the guard changes), so the Unity test project cannot load the output produced by this commit.
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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/VATBaker_test.cpp (1)
95-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd mesh-file prerequisite assertion in fixture setup.
VATBakerEndToEndTest::SetUp()should also assertcanLoadMeshFiles()so Ogre-dependent failures abort early with a clear cause.Suggested patch
void SetUp() override { ASSERT_TRUE(tryInitOgre()) << "Ogre init required"; + ASSERT_TRUE(canLoadMeshFiles()) << "Mesh file loading required"; }Based on learnings: "fixture SetUp must fail loudly in CI by using
ASSERT_TRUE(tryInitOgre())andASSERT_TRUE(canLoadMeshFiles())."🤖 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/VATBaker_test.cpp` around lines 95 - 98, In VATBakerEndToEndTest::SetUp() add an assertion for mesh-file availability: after the existing ASSERT_TRUE(tryInitOgre()) << "Ogre init required"; call ASSERT_TRUE(canLoadMeshFiles()) << "Mesh files required"; so the fixture fails loudly when mesh assets are missing; update the SetUp override to call canLoadMeshFiles() alongside tryInitOgre().src/MCPServer.cpp (1)
4262-4277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn the OpenVAT artifact metadata as structured fields, not a JSON string.
This currently hides
texture,sidecar,frameCount,vertexCount,animation,fps, andboundsinsidecontent[0].text, so MCP/HTTP clients lose the machine-readablebake_vatcontract and would have to parse pretty-printed text. Other structured tools in this file expose their payload at the top level alongsidecontent;bake_vatshould do the same.Proposed fix
- QJsonObject content; - content["ok"] = true; - content["texture"] = result.posTexPath; - content["sidecar"] = result.jsonPath; - content["frameCount"] = result.frameCount; - content["vertexCount"] = result.vertexCount; - content["animation"] = animName; - content["fps"] = fps; + QJsonObject payload; + payload["ok"] = true; + payload["texture"] = result.posTexPath; + payload["sidecar"] = result.jsonPath; + payload["frameCount"] = result.frameCount; + payload["vertexCount"] = result.vertexCount; + payload["animation"] = animName; + payload["fps"] = fps; QJsonObject bounds, lo, hi; lo["x"] = result.minBound.x; lo["y"] = result.minBound.y; lo["z"] = result.minBound.z; hi["x"] = result.maxBound.x; hi["y"] = result.maxBound.y; hi["z"] = result.maxBound.z; bounds["min"] = lo; bounds["max"] = hi; - content["bounds"] = bounds; - - return makeSuccessResult( - QString::fromUtf8(QJsonDocument(content).toJson(QJsonDocument::Indented))); + payload["bounds"] = bounds; + + QJsonObject response = makeSuccessResult( + QString("Baked OpenVAT '%1' (%2 frames × %3 vertices)") + .arg(animName) + .arg(result.frameCount) + .arg(result.vertexCount)); + for (auto it = payload.begin(); it != payload.end(); ++it) + response.insert(it.key(), it.value()); + return response;🤖 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/MCPServer.cpp` around lines 4262 - 4277, The code is embedding the bake_vat metadata as a pretty-printed JSON string inside the response instead of returning structured fields; update the handler to return the metadata as top-level JSON members. Replace the current QJsonObject content -> QJsonDocument(...).toJson(QString) usage by constructing a top-level QJsonObject (or reuse content) that sets "ok", "texture" (result.posTexPath), "sidecar" (result.jsonPath), "frameCount" (result.frameCount), "vertexCount" (result.vertexCount), "animation" (animName), "fps" (fps) and "bounds" (with min/max using result.minBound/result.maxBound) and pass that structured object into makeSuccessResult so clients receive machine-readable fields instead of a pretty-printed text blob; keep references to makeSuccessResult, content, result, animName and fps to locate the change.
🤖 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_test.cpp`:
- Around line 306-314: The test currently uses a hardcoded outputDir "/tmp";
replace this with a QTemporaryDir instance, assert the temp dir is valid (e.g.,
Q_ASSERT(temp.isValid()) or EXPECT_TRUE(temp.isValid())), set opts.outputDir to
the temporary directory path (use temp.path()), and then call
VATBaker::bake(entity, opts) as before; keep references to VATBaker::Options,
opts.outputDir, and VATBaker::bake so the change is localized and the temporary
directory is automatically cleaned up when the QTemporaryDir goes out of scope.
- Around line 173-176: The test compares adjacent rows using QImage::pixel(),
which quantizes 16-bit Format_RGBX64 data to 8-bit and can hide differences;
change the comparison to use QImage::constScanLine(row) and cast the returned
pointer to QRgba64* to compare full 16-bit pixels directly (replace uses of
png.pixel(col, row) / png.pixel(col, row + 1) in the nested loop that sets
foundDifference with 16-bit reads from constScanLine for rows `row` and
`row+1`), ensuring you still iterate over png.width() columns and compare the
appropriate QRgba64 elements.
In `@tools/godot-vat-test/bake_and_stage.sh`:
- Around line 40-44: The --fps case in the argument parsing (inside the
while/case block handling --fps and assigning FPS) needs an explicit arity check
to avoid failing under set -u when no value is provided; before assigning
FPS="$2" (in the --fps branch) validate that a second argument exists and is not
another option (e.g., check that $# -ge 2 and "$2" does not start with --), and
if the check fails print a usage/unknown-arg error to stderr and exit with
status 2.
In `@tools/godot-vat-test/README.md`:
- Around line 65-84: The fenced code block in tools/godot-vat-test/README.md is
missing a language identifier (triggering markdownlint MD040); edit the
README.md and change the opening fence from ``` to a language-tagged fence such
as ```text or ```none (or ```bash if you prefer) so the project-layout block is
explicitly labelled, keeping the same block contents and formatting.
- Line 37: Update the README examples and verification text that reference the
old OpenVAT flags/outputs: remove mentions of the deprecated flags (--encoding,
--target) and any references to separate normal outputs like `_nrm.png` or
generated `.gdshader`; instead show the new OpenVAT outputs (`*_pos.png` which
contains packed normals) and the `*-remap_info.json` file. Replace the example
command line shown (the line containing "media/models/Twist Dance.fbx"
"mixamo.com" --encoding rgba8 --fps 24) with the simplified OpenVAT invocation
(omit --encoding/--target) and then update the verification steps that mention
`_nrm.png`, `.gdshader`, and per-texture target flags in the sections called out
(around the example lines and the verification sections) to instruct checking
for the `_pos.png` packed-normal image and the `-remap_info.json` remapping
metadata instead. Ensure all occurrences noted (including the similar blocks at
the other ranges mentioned) are changed consistently.
In `@tools/godot-vat-test/scripts/VATPlayer.gd`:
- Around line 115-168: The _load_bake() parser still expects legacy sidecar keys
(frameCount, bounds, posTexture, optional nrmTexture) which breaks OpenVAT
bakes; update _load_bake() to detect and support the OpenVAT contract by: first
checking for an OpenVAT remap sidecar (e.g. filename containing
"-remap_info.json") or for OpenVAT keys ("Min","Max","Frames") and if present
parse those fields (map Min->bounds.min, Max->bounds.max, Frames->frameCount or
set _frame_count from "Frames"), treat normals as packed into the position
texture (so do not require nrmTexture and drop use of nrmTexture when OpenVAT is
detected), and keep backward compatibility by falling back to the legacy keys
(frameCount, bounds, posTexture, nrmTexture) when OpenVAT keys/sidecar are
absent; update the required-keys check and the pos_texture resolution logic in
_load_bake() accordingly so both formats load correctly.
- Around line 330-350: The v coordinate is computed against the full texture
height, so positions and normals read from the two packed halves collide; change
vertex() to compute separate v_pos and v_nrm that map into the top/bottom halves
of a texture with height = 2 * frame_count (e.g. v_pos = (current_frame + 0.5) /
(2.0 * frame_count) and v_nrm = v_pos + 0.5), then call sample_pos(global_vid,
v_pos) for VERTEX and use v_nrm when sampling nrm_tex for NORMAL (keep the
existing u, negation and normalize logic). Ensure you update uses of v to
v_pos/v_nrm in sample_pos and texture(nrm_tex, ...) respectively.
In `@tools/unity-vat-test/Assets/VAT/Editor/VATTestSetup.cs`:
- Around line 68-83: The file paths returned by Directory.GetFiles use
backslashes on Windows which break AssetDatabase.LoadAssetAtPath; before calling
AssetDatabase.LoadAssetAtPath for posTexPath, nrmTexPath, sidecarPath and
gltfPath, normalize each path by replacing '\' with '/' (e.g. posTexPath =
posTexPath?.Replace('\\','/')), then call AssetDatabase.LoadAssetAtPath as
before; after loading, add explicit null checks for posTex and sidecarTxt
(similar to the existing gltfPrefab check) and show an
EditorUtility.DisplayDialog / return on failure so failures aren’t silent.
In `@tools/unity-vat-test/Assets/VAT/Scripts/VATPlayer.cs`:
- Around line 140-142: The code assumes _sidecar.bounds.min and .max are
non-null arrays of length >=3 before indexing; add a guard in VATPlayer (around
where boundsMin and boundsMax are constructed) that validates _sidecar != null,
_sidecar.bounds != null, and that both _sidecar.bounds.min and
_sidecar.bounds.max are non-null and have Length (or Count) >= 3; if the checks
fail, log or throw a clear, fast-fail error mentioning VATPlayer and the
malformed sidecar (and return/abort initialization) so you never index into
missing/short arrays when creating boundsMin and boundsMax.
- Around line 44-56: The VATSidecar class models a legacy sidecar; update it to
match OpenVAT os-remap schema by replacing frameCount with Frames (int),
removing nrmTexture (normals are packed into posTexture), replacing
bounds/VATSidecarBounds with Min and Max (Vector3 or float[3]) fields to
represent remap bounds, and remove or adapt vertexCount if no longer present in
the new schema; also update any parsing/usage sites that read
VATSidecar.frameCount, VATSidecar.vertexCount, VATSidecar.bounds.*, and
VATSidecar.nrmTexture (including the parser around the other usages noted) to
read VATSidecar.Frames and VATSidecar.Min/Max and to use posTexture for normals.
In `@tools/unity-vat-test/bake_and_stage.sh`:
- Around line 5-7: Update the stale OpenVAT references: replace the comment and
any user-facing output that mentions the old "--target unity" with the new
OpenVAT-only wording, and change any reference to "sidecar.json" to the current
sidecar filename used by OpenVAT; specifically search for the exact strings
"--target unity" and "sidecar.json" in bake_and_stage.sh and update the comment
that describes the staging location (the string
"tools/unity-vat-test/Assets/VAT/Bakes/<basename_anim>/") and any echo/log calls
so they reflect the OpenVAT-only behavior and the correct sidecar filename.
- Around line 31-35: In the while/case parsing block in bake_and_stage.sh, guard
the --fps branch so it validates that a value exists before reading "$2" (to
avoid set -u failing); check that "$2" is present and not another option (e.g.,
starts with '-') and if invalid print a clear usage/error message and exit
non‑zero, otherwise assign FPS="$2" and shift 2 as currently done; update the
case handling for --fps accordingly to perform this validation and error
reporting.
In `@tools/unity-vat-test/Packages/manifest.json`:
- Around line 2-5: Add the official glTFast importer to the Unity package
manifest so .gltf assets can be imported and VATTestSetup can load source.gltf;
update the "dependencies" object in manifest.json to include the package key
"com.unity.cloud.gltfast" with version "6.10.3". Ensure the entry is added
alongside the existing keys ("com.unity.modules.unitywebrequest" and
"com.unity.collab-proxy") inside the same dependencies object and save the
manifest so Unity will restore the package before running VATTestSetup.
In `@tools/unity-vat-test/ProjectSettings/ProjectVersion.txt`:
- Line 2: Replace the placeholder text in the ProjectVersion.txt entry by
updating the m_EditorVersionWithRevision value to include the actual Unity
changeset hash in parentheses (format: "<version> (<changeset-hash>)"); locate
the m_EditorVersionWithRevision line in ProjectVersion.txt and change
"2022.3.40f1 (placeholder — Unity Hub will offer to upgrade on first open)" to
something like "2022.3.40f1 (a1b2c3d4e5f6)" using the real revision hash for
this build so Unity Hub won't trigger unnecessary metadata updates.
In `@tools/unity-vat-test/README.md`:
- Around line 81-83: Remove the stale reference to the removed CLI flag
`--normals` in the README troubleshooting text that mentions the shader
`_HasNrmTex`; update the sentence (the block that currently reads "If you bake
without `--normals`, the shader falls back..." and the follow-up "Re-bake with
`--normals` for proper lighting") to instead state that normals are managed by
the OpenVAT contract (or direct the reader to the OpenVAT baking workflow) and
remove the instruction to re-bake with `--normals`, ensuring `_HasNrmTex`
behavior is still documented accurately.
---
Outside diff comments:
In `@src/MCPServer.cpp`:
- Around line 4262-4277: The code is embedding the bake_vat metadata as a
pretty-printed JSON string inside the response instead of returning structured
fields; update the handler to return the metadata as top-level JSON members.
Replace the current QJsonObject content -> QJsonDocument(...).toJson(QString)
usage by constructing a top-level QJsonObject (or reuse content) that sets "ok",
"texture" (result.posTexPath), "sidecar" (result.jsonPath), "frameCount"
(result.frameCount), "vertexCount" (result.vertexCount), "animation" (animName),
"fps" (fps) and "bounds" (with min/max using result.minBound/result.maxBound)
and pass that structured object into makeSuccessResult so clients receive
machine-readable fields instead of a pretty-printed text blob; keep references
to makeSuccessResult, content, result, animName and fps to locate the change.
In `@src/VATBaker_test.cpp`:
- Around line 95-98: In VATBakerEndToEndTest::SetUp() add an assertion for
mesh-file availability: after the existing ASSERT_TRUE(tryInitOgre()) << "Ogre
init required"; call ASSERT_TRUE(canLoadMeshFiles()) << "Mesh files required";
so the fixture fails loudly when mesh assets are missing; update the SetUp
override to call canLoadMeshFiles() alongside tryInitOgre().
🪄 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: 069a7873-eeff-49c3-8491-055a3e0ffe98
⛔ Files ignored due to path filters (1)
tools/unity-vat-test/Assets/VAT/Shaders/VAT.shaderis excluded by!**/*.shader
📒 Files selected for processing (23)
src/CLIPipeline.cppsrc/MCPServer.cppsrc/VATBaker.cppsrc/VATBaker.hsrc/VATBakerController.cppsrc/VATBakerController.hsrc/VATBakerController_test.cppsrc/VATBaker_test.cpptools/godot-vat-test/.gitignoretools/godot-vat-test/README.mdtools/godot-vat-test/bake_and_stage.shtools/godot-vat-test/project.godottools/godot-vat-test/scenes/Main.tscntools/godot-vat-test/scripts/Main.gdtools/godot-vat-test/scripts/SkeletalLoader.gdtools/godot-vat-test/scripts/VATPlayer.gdtools/unity-vat-test/.gitignoretools/unity-vat-test/Assets/VAT/Editor/VATTestSetup.cstools/unity-vat-test/Assets/VAT/Scripts/VATPlayer.cstools/unity-vat-test/Packages/manifest.jsontools/unity-vat-test/ProjectSettings/ProjectVersion.txttools/unity-vat-test/README.mdtools/unity-vat-test/bake_and_stage.sh
BREAKING: the `qtmesh vat` CLI subcommand, the `bake_vat` MCP tool, and the `VATBakerController::bake()` Qt invokable all dropped their `--encoding`, `--target`, and `--normals` flags. There is only one VAT output format now: OpenVAT (https://github.com/sharpen3d/openvat). No prior consumers exist outside this repo — VAT has not yet been released. ## Why The pre-existing `agnostic` / `unity` / `unreal` / `godot` targets were all variations of "QtMeshEditor's own bake," tailored to per-engine axis conventions and sidecar shapes. None of them produced output that dropped into a stock engine project — each was paired with our own hand-written harness shader. Five targets where really one was needed. OpenVAT is the cross-engine community convention: a Blender add-on bakes, and reference shaders for Godot / Unity / Unreal / Blender all consume the same `os-remap` JSON + packed-normals 16-bit RGB PNG. Real downstream consumers exist; we should emit what they read. ## What changes `VATBaker::Options` loses `target`, `encoding`, `bakeNormals`. The only knobs left are `animationName`, `fps`, `startTime`, `endTime`, `outputDir`, `basename`. `VATBaker::BakeResult` loses `nrmTexPath` (normals are inside `posTexPath` now), `unityMetaPath`, `godotShaderPath`. `VATBaker::Target` and `VATBaker::Encoding` enums deleted. So are the public encode/decode helpers (`encodeRGBA8`/`decodeRGBA8`/etc.) and the `swizzleForTarget` / `targetFlipsRowsAtWriteTime` / `buildUnityMeta` / `buildGodotShader` internal helpers. Output is now: - `<basename>_pos.png` — 16-bit-per-channel PNG, width = vertexCount, height = 2 × frameCount. Rows [0..frameCount) hold positions normalized to the bake's bounds; rows [frameCount..2×frameCount) hold unit normals as `(n+1)/2`. - `<basename>-remap_info.json` — canonical OpenVAT sidecar: { "os-remap": { "Min": ["-1.30000000","-0.10000000","-0.70000000"], "Max": [ "0.70000000", "2.00000000", "0.40000000"], "Frames": 71 }, "_origin": "ogre-y-up-rh" } `Min`/`Max` are stringified 8-decimal-place floats with bounds rounded outward to the nearest 0.1 (matches the Blender add-on's `CustomEncoder` + `round_to_nearest_ten`). `_origin` is an extension key — openvat consumer shaders ignore unknown fields, but our own tooling uses it to know the source coordinate space. The mesh must expose per-vertex normals; bakes without them fail loudly rather than emitting half-populated textures. ## Caller fan-out - CLI: `qtmesh vat <file> --anim <name> [--fps N] [-o <dir>] [--json]` - MCP tool: `bake_vat` schema loses `encoding`, `target`, `normals` - Inspector: `VATBakerController::bake(anim, fps, outputDir, basename)` — 4 args, was 7 Test harnesses (`tools/godot-vat-test/`, `tools/unity-vat-test/`) keep their old bake_and_stage.sh wiring with the legacy CLI flags removed; their runtime shaders (Godot `VATPlayer.gd`, Unity `VAT.shader`) still target the pre-OpenVAT layout and will need a follow-up rewrite to consume the packed-normals texture + os-remap sidecar before the harnesses run again. Not blocking real users — shipped output is now canonical OpenVAT, and any consumer with a working openvat shader will read it directly. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tes + UI move
P1 fix (correctness, decode drift)
----------------------------------
The texture used to be encoded against the raw `lo`/`hi` min/max while
the sidecar published outward-rounded bounds (e.g. -0.616 → -0.700).
A consumer decoding through the sidecar would have systematic drift of
up to one rounding step (~0.05 per axis on a 1-unit model) on every
sample. Now bake() snaps both to `roundedLo`/`roundedHi` once and uses
them everywhere — texture encoder, sidecar emitter, and BakeResult.bounds
all agree to the bit. New test `OpenVATBoundsRoundedOutwardToTenth`
asserts the sidecar Min/Max equal BakeResult.minBound/maxBound exactly.
Sidecar extension key — replaced the made-up `_origin` field with two
non-conflicting underscore-prefixed keys that openvat consumers ignore:
- `_producer: "QtMeshEditor"` — tool identifier
- `_axes: "y-up-rh"` — source coordinate convention so consumer
shaders know what swizzle (if any) to apply on read
Inspector UI move (per user ask)
--------------------------------
Bake VAT controls moved from the Scene tab's Animations subgroup into
a dedicated "VAT" CollapsibleSection in Mode Tools, surfaced when
AnimationMode is active. New `VATBakerController::chooseOutputDir()`
opens a Qt-rendered (non-native) folder picker — matches the
`MaterialEditorQML::openFileDialog` pattern that reliably opens from
inside a QQuickWidget on macOS.
Harness rewrites — OpenVAT-aware Godot + Unity
----------------------------------------------
Godot `VATPlayer.gd`:
- Reads `<basename>-remap_info.json` instead of the legacy QtMeshEditor
v1 sidecar.
- Parses `os-remap.{Min, Max, Frames}` (stringified arrays).
- Embedded shader samples one packed texture at two V coordinates:
`v_pos = (frame + 0.5) / (2*frames)` and `v_nrm = v_pos + 0.5`.
- `fps_override` exposes playback rate (OpenVAT sidecar carries no fps).
Unity `VATPlayer.cs` + `VAT.shader` + `VATTestSetup.cs`:
- Same os-remap parser (small manual JSON walk — JsonUtility can't
handle hyphens in keys).
- Shader updated to packed-normals layout, drops the `_NrmTex` /
`_HasNrmTex` properties.
- Editor scene builder looks for `*-remap_info.json` + `*_pos.png`,
drops normal-map references.
- Path normalization (`\` → `/`) so AssetDatabase.LoadAssetAtPath works
on Windows (Directory.GetFiles returns backslash paths there).
Other review fixups
-------------------
- `--fps` arity guard in both bake_and_stage.sh scripts — explicit
error when the flag is passed without a value (set -u was bailing
with a confusing shell error).
- Test `ProducesDistinctRowsAcrossFrames` now compares raw scanline
bytes via `memcmp` instead of `QImage::pixel()` — pixel() truncates
16-bit RGBX64 to 8-bit QRgb which could mask sub-byte motion.
- Test `RejectsMissingAnimationOnLiveEntity` swapped hardcoded /tmp
for QTemporaryDir.
- README cleanups in both harnesses — removed stale `--target`,
`--encoding`, `--normals`, `<name>.gdshader`, `<name>_nrm.png`
references.
- Unity `Packages/manifest.json` adds `com.unity.cloud.gltfast` so
`.gltf` imports work out-of-the-box (Unity 2022.3 has no built-in
glTF importer).
- Unity `ProjectVersion.txt` swaps the placeholder comment for a real
revision hash so Unity Hub doesn't complain.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
fernandotonon
force-pushed
the
vat/openvat-only
branch
from
May 19, 2026 14:51
14e6dea to
5e6b641
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
src/VATBakerController.cpp (1)
188-207: ⚡ Quick winScope the non-native dialog workaround to macOS.
The comment documents a macOS-specific issue, but the implementation disables native dialogs on every platform. That regresses the picker UX on Windows/Linux for a platform-local workaround.
Proposed fix
- const QString chosen = QFileDialog::getExistingDirectory( + QFileDialog::Options options = QFileDialog::ShowDirsOnly + | QFileDialog::DontUseCustomDirectoryIcons; +#ifdef Q_OS_MACOS + options |= QFileDialog::DontUseNativeDialog; +#endif + + const QString chosen = QFileDialog::getExistingDirectory( parent, QStringLiteral("Choose OpenVAT output folder"), seed, - QFileDialog::ShowDirsOnly - | QFileDialog::DontUseNativeDialog - | QFileDialog::DontUseCustomDirectoryIcons); + options);As per coding guidelines,
src/**/*.{cpp,h}: Guard all platform-specific APIs with#ifdefQ_OS_WIN,#ifdefQ_OS_MACOS, or#ifdefQ_OS_LINUX preprocessor directives.🤖 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.cpp` around lines 188 - 207, The code currently forces QFileDialog::DontUseNativeDialog for all platforms; restrict this macOS-only workaround by conditionally adding DontUseNativeDialog when building for macOS. Modify the call that builds the flags for QFileDialog::getExistingDirectory (around the QFileDialog::ShowDirsOnly | ... expression) to include QFileDialog::DontUseNativeDialog only inside an `#ifdef` Q_OS_MACOS / `#endif` block (leave QApplication::processEvents(), parent->raise()/activateWindow() and other behavior unchanged).
🤖 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 699-760: The Browse and Bake controls are Rectangle+MouseArea
(animBrowseMa, animBakeMa) and lack keyboard focus/activation; make them
keyboard-accessible by either replacing them with Qt Quick Controls Button or
adding activeFocusOnTab: true to the Rectangle, giving the MouseArea focus: true
and keys handlers (e.g., keys.onPressed to trigger onClicked for Enter/Space),
and adding accessibility metadata (accessible.name/accessible.role) so screen
readers can announce them; ensure enabled state (used in cursorShape and
opacity) is respected in key handlers and that the click logic on animBrowseMa
and VATBakerController.bake(...) is invoked from the keyboard handlers as well.
In `@src/VATBaker.cpp`:
- Around line 551-558: Check the return value of jf.write(sidecar.toUtf8()) and
handle failures: after attempting the write in the block that opens QFile jf
(using result.jsonPath), verify the number of bytes written matches
sidecar.toUtf8().size() (or that write() did not return -1), set result.error to
a descriptive message and return result on failure, and only set result.ok =
true after a successful full write and jf.close(); ensure references are to jf,
jf.write(...), sidecar.toUtf8(), result.jsonPath and result.ok.
- Around line 513-557: Add Sentry breadcrumbing for the OpenVAT export steps:
call SentryReporter::addBreadcrumb("file.export", ...) before the directory
creation (around QDir().mkpath(opts.outputDir)), before saving the image (around
img.save(result.posTexPath, "PNG")) and before writing the sidecar (around
jf.open(...) / jf.write(...)); include the path (result.posTexPath or
result.jsonPath) in each message so Sentry shows which file operation was
attempted. Ensure calls use the "file.export" category and descriptive messages
like "create output dir: <outputDir>", "write pos texture: <posTexPath>", and
"write remap sidecar: <jsonPath>".
In `@src/VATBakerController.cpp`:
- Around line 180-208: In chooseOutputDir, add Sentry breadcrumbs via
SentryReporter::addBreadcrumb("ui.action", ...) to record when the folder picker
is opened and when it is accepted or cancelled; call addBreadcrumb just before
calling QFileDialog::getExistingDirectory to log the "open" action, then after
the call inspect the returned QString chosen and call addBreadcrumb with an
"accept" message including the chosen path when non-empty or a "cancel" message
when empty so UI actions are traceable in Sentry.
- Around line 145-147: Change the Sentry breadcrumb category from "ui.action" to
"file.export" for the VAT bake start/end breadcrumbs: update the
SentryReporter::addBreadcrumb calls that log "OpenVAT bake start: anim=%1
fps=%2" (and the corresponding bake end message) to use "file.export" and
include the bake output path/identifier alongside animationName and fps so the
breadcrumb clearly reflects an export operation.
In `@tools/godot-vat-test/scripts/VATPlayer.gd`:
- Around line 241-242: Clamp loop_frames to not exceed _frame_count before using
it to advance _current_frame: replace the loop_count computation in VATPlayer.gd
(the var loop_count and subsequent _current_frame update) so loop_count =
(min(loop_frames, _frame_count) if loop_frames > 0 else _frame_count) (or use
clamp(loop_frames, 1, _frame_count) style) so _current_frame =
fposmod(_current_frame + delta * fps_override, float(loop_count)) never wraps
past the baked frame rows.
- Around line 110-116: Currently _load_bake() picks the first "-remap_info.json"
and the first "_pos.png" independently which can mix files from different bakes;
instead, when you find the selected json_path (variable json_path from the loop)
derive the matching pos_path by using the same base name: replace the
"-remap_info.json" suffix with "_pos.png" (or search for a file whose name
starts with the json's base and ends with "_pos.png") and set pos_path to that
match; keep the loop but only set pos_path when its basename matches json_path's
basename so json_path and pos_path always come from the same bake.
In `@tools/unity-vat-test/Assets/VAT/Scripts/VATPlayer.cs`:
- Around line 101-103: loopFrames can exceed the baked frame count causing
_currentFrame to index beyond valid rows; fix by clamping the computed loop
value to _frameCount before using it for modulus. Replace the current loop
calculation with one that sets loop = Mathf.Min(loopFrames > 0 ? loopFrames :
_frameCount, _frameCount) (or equivalent), and use that clamped loop when
computing _currentFrame = (_currentFrame + Time.deltaTime * fpsOverride) % loop
so _currentFrame never rolls past the baked frames; update references to
loopFrames/_currentFrame in VATPlayer accordingly.
- Around line 215-222: Replace the brittle float.Parse calls used to populate
_boundsMin and _boundsMax from parsed.Min/parsed.Max with defensive parsing
using float.TryParse (with System.Globalization.CultureInfo.InvariantCulture)
for each component; if any TryParse fails, log a clear error that includes the
offending string(s) and the context (e.g., which bound/component failed) and
return false instead of letting an exception bubble up—update the code paths
around the parsing in VATPlayer (the block that assigns _boundsMin/_boundsMax)
to perform these checks for all 6 values before assigning.
---
Nitpick comments:
In `@src/VATBakerController.cpp`:
- Around line 188-207: The code currently forces
QFileDialog::DontUseNativeDialog for all platforms; restrict this macOS-only
workaround by conditionally adding DontUseNativeDialog when building for macOS.
Modify the call that builds the flags for QFileDialog::getExistingDirectory
(around the QFileDialog::ShowDirsOnly | ... expression) to include
QFileDialog::DontUseNativeDialog only inside an `#ifdef` Q_OS_MACOS / `#endif` block
(leave QApplication::processEvents(), parent->raise()/activateWindow() and other
behavior unchanged).
🪄 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: fcb2fd68-6071-43b4-b16a-7b68610d007e
⛔ Files ignored due to path filters (1)
tools/unity-vat-test/Assets/VAT/Shaders/VAT.shaderis excluded by!**/*.shader
📒 Files selected for processing (18)
qml/PropertiesPanel.qmlsrc/CLIPipeline.cppsrc/MCPServer.cppsrc/VATBaker.cppsrc/VATBaker.hsrc/VATBakerController.cppsrc/VATBakerController.hsrc/VATBakerController_test.cppsrc/VATBaker_test.cpptools/godot-vat-test/README.mdtools/godot-vat-test/bake_and_stage.shtools/godot-vat-test/scripts/VATPlayer.gdtools/unity-vat-test/Assets/VAT/Editor/VATTestSetup.cstools/unity-vat-test/Assets/VAT/Scripts/VATPlayer.cstools/unity-vat-test/Packages/manifest.jsontools/unity-vat-test/ProjectSettings/ProjectVersion.txttools/unity-vat-test/README.mdtools/unity-vat-test/bake_and_stage.sh
✅ Files skipped from review due to trivial changes (3)
- tools/unity-vat-test/ProjectSettings/ProjectVersion.txt
- tools/unity-vat-test/README.md
- tools/godot-vat-test/README.md
🚧 Files skipped from review as they are similar to previous changes (7)
- tools/godot-vat-test/bake_and_stage.sh
- tools/unity-vat-test/Packages/manifest.json
- src/MCPServer.cpp
- src/VATBakerController_test.cpp
- src/CLIPipeline.cpp
- src/VATBaker.h
- src/VATBaker_test.cpp
Comment on lines
+699
to
+760
| Rectangle { | ||
| width: 60; height: 24; radius: 3 | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| color: animBrowseMa.pressed | ||
| ? Qt.darker(PropertiesPanelController.headerColor, 1.2) | ||
| : animBrowseMa.containsMouse | ||
| ? Qt.lighter(PropertiesPanelController.headerColor, 1.2) | ||
| : PropertiesPanelController.headerColor | ||
| border.color: PropertiesPanelController.borderColor | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: "Browse" | ||
| color: PropertiesPanelController.textColor; font.pixelSize: 10 | ||
| } | ||
| MouseArea { | ||
| id: animBrowseMa | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| enabled: !VATBakerController.isBaking | ||
| cursorShape: enabled ? Qt.PointingHandCursor : Qt.ForbiddenCursor | ||
| onClicked: { | ||
| const picked = VATBakerController.chooseOutputDir(animToolsCol.outputDir) | ||
| if (picked && picked.length > 0) | ||
| animToolsCol.outputDir = picked | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Bake button — same look as Animation Control's play button | ||
| // (lighten/darken on the header color; no highlight ramp). | ||
| Rectangle { | ||
| width: parent.width - 16; height: 26; radius: 3 | ||
| color: animBakeMa.pressed | ||
| ? Qt.darker(PropertiesPanelController.headerColor, 1.2) | ||
| : animBakeMa.containsMouse && animBakeMa.enabled | ||
| ? Qt.lighter(PropertiesPanelController.headerColor, 1.2) | ||
| : PropertiesPanelController.headerColor | ||
| opacity: animBakeMa.enabled ? 1.0 : 0.45 | ||
| border.color: PropertiesPanelController.borderColor | ||
|
|
||
| Text { | ||
| anchors.centerIn: parent | ||
| text: VATBakerController.isBaking ? "Baking…" : "Bake VAT" | ||
| color: PropertiesPanelController.textColor; font.pixelSize: 11 | ||
| } | ||
| MouseArea { | ||
| id: animBakeMa | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| enabled: !VATBakerController.isBaking | ||
| && animToolsCol.animName !== "" | ||
| && animToolsCol.outputDir !== "" | ||
| cursorShape: enabled ? Qt.PointingHandCursor : Qt.ForbiddenCursor | ||
| onClicked: { | ||
| VATBakerController.bake( | ||
| animToolsCol.animName, | ||
| animToolsCol.fps, | ||
| animToolsCol.outputDir, | ||
| "") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 94
🏁 Script executed:
wc -l qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 99
🏁 Script executed:
sed -n '699,760p' qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 3164
🏁 Script executed:
grep -n "Button" qml/PropertiesPanel.qml | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 771
🏁 Script executed:
grep -n "activeFocusOnTab\|KeyNavigation\|Keys.onPressed\|Accessible" qml/PropertiesPanel.qml | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
sed -n '680,800p' qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 5907
🏁 Script executed:
sed -n '1990,2020p' qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 1116
🏁 Script executed:
sed -n '3980,4000p' qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 964
🏁 Script executed:
sed -n '650,700p' qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 2104
🏁 Script executed:
sed -n '699,760p' qml/PropertiesPanel.qml | grep -i "keys\|focus\|accessible"Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
rg "Keys\.on|activeFocusOnTab|Accessible\." qml/PropertiesPanel.qml -A 2 -B 2 | head -40Repository: fernandotonon/QtMeshEditor
Length of output: 1231
🏁 Script executed:
sed -n '620,780p' qml/PropertiesPanel.qml | cat -nRepository: fernandotonon/QtMeshEditor
Length of output: 8038
🏁 Script executed:
rg "Rectangle.*MouseArea" qml/PropertiesPanel.qml | head -20Repository: fernandotonon/QtMeshEditor
Length of output: 52
🏁 Script executed:
sed -n '3980,4010p' qml/PropertiesPanel.qmlRepository: fernandotonon/QtMeshEditor
Length of output: 1229
Make the Browse and Bake controls keyboard-accessible.
Both controls are Rectangle + MouseArea with no tab focus or key handling, creating a keyboard navigation gap in the VAT bake flow. The animation picker (ComboBox), FPS (SpinBox), and output directory (TextField) all support keyboard interaction, but Browse and Bake do not. A keyboard-only user cannot complete the workflow from this panel. Prefer Button or add activeFocusOnTab, Enter/Space handlers, and accessibility metadata to maintain consistency.
🤖 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 `@qml/PropertiesPanel.qml` around lines 699 - 760, The Browse and Bake controls
are Rectangle+MouseArea (animBrowseMa, animBakeMa) and lack keyboard
focus/activation; make them keyboard-accessible by either replacing them with Qt
Quick Controls Button or adding activeFocusOnTab: true to the Rectangle, giving
the MouseArea focus: true and keys handlers (e.g., keys.onPressed to trigger
onClicked for Enter/Space), and adding accessibility metadata
(accessible.name/accessible.role) so screen readers can announce them; ensure
enabled state (used in cursorShape and opacity) is respected in key handlers and
that the click logic on animBrowseMa and VATBakerController.bake(...) is invoked
from the keyboard handlers as well.
Comment on lines
513
to
557
| QDir().mkpath(opts.outputDir); | ||
| const QString base = opts.basename.isEmpty() ? opts.animationName : opts.basename; | ||
| result.posTexPath = QDir(opts.outputDir).filePath(base + "_pos.png"); | ||
| result.jsonPath = QDir(opts.outputDir).filePath(base + ".json"); | ||
| result.jsonPath = QDir(opts.outputDir).filePath(base + "-remap_info.json"); | ||
|
|
||
| const bool flipRows = targetFlipsRowsAtWriteTime(opts.target); | ||
|
|
||
| // Encode + write the position texture. PNG preserves both 8-bit | ||
| // and 16-bit channel depths; QImage handles the format switch for us. | ||
| if (opts.encoding == Encoding::RGBA8) { | ||
| auto rgba = encodeRGBA8(flat, frameCount, vertexCount, lo, hi); | ||
| if (rgba.empty()) { | ||
| result.error = QStringLiteral("encodeRGBA8 produced empty buffer"); | ||
| return result; | ||
| } | ||
| QImage img(vertexCount, frameCount, QImage::Format_RGBA8888); | ||
| for (int y = 0; y < frameCount; ++y) { | ||
| const int srcRow = flipRows ? (frameCount - 1 - y) : y; | ||
| const unsigned char* src = rgba.data() + static_cast<size_t>(srcRow) | ||
| * static_cast<size_t>(vertexCount) * 4u; | ||
| std::memcpy(img.scanLine(y), src, static_cast<size_t>(vertexCount) * 4u); | ||
| } | ||
| if (!img.save(result.posTexPath, "PNG")) { | ||
| result.error = QStringLiteral("failed to write position texture: %1") | ||
| .arg(result.posTexPath); | ||
| return result; | ||
| } | ||
| } else { // RGBA16 | ||
| auto rgba = encodeRGBA16(flat, frameCount, vertexCount, lo, hi); | ||
| if (rgba.empty()) { | ||
| result.error = QStringLiteral("encodeRGBA16 produced empty buffer"); | ||
| return result; | ||
| } | ||
| // Format_RGBA64 is 16 bits per channel — Qt's `save("PNG")` writes | ||
| // a 16-bit PNG which Godot/Unity/Unreal all read losslessly. | ||
| QImage img(vertexCount, frameCount, QImage::Format_RGBA64); | ||
| for (int y = 0; y < frameCount; ++y) { | ||
| const int srcRow = flipRows ? (frameCount - 1 - y) : y; | ||
| const uint16_t* src = rgba.data() + static_cast<size_t>(srcRow) | ||
| * static_cast<size_t>(vertexCount) * 4u; | ||
| std::memcpy(img.scanLine(y), src, static_cast<size_t>(vertexCount) * 4u * sizeof(uint16_t)); | ||
| } | ||
| if (!img.save(result.posTexPath, "PNG")) { | ||
| result.error = QStringLiteral("failed to write 16-bit position texture: %1") | ||
| .arg(result.posTexPath); | ||
| return result; | ||
| } | ||
| auto packed = packOpenVAT16(flat, normals, frameCount, vertexCount, | ||
| roundedLo, roundedHi); | ||
| if (packed.empty()) { | ||
| result.error = QStringLiteral("OpenVAT pack produced empty buffer"); | ||
| return result; | ||
| } | ||
|
|
||
| // Normal texture — only when requested. Same layout / size, normals | ||
| // mapped from [-1, 1] → [0, MAX] per channel. | ||
| if (opts.bakeNormals) { | ||
| result.nrmTexPath = QDir(opts.outputDir).filePath(base + "_nrm.png"); | ||
| if (opts.encoding == Encoding::RGBA8) { | ||
| auto rgba = encodeNormalsRGBA8(normals, frameCount, vertexCount); | ||
| if (rgba.empty()) { | ||
| result.error = QStringLiteral("encodeNormalsRGBA8 produced empty buffer"); | ||
| return result; | ||
| } | ||
| QImage img(vertexCount, frameCount, QImage::Format_RGBA8888); | ||
| for (int y = 0; y < frameCount; ++y) { | ||
| const int srcRow = flipRows ? (frameCount - 1 - y) : y; | ||
| const unsigned char* src = rgba.data() + static_cast<size_t>(srcRow) | ||
| * static_cast<size_t>(vertexCount) * 4u; | ||
| std::memcpy(img.scanLine(y), src, static_cast<size_t>(vertexCount) * 4u); | ||
| } | ||
| if (!img.save(result.nrmTexPath, "PNG")) { | ||
| result.error = QStringLiteral("failed to write normal texture: %1") | ||
| .arg(result.nrmTexPath); | ||
| return result; | ||
| } | ||
| } else { // RGBA16 | ||
| auto rgba = encodeNormalsRGBA16(normals, frameCount, vertexCount); | ||
| if (rgba.empty()) { | ||
| result.error = QStringLiteral("encodeNormalsRGBA16 produced empty buffer"); | ||
| return result; | ||
| } | ||
| QImage img(vertexCount, frameCount, QImage::Format_RGBA64); | ||
| for (int y = 0; y < frameCount; ++y) { | ||
| const int srcRow = flipRows ? (frameCount - 1 - y) : y; | ||
| const uint16_t* src = rgba.data() + static_cast<size_t>(srcRow) | ||
| * static_cast<size_t>(vertexCount) * 4u; | ||
| std::memcpy(img.scanLine(y), src, static_cast<size_t>(vertexCount) * 4u * sizeof(uint16_t)); | ||
| } | ||
| if (!img.save(result.nrmTexPath, "PNG")) { | ||
| result.error = QStringLiteral("failed to write 16-bit normal texture: %1") | ||
| .arg(result.nrmTexPath); | ||
| return result; | ||
| } | ||
| const int imgHeight = frameCount * 2; | ||
| // RGBX64 is Qt's 16-bit-per-channel 4-channel format. The X channel | ||
| // is padding; PNG can store 3-channel data losslessly but Qt's PNG | ||
| // writer infers RGB-vs-RGBA from the QImage format, and Format_RGB | ||
| // doesn't exist at 16-bit precision. Padding to RGBX64 costs a few | ||
| // hundred KB on a 5828×142 image — acceptable for a one-off bake. | ||
| QImage img(vertexCount, imgHeight, QImage::Format_RGBX64); | ||
| img.fill(0); | ||
| for (int y = 0; y < imgHeight; ++y) { | ||
| const uint16_t* src = packed.data() | ||
| + static_cast<size_t>(y) | ||
| * static_cast<size_t>(vertexCount) * 3u; | ||
| auto* dst = reinterpret_cast<uint16_t*>(img.scanLine(y)); | ||
| for (int x = 0; x < vertexCount; ++x) { | ||
| dst[x * 4 + 0] = src[x * 3 + 0]; | ||
| dst[x * 4 + 1] = src[x * 3 + 1]; | ||
| dst[x * 4 + 2] = src[x * 3 + 2]; | ||
| dst[x * 4 + 3] = 65535; | ||
| } | ||
| } | ||
| if (!img.save(result.posTexPath, "PNG")) { | ||
| result.error = QStringLiteral("failed to write OpenVAT texture: %1") | ||
| .arg(result.posTexPath); | ||
| return result; | ||
| } | ||
|
|
||
| // Sidecar. | ||
| const QString sidecar = buildSidecarJson(result, opts); | ||
| const QString sidecar = buildOpenVATSidecar(frameCount, roundedLo, roundedHi); | ||
| QFile jf(result.jsonPath); | ||
| if (!jf.open(QIODevice::WriteOnly | QIODevice::Truncate)) { | ||
| result.error = QStringLiteral("failed to open JSON for write: %1") | ||
| result.error = QStringLiteral("failed to open OpenVAT sidecar for write: %1") | ||
| .arg(result.jsonPath); | ||
| return result; | ||
| } | ||
| jf.write(sidecar.toUtf8()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add file.export breadcrumbs around the OpenVAT writes.
This path creates the export directory and writes both output artifacts, but none of those operations are breadcrumbed. That makes export failures much harder to reconstruct in Sentry.
Suggested change
+ SentryReporter::addBreadcrumb(
+ QStringLiteral("file.export"),
+ QStringLiteral("Preparing OpenVAT export in %1").arg(opts.outputDir));
QDir().mkpath(opts.outputDir);
@@
+ SentryReporter::addBreadcrumb(
+ QStringLiteral("file.export"),
+ QStringLiteral("Writing OpenVAT texture to %1").arg(result.posTexPath));
if (!img.save(result.posTexPath, "PNG")) {
@@
+ SentryReporter::addBreadcrumb(
+ QStringLiteral("file.export"),
+ QStringLiteral("Writing OpenVAT sidecar to %1").arg(result.jsonPath));
QFile jf(result.jsonPath);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".
🤖 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/VATBaker.cpp` around lines 513 - 557, Add Sentry breadcrumbing for the
OpenVAT export steps: call SentryReporter::addBreadcrumb("file.export", ...)
before the directory creation (around QDir().mkpath(opts.outputDir)), before
saving the image (around img.save(result.posTexPath, "PNG")) and before writing
the sidecar (around jf.open(...) / jf.write(...)); include the path
(result.posTexPath or result.jsonPath) in each message so Sentry shows which
file operation was attempted. Ensure calls use the "file.export" category and
descriptive messages like "create output dir: <outputDir>", "write pos texture:
<posTexPath>", and "write remap sidecar: <jsonPath>".
Comment on lines
551
to
558
| QFile jf(result.jsonPath); | ||
| if (!jf.open(QIODevice::WriteOnly | QIODevice::Truncate)) { | ||
| result.error = QStringLiteral("failed to open JSON for write: %1") | ||
| result.error = QStringLiteral("failed to open OpenVAT sidecar for write: %1") | ||
| .arg(result.jsonPath); | ||
| return result; | ||
| } | ||
| jf.write(sidecar.toUtf8()); | ||
| jf.close(); |
There was a problem hiding this comment.
Check the sidecar write result before reporting success.
QFile::write() is ignored here, so a short/failed write still falls through to result.ok = true and leaves a truncated -remap_info.json behind.
Suggested fix
QFile jf(result.jsonPath);
if (!jf.open(QIODevice::WriteOnly | QIODevice::Truncate)) {
result.error = QStringLiteral("failed to open OpenVAT sidecar for write: %1")
.arg(result.jsonPath);
return result;
}
- jf.write(sidecar.toUtf8());
+ const QByteArray payload = sidecar.toUtf8();
+ if (jf.write(payload) != payload.size() || !jf.flush()) {
+ result.error = QStringLiteral("failed to write OpenVAT sidecar: %1")
+ .arg(jf.errorString());
+ jf.close();
+ return result;
+ }
jf.close();🤖 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/VATBaker.cpp` around lines 551 - 558, Check the return value of
jf.write(sidecar.toUtf8()) and handle failures: after attempting the write in
the block that opens QFile jf (using result.jsonPath), verify the number of
bytes written matches sidecar.toUtf8().size() (or that write() did not return
-1), set result.error to a descriptive message and return result on failure, and
only set result.ok = true after a successful full write and jf.close(); ensure
references are to jf, jf.write(...), sidecar.toUtf8(), result.jsonPath and
result.ok.
Comment on lines
+145
to
+147
| SentryReporter::addBreadcrumb("ui.action", | ||
| QStringLiteral("VAT bake start: anim=%1 fps=%2 encoding=%3 target=%4 normals=%5") | ||
| .arg(animationName).arg(fps).arg(enc, tgt).arg(bakeNormals ? "yes" : "no")); | ||
| QStringLiteral("OpenVAT bake start: anim=%1 fps=%2") | ||
| .arg(animationName).arg(fps)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use file.export breadcrumbs for the bake path.
This is an export operation, so logging it as ui.action will mix file-output telemetry into click noise. Please switch the bake start/end breadcrumbs to file.export.
Proposed fix
- SentryReporter::addBreadcrumb("ui.action",
+ SentryReporter::addBreadcrumb("file.export",
QStringLiteral("OpenVAT bake start: anim=%1 fps=%2")
.arg(animationName).arg(fps));
...
- SentryReporter::addBreadcrumb("ui.action",
+ SentryReporter::addBreadcrumb("file.export",
result.ok
? QStringLiteral("VAT bake ok: %1 frames × %2 vertices → %3")
.arg(result.frameCount).arg(result.vertexCount).arg(result.posTexPath)
: QStringLiteral("VAT bake failed: %1").arg(result.error));As per coding guidelines, src/**/*.cpp: 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.
🤖 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.cpp` around lines 145 - 147, Change the Sentry
breadcrumb category from "ui.action" to "file.export" for the VAT bake start/end
breadcrumbs: update the SentryReporter::addBreadcrumb calls that log "OpenVAT
bake start: anim=%1 fps=%2" (and the corresponding bake end message) to use
"file.export" and include the bake output path/identifier alongside
animationName and fps so the breadcrumb clearly reflects an export operation.
Comment on lines
+180
to
+208
| QString VATBakerController::chooseOutputDir(const QString& startDir) | ||
| { | ||
| QString seed = startDir; | ||
| if (seed.isEmpty() || !QFileInfo(seed).isDir()) | ||
| seed = QStandardPaths::writableLocation(QStandardPaths::DocumentsLocation); | ||
| if (seed.isEmpty()) | ||
| seed = QDir::homePath(); | ||
|
|
||
| // Mirror the MaterialEditorQML::openFileDialog dance: process pending | ||
| // events + raise the active window before opening the dialog, and | ||
| // force the Qt-rendered dialog rather than the native one. Native | ||
| // file dialogs hosted from inside a QQuickWidget have been observed | ||
| // to silently no-op on macOS — DontUseNativeDialog reliably opens. | ||
| QApplication::processEvents(); | ||
| QWidget* parent = QApplication::activeWindow(); | ||
| if (parent) { | ||
| parent->raise(); | ||
| parent->activateWindow(); | ||
| } | ||
| QApplication::processEvents(); | ||
|
|
||
| const QString chosen = QFileDialog::getExistingDirectory( | ||
| parent, | ||
| QStringLiteral("Choose OpenVAT output folder"), | ||
| seed, | ||
| QFileDialog::ShowDirsOnly | ||
| | QFileDialog::DontUseNativeDialog | ||
| | QFileDialog::DontUseCustomDirectoryIcons); | ||
| return chosen; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Breadcrumb the folder-picker flow.
chooseOutputDir(...) is a new user-facing action, but it currently leaves no trace in Sentry. Add ui.action breadcrumbs for open + accept/cancel so VAT failures can be correlated with picker behavior.
Proposed fix
QString VATBakerController::chooseOutputDir(const QString& startDir)
{
QString seed = startDir;
if (seed.isEmpty() || !QFileInfo(seed).isDir())
seed = QStandardPaths::writableLocation(QStandardPaths::DocumentsLocation);
if (seed.isEmpty())
seed = QDir::homePath();
+
+ SentryReporter::addBreadcrumb("ui.action",
+ "VAT output directory picker opened");
// Mirror the MaterialEditorQML::openFileDialog dance: process pending
// events + raise the active window before opening the dialog, and
@@
const QString chosen = QFileDialog::getExistingDirectory(
parent,
QStringLiteral("Choose OpenVAT output folder"),
seed,
QFileDialog::ShowDirsOnly
| QFileDialog::DontUseNativeDialog
| QFileDialog::DontUseCustomDirectoryIcons);
+ SentryReporter::addBreadcrumb("ui.action",
+ chosen.isEmpty()
+ ? "VAT output directory picker cancelled"
+ : "VAT output directory picker accepted");
return chosen;
}As per coding guidelines, src/**/*.cpp: 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.
🤖 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.cpp` around lines 180 - 208, In chooseOutputDir, add
Sentry breadcrumbs via SentryReporter::addBreadcrumb("ui.action", ...) to record
when the folder picker is opened and when it is accepted or cancelled; call
addBreadcrumb just before calling QFileDialog::getExistingDirectory to log the
"open" action, then after the call inspect the returned QString chosen and call
addBreadcrumb with an "accept" message including the chosen path when non-empty
or a "cancel" message when empty so UI actions are traceable in Sentry.
Comment on lines
+110
to
+116
| var json_path := "" | ||
| var shader_path := "" | ||
| var pos_path := "" | ||
| for f in dir.get_files(): | ||
| if f.ends_with(".json") and json_path.is_empty(): | ||
| if f.ends_with("-remap_info.json") and json_path.is_empty(): | ||
| json_path = bake_dir.path_join(f) | ||
| elif f.ends_with(".gdshader") and shader_path.is_empty(): | ||
| shader_path = bake_dir.path_join(f) | ||
| elif f.ends_with("_pos.png") and pos_path.is_empty(): | ||
| pos_path = bake_dir.path_join(f) |
There was a problem hiding this comment.
Match the PNG to the selected sidecar basename.
_load_bake() currently grabs the first *-remap_info.json and the first *_pos.png independently. If bake_dir contains more than one staged bake, this can pair metadata from one bake with texture data from another and silently corrupt playback.
Proposed fix
var json_path := ""
var pos_path := ""
+var bake_base := ""
for f in dir.get_files():
if f.ends_with("-remap_info.json") and json_path.is_empty():
+ bake_base = f.trim_suffix("-remap_info.json")
json_path = bake_dir.path_join(f)
- elif f.ends_with("_pos.png") and pos_path.is_empty():
- pos_path = bake_dir.path_join(f)
+ pos_path = bake_dir.path_join("%s_pos.png" % bake_base)
if json_path.is_empty():
push_error("VATPlayer: no *-remap_info.json sidecar in %s — " % bake_dir +
"is this an OpenVAT bake? Try rerunning bake_and_stage.sh.")
return false
-if pos_path.is_empty():
- push_error("VATPlayer: no *_pos.png in %s" % bake_dir)
+if pos_path.is_empty() or not FileAccess.file_exists(pos_path):
+ push_error("VATPlayer: no matching *_pos.png in %s" % bake_dir)
return false🤖 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 `@tools/godot-vat-test/scripts/VATPlayer.gd` around lines 110 - 116, Currently
_load_bake() picks the first "-remap_info.json" and the first "_pos.png"
independently which can mix files from different bakes; instead, when you find
the selected json_path (variable json_path from the loop) derive the matching
pos_path by using the same base name: replace the "-remap_info.json" suffix with
"_pos.png" (or search for a file whose name starts with the json's base and ends
with "_pos.png") and set pos_path to that match; keep the loop but only set
pos_path when its basename matches json_path's basename so json_path and
pos_path always come from the same bake.
Comment on lines
+241
to
+242
| var loop_count: int = (loop_frames if loop_frames > 0 else _frame_count) | ||
| _current_frame = fposmod(_current_frame + delta * _fps, float(loop_count)) | ||
| # Drive every per-surface material's current_frame uniformly. | ||
| _current_frame = fposmod(_current_frame + delta * fps_override, float(loop_count)) |
There was a problem hiding this comment.
Clamp loop_frames to _frame_count.
If loop_frames is set above the baked frame count, _current_frame eventually points past the position half of the packed texture, so the shader starts sampling the wrong rows.
Proposed fix
- var loop_count: int = (loop_frames if loop_frames > 0 else _frame_count)
+ var loop_count: int = (_frame_count if loop_frames <= 0 else min(loop_frames, _frame_count))
_current_frame = fposmod(_current_frame + delta * fps_override, float(loop_count))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var loop_count: int = (loop_frames if loop_frames > 0 else _frame_count) | |
| _current_frame = fposmod(_current_frame + delta * _fps, float(loop_count)) | |
| # Drive every per-surface material's current_frame uniformly. | |
| _current_frame = fposmod(_current_frame + delta * fps_override, float(loop_count)) | |
| var loop_count: int = (_frame_count if loop_frames <= 0 else min(loop_frames, _frame_count)) | |
| _current_frame = fposmod(_current_frame + delta * fps_override, float(loop_count)) |
🤖 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 `@tools/godot-vat-test/scripts/VATPlayer.gd` around lines 241 - 242, Clamp
loop_frames to not exceed _frame_count before using it to advance
_current_frame: replace the loop_count computation in VATPlayer.gd (the var
loop_count and subsequent _current_frame update) so loop_count =
(min(loop_frames, _frame_count) if loop_frames > 0 else _frame_count) (or use
clamp(loop_frames, 1, _frame_count) style) so _current_frame =
fposmod(_current_frame + delta * fps_override, float(loop_count)) never wraps
past the baked frame rows.
Comment on lines
101
to
103
| int loop = (loopFrames > 0) ? loopFrames : _frameCount; | ||
| _currentFrame = (_currentFrame + Time.deltaTime * fpsOverride) % loop; | ||
| for (int i = 0; i < _materials.Length; i++) { |
There was a problem hiding this comment.
Clamp loopFrames to baked frame count.
If loopFrames is set larger than _frameCount, _CurrentFrame can step beyond valid baked rows and produce incorrect sampling.
Proposed fix
- int loop = (loopFrames > 0) ? loopFrames : _frameCount;
+ int loop = (loopFrames > 0) ? Mathf.Min(loopFrames, _frameCount) : _frameCount;
_currentFrame = (_currentFrame + Time.deltaTime * fpsOverride) % loop;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| int loop = (loopFrames > 0) ? loopFrames : _frameCount; | |
| _currentFrame = (_currentFrame + Time.deltaTime * fpsOverride) % loop; | |
| for (int i = 0; i < _materials.Length; i++) { | |
| int loop = (loopFrames > 0) ? Mathf.Min(loopFrames, _frameCount) : _frameCount; | |
| _currentFrame = (_currentFrame + Time.deltaTime * fpsOverride) % loop; | |
| for (int i = 0; i < _materials.Length; i++) { |
🤖 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 `@tools/unity-vat-test/Assets/VAT/Scripts/VATPlayer.cs` around lines 101 - 103,
loopFrames can exceed the baked frame count causing _currentFrame to index
beyond valid rows; fix by clamping the computed loop value to _frameCount before
using it for modulus. Replace the current loop calculation with one that sets
loop = Mathf.Min(loopFrames > 0 ? loopFrames : _frameCount, _frameCount) (or
equivalent), and use that clamped loop when computing _currentFrame =
(_currentFrame + Time.deltaTime * fpsOverride) % loop so _currentFrame never
rolls past the baked frames; update references to loopFrames/_currentFrame in
VATPlayer accordingly.
Comment on lines
+215
to
+222
| _boundsMin = new Vector3( | ||
| float.Parse(parsed.Min[0], System.Globalization.CultureInfo.InvariantCulture), | ||
| float.Parse(parsed.Min[1], System.Globalization.CultureInfo.InvariantCulture), | ||
| float.Parse(parsed.Min[2], System.Globalization.CultureInfo.InvariantCulture)); | ||
| _boundsMax = new Vector3( | ||
| float.Parse(parsed.Max[0], System.Globalization.CultureInfo.InvariantCulture), | ||
| float.Parse(parsed.Max[1], System.Globalization.CultureInfo.InvariantCulture), | ||
| float.Parse(parsed.Max[2], System.Globalization.CultureInfo.InvariantCulture)); |
There was a problem hiding this comment.
Guard sidecar float parsing to avoid hard exceptions.
float.Parse will throw on malformed values and bypass your false-return error path. Parse defensively and fail with a clear log instead.
Proposed fix
- _boundsMin = new Vector3(
- float.Parse(parsed.Min[0], System.Globalization.CultureInfo.InvariantCulture),
- float.Parse(parsed.Min[1], System.Globalization.CultureInfo.InvariantCulture),
- float.Parse(parsed.Min[2], System.Globalization.CultureInfo.InvariantCulture));
- _boundsMax = new Vector3(
- float.Parse(parsed.Max[0], System.Globalization.CultureInfo.InvariantCulture),
- float.Parse(parsed.Max[1], System.Globalization.CultureInfo.InvariantCulture),
- float.Parse(parsed.Max[2], System.Globalization.CultureInfo.InvariantCulture));
+ if (!float.TryParse(parsed.Min[0], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var minX) ||
+ !float.TryParse(parsed.Min[1], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var minY) ||
+ !float.TryParse(parsed.Min[2], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var minZ) ||
+ !float.TryParse(parsed.Max[0], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var maxX) ||
+ !float.TryParse(parsed.Max[1], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var maxY) ||
+ !float.TryParse(parsed.Max[2], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var maxZ)) {
+ Debug.LogError($"[VATPlayer] {name}: os-remap Min/Max contain non-numeric values");
+ return false;
+ }
+ _boundsMin = new Vector3(minX, minY, minZ);
+ _boundsMax = new Vector3(maxX, maxY, maxZ);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| _boundsMin = new Vector3( | |
| float.Parse(parsed.Min[0], System.Globalization.CultureInfo.InvariantCulture), | |
| float.Parse(parsed.Min[1], System.Globalization.CultureInfo.InvariantCulture), | |
| float.Parse(parsed.Min[2], System.Globalization.CultureInfo.InvariantCulture)); | |
| _boundsMax = new Vector3( | |
| float.Parse(parsed.Max[0], System.Globalization.CultureInfo.InvariantCulture), | |
| float.Parse(parsed.Max[1], System.Globalization.CultureInfo.InvariantCulture), | |
| float.Parse(parsed.Max[2], System.Globalization.CultureInfo.InvariantCulture)); | |
| if (!float.TryParse(parsed.Min[0], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var minX) || | |
| !float.TryParse(parsed.Min[1], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var minY) || | |
| !float.TryParse(parsed.Min[2], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var minZ) || | |
| !float.TryParse(parsed.Max[0], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var maxX) || | |
| !float.TryParse(parsed.Max[1], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var maxY) || | |
| !float.TryParse(parsed.Max[2], System.Globalization.NumberStyles.Float, System.Globalization.CultureInfo.InvariantCulture, out var maxZ)) { | |
| Debug.LogError($"[VATPlayer] {name}: os-remap Min/Max contain non-numeric values"); | |
| return false; | |
| } | |
| _boundsMin = new Vector3(minX, minY, minZ); | |
| _boundsMax = new Vector3(maxX, maxY, maxZ); |
🤖 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 `@tools/unity-vat-test/Assets/VAT/Scripts/VATPlayer.cs` around lines 215 - 222,
Replace the brittle float.Parse calls used to populate _boundsMin and _boundsMax
from parsed.Min/parsed.Max with defensive parsing using float.TryParse (with
System.Globalization.CultureInfo.InvariantCulture) for each component; if any
TryParse fails, log a clear error that includes the offending string(s) and the
context (e.g., which bound/component failed) and return false instead of letting
an exception bubble up—update the code paths around the parsing in VATPlayer
(the block that assigns _boundsMin/_boundsMax) to perform these checks for all 6
values before assigning.
- Sidecar QFile::write short-write check VATBaker.cpp ignored the byte count from QFile::write(), so a truncated sidecar (disk full, network volume hiccup) would still report ok=true. Now compares the return value against the buffer size, removes the partial file, and surfaces the failure through BakeResult.error. - Bake breadcrumbs → file.export category VATBakerController used 'ui.action' for bake start/end. That's output telemetry, not a click — Sentry split-by-category was mixing it into generic UI noise. Switched to 'file.export' (matches CLI's category). - chooseOutputDir gains ui.action breadcrumbs Was silent. Now logs the picker open with the seed dir, then the accept/cancel outcome — so a VAT failure can be correlated against whether the user actually picked a folder. - Godot harness: pair PNG to sidecar by basename _load_bake() picked the first *-remap_info.json and the first *_pos.png independently. If bake_dir held two staged bakes (someone re-ran the staging script without clearing), the metadata from one would be applied to the texture of the other and produce visually plausible but wrong frames. Now derives the basename from the picked sidecar and loads <basename>_pos.png explicitly. - Godot + Unity: clamp loop_frames ≤ frame_count A user-set loopFrames > frameCount would push current_frame past the position half of the packed texture and into the normal half, rendering garbage rows as positions. Both harnesses now Mathf.Min / mini against the bake's frame count. - Unity: defensive float.TryParse for os-remap bounds float.Parse throws FormatException on malformed strings, which bypassed the bool-returning error path and left Unity in a half- configured state. New TryParseBoundsVec helper logs a clear error and returns false instead. - QML Inspector: keyboard-accessible Browse + Bake buttons Both were Rectangle + MouseArea with no tab focus or key handling. A keyboard-only user couldn't complete the bake workflow from this panel. Added activeFocusOnTab, Keys.onSpacePressed/Return/Enter handlers that call into the MouseArea click logic, Accessible.role + Accessible.name metadata for screen readers, and a focus ring via the existing borderColor → highlightColor + border.width=2 pattern. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 689-735: Replace the two-way update handler on the TextField by
using onTextEdited instead of onTextChanged (i.e., in animOutField use
onTextEdited: animToolsCol.outputDir = text) so programmatic updates don't drop
the binding, and after the Browse selection (inside animBrowseMa.onClicked where
you call VATBakerController.chooseOutputDir and set animToolsCol.outputDir =
picked) explicitly resync the field by setting animOutField.text = picked;
additionally add a Connections block targeting animToolsCol to handle its
outputDir change (e.g., Connections { target: animToolsCol; onOutputDirChanged:
if (!animOutField.activeFocus) animOutField.text = animToolsCol.outputDir } ) so
external model updates are reflected when the user is not actively editing.
🪄 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: abc4c829-de9d-4378-b43e-212d935edc91
📒 Files selected for processing (5)
qml/PropertiesPanel.qmlsrc/VATBaker.cppsrc/VATBakerController.cpptools/godot-vat-test/scripts/VATPlayer.gdtools/unity-vat-test/Assets/VAT/Scripts/VATPlayer.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- tools/unity-vat-test/Assets/VAT/Scripts/VATPlayer.cs
- src/VATBakerController.cpp
- tools/godot-vat-test/scripts/VATPlayer.gd
- src/VATBaker.cpp
Comment on lines
+689
to
+735
| TextField { | ||
| id: animOutField | ||
| width: parent.width - animToolsCol.labelWidth - 6 - 60 - 6 | ||
| height: 24 | ||
| font.pixelSize: 11 | ||
| text: animToolsCol.outputDir | ||
| onTextChanged: animToolsCol.outputDir = text | ||
| placeholderText: "/path/to/output" | ||
| enabled: !VATBakerController.isBaking | ||
| } | ||
| Rectangle { | ||
| id: animBrowseBtn | ||
| width: 60; height: 24; radius: 3 | ||
| anchors.verticalCenter: parent.verticalCenter | ||
| color: animBrowseMa.pressed | ||
| ? Qt.darker(PropertiesPanelController.headerColor, 1.2) | ||
| : (animBrowseMa.containsMouse || animBrowseBtn.activeFocus) | ||
| ? Qt.lighter(PropertiesPanelController.headerColor, 1.2) | ||
| : PropertiesPanelController.headerColor | ||
| border.color: animBrowseBtn.activeFocus | ||
| ? PropertiesPanelController.highlightColor | ||
| : PropertiesPanelController.borderColor | ||
| border.width: animBrowseBtn.activeFocus ? 2 : 1 | ||
| activeFocusOnTab: !VATBakerController.isBaking | ||
| Accessible.role: Accessible.Button | ||
| Accessible.name: "Browse for VAT output folder" | ||
| Keys.onSpacePressed: animBrowseMa.clicked(null) | ||
| Keys.onReturnPressed: animBrowseMa.clicked(null) | ||
| Keys.onEnterPressed: animBrowseMa.clicked(null) | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: "Browse" | ||
| color: PropertiesPanelController.textColor; font.pixelSize: 10 | ||
| } | ||
| MouseArea { | ||
| id: animBrowseMa | ||
| anchors.fill: parent | ||
| hoverEnabled: true | ||
| enabled: !VATBakerController.isBaking | ||
| cursorShape: enabled ? Qt.PointingHandCursor : Qt.ForbiddenCursor | ||
| onClicked: { | ||
| if (!animBrowseMa.enabled) return | ||
| const picked = VATBakerController.chooseOutputDir(animToolsCol.outputDir) | ||
| if (picked && picked.length > 0) | ||
| animToolsCol.outputDir = picked | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Qt Quick/QML, if a TextFieldusestext: somePropertyand also updates that property from a change handler, can later imperative assignments tosomePropertystop updating the visible field after user edits? IsonTextEdited plus explicit resync the recommended pattern?
💡 Result:
Yes—an imperative assignment to the same property that originally had a binding can stop the field from reflecting further changes, because the assignment destroys the existing binding. In QML, bindings are replaced/lost when a property is set imperatively (“overwrite the property’s value”), so later changes to the original source won’t automatically propagate to the property anymore [1]. Applied to your scenario: if TextField.text is declared as text: someProperty, that is a binding. If, in some handler, you later do an imperative assignment to someProperty (or otherwise reassign the TextField’s text through that same property), you can end up breaking the intended “live sync” between your model and the visible text. This is the same general binding-removal behavior described for QML properties [1] and diagnosed by Qt’s binding-removal debug logging [2]. Regarding signals: onTextEdited is specifically user-edit driven and is not emitted when text is changed programmatically (e.g., by setting text or calling clear()), which makes it suitable for implementing “write back only when the user edits” logic without feedback loops [3]. Recommended pattern: use onTextEdited (or onEditingFinished if you only want commit at the end) to update your backing property, and avoid imperative assignments that overwrite/replace the TextField’s binding. That is, make the TextField driven by a single source of truth and only update that source from user edits. Example (one-way binding TextField -> model on user edits; model -> TextField via binding): TextField { id: tf text: someProperty // binding onTextEdited: someProperty = text // user-driven update } If you truly need to temporarily override the behavior of the text binding, you’d need to conditionally control the binding or explicitly restore it (e.g., using Qt.binding()), rather than doing unqualified imperative assignments that remove the binding [1].
Citations:
- 1: https://stackoverflow.com/questions/30087225/why-qt-qml-doesnt-properly-bind-properties
- 2: https://www.kdab.com/new-qt-5-10-diagnostics-breaking-qml-bindings/
- 3: https://doc.qt.io/Qt-6/qml-qtquick-textinput.html
Fix the two-way binding by using onTextEdited instead of onTextChanged and explicitly resync after Browse selections.
When a TextField has a binding like text: animToolsCol.outputDir and also updates the bound property via onTextChanged, later imperative assignments (such as animToolsCol.outputDir = picked from Browse) destroy the binding. This leaves the visible field out of sync with the actual value used during baking.
Qt's binding behavior is explicit: imperative assignments remove bindings. The standard pattern is to use onTextEdited (which fires only on user input, not programmatic changes) for model updates, and rely on the binding to keep the field in sync when the model changes from other sources. Add a Connections block to explicitly resync if the model is updated while the user has focus.
🛠️ Suggested fix
TextField {
id: animOutField
width: parent.width - animToolsCol.labelWidth - 6 - 60 - 6
height: 24
font.pixelSize: 11
text: animToolsCol.outputDir
- onTextChanged: animToolsCol.outputDir = text
+ onTextEdited: animToolsCol.outputDir = text
placeholderText: "/path/to/output"
enabled: !VATBakerController.isBaking
}
+ Connections {
+ target: animToolsCol
+ function onOutputDirChanged() {
+ if (animOutField.text !== animToolsCol.outputDir)
+ animOutField.text = animToolsCol.outputDir
+ }
+ }🤖 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 `@qml/PropertiesPanel.qml` around lines 689 - 735, Replace the two-way update
handler on the TextField by using onTextEdited instead of onTextChanged (i.e.,
in animOutField use onTextEdited: animToolsCol.outputDir = text) so programmatic
updates don't drop the binding, and after the Browse selection (inside
animBrowseMa.onClicked where you call VATBakerController.chooseOutputDir and set
animToolsCol.outputDir = picked) explicitly resync the field by setting
animOutField.text = picked; additionally add a Connections block targeting
animToolsCol to handle its outputDir change (e.g., Connections { target:
animToolsCol; onOutputDirChanged: if (!animOutField.activeFocus)
animOutField.text = animToolsCol.outputDir } ) so external model updates are
reflected when the user is not actively editing.
Two bugs the user spotted in the Godot harness: 1) One-frame "blob" artifact at certain frames Root cause: `filter_nearest` on the packed-normals texture rounds (not floors) when computing nearest texel, so V values landing near the half-texture boundary at frame N (the last position row) could slip into row N (the first normal row). The decoded normal vector got reinterpreted as a position offset → vertices shoot to "garbage" locations for one frame, then snap back. Fix: stop relying on sampler rounding. Compute integer row indices `curr_row` / `next_row` from `floor(current_frame)`, sample both rows via texelFetch (Godot) / `_PosTex.Load()` (Unity HLSL), then `mix()`/`lerp()` based on the fractional part. Bypasses the sampler path entirely — there is no boundary to round across. Bonus: this is what the sharpen3d/openvat reference shader does for the exact same reason (LegacyPBR-GLSL.gdshader L74-91). Bonus 2: free inter-frame interpolation, no longer "bit-exact at fps fractions" only. 2) Wrong normals (lighting inverted) The FBX → Ogre import path applies `aiProcess_ConvertToLeftHanded`, which flips winding without flipping the captured normal vector. The baked normals point INTO the surface. The previous version of this shader (pre-OpenVAT collapse) negated the normal on read; the collapse removed the negation, and the "dark where there should be light" symptom returned. Fix: `NORMAL = -normalize(n)` in Godot; `-n` in the Unity Object → World transform. Both restored from pre-OpenVAT-collapse state. Both fixes applied to both harness shaders so Godot + Unity stay in visual lockstep. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
4 tasks
fernandotonon
added a commit
that referenced
this pull request
May 20, 2026
* feat(vat): UV2-based engine shader templates + harness rewrite Follow-up to #620. The merged PR landed the OpenVAT-only baker; this extends the consumer side: harness shaders that handle real-world bake variants and drop-in engine templates for users. Harness changes (tools/godot-vat-test/) --------------------------------------- - Replaced sampler-based frame addressing with `texelFetch` / integer row arithmetic. The previous shader's `filter_nearest` rounded (not floored) V at the half-texture boundary, producing a one-frame position-into-normal slip ("blob artifact"). The new path computes `curr_row` / `next_row` manually and `mix()`es — matches sharpen3d's reference shader. - Switched to UV2-based texture addressing. Each vertex's UV2 holds its (col, base_row) into the texture — same as the canonical OpenVAT Godot shader. Handles both single-row layouts (QtMeshEditor's own bakes) AND multi-row tile layouts (Blender's "Use Single Row OFF" mode, e.g. the Barril sample). Meshes lacking UV2 get one synthesized in `_ensure_uv2_on_mesh` from the bake's known width + frame count. - Auto-detect packed vs. separate-normals layout by filename (`*_pos.*` vs `*_vat.*` + `*_vnrm.*`). Separate mode is what Blender exports when "Vertex Normals = Separate"; the bundled Barril EXR sample is in that mode. - FBX path through Godot's editor-side import (4.3+). The runtime GLTFDocument path is kept for .gltf/.glb. Sidecar JSON is now optional with a unit-bounds fallback so a Blender export missing remap_info.json renders something instead of nothing. - Various correctness: Main.gd null-guards before reading VATPlayer internals; SkeletalLoader same .fbx/.gltf branching; `loop_frames` clamped to `_frame_count`; tscn `source_gltf` path normalized. Engine shader templates (tools/vat-shaders/) -------------------------------------------- Drop-in shaders + a one-page README for users who want to play VAT bakes without writing engine code: openvat.gdshader Godot 4 spatial shader, 100 lines. openvat.shader Unity 2022+ BiRP, 175 lines, includes a URP migration note + a C# UV2-synthesis helper. openvat.usf Unreal 5 HLSL snippet for a Material's Custom node, with full Material-editor wiring walk- through (Surface domain, Shading Model, the ScalarParameter / VectorParameter inputs). README.md Texture-import settings per engine, UV2 gotcha and three ways to satisfy it, sidecar string- float parsing, normal-flip toggle (drop the negate for non-QtMeshEditor sources). CLI integration: - `qtmesh --help` description of `vat` now references the `tools/vat-shaders/` directory. - post-bake output adds a `shaders:` line pointing at the dir. Skipped (deliberate) -------------------- - Separate-EXR output mode in our exporter. Our packed PNG is already 16-bit per channel, so the only argument for separate EXR is the half-float `no_remap` Niagara workflow — not a use case we have a real user for yet. The harness consumes Blender's separate-EXR bakes for verification; QtMeshEditor only writes the packed format. - glTF exporter UV2 preservation. `qtmesh convert` strips TEXCOORD_1 today (Ogre→assimp re-export drops the second channel). Tracked follow-up; the README documents the `assimp export` workaround. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review(vat-test): pair sidecar + texture by basename (P1) Codex flagged a P1 in VATPlayer._load_bake: the loader picked the first *-remap_info.json AND the first *_pos/_vat texture independently. With multiple/partially-staged bakes in `bake_dir`, this would silently decode pixels from one bake with metadata (frame count + bounds) from another, producing wrong scale and animation without failing fast. Fix: anchor by sidecar basename when one exists. Strip '-remap_info.json' to get the basename, then probe for <basename>_pos.{png,exr} (packed) or <basename>_vat.{png,exr} (separate). Hard error if the matching texture is missing — no silent fallback to whichever file happens to enumerate first. Sidecar-less bakes keep the fallback "pick the first texture, derive basename from it" path because they're already a degraded mode (unit- bounds fallback); same-folder ambiguity is the user's problem to clean up. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * review(vat): Unreal WPO delta + README fenced-block languages PR #640 review feedback: 1. CodeRabbit Major: Unreal's WorldPositionOffset expects a DELTA from the bind pose, not an absolute object-space position. The previous Custom node returned `absolutePos` and the setup instructions deferred the subtraction to the material graph as a "remember to wire this" note — which is exactly the kind of step users miss. Fix: add `LocalPosition` as a Custom node input (wired in via UE's built-in `LocalPosition` material node) and return `absolutePos - LocalPosition` inside the HLSL. The user now wires the Custom node's WorldPositionOffset output straight to the material's WPO pin — no extra subtraction node needed. Renamed the struct field from `ObjectPos` to `WorldPositionOffset` so the output name matches the material pin it's intended for. 2. CodeRabbit Minor (markdownlint MD040): Two fenced code blocks in README.md lacked language identifiers. Added `text` for the bake file layout diagram and `cpp` for the shader-snippet comment block. The Codex P1 (sidecar/texture basename pairing) was already addressed in 53495a1; no further code change needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(vat): Godot demo project — website embed + perf comparison Three scenes in tools/godot-vat-demo/: scenes/demo_web.tscn Single Rumba VAT dancer with orbit camera. For the website embed. Drag to orbit, wheel/+/- to zoom. Polished lighting + glow + light contrast bump for a punchier first impression. scenes/demo_perf_vat.tscn 1000 Rumba instances driven by VAT, in a 32×32 grid with random rotations and desynchronised frame phases (so the GPU can't optimise away identical work). FPS overlay shows current FPS, rolling 1-second window minimum, and worst-since-start. scenes/demo_perf_skeleton.tscn Same 1000 instances driven by Godot's SkinnedMeshRenderer + AnimationPlayer for direct comparison. Each instance is a runtime- instantiated PackedScene (loaded once and cached so we don't pay 1000× GLTFDocument parses at startup) with seek()ed start phase. Shared infra: scripts/VATInstance.gd — streamlined VAT player (vs. the test harness's `VATPlayer.gd` which has multi-bake + Barril/EXR support). Single bake, packed normals, supports self-driven or externally-driven current_frame for shared-clock setups. scripts/OrbitCamera.gd — mouse drag orbit, wheel + keyboard zoom, configurable distance bounds. Targets the scene's main subject. scripts/FPSOverlay.gd — Label child of a CanvasLayer. Updates 4×/sec with rolling-window min FPS so hitches surface clearly (averages hide them). scripts/PerfSpawner*.gd — both spawners build the grid in _ready, stagger frame phases via random seed, share the same source mesh + bake_dir defaults so swapping scenes is a no-op. Bake assets live at tools/godot-vat-demo/assets/Rumba/ — a copy of the Rumba bake from tools/godot-vat-test/assets/ so the demo project is self-contained (the test project keeps its own copy for the side-by-side harness). README at tools/godot-vat-demo/README.md covers: - How to run each scene - Web export steps - What to look for in the perf comparison (the rolling minimum is the most honest metric) - One-paragraph explainer of what VAT actually is for users landing on the demo without prior context 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(vat-demo): UV2 must use texture width + global vertex index The web demo rendered as an "egg of triangles" — every submesh was sampling the SAME columns of the position texture. Rumba's source.gltf has 11 submeshes (1024 + 414 + 112 + 1306 + 381 + 454 + 93 + 93 + 381 + 532 + 1038 = 5828 verts) and the bake's texture is 5828 wide, so each vertex needs to land on a UNIQUE column. Two bugs in `_ensure_uv2_on_mesh`: 1. `width` was computed from `mesh.surface_get_arrays(0)[ARRAY_VERTEX].size()` — the FIRST submesh's vertex count (1024). The other 10 submeshes wrapped at column 1024 → garbled animation. 2. `col = j % width` used the PER-SUBMESH index, so vertex 0 of every submesh sampled the same column. Visually that overlays all submeshes on top of each other → "blob" silhouette. Fix: pass the bake texture's actual width (`pos_tex.get_width()`) to the synthesizer, and accumulate a running offset across surfaces so the column comes from the GLOBAL vertex index, not the per-submesh one. Mirrors how the test harness's VATPlayer.gd already does it. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(vat): qtmesh vat exports a vertex-order-matching source.gltf The "egg of triangles" symptom in the demos was a vertex-order mismatch between the bake's texture columns and the source mesh's vertex indices. The bake walks Ogre's submeshes in submesh-index order; a separate `qtmesh convert` re-imports the FBX, runs it through assimp's post-processing (JoinIdenticalVertices, OptimizeMeshes, cache reordering), and emits the glTF in a DIFFERENT vertex order. Vertex `i` in the texture column no longer corresponds to vertex `i` in the glTF, so reconstructed positions land randomly across the silhouette — egg blob. Fix: `qtmesh vat` now ALSO writes a `<outDir>/source.gltf` immediately AFTER importing the FBX and BEFORE running the bake. Both the bake's collect loop and the glTF exporter iterate the SAME Ogre entity in the SAME submesh-index order. The bake's column `i` is now guaranteed to correspond to glTF vertex `i`. Three subtleties hit during the fix: 1. Doing the export AFTER the bake produced a malformed glTF (meshes-as-dict instead of meshes-as-array). The bake's software-skinning request leaves Ogre in a state the exporter mishandles. Exporting BEFORE the bake sidesteps this entirely. 2. The output directory has to be `mkpath`'d before the export — `VATBaker::bake` creates it inside its own flow, which is too late for the pre-bake glTF write. 3. `MeshImporterExporter::exporter` takes the display-name format string (e.g. "glTF 2.0 (*.gltf)"), not the short id ("gltf2"). Short forms route to a different code path that produces malformed glTF. Use `formatForExtension` to get the right name from the output path. Also re-staged the demo project's `assets/Rumba/source.gltf` against the new self-exporting `qtmesh vat` so the web + perf demos render correctly. The demo project's VATInstance.gd kept its UV2 synthesis math; the math was correct all along — the source mesh just had the wrong vertex order. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(vat): texelFetch path + MultiMesh perf spawner Two related fixes after testing the demo end-to-end. 1) Single-row "egg of triangles" silhouette --------------------------------------------------------------- Our Rumba bake is 71 frames × 5828 verts in a 5828×142 texture. The previous UV2-based shader computed v_pos = 1 - 70/142 - 0.5/142 = 0.5035, which puts the position sample for frame 0 EXACTLY on the V=0.5 boundary between the position half and normal half of the texture. With filter_nearest, GPU rounding behavior at .5 is hardware-dependent — on Apple Silicon Metal it lands in the normal half and decodes encoded normals as positions, producing the "egg of triangles" silhouette the user reported. Fix: when GDScript synthesizes UV2 (the QtMeshEditor case where the imported mesh lacks an authored UV2 channel), pack UV2 as INTEGER (column, row_block) pixel coordinates and have the shader sample via `texelFetch(pos_tex, ivec2(col, base_row + frame), 0)`. Integer indexing has no half-pixel boundary, no rounding ambiguity. The shader keeps the textureLod path alongside for Blender-authored float UV2 (e.g. the Barril sample), gated by a `synthesized_uv2` uniform that the GDScript sets per bake source. Applied to: tools/godot-vat-test/scripts/VATPlayer.gd (harness), tools/godot-vat-demo/scripts/VATInstance.gd (demo), tools/vat-shaders/openvat.gdshader (Godot template), tools/vat-shaders/openvat.shader (Unity template), tools/vat-shaders/openvat.usf (Unreal template). All three templates carry the dual-path shader so consumers don't have to choose at integration time. 2) VAT perf demo slower than skeleton perf demo --------------------------------------------------------------- The perf comparison was upside down — VAT should be ~2-3× faster than skeletal at 1000 instances, but the user saw the opposite. Root cause: my spawner created 1000 independent VATInstance nodes, each with its own unique ShaderMaterial → 1000 unique materials × 11 submeshes = 11,000 draw calls per frame. Plus each instance ran a fresh GLTFDocument load on _ready, so spawn time was several seconds. Fix: spawn via a single MultiMeshInstance3D. Mesh, texture, and shader load ONCE. The crowd is 1000 instance transforms + INSTANCE_CUSTOM data fed into one MultiMesh. Result: one batched draw call per surface for the whole crowd, ~50 ms to spawn 1000 instances. Per-instance frame phase moves from individual material uniforms to `INSTANCE_CUSTOM.r` (Godot's built-in 4-float per-instance attribute, readable in the vertex shader). The crowd remains desynchronised so the GPU's texture cache can't elide repeated work — VAT's honest worst-case cost. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(vat-demo): uncap FPS on perf scenes + skeleton spawner notes Two follow-ups after the perf reset: 1) Uncap FPS in both perf scenes Default Godot caps render to display vsync — on a ProMotion Mac that pins the FPS overlay at 120 even when there's significant GPU headroom. Both spawners now call: DisplayServer.window_set_vsync_mode(VSYNC_DISABLED) Engine.max_fps = 0 so the overlay shows the actual ceiling and the comparison surfaces VAT's real headroom over the skeletal path. 2) Skeleton spawner header comments Explain what's already shared across the 1000 instances (Mesh, Material, Animation resources via the PackedScene cache; automatic in Godot) and what CAN'T be (Skeleton bone state, SkinReference, AnimationPlayer time — each instance has its own). This is the realistic skinned-NPC path; Godot has no built-in MultiMesh equivalent for skinned meshes. The comparison against the MultiMesh-VAT spawner is fair real-world. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(vat-demo): vsync off at project level for perf scenes The runtime `DisplayServer.window_set_vsync_mode(VSYNC_DISABLED)` in the perf spawners wasn't taking effect — FPS overlay stayed pinned at 120 (ProMotion display refresh). Godot's macOS Metal backend binds vsync at window-creation time, before any GDScript runs, so the runtime call landed too late. Fix: set `display/window/vsync/vsync_mode=0` in project.godot. The runtime call is kept as a defensive belt-and-braces in case the project setting gets reverted in the editor. Note this also uncaps the web demo scene at startup. That's intentional — for a one-character orbiting demo the GPU is doing basically nothing per frame and the overlay headroom isn't user- facing anyway. If we ship a polished web build we can flip vsync back on for the web demo only via a per-scene runtime call. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(website): embed VAT demo as live iframe The marketing page now has a "Live demo" section (between Mixamo workflow and Pipeline) embedding the Godot Web export of the VAT showcase. Drag-to-orbit, scroll-to-zoom, no install needed — a visitor lands on the page and sees a baked skeletal animation playing back via vertex shader in real-time. Pieces ------ tools/godot-vat-demo/export_presets.cfg Web export preset. variant/thread_support=false → single-threaded build (~38 MB total: 36 MB WASM runtime + 1.7 MB pck + glue scripts). Single-thread skips the SharedArrayBuffer requirement, so the bundle works in any iframe without COOP/COEP headers. Output path: ../../website/public/demo/index.html (relative to the demo project), so re-exporting drops files directly into the Vite public dir. tools/godot-vat-demo/scripts/WebDemoMain.gd Scene-level controller for demo_web.tscn that re-enables vsync. project.godot has vsync OFF (perf scenes need it that way for honest FPS measurement); browser-embedded demos want smooth 60 Hz, not uncapped CPU/GPU use. Per-scene override via DisplayServer.window_set_vsync_mode(VSYNC_ENABLED). website/public/demo/ The Godot Web export output (committed). README.md documents how to regenerate (`godot --headless --export-release "Web"` from the demo project dir). website/src/App.jsx + App.module.css New <Section id="vat-demo"> with an <iframe src="demo/index.html">. Loading="lazy" so the 36 MB WASM doesn't block first-paint. 16:9 aspect-ratio container, max-height: 540px so it doesn't dominate tall viewports. Caption block below links to the CLI usage and the tools/vat-shaders/ templates for engine integration. tools/godot-vat-demo/.gitignore Standard Godot editor cache excludes (.godot/, *.uid, *.import). Re-exporting after demo changes ------------------------------- The preset writes back into website/public/demo/ automatically. The website build picks up the new files on next `npm run build` — Vite copies website/public/* verbatim into the build output. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
unity/unreal/godot/agnostictargets and thergba8/rgba16encoding switch and the--normalsflag are all gone.<basename>_pos.png— 16-bit RGB, width = vertexCount, height = 2 × frameCount, packed (top half positions, bottom half normals)<basename>-remap_info.json— canonical{ "os-remap": { Min, Max, Frames } }sidecar with stringified 8-decimal-place floats, bounds rounded outward to 0.1Why
The pre-existing per-engine targets were all "QtMeshEditor's own bake" variants that only our handwritten harness shaders consumed. None dropped into a stock engine project. OpenVAT is the cross-engine community convention with real consumer shaders. Real downstream readers exist; emit what they read.
Breaking
qtmesh vatflag removals:--target,--encoding,--normals. Thebake_vatMCP tool schema loses the same.VATBakerController::bake()goes from 7 args to 4.VATBaker::Optionsdropstarget/encoding/bakeNormals.VATBaker::BakeResultdropsnrmTexPath/unityMetaPath/godotShaderPath. Public encode/decode static helpers onVATBakerare removed.No external consumers — VAT has not been released yet.
Verified
Follow-up
The Godot / Unity test harnesses (
tools/godot-vat-test/,tools/unity-vat-test/) need new runtime shaders that consume the packed-normals texture + os-remap sidecar. The staging shell scripts already pass through to the new CLI shape; only the GDScript/C# shaders need a rewrite. Not blocking — real users get canonical OpenVAT.Test plan
OpenVATSidecarMatchesReferenceShape,OpenVATBoundsRoundedOutwardToTenth,OpenVATTextureIs16BitRgb🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor / Changes
Documentation
Tests