fix(export): glTF/GLB skip ConvertToLeftHanded — fixes VAT mesh artifacts - #646
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds VAT vertex-permutation support (CLI helpers, VATBaker API and packing) to align exported glTF vertex order with Ogre bake ordering; refines Assimp material/export handling (header, diffuse texture-unit routing, handedness/UV flags for glTF); updates demo assets, export preset, Godot VAT playback, and demo file-size. ChangesVAT permutation and baking
Assimp handedness & material routing
Demo and Godot VAT playback
Sequence Diagram(s)sequenceDiagram
participant CmdVat
participant AssimpExporter
participant GLTFParser
participant OgreVB
participant VATBaker
CmdVat->>AssimpExporter: request export -> produces source.gltf (+ .bin buffers)
AssimpExporter-->>CmdVat: source.gltf + URIs
CmdVat->>GLTFParser: parse POSITION/NORMAL/TEXCOORD_0 from source.gltf
GLTFParser->>CmdVat: flattened vertex signatures (with UV V flip)
CmdVat->>OgreVB: readOgreBindVertices() from Ogre vertex buffers
OgreVB-->>CmdVat: Ogre bind-pose signatures
CmdVat->>CmdVat: buildVertexPermutation() (quantized matching per submesh)
CmdVat->>VATBaker: bake(opts with vertexPermutation)
VATBaker-->>CmdVat: baked VAT PNG (packed using mapped columns)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
9b72805 to
6b9e177
Compare
The main Assimp exporter path applied ConvertToLeftHanded for every
format except DirectX (.x). glTF is right-handed like Ogre, so the
conversion negates Z and reverses triangle winding on every exported
.gltf / .glb.
This is silent for typical round-trips (an importer just sees the
mirrored mesh and doesn't care) but breaks any consumer that pairs
the glTF with a separately-computed vertex stream — concretely the
OpenVAT bake. `qtmesh vat` emits:
- <anim>_pos.png (positions read from Ogre's RH vertex buffer;
columns indexed by Ogre's vertex order)
- source.gltf (Assimp export; pre-fix had Z flipped)
The two were in different handedness spaces, so VAT shaders that
texelFetch positions by vertex index displaced every vertex against
a Z-mirrored bind pose. From the diff of two source.gltf files
baked before/after this change:
v0 before: x=-0.01144 y=+1.28867 z=+0.22631
v0 after: x=-0.01144 y=+1.28867 z=-0.22631
texel v0: x=+0.01476 y=+1.26171 z=-0.22101 <-- matches "after"
The pose exporter (`exportCurrentPose`) already has the correct
skip list ("x" || "gltf2" || "glb2"); this brings the main
`exporter()` path into alignment.
Demo asset re-bake is intentionally NOT included here: the current
Assimp export drops material/image references in fresh bakes (a
separate regression worth tracking on its own). Shipping just the
C++ fix avoids dragging that into this PR; the bundled
tools/godot-vat-demo/assets/Rumba/source.gltf will be regenerated
once the texture-binding regression is also resolved.
6b9e177 to
bb740d1
Compare
The website demo and any other OpenVAT consumer rendering meshes baked
by qtmesh vat hit three distinct alignment bugs. This commit lands them
together because they only become visible in combination, and isolating
any one made the other two appear "fixed" by accident.
1. ConvertToLeftHanded on glTF/GLB export (handedness)
`MeshImporterExporter::exporter` applied aiProcess_ConvertToLeftHanded
for every format except DirectX (.x). glTF/GLB are right-handed like
Ogre, so the conversion negates Z and reverses triangle winding on
every exported file.
The pose exporter (`exportCurrentPose`) already had the correct skip
list ("x" || "gltf2" || "glb2"); this brings the main exporter into
alignment. Without the fix, source.gltf was Z-mirrored relative to
the position texture the bake wrote — consumers fetched a vertex
from the wrong half of the silhouette and the model "looked rotated".
2. Vertex-column permutation (defensive remap)
Assimp's gltf2 exporter hardcodes
`aiProcess_JoinIdenticalVertices | aiProcess_SortByPType` as
mandatory pre-processing (assimp/code/Common/Exporter.cpp:186) —
no flag from our calling code can disable them. For meshes with
non-unique skinning weights JoinIdenticalVertices does merge or
reorder vertices within a primitive, which would silently break
the bake's column-index → vertex-index relationship.
`cmdVat` now exports the glTF, reads its position buffer back, and
builds a per-vertex permutation by matching positions against
Ogre's bind-pose buffer. The permutation is passed via
`VATBaker::Options::vertexPermutation` and applied at PNG-encode
time so each row's columns land in the glTF's vertex order.
For meshes where Assimp's join is a no-op (this is the common case
— combinations 1+3 mask 2 in current asset coverage) the
permutation is identity and costs nothing.
3. Texture references dropped on glTF export (auto-numeric TUS names)
When `qtmesh vat` re-runs against an asset that already has a
`.material` sidecar from a prior export, Ogre auto-parses that
.material on `initialiseResourceGroup`. The script's
`texture_unit { texture ... }` blocks (no explicit name in the
block header) get Ogre's auto-generated names "0", "1", ...
`MaterialProcessor` then sees the material as "already exists"
and takes the additive existing-material path that does not
rewrite TUS names back to "diffuse_map".
When `buildAiMaterialFromOgre` then walked those TUS for the
glTF export, "0" matched none of the canonical slot names and
fell through to `aiTextureType_UNKNOWN` — which Assimp's gltf2
exporter does not surface as a `baseColorTexture`. End result:
meshes rendered without diffuse on every re-bake.
Fix: treat all-digits TUS names (Ogre's auto-numeric default)
the same as empty and "diffuse_map" — route as DIFFUSE.
Also re-runs the bundled demo bake + Godot web re-export against
the fixed binary so `website/public/demo/index.html` reflects all
three fixes on the next deploy.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/CLIPipeline.cpp (1)
5266-5272: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winBreadcrumb the new
source.gltfexport.This PR adds a second file export in
cmdVat, but only the final bake write is breadcrumbed. Adding afile.exportbreadcrumb around thissource.gltfwrite will keep Sentry traces aligned with the new I/O step and its fallback path. As per coding guidelines "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message). Use '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/CLIPipeline.cpp` around lines 5266 - 5272, The new export of "source.gltf" is not breadcrumbed for Sentry; wrap the export call in a breadcrumb by calling SentryReporter::addBreadcrumb("file.export", "<path or message>") immediately before (and optionally after on success/failure) the MeshImporterExporter::exporter(...) invocation that writes QFileInfo(QDir(outDir).filePath("source.gltf")).absoluteFilePath() (the gltfPath used in cmdVat), so the I/O step and its fallback path are recorded alongside the final bake breadcrumb.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/CLIPipeline.cpp`:
- Around line 5144-5162: The current matching uses only quantized POSITION
(Key/quantize) and resolves collisions by FIFO (bucket.front()/erase), which can
misassign vertices across UV/normal/skin splits; instead, when resolving a
bucket for each ogre[i] iterate the bucket[k] candidates and select the
candidate whose full exported vertex signature (positions already matched plus
normals, UVs, and skinning/joint/weight data from gltf[] vs ogre[]) equals
ogre[i]; if exactly one matches assign that index into perm[i] and remove that
candidate; if multiple or none match, do not pick arbitrarily — clear perm and
return perm (or fall back to the safer code path). Update the resolution logic
referencing bucket, Key, quantize, perm, gltf[], and ogre[] to perform
attribute-wise comparison (normals/uvs/joints/weights) before assigning and only
erase the matched candidate.
- Around line 5286-5316: The code currently keeps gltfPath advertised even when
readGltfPositions fails or when vertexPerm is empty (alignment failed), which
misrepresents a degraded bake as matching glTF vertex order; update the logic
after calling readGltfPositions(...) and after building vertexPerm() (in the
block around readGltfPositions, submeshStarts, and buildVertexPermutation) to
clear or invalidate gltfPath (or set a boolean flag indicating alignment
success) whenever readGltfPositions returns false, the ogre/gltf counts differ,
or vertexPerm.empty() is true, and ensure the later JSON/text output only emits
source.gltf / "vertex order matches the bake" when that alignment-success flag
is true (also apply the same change to the other similar block around lines
5355-5374 that uses the same readGltfPositions/buildVertexPermutation flow).
In `@src/VATBaker.cpp`:
- Around line 538-547: The current validation only checks
opts.vertexPermutation.size() but must also ensure every entry is in range [0,
vertexCount) and all entries are unique before calling packOpenVAT16; update the
validation around opts.vertexPermutation (the same block that sets result.error)
to iterate the permutation, confirm each value is >=0 and < vertexCount and that
no index is repeated (e.g., using a seen vector or set), and if any out-of-range
or duplicate is found set result.error to a clear message and return result so
packOpenVAT16 is never called with an invalid permutation.
---
Outside diff comments:
In `@src/CLIPipeline.cpp`:
- Around line 5266-5272: The new export of "source.gltf" is not breadcrumbed for
Sentry; wrap the export call in a breadcrumb by calling
SentryReporter::addBreadcrumb("file.export", "<path or message>") immediately
before (and optionally after on success/failure) the
MeshImporterExporter::exporter(...) invocation that writes
QFileInfo(QDir(outDir).filePath("source.gltf")).absoluteFilePath() (the gltfPath
used in cmdVat), so the I/O step and its fallback path are recorded alongside
the final bake breadcrumb.
🪄 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: b5af709c-6686-48e3-8c4d-16df606c4ad0
⛔ Files ignored due to path filters (3)
tools/godot-vat-demo/assets/Rumba/mixamo.com_pos.pngis excluded by!**/*.pngtools/godot-vat-demo/assets/Rumba/source.binis excluded by!**/*.bintools/godot-vat-demo/assets/Rumba/source.gltfis excluded by!**/*.gltf
📒 Files selected for processing (6)
src/CLIPipeline.cppsrc/MeshImporterExporter.cppsrc/VATBaker.cppsrc/VATBaker.hwebsite/public/demo/index.htmlwebsite/public/demo/index.pck
✅ Files skipped from review due to trivial changes (1)
- website/public/demo/index.html
Addresses three CodeRabbit review notes on PR #646: 1) **Critical / VATBaker** — validate `vertexPermutation` is a full bijection over `[0, vertexCount)` before passing to `packOpenVAT16`. Length-only check let through entries that walked past the PNG row stride or duplicated columns, silently corrupting the bake. 2) **Major / CLIPipeline** — only advertise `source.gltf` in the bake report when alignment actually succeeded. Previously a degraded bake (failed glTF read-back, count mismatch, or ambiguous permutation) silently fell back to identity packing while the report still claimed "vertex order matches the bake" — handing consumers a mesh that didn't match the texture columns. Now gated behind a `sourceMeshMatchesBake` flag set only on a successful permutation match, with explicit warnings on every fallback path. 3) **Major / CLIPipeline** — extend the bake↔glTF match key from position-only to (position, normal, UV0). On UV seams, hard edges, and weight-splits, multiple distinct vertices share the same bind-pose position; matching by position alone would FIFO-resolve them arbitrarily and misroute normals between them, producing subtle shading glitches. Two-tier quantization: positions at 1e-5 (sub-mm on a 1-unit model, exact through float32 round-trip), normals + UVs at 1e-3 (Assimp re-normalises normals on glTF export — empirically the normal magnitudes drift by up to ~1e-5 per component, which a 1e-5 quantizer split into separate buckets). UVs are V-flipped on the glTF side to undo Assimp's implicit `aiProcess_FlipUVs`. Ambiguous buckets (two glTF vertices with identical quantized signature) now refuse the permutation and fall back to identity packing rather than guessing — same outcome as a missing match. Tested against Rumba Dancing.fbx: full 5828-vertex bind-pose signature matches the post-export glTF exactly, permutation is identity for this asset (Assimp's JoinIdenticalVertices was a no-op), `mesh: ... (vertex order matches the bake)` correctly advertised.
|
@coderabbitai all three notes from this pass were already addressed in commit
Verified end-to-end against Rumba Dancing.fbx: the matcher walks all 5828 verts, finds unique signatures for every one, permutation is identity for this asset (Assimp's JoinIdenticalVertices happens to be a no-op here), and |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 402
The identity-permutation result on Rumba Dancing.fbx (5828 verts, [resolve] 🐇 ✨ |
The regression you saw on the website demo (shattered triangles, no diffuse — screenshots #2 and #3) traces back to PR #640's switch to UV2-based bake addressing. Symptom and root cause: - The OpenVAT bake walks Ogre's vertex-buffer in submesh-index order and writes texture column `c` for the c-th Ogre vertex. - The consumer's UV2 is supposed to carry that same column index so the shader's `texelFetch(pos_tex, ivec2(col, frame), 0)` reads the right vertex. - The original UV2 setup ran INSIDE Godot at runtime and assumed that Godot's loaded vertex order matches the file's vertex order — i.e. that `mesh.surface_get_arrays(i)[ARRAY_VERTEX][j]` is whatever Ogre wrote at column `running_offset + j`. That assumption is false. Godot's resource importer reorders the imported mesh's vertex buffer (cache locality + vertex compression). The same broken assumption sits inside Unity's and Unreal's import pipelines too — anywhere we shipped a bake against a separately- imported source mesh, the column index → vertex index relationship was silently broken at engine-import time. This commit fixes the demo (and unblocks the same pattern for the Unity / Unreal shader templates) by shipping a small per-vertex **bind sidecar** that the consumer uses to recompute UV2 against its own post-import vertex order. Pipeline: C++ (qtmesh vat): Emit `<basename>_ogre_bind.bin` alongside the texture. Layout: uint32 magic = 0x42565442 ("BTVB"), uint32 version = 1, uint32 vertexCount, uint32 flags (pos | normal | uv), then per-vertex: 3 floats position, 3 floats normal, 2 floats UV0. Total cost on Rumba Dancing: 186 KB for 5828 verts. GDScript (VATInstance): On _ready, parse the sidecar. Build a (pos, normal, UV0) → ogre_index lookup. For each Godot vertex, look up its sidecar column by quantized-signature match (1e-5 for positions, 1e-2 for normals after observing 5e-3 drift from Godot's re- normalisation, 1e-3 for UVs). Undo Godot's glTF V-flip on the consumer side before matching. Write the resulting Ogre column index as UV2. Ambiguous buckets (verts that share all three fields — degenerate weight splits) fall back to identity and surface a count via push_warning. Godot export preset: include_filter="*.bin" so the sidecar survives `--export-release` (default `all_resources` filter doesn't ship non-resource files). End-to-end on Rumba Dancing: 5791/5828 (99.4%) of Godot vertices get the exact Ogre column they should. 37 verts (0.6%) hit the ambiguity sentinel and fall back to identity — these are degenerate weight-only splits where the signature can't disambiguate. They'd visually overlap regardless of which side we pick. Also bumps the bundled Godot web export + the demo `source.gltf` / `_pos.png` / `_ogre_bind.bin` so the live website picks up the fix on next deploy.
The signature-bucket approach left 37 verts on Rumba unmatched
because Godot's importer drift on normals (~5e-3 per component)
straddles the 1e-2 quantization grid for some verts — value
-0.324984 (Ogre) buckets to -32 while value -0.325031 (Godot,
post-renormalise) buckets to -33, even though they're the same
underlying normal. Loosening to a 5e-2 grid still missed 12 verts
because boundary cases just moved to the new grid.
Drop the quantize-then-bucket approach for normals + UVs. Instead:
- Bucket by quantized POSITION only (1e-5, exact through float32)
- Each bucket holds ALL candidate Ogre indices that share that
position (degenerate splits — same point in space, different
weights/normals/uvs)
- At match time, pick the candidate whose actual normal and UV
are closest by squared-distance — continuous comparison, no
further quantization, no grid-boundary effects
On Rumba: 5828/5828 verts now match (was 5791/5828 = 99.4%). The
remaining 37 were the verts I was warning about in the previous
commit — they're now disambiguated correctly. Cost is still
O(N + matches) since the typical bucket holds 1 or 2 candidates
and the distance check is constant-work per candidate.
Re-runs the Godot web export so the website demo picks up the
fully-matched bake.
When I dropped aiProcess_ConvertToLeftHanded for glTF in the earlier handedness fix, I also dropped the V-flip that was bundled into it. Symptom: every glTF consumer (Godot, three.js, Blender) V-flips on import per the glTF spec (V=0 at top), so without a compensating flip on EXPORT the UVs end up upside- down — face textures land on legs, shirt on hat, etc. The website demo screenshot showed exactly this. Bring back V-flip as a standalone aiProcess_FlipUVs flag for glTF/GLB only. Unlike ConvertToLeftHanded, FlipUVs touches only the UV V coordinate — it leaves positions, normals, and winding in Ogre's RH convention. So the OpenVAT bake (which doesn't depend on UVs at all — texture columns are vertex-index addressed) is unaffected, and consumers that round-trip through the glTF + their own importer's V-flip land in the right convention. Also drops the 1-V flip from VATInstance.gd's matcher — the sidecar V (Ogre native) and Godot's post-import V are now equal, so no in-script flip is needed. Re-runs the bundled demo bake + Godot web re-export so the website picks up the texture fix on next deploy.
The shader was negating the decoded normal to compensate for a historical inconsistency: aiProcess_ConvertToLeftHanded was applied on the IMPORT side (FBX → Ogre) but not on the EXPORT side (Ogre → glTF), so the bake's source mesh and the exported glTF disagreed on the Z sign — and the negation papered over it. After the recent exporter rework, both sides are in the same RH frame: positions, normals, and winding all consistent. The texture's normals are already in Godot's expected convention. The negation now flips them WRONG and produces back-lit shading. Re-runs the Godot web export so the website picks up the correct lighting.
|
…#647) PR #646 fixed VATInstance.gd to read the per-vertex Ogre bind sidecar (`<basename>_ogre_bind.bin`) and remap each Godot vertex back to its baker-side column index, so the bake survives Godot's import-side vertex reorder. PerfSpawnerVAT.gd is the MultiMesh variant of the same script and was missed in that commit — it still used the naive `running_offset + j` identity UV2 + the historical `NORMAL = -normalize(n)` negation, so the 1000-instance perf scene rendered with scattered triangles AND inverted lighting. This commit: - Inlines the bind sidecar loader, the position-bucket lookup, and the continuous-distance tiebreaker from VATInstance.gd (kept inline rather than shared via class_name to avoid a cross-dep between the single-instance player and the spawner). - Drops the `NORMAL = -normalize(n)` negation in the inline shader, mirroring the VATInstance shader fix. Verified by spawning the 1000-instance scene headless: all 5828/5828 verts match the sidecar with no fallback. PerfSpawnerSkeleton doesn't touch VAT (pure skeletal animation through Godot's stock SkinnedMeshRenderer), so no change needed there. Re-runs the Godot web export so the website's perf-VAT tab picks up the fix on next deploy.
Bumps the project version to 3.3.0 in CMakeLists.txt (single source of truth — sync-doc-versions-from-cmake.sh propagates to README and the website's pinned action ref). The 3.x.y → 3.3.0 minor bump reflects the VAT pipeline (PRs #640 / #644 / #646 / #647 landing together): - `qtmesh vat <file> --anim <name> -o <dir>` CLI subcommand - OpenVAT-format 16-bit position+normal bake - Vertex-order-aligned source.gltf + Ogre bind sidecar so engine importers can realign UV2 to the bake's column order - Drop-in shader templates for Godot/Unity/Unreal at tools/vat-shaders/ - Live website demo at /#vat-demo (Showcase + 1000× VAT vs 1000× skeletal perf comparison) Website updates: - Deep-linking to any section (e.g. `/#vat-demo`, `/#install`, `/#cli`) now works on both initial load (React-mount-aware scroll-into-view via requestAnimationFrame) and during in-session hashchanges (smooth scroll). - Each Section title gains a hover-visible `#` anchor link so visitors can grab a shareable URL without dev-tools. - New "VAT" tab on the home page's CLI examples panel with a full `qtmesh vat` command demo. - New `cmd-vat` reference section in the docs (synopsis, options, examples, sidebar entry). - `scroll-behavior: smooth` + `scroll-margin-top: 1.5rem` on `section[id]` so anchor-scroll lands with breathing room rather than flush against the viewport top edge.



Summary
The main Assimp exporter path applied
aiProcess_ConvertToLeftHandedfor every format except DirectX (
.x). glTF is right-handed likeOgre, so the conversion flips the X axis and reverses triangle
winding on every exported
.gltf/.glb.For a typical round-trip (re-import the file back into another DCC
tool) this was silent — the importer just sees a mirrored mesh and
doesn't care. It became visible the moment we paired the glTF with a
separately-computed vertex stream: the OpenVAT bake.
qtmesh vatwrites two artifacts that must agree on vertex order:<anim>_pos.pngsource.gltfConvertToLeftHandedThe two were in different coord spaces, so the Godot/Unity/Unreal
shader (which fetches positions from the texture by vertex index)
displaced every vertex against a mirrored mesh — exactly the
"weird artifacts" the user reported. From the diff of two
source.gltffiles (pre-fix vs post-fix), every accessor min/max was swapped on
the X axis:
The pose exporter (
exportCurrentPoseat line 3080) already had thecorrect skip list (
"x" || "gltf2" || "glb2"); this PR brings themain
exporter()path (line 2804) into alignment.Bundled demo re-bake
Also re-runs the website's VAT demo bake + Godot web re-export so
the live demo at
website/public/demo/index.htmlreflects the fixon the next deploy. The OpenVAT sidecar JSON is byte-identical
(bounds unchanged — the bake itself was always RH and never wrong);
only
source.gltfand the web-export.pckmove.Test plan
cmake --build build_local --target QtMeshEditor— cleanrumba115/Rumba Dancing.fbxwith the fixed binary;resulting
source.gltfhas un-flipped accessor mins/maxesmore "weird artifacts" on the showcase or perf scenes
Related
Builds on top of master after #640. Independent of #644 (the
website tabs/MultiMesh PR) — both can land in either order.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores