Skip to content

fix(renderer3d): static batch overflow renders objects invisible, expose missing config to SDK - #667

Merged
aram-devdocs merged 4 commits into
mainfrom
fix/static-batch-overflow-rendering
Apr 3, 2026
Merged

fix(renderer3d): static batch overflow renders objects invisible, expose missing config to SDK#667
aram-devdocs merged 4 commits into
mainfrom
fix/static-batch-overflow-rendering

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Overview

Type: fix | perf

Summary:
Static objects exceeding the 50k vertex batch budget were silently dropped from both the static batch AND the individual draw path, making them invisible in character_sandbox. This PR fixes the overflow fallback, adds missing dirty flags for static object transforms, corrects bounding sphere computation for skinned models, exposes 4 previously hidden config knobs to all SDKs, and removes a per-frame allocation in the render loop.

Related Issues: Regression from #664 and frustum culling optimization (d4ad278)


Changes Made

Engine Core (goud_engine/src/)

  • Static batch overflow fix: Added static_batched_ids: HashSet<u32> to track which objects are actually in the batch. Render loop now skips only batched objects, so overflow objects fall back to per-object draws instead of becoming invisible.
  • Transform dirty flags: set_object_position/rotation/scale and set_model_transform now set static_batch_dirty = true when the affected object is static, ensuring the batch rebuilds with correct transforms.
  • Bounding sphere FPV fix: compute_bounding_sphere() now accepts floats_per_vertex parameter instead of hardcoding 8, fixing wrong bounding spheres for skinned (16-FPV) models.
  • Render loop optimization: Replaced per-frame clone() of static_batch_groups with std::mem::take + restore pattern.
  • Buffer failure fallback: Static batch buffer creation failure now clears static_batched_ids so all static objects fall back to individual draws.

FFI Layer (goud_engine/src/ffi/)

  • Added 4 setter FFI functions: set_max_static_batch_vertices, set_max_bones_per_mesh, set_shadow_strength, set_shadow_auto_disable_threshold
  • Added 4 matching getter FFI functions

C# SDK (sdks/csharp/)

  • Generated: 8 new methods on GoudGame (SetMaxStaticBatchVertices, GetMaxStaticBatchVertices, SetMaxBonesPerMesh, GetMaxBonesPerMesh, SetShadowStrength, GetShadowStrength, SetShadowAutoDisableThreshold, GetShadowAutoDisableThreshold)

Python SDK (sdks/python/)

  • Generated: 8 new methods (snake_case equivalents)

TypeScript SDK (sdks/typescript/)

  • Node napi binding changes
  • Type definition changes
  • Generated: 8 new methods (camelCase equivalents)

Codegen Pipeline (codegen/)

  • Schema changes
  • ffi_mapping changes
  • Added 8 new method definitions to goud_sdk.schema.json methods array and FFI mapping section
  • Added 8 new FFI function entries to ffi_mapping.json
  • Validation passes: schema and resolved ffi_mapping are in sync

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: New exports use #[no_mangle] extern "C" and #[repr(C)] where needed
  • Dependency flow: Imports follow layer hierarchy (down only)
  • SDK parity: Changes exposed via FFI are wrapped in C#, Python, TypeScript, Go, Swift, Kotlin, and Lua SDKs
  • Unsafe discipline: No new unsafe blocks
  • File size: No file exceeds 500 lines

Testing

  • cargo test passes
  • cargo clippy -- -D warnings is clean
  • cargo fmt --all -- --check passes
  • Python SDK tests pass (python3 sdks/python/test_bindings.py) — if SDK changed
  • C# SDK tests pass (dotnet test sdks/csharp.tests/) — if SDK changed
  • TypeScript SDK tests pass (cd sdks/typescript && npm test) — if TS SDK changed
  • Codegen produces consistent output (python3 codegen/validate.py && python3 codegen/validate_coverage.py)
  • 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

  • Updated relevant AGENTS.md files (if architecture changed)
  • Updated README.md (if user-facing behavior changed)
  • Added or updated doc comments on new public APIs

Breaking Changes

None

  • API changes: None (additive only — 8 new SDK methods)
  • FFI signature changes: compute_bounding_sphere internal signature changed (not FFI-exposed)
  • SDK interface changes: None (additive only)

Version Bump

Bump type: patch
Justification: Bug fix for static batch rendering regression + additive SDK API expansion


Security

  • No new unsafe blocks — or each one has a // SAFETY: comment and is necessary
  • No new FFI pointer parameters without null checks
  • No new dependencies with known advisories (cargo deny check)
  • No secrets or credentials in committed files

Performance

  • Removed per-frame Vec::clone() of static_batch_groups in render loop (replaced with zero-copy std::mem::take + restore)
  • Added HashSet<u32> lookup in visibility loop (~O(1) per object, negligible vs existing HashMap lookups)
  • Static objects that overflow the batch budget now render via individual draws instead of being invisible — slight draw call increase for overflow scenes, but correct rendering

Deployment

  • NuGet package version updated (if SDK changed)
  • Python package version updated (if SDK changed)
  • npm package version updated (if TS SDK changed)
  • Native library builds on all targets (macOS, Linux, Windows)

Reviewer Notes

  • The native_main_thread integration test fails on main (requires live GPU context) — pre-existing, not related to this PR
  • CI on main currently has an Android Build Pipeline emulator smoke test failure — also pre-existing
  • The Visible: 1/1281 stat in character_sandbox should increase significantly after this fix since overflow static objects now appear in the individual draw path
  • Game developers can now tune SetMaxStaticBatchVertices() from SDK code if the default 50k is insufficient for their scene

🤖 Generated with Claude Code

…ose missing config to SDK

Static objects exceeding the 50k vertex batch budget were silently
dropped from both the static batch AND the individual draw path, making
them invisible. Track which objects are actually batched via a HashSet
so overflow objects fall back to per-object draws.

Also fixes:
- Model transform changes (position/rotation/scale) now trigger static
  batch rebuild when the affected object is static
- compute_bounding_sphere accepts floats_per_vertex parameter instead
  of hardcoding 8, fixing wrong bounding spheres for skinned models
- Expose max_static_batch_vertices, max_bones_per_mesh, shadow_strength,
  and shadow_auto_disable_threshold to all SDKs via FFI
- Remove per-frame clone of static_batch_groups in render loop

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @aram-devdocs's task in 3m 46s —— View job


PR Review — fix(renderer3d): static batch overflow renders objects invisible

  • Gather context (AGENTS.md, PR description)
  • Phase 1: Context gathering — identify layers touched
  • Phase 2: Architecture validation
  • Phase 3: Anti-pattern scan
  • Phase 4: Quality assessment
  • Phase 5: Security review (FFI changes)
  • Phase 6: Scope verification
  • Phase 7: Final verdict

Verdict: ✅ APPROVED

The core fix is sound, all SDK parity requirements are met, and no blockers were found. Two non-blocking warnings worth addressing in a follow-up.


Phase 1 — Layers Touched

Layer Files Notes
Libs (graphics) renderer3d/core/, core_static_batch.rs, render/, render_instanced.rs, types.rs, core_models/ Core bug fix
FFI ffi/renderer3d/environment/config_setters.rs, config_getters.rs 8 new exports
SDK (all languages) C#, Python, TypeScript, Go, Kotlin, Lua, Swift Generated parity
Codegen ffi_manifest.json, ffi_mapping.json, goud_sdk.schema.json Schema-driven

Phase 2 — Architecture ✅

  • Dependency flow is downward only — no upward imports introduced
  • All logic lives in Rust; SDKs are confirmed thin wrappers
  • No raw GPU calls outside backend/
  • config_setters.rs is 496 lines — within 4 lines of the 500-line limit; keep an eye on this

Phase 3 — Anti-Pattern Scan ✅

  • All new FFI functions have #[no_mangle] extern "C"
  • No new unsafe blocks introduced ✅
  • No todo!() / unimplemented!()
  • No unwrap() / expect() in library code ✅
  • No logic duplication between Rust and SDKs ✅

