vat: UV2-based engine shader templates + harness rewrite - #640
Merged
Conversation
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>
|
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 (5)
📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR adds OpenVAT shader templates and docs, updates CLI VAT to export/record source.gltf and reference shader templates, and enhances Godot test/demo tooling for multi-format mesh loading, UV2 synthesis, bake sidecar pairing, and shader-driven VAT playback including single-instance and 1000-instance demos. ChangesOpenVAT shader templates and test harness integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cccf2537db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+174
to
+177
| if f.ends_with("-remap_info.json") and json_path.is_empty(): | ||
| json_path = bake_dir.path_join(f) | ||
| break | ||
| 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.") | ||
| elif pos_path.is_empty() and (f.ends_with("_pos.png") or f.ends_with("_pos.exr")): | ||
| pos_path = bake_dir.path_join(f) |
There was a problem hiding this comment.
Selecting the first *-remap_info.json independently from the first *_pos/*_vat texture can silently combine metadata from one bake with pixels from another when bake_dir has multiple or partially-staged outputs (exactly the case this function comments about). In that scenario, _frame_count and bounds are parsed from one sidecar but applied to a different texture, producing incorrect animation/scale without failing fast. The loader should derive the texture(s) from the chosen sidecar basename (or validate all selected files share one basename) before decoding.
Useful? React with 👍 / 👎.
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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tools/vat-shaders/openvat.usf`:
- Around line 24-35: The setup instructions currently omit that World Position
Offset must receive a delta from the bind pose; update the Custom node and docs
so the VAT sample returns a displacement instead of absolute position by adding
a LocalPosition input to the Custom node (alongside PosTex, UV2, CurrentFrame,
FrameCount, BoundsMin, BoundsMax) and perform WorldPositionDelta =
SampledWorldPosition - LocalPosition inside the Custom node before assigning to
WorldPositionOffset.xyz; update the step-5 wiring text to state that the Custom
node outputs WorldPositionOffset (delta) and Normal, so users wire the delta
directly to "World Position Offset" without extra material-graph subtraction.
In `@tools/vat-shaders/README.md`:
- Around line 23-29: The two unlabeled fenced code blocks in
tools/vat-shaders/README.md (the block showing "<basename>_pos.png ...
<basename>-remap_info.json" and the shader/comment block starting with "//
Godot: NORMAL = normalize(n);") should be given language identifiers to
satisfy markdownlint MD040; update the first fenced block to use a plain text
language (e.g., change ``` to ```text) and update the shader/comment block to
use a language suited for comments (e.g., change ``` to ```cpp) so the
renderer/linter recognizes the blocks correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98412b34-0b89-4882-b087-6981b88c27fc
⛔ Files ignored due to path filters (1)
tools/vat-shaders/openvat.shaderis excluded by!**/*.shader
📒 Files selected for processing (8)
src/CLIPipeline.cpptools/godot-vat-test/scenes/Main.tscntools/godot-vat-test/scripts/Main.gdtools/godot-vat-test/scripts/SkeletalLoader.gdtools/godot-vat-test/scripts/VATPlayer.gdtools/vat-shaders/README.mdtools/vat-shaders/openvat.gdshadertools/vat-shaders/openvat.usf
💤 Files with no reviewable changes (1)
- tools/godot-vat-test/scenes/Main.tscn
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>
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>
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>
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>
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>
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/godot-vat-test/scripts/VATPlayer.gd (1)
455-499:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep UV2 encoding consistent across every surface once
synthesized_uv2is enabled.
synthesized_uv2is a single boolean for all materials, but this function only rewrites the surfaces that were missing UV2. On a mixed mesh, the shader switches every surface to the integertexelFetchpath while untouched surfaces still hold authored[0,1]UV2, so those surfaces will sample garbage.Either synthesize packed UV2 for all surfaces once any surface needs it, or track the mode per surface instead of per mesh.
🤖 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 455 - 499, The mesh ends up with mixed UV2 encodings because synthesized_uv2 is global but you only replace UV2 for surfaces that lack it; change the logic so that when any_synthesized becomes true you synthesize and write the packed integer UV2 for every surface (not just those where have_uv2 was false) before calling rebuilt.add_surface_from_arrays, or alternatively track per-surface mode; specifically update the loop around mesh.surface_get_arrays / Mesh.ARRAY_TEX_UV2 to generate the PackedVector2Array for all surfaces once any_synthesized is set and then use rebuilt.add_surface_from_arrays and rebuilt.surface_set_material as before so all surfaces use the same packed UV2 encoding that the synthesized_uv2 shader expects.
🤖 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 5076-5093: Add a Sentry breadcrumb for the source.gltf export so
the file export is tracked: immediately before calling
MeshImporterExporter::exporter (the block that creates outDir, computes
gltfPath, and calls exporter(entity->getParentSceneNode(), gltfPath,
formatForExtension(gltfPath))), invoke
SentryReporter::addBreadcrumb("file.export", gltfPath) (or a descriptive message
containing "source.gltf") so the export I/O is recorded in the timeline.
In `@tools/godot-vat-demo/README.md`:
- Line 9: The README references Unity's `SkinnedMeshRenderer`, which is
misleading for a Godot demo; update the line for
`scenes/demo_perf_skeleton.tscn` to use Godot-native terminology such as
`Skeleton3D` and/or "skinned mesh with AnimationPlayer" (or "skinned mesh
animation path") so readers expect Godot's Skeleton3D + AnimationPlayer setup
instead of Unity components.
In `@tools/godot-vat-demo/scripts/FPSOverlay.gd`:
- Around line 32-33: The rolling window is being trimmed by frame count using
Engine.get_frames_per_second(), which can be zero transiently and empty the
window causing _min_fps to stick at 0; change trimming to be time-based by
tracking elapsed time for entries (e.g., store per-sample delta or timestamp
alongside each FPS sample in _rolling_window or maintain a parallel
_rolling_window_duration), when adding a sample append its delta and while the
accumulated duration > 1.0s pop_front and subtract that popped duration from the
accumulator, then recompute _min_fps/_max_fps from the time-windowed samples;
stop using Engine.get_frames_per_second() for window size.
In `@tools/godot-vat-demo/scripts/OrbitCamera.gd`:
- Around line 7-8: OrbitCamera.gd advertises touch drag/pinch but only handles
mouse/keyboard; implement touch handling in the existing input handler (e.g.,
_input or _unhandled_input) to map single-finger drags to orbit and two-finger
pinch to zoom. Specifically: detect InputEventScreenTouch and
InputEventScreenDrag (or Input.get_last_mouse_speed equivalents for touch),
track one active touch id and use its delta to update the camera yaw/pitch the
same way mouse motion does (reuse the same rotation logic or functions that
currently handle mouse orbit), and when two touches are present compute the
distance between them each frame to derive a pinch delta and apply it to the
existing zoom/distance variable (reuse the same zoom function/limits used for
mouse wheel). Ensure touch state is cleared on touch release and that touch
gestures respect the same sensitivity/clamping as mouse controls (refer to the
camera rotation variables like yaw/pitch and zoom/distance used elsewhere in
OrbitCamera.gd).
In `@tools/godot-vat-demo/scripts/PerfSpawnerSkeleton.gd`:
- Around line 68-74: The code calls ap.play(clip) and ap.seek(...) even when
ap.get_animation(clip) returns null, causing runtime errors for missing clips;
modify the block so that after obtaining var anim := ap.get_animation(clip) you
only call anim.loop_mode = ..., ap.play(clip) and ap.seek(...) inside the anim
!= null guard (i.e., move the play/seek lines into the same conditional that
checks anim != null) so playback and seeking happen only when the animation
exists.
In `@tools/godot-vat-demo/scripts/PerfSpawnerVAT.gd`:
- Around line 44-46: Guard the result of doc.generate_scene(state) for null
before passing it to _first_mesh_in or calling scene.queue_free: after calling
generate_scene in PerfSpawnerVAT.gd, check if scene is null and if so call
push_error with a descriptive message and return early (or otherwise skip the
mesh extraction), otherwise proceed to call _first_mesh_in(scene) and
scene.queue_free(); this ensures generate_scene(), _first_mesh_in(), and
scene.queue_free() are only invoked on a valid Scene instance.
- Around line 63-68: The code assumes JSON.parse_string(json_path) returns a
valid Dictionary (sidecar) and immediately indexes sidecar["os-remap"], which
will crash on malformed JSON; add validation after parsing: check that sidecar
is not null and is a Dictionary, verify it contains the "os-remap" key and that
that value is a Dictionary before accessing Frames/Min/Max, and on failure emit
a clear error (or return/raise) so the loader reports a useful message; update
the logic around the sidecar variable and the code that sets _frame_count,
_bounds_min and _bounds_max to only run after these checks.
In `@tools/vat-shaders/openvat.usf`:
- Around line 24-33: Add the missing "SynthesizedUV2" input to the Custom node
checklist in openvat.usf step 4: document it as a ScalarParameter named
SynthesizedUV2 with description "0 = authored UV2, 1 = synthesized integer UV2"
so the Custom-node has the required pin; keep it alongside the existing inputs
(PosTex, UV2, LocalPosition, CurrentFrame, FrameCount, BoundsMin, BoundsMax) and
ensure the name matches the HLSL reference used later.
---
Outside diff comments:
In `@tools/godot-vat-test/scripts/VATPlayer.gd`:
- Around line 455-499: The mesh ends up with mixed UV2 encodings because
synthesized_uv2 is global but you only replace UV2 for surfaces that lack it;
change the logic so that when any_synthesized becomes true you synthesize and
write the packed integer UV2 for every surface (not just those where have_uv2
was false) before calling rebuilt.add_surface_from_arrays, or alternatively
track per-surface mode; specifically update the loop around
mesh.surface_get_arrays / Mesh.ARRAY_TEX_UV2 to generate the PackedVector2Array
for all surfaces once any_synthesized is set and then use
rebuilt.add_surface_from_arrays and rebuilt.surface_set_material as before so
all surfaces use the same packed UV2 encoding that the synthesized_uv2 shader
expects.
🪄 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: c364f337-2249-4c77-af01-a8da6a1e072b
⛔ Files ignored due to path filters (5)
tools/godot-vat-demo/assets/Rumba/Boss_diffuse.pngis excluded by!**/*.pngtools/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!**/*.gltftools/vat-shaders/openvat.shaderis excluded by!**/*.shader
📒 Files selected for processing (17)
src/CLIPipeline.cpptools/godot-vat-demo/README.mdtools/godot-vat-demo/assets/Rumba/mixamo.com-remap_info.jsontools/godot-vat-demo/assets/Rumba/source.materialtools/godot-vat-demo/project.godottools/godot-vat-demo/scenes/demo_perf_skeleton.tscntools/godot-vat-demo/scenes/demo_perf_vat.tscntools/godot-vat-demo/scenes/demo_web.tscntools/godot-vat-demo/scripts/FPSOverlay.gdtools/godot-vat-demo/scripts/OrbitCamera.gdtools/godot-vat-demo/scripts/PerfSpawnerSkeleton.gdtools/godot-vat-demo/scripts/PerfSpawnerVAT.gdtools/godot-vat-demo/scripts/VATInstance.gdtools/godot-vat-test/scripts/VATPlayer.gdtools/vat-shaders/README.mdtools/vat-shaders/openvat.gdshadertools/vat-shaders/openvat.usf
✅ Files skipped from review due to trivial changes (3)
- tools/godot-vat-demo/assets/Rumba/source.material
- tools/godot-vat-demo/assets/Rumba/mixamo.com-remap_info.json
- tools/vat-shaders/README.md
Comment on lines
+5076
to
+5093
| // Export the source mesh as glTF BEFORE running the bake. Both | ||
| // the bake's vertex walk and the glTF exporter iterate Ogre | ||
| // submeshes in submesh-index order, so writing them from the same | ||
| // entity guarantees the bake's column index `i` corresponds to | ||
| // glTF vertex `i`. Doing this AFTER the bake produced a malformed | ||
| // glTF (the bake's software-skinning request leaves Ogre's | ||
| // animation state in a half-state that the exporter mishandles — | ||
| // we get a meshes dict instead of an array). Doing it BEFORE | ||
| // sidesteps the issue entirely and gives consumers a mesh that | ||
| // matches the bake bit-for-bit on vertex order. | ||
| // VATBaker::bake creates `outDir` itself; we need it now too. | ||
| QDir().mkpath(outDir); | ||
| QString gltfPath = QFileInfo(QDir(outDir).filePath("source.gltf")).absoluteFilePath(); | ||
| // Use the display-name format string (matches `cmdConvert`). | ||
| // Short forms like "gltf2" route to a different exporter path | ||
| // that produces a malformed meshes-as-dict glTF. | ||
| int exportResult = MeshImporterExporter::exporter( | ||
| entity->getParentSceneNode(), gltfPath, formatForExtension(gltfPath)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Breadcrumb the source.gltf export too.
This adds a second export step, but only the VAT bake write is breadcrumbed. Add a file.export breadcrumb around the source.gltf export so Sentry timelines show both I/O operations.
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 5076 - 5093, Add a Sentry breadcrumb for
the source.gltf export so the file export is tracked: immediately before calling
MeshImporterExporter::exporter (the block that creates outDir, computes
gltfPath, and calls exporter(entity->getParentSceneNode(), gltfPath,
formatForExtension(gltfPath))), invoke
SentryReporter::addBreadcrumb("file.export", gltfPath) (or a descriptive message
containing "source.gltf") so the export I/O is recorded in the timeline.
| |---|---| | ||
| | `scenes/demo_web.tscn` | Single Rumba dancer + orbit camera. The one we embed in the website. | | ||
| | `scenes/demo_perf_vat.tscn` | 1000 instances driven by VAT. FPS readout in the corner. | | ||
| | `scenes/demo_perf_skeleton.tscn` | 1000 instances driven by Godot's `SkinnedMeshRenderer` + `AnimationPlayer`. Same FPS readout. | |
There was a problem hiding this comment.
Use Godot terminology for the skeletal path.
Line 9 calls out SkinnedMeshRenderer, which is a Unity component name and is confusing in a Godot demo README. Use Godot-native wording (e.g., Skeleton3D/skinned mesh animation path) to avoid misleading setup expectations.
🤖 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-demo/README.md` at line 9, The README references Unity's
`SkinnedMeshRenderer`, which is misleading for a Godot demo; update the line for
`scenes/demo_perf_skeleton.tscn` to use Godot-native terminology such as
`Skeleton3D` and/or "skinned mesh with AnimationPlayer" (or "skinned mesh
animation path") so readers expect Godot's Skeleton3D + AnimationPlayer setup
instead of Unity components.
Comment on lines
+32
to
+33
| while not _rolling_window.is_empty() and _rolling_window.size() > Engine.get_frames_per_second(): | ||
| _rolling_window.pop_front() |
There was a problem hiding this comment.
Min (1s window) can be incorrect and get stuck at 0.
The rolling window is trimmed by frame count, not elapsed time, and if Engine.get_frames_per_second() is 0 transiently, the window can be emptied and _min_fps becomes 0 permanently. Track/purge by accumulated delta-time instead.
Suggested direction
var _rolling_window: Array[float] = [] ## frame deltas over the last 1s
+var _rolling_sum: float = 0.0
@@
_rolling_window.append(delta)
+ _rolling_sum += delta
- while not _rolling_window.is_empty() and _rolling_window.size() > Engine.get_frames_per_second():
- _rolling_window.pop_front()
+ while not _rolling_window.is_empty() and _rolling_sum > 1.0:
+ _rolling_sum -= _rolling_window.pop_front()
@@
- var window_min_fps := (1.0 / window_max_delta) if window_max_delta > 0 else 0.0
- _min_fps = min(_min_fps, window_min_fps)
+ if window_max_delta > 0.0:
+ var window_min_fps := 1.0 / window_max_delta
+ _min_fps = min(_min_fps, window_min_fps)Also applies to: 42-43
🤖 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-demo/scripts/FPSOverlay.gd` around lines 32 - 33, The rolling
window is being trimmed by frame count using Engine.get_frames_per_second(),
which can be zero transiently and empty the window causing _min_fps to stick at
0; change trimming to be time-based by tracking elapsed time for entries (e.g.,
store per-sample delta or timestamp alongside each FPS sample in _rolling_window
or maintain a parallel _rolling_window_duration), when adding a sample append
its delta and while the accumulated duration > 1.0s pop_front and subtract that
popped duration from the accumulator, then recompute _min_fps/_max_fps from the
time-windowed samples; stop using Engine.get_frames_per_second() for window
size.
Comment on lines
+7
to
+8
| ## - Touch drag → orbit (single finger) | ||
| ## - Pinch → zoom (two fingers) |
There was a problem hiding this comment.
Touch controls are documented but not implemented.
The script advertises touch drag/pinch, but input handling currently only supports mouse + keyboard. On touch devices the camera controls will not match expected behavior.
Suggested direction
func _unhandled_input(event: InputEvent) -> void:
+ # Touch orbit
+ if event is InputEventScreenDrag:
+ var sd := event as InputEventScreenDrag
+ yaw_deg = fposmod(yaw_deg - sd.relative.x * orbit_sensitivity, 360.0)
+ pitch_deg = clamp(pitch_deg - sd.relative.y * orbit_sensitivity, min_pitch, max_pitch)
+ _apply_orbit()
+ return
+ elif event is InputEventMagnifyGesture:
+ var mg := event as InputEventMagnifyGesture
+ distance = clamp(distance / max(mg.factor, 0.01), min_distance, max_distance)
+ _apply_orbit()
+ return
+
# Mouse orbit
if event is InputEventMouseButton:Also applies to: 33-56
🤖 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-demo/scripts/OrbitCamera.gd` around lines 7 - 8,
OrbitCamera.gd advertises touch drag/pinch but only handles mouse/keyboard;
implement touch handling in the existing input handler (e.g., _input or
_unhandled_input) to map single-finger drags to orbit and two-finger pinch to
zoom. Specifically: detect InputEventScreenTouch and InputEventScreenDrag (or
Input.get_last_mouse_speed equivalents for touch), track one active touch id and
use its delta to update the camera yaw/pitch the same way mouse motion does
(reuse the same rotation logic or functions that currently handle mouse orbit),
and when two touches are present compute the distance between them each frame to
derive a pinch delta and apply it to the existing zoom/distance variable (reuse
the same zoom function/limits used for mouse wheel). Ensure touch state is
cleared on touch release and that touch gestures respect the same
sensitivity/clamping as mouse controls (refer to the camera rotation variables
like yaw/pitch and zoom/distance used elsewhere in OrbitCamera.gd).
Comment on lines
+44
to
+46
| var scene: Node = doc.generate_scene(state) | ||
| var found_mesh: ArrayMesh = _first_mesh_in(scene) | ||
| scene.queue_free() |
There was a problem hiding this comment.
Guard generate_scene() before using it.
doc.generate_scene(state) can return null; _first_mesh_in(scene) and scene.queue_free() then turn a bad source asset into a runtime error instead of the existing push_error path.
Suggested fix
var scene: Node = doc.generate_scene(state)
+ if scene == null:
+ push_error("PerfSpawnerVAT: generate_scene failed")
+ return
var found_mesh: ArrayMesh = _first_mesh_in(scene)
scene.queue_free()📝 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 scene: Node = doc.generate_scene(state) | |
| var found_mesh: ArrayMesh = _first_mesh_in(scene) | |
| scene.queue_free() | |
| var scene: Node = doc.generate_scene(state) | |
| if scene == null: | |
| push_error("PerfSpawnerVAT: generate_scene failed") | |
| return | |
| var found_mesh: ArrayMesh = _first_mesh_in(scene) | |
| scene.queue_free() |
🤖 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-demo/scripts/PerfSpawnerVAT.gd` around lines 44 - 46, Guard
the result of doc.generate_scene(state) for null before passing it to
_first_mesh_in or calling scene.queue_free: after calling generate_scene in
PerfSpawnerVAT.gd, check if scene is null and if so call push_error with a
descriptive message and return early (or otherwise skip the mesh extraction),
otherwise proceed to call _first_mesh_in(scene) and scene.queue_free(); this
ensures generate_scene(), _first_mesh_in(), and scene.queue_free() are only
invoked on a valid Scene instance.
Comment on lines
+63
to
+68
| var sidecar: Variant = JSON.parse_string(FileAccess.get_file_as_string(json_path)) | ||
| var os: Dictionary = sidecar["os-remap"] | ||
| _frame_count = int(os["Frames"]) | ||
| var mn: Array = os["Min"]; var mx: Array = os["Max"] | ||
| _bounds_min = Vector3(float(mn[0]), float(mn[1]), float(mn[2])) | ||
| _bounds_max = Vector3(float(mx[0]), float(mx[1]), float(mx[2])) |
There was a problem hiding this comment.
Validate the parsed sidecar before indexing os-remap.
JSON.parse_string() returns null on malformed input, so sidecar["os-remap"] will fail before this loader can report a useful error.
Suggested fix
var pos_path: String = bake_dir.path_join(basename + "_pos.png")
var sidecar: Variant = JSON.parse_string(FileAccess.get_file_as_string(json_path))
- var os: Dictionary = sidecar["os-remap"]
+ if typeof(sidecar) != TYPE_DICTIONARY or not sidecar.has("os-remap"):
+ push_error("PerfSpawnerVAT: malformed sidecar")
+ return
+ var os: Dictionary = sidecar["os-remap"]📝 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 sidecar: Variant = JSON.parse_string(FileAccess.get_file_as_string(json_path)) | |
| var os: Dictionary = sidecar["os-remap"] | |
| _frame_count = int(os["Frames"]) | |
| var mn: Array = os["Min"]; var mx: Array = os["Max"] | |
| _bounds_min = Vector3(float(mn[0]), float(mn[1]), float(mn[2])) | |
| _bounds_max = Vector3(float(mx[0]), float(mx[1]), float(mx[2])) | |
| var sidecar: Variant = JSON.parse_string(FileAccess.get_file_as_string(json_path)) | |
| if typeof(sidecar) != TYPE_DICTIONARY or not sidecar.has("os-remap"): | |
| push_error("PerfSpawnerVAT: malformed sidecar") | |
| return | |
| var os: Dictionary = sidecar["os-remap"] | |
| _frame_count = int(os["Frames"]) | |
| var mn: Array = os["Min"]; var mx: Array = os["Max"] | |
| _bounds_min = Vector3(float(mn[0]), float(mn[1]), float(mn[2])) | |
| _bounds_max = Vector3(float(mx[0]), float(mx[1]), float(mx[2])) |
🤖 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-demo/scripts/PerfSpawnerVAT.gd` around lines 63 - 68, The
code assumes JSON.parse_string(json_path) returns a valid Dictionary (sidecar)
and immediately indexes sidecar["os-remap"], which will crash on malformed JSON;
add validation after parsing: check that sidecar is not null and is a
Dictionary, verify it contains the "os-remap" key and that that value is a
Dictionary before accessing Frames/Min/Max, and on failure emit a clear error
(or return/raise) so the loader reports a useful message; update the logic
around the sidecar variable and the code that sets _frame_count, _bounds_min and
_bounds_max to only run after these checks.
Comment on lines
+24
to
+33
| // 4. Wire these inputs to the Custom node (the names must match | ||
| // exactly — the HLSL references them by identifier): | ||
| // - PosTex → Texture2D (TextureSampleParameter2D, sRGB OFF) | ||
| // - UV2 → TextureCoordinate (Coordinate Index = 1) | ||
| // - LocalPosition → LocalPosition node (no offset — the bind-pose | ||
| // object-space vertex coord) | ||
| // - CurrentFrame → ScalarParameter "CurrentFrame" | ||
| // - FrameCount → ScalarParameter "FrameCount" | ||
| // - BoundsMin → VectorParameter "BoundsMin" (xyz from os-remap.Min) | ||
| // - BoundsMax → VectorParameter "BoundsMax" (xyz from os-remap.Max) |
There was a problem hiding this comment.
Add SynthesizedUV2 to the setup checklist.
The snippet reads SynthesizedUV2 later, but step 4 never tells users to add that Custom-node input. Following the instructions as written leaves the node missing a required pin.
At minimum, document it here as a scalar input with 0 = authored UV2 and 1 = synthesized integer UV2.
🤖 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/vat-shaders/openvat.usf` around lines 24 - 33, Add the missing
"SynthesizedUV2" input to the Custom node checklist in openvat.usf step 4:
document it as a ScalarParameter named SynthesizedUV2 with description "0 =
authored UV2, 1 = synthesized integer UV2" so the Custom-node has the required
pin; keep it alongside the existing inputs (PosTex, UV2, LocalPosition,
CurrentFrame, FrameCount, BoundsMin, BoundsMax) and ensure the name matches the
HLSL reference used later.
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>
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>
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>
|
5 tasks
fernandotonon
added a commit
that referenced
this pull request
May 20, 2026
…644) * vat: website demo tabs + MultiMesh perf comparison + web build fixes Builds on PR #640 (which landed the bake/harness pipeline). This patch wires the demo into the marketing site as three browser-runnable tabs: - "Showcase" single VAT dancer + orbit camera - "1000× VAT" MultiMeshInstance3D + INSTANCE_CUSTOM frame phase - "1000× skeletal" 1000 SkinnedMeshRenderer clones (cached PackedScene) Notable fixes: - Web export couldn't find the dancer (raw .gltf/.png aren't bundled in the Godot .pck — only imported .scn/.ctex). VATInstance.gd and both perf spawners now go through `load("res://...")` so resources resolve in the web sandbox. - VAT perf demo was slower than skeletal because every instance had its own ShaderMaterial. Switched to a single MultiMesh + per- instance phase via INSTANCE_CUSTOM — one draw call per surface instead of N. - Vsync now off at project level (`window/vsync/vsync_mode=0`) — on macOS Metal the runtime DisplayServer call alone is too late. - Single Godot web export with URL-based scene routing (`?scene=web|perf_vat|perf_skeleton`) via a Bootstrap.gd entry — avoids 3× the 36 MB WASM in `website/public/demo/`. Frontend: - New <VATDemo> React component with tab switcher (a11y roles + keyboard nav) and per-tab captions explaining what the visitor is looking at. - Drops the old static screenshot block from App.jsx / App.module.css. * review(vat-demo): full WAI-ARIA tab pattern (Codex P2) Codex flagged that the tablist/tab semantics on <VATDemo> weren't backed by the rest of the ARIA tab pattern. Added: - aria-controls + aria-labelledby wiring between each tab button and its panel, with stable id="vat-tab-<id>" / "vat-panel-<id>" - role="tabpanel" on the iframe wrapper - Roving tabIndex (the active tab is the only one in the tab order; others are -1) - Arrow-key navigation: Left/Right cycle, Home/End jump to ends — focus follows selection, matching the WAI-ARIA "tabs with automatic activation" pattern
4 tasks
fernandotonon
added a commit
that referenced
this pull request
May 20, 2026
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.
fernandotonon
added a commit
that referenced
this pull request
May 20, 2026
…acts (#646) * fix(export): glTF/GLB skip aiProcess_ConvertToLeftHanded 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. * fix(vat): three independent bugs that broke OpenVAT consumers 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. * review(vat): match on full vertex signature + validate permutation 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. * fix(vat-demo): bind-pose sidecar so consumers survive importer reorder 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. * fix(vat-demo): tiebreak ambiguous matches with continuous distance 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. * fix(export): keep aiProcess_FlipUVs on glTF export 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. * fix(vat-demo): drop normal negation in shader 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.
5 tasks
fernandotonon
added a commit
that referenced
this pull request
May 20, 2026
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.
6 tasks
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.
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.
Summary
Harness rewrite (Godot test rig):
texelFetch+ integer row arithmetic. Fixes the one-frame "blob" glitch caused byfilter_nearestrounding V into the normal half at the boundary.loop_framesclamp, .tscn path fixes.Engine shader templates (new
tools/vat-shaders/):openvat.gdshaderopenvat.shaderopenvat.usfREADME.mdCLI now references the templates in
--helpand post-bake output.Deliberately skipped
no_remaphalf-float Niagara workflow, no real user yet. The harness still consumes Blender's separate-EXR bakes for verification — only QtMeshEditor's own output is locked to packed.qtmesh convertstrips TEXCOORD_1 today. The README documents theassimp exportworkaround. Tracked as a follow-up.Verified
Godot harness loads both a Rumba bake (QtMeshEditor packed mode) and a Barril sample (Blender separate-EXR + UV2 tile layout) cleanly.
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation