fix(renderer3d): populate Object3D.vertices for static batch (#630)

[aram-devdocs]

Overview

Type: fix

Summary:
Fix SetModelStatic(true) making models invisible by populating Object3D.vertices for non-skinned models and instances. The static batch rebuild reads CPU-side vertex data from this field, but it was always Vec::new() for models (only primitives populated it correctly).

Related Issues: Fixes #630

---

Changes Made

Engine Core (goud_engine/src/)

• libs/graphics/renderer3d/core_models/mod.rs: Populate Object3D.vertices with verts.clone() for non-skinned models (skinned models use 16 FPV layout incompatible with 8 FPV static batch)
• libs/graphics/renderer3d/core_model_instances.rs: Clone vertices from source object when instantiating
• libs/graphics/renderer3d/tests.rs: Add regression test verifying static primitives render via batch path

FFI Layer (goud_engine/src/ffi/)

No changes

C# SDK (sdks/csharp/)

No changes

Python SDK (sdks/python/)

No changes

TypeScript SDK (sdks/typescript/)

No changes

Codegen Pipeline (codegen/)

No changes

Proc Macros (goud_engine_macros/)

No changes

Tools (tools/)

No changes

WASM (goud_engine/src/wasm/)

No changes

Examples (examples/)

No changes

Documentation

No changes

---

Architectural Compliance

• Rust-first: All logic lives in Rust; SDKs are thin wrappers
• FFI boundary: No FFI changes
• Dependency flow: Imports follow layer hierarchy (down only)
• SDK parity: No FFI surface change
• Unsafe discipline: No unsafe blocks added
• File size: No file exceeds 500 lines

---

Testing

• cargo test passes (pre-existing native_main_thread GPU test failure unrelated)
• cargo clippy -- -D warnings is clean
• cargo fmt --all -- --check passes
• Python SDK tests pass — N/A (no SDK changes)
• C# SDK tests pass — N/A (no SDK changes)
• TypeScript SDK tests pass — N/A (no SDK changes)
• Codegen produces consistent output — N/A (no codegen changes)
• Pre-commit hooks pass

---

Code Quality

• No todo!() or unimplemented!() in production code
• No #[allow(unused)] without justification comment
• Error handling uses Result, not unwrap()/expect() in library code
• Public items have doc comments

---

Documentation

• N/A — no architecture or user-facing behavior changes

---

Breaking Changes

None

---

Version Bump

Bump type: patch
Justification: Bug fix for static model rendering

---

Security

• No new unsafe blocks
• No new FFI pointer parameters
• No new dependencies
• No secrets or credentials

---

Performance

Non-skinned models now retain a CPU-side copy of vertex data in Object3D.vertices (same pattern primitives already use). This trades additional memory for correct static batching. For large scenes with many static models, the static batch reduces draw calls significantly — a net performance win at the cost of ~1x vertex memory per non-skinned model.

---

Deployment

• N/A — engine-only change, no SDK version bumps needed

---

Reviewer Notes

Root cause: rebuild_static_batch() reads obj.vertices to bake world-space transforms into a single VBO. Models and instances were created with vertices: Vec::new(), producing zero vertices in the batch while being excluded from the dynamic render pass. Primitives worked because create_primitive() already populated this field.

[claude]

Claude finished @aram-devdocs's task in 2m 9s —— View job

---

Code Review — fix(renderer3d): populate Object3D.vertices for static batch (#630)

• Gather context and read CLAUDE.md
• Read changed files
• Phase 1: Context gathering
• Phase 2: Architecture validation
• Phase 3: Anti-pattern scan
• Phase 4: Quality assessment
• Phase 5: Security review (N/A — no FFI/unsafe)
• Phase 6: Scope verification
• Phase 7: Review output

---

Verdict: APPROVED ✅

The root cause diagnosis is accurate, the fix is minimal and correct. No blockers.

---

BLOCKERS

None.

---

WARNINGS

#	File	Line	Issue
W1	tests.rs	386–415	Regression test targets primitives, which already worked before this PR
W2	tests.rs	407–410	Assertion draw_calls >= 1 is weak — doesn't verify the static batch produced geometry
W3	core_models/mod.rs	114 + 134	Non-skeletal models now clone verts twice: once into bind_pose_vertices (used only for CPU-skinned instancing) and once into Object3D.vertices

W1 detail: The PR description says primitives already worked because create_primitive() already populated vertices. The actual regression was for load_model and instantiate_model paths — but neither is covered by the new test. Loading a real model in a unit test requires constructing ModelData, which is admittedly heavier, but a direct unit test of Object3D.vertices being non-empty after load_model would be more targeted.

W2 detail: Since the cube is the only object in the test and it's marked static, a precise assertion would be assert_eq!(stats.draw_calls, 1). The current form (>= 1) would pass even if something unrelated produced a draw call.

W3 detail: In load_model, for non-skeletal models (has_skeleton = false, is_skinned = false):

bind_pose_vertices.push(verts.clone()); // line 114 — pre-existing, used only for CPU-skinned animation
// ...
vertices: if is_skinned { Vec::new() } else { verts.clone() }, // line 135 — new

bind_pose_vertices[i] for non-skeletal models is never accessed (the instancing path guards on has_skeleton, the animation path gather_skin_uploads is only called for skinned models). So for every non-skeletal static model there are two identical vertex copies in memory. This is a pre-existing waste amplified by the new clone. Not a blocker, but worth tracking in a follow-up.

---

Positive Callouts

• Minimal and targeted fix. Only the two construction sites where vertices: Vec::new() were wrong are changed. No unnecessary refactoring.
• Skinned vs non-skinned guard is correct. GPU-skinned models use 16 FPV (8 base + 4 bone indices + 4 weights), which is incompatible with the static batch's 8 FPV layout. Keeping Vec::new() for is_skinned prevents silent vertex misinterpretation.
• Clear comment at the fix site (// CPU-side vertex copy for static batching (8 FPV layout). Skinned models use 16 FPV and are never static-batched.) makes the intent immediately legible.
• Instance propagation in core_model_instances.rs correctly inherits the populated vertices from the source object via o.vertices.clone(), ensuring instances also work for static batching.
• PR description is thorough — root cause, affected paths, and memory trade-off are all documented clearly.

---