Phase 4 — Quality Assessment

Positives:

  • static_batched_ids: HashSet<u32> cleanly and precisely captures which objects entered the batch — correct root-cause fix
  • std::mem::take + restore pattern is idiomatic for borrow-splitting without cloning
  • Dirty flag propagation (set_object_position/rotation/scale + set_model_transform) is consistent and correctly placed
  • compute_bounding_sphere(floats_per_vertex) parameterization is the right generalization — fixes skinned model bounds correctly
  • All 8 new FFI functions follow the established clone-config → mutate → set_render_config → return 0/-1 pattern

Warning 1 — static_batch_groups is silently dropped on early return

render_instanced.rs:190-195

let groups = std::mem::take(&mut self.static_batch_groups);  // line 190
let batch_buf = match self.static_batch_buffer {
    Some(buf) => buf,
    None => {
        log::error!("…");
        return;  // groups is DROPPED here — self.static_batch_groups stays empty
    }
};
// …
self.static_batch_groups = groups;  // only reached on happy path

If the None branch fires, self.static_batch_groups is permanently emptied until the next full batch rebuild. In practice this path can't be reached (caller guards with has_static_batch = static_batch_buffer.is_some()), but it's a latent silent failure. Consider restoring before the early return:

None => {
    log::error!("…");
    self.static_batch_groups = groups; // restore
    return;
}

Fix this →

Warning 2 — set_max_static_batch_vertices doesn't trigger a batch rebuild

config_setters.rs:420-435

set_render_config only stores the new config; it doesn't set static_batch_dirty. So calling SetMaxStaticBatchVertices(100_000) won't expand the batch to include previously-overflowed objects until the next mutation that independently marks dirty. This is consistent with the pre-existing behavior of set_static_batching_enabled, but it's worth documenting in the SDK doc comment or the setter itself. Game developers may expect the larger budget to apply immediately.


Phase 5 — Security / FFI ✅

  • All 4 new setters validate GOUD_INVALID_CONTEXT_ID before dereferencing the context ✅
  • New getters rely on with_renderer returning None (and unwrap_or) — this is safe and consistent with the majority of existing getters (some do add explicit GOUD_INVALID_CONTEXT_ID fast-path, some do not — pre-existing inconsistency, not a regression here)
  • No pointer parameters in any of the 8 new functions — no null checks needed ✅
  • vertices.max(1) and bones.max(1) guard against zero-budget pathological inputs ✅
  • strength.clamp(0.0, 1.0) properly sanitizes shadow strength ✅

Phase 6 — Scope ✅

  • Changes match the PR description exactly
  • No unrelated code changes observed
  • ffi_manifest.json and codegen files are all generated output, consistent with ./codegen.sh pipeline
  • core/mod.rs is 494 lines — within limit

Summary

The overflow fix (static_batched_ids tracking + render loop skip guard) is the correct approach to distinguish batched vs overflow objects. The std::mem::take optimization is a clean improvement. The bounding sphere parameterization fix is correct and properly threaded to all call sites. SDK parity is complete across all 7 languages.

Warning 1 (groups not restored on early return) is safe in practice today due to the call-site guard, but is worth a one-liner fix to eliminate the latent failure mode. Warning 2 is a documentation/behavior note with no correctness impact.

The IGoudGame interface gained 8 new methods but the web GoudGame class
was missing stub implementations, causing a TS2420 compilation error in
the WASM build CI lane.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Apr 3, 2026
Added 8 stub methods to gen_ts_web.py so the web GoudGame class
implements the new IGoudGame interface methods. Regenerated
web/index.g.ts via codegen pipeline.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Apr 3, 2026
The Kotlin SDK gamepad methods take Int parameters but the Android
example was passing GamepadButton enum directly. Append .value to
extract the underlying Int.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL and removed size/XL labels Apr 3, 2026
@aram-devdocs
aram-devdocs merged commit 5edccd7 into main Apr 3, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant