perf(renderer): spatial-index frustum cull (#678)

[aram-devdocs]

Overview

Type: perf

Summary:
3D scene frustum culling previously linear-scanned the full objects
registry every frame, so BeginFrame + EndFrame scaled with TOTAL
sub-mesh count even when 80%+ were culled. throne_ge GameWorld
(~55k sub-meshes) sat at 7 fps with the per-frame cull cost growing to
30–45 ms.

This PR adds a sparse uniform-grid spatial index keyed by world-space
AABB, maintained incrementally on every object create/transform/remove
and queried each frame against the frustum's world AABB to shrink the
candidate set fed to the existing per-object frustum-sphere test. The
query auto-picks between an AABB cell sweep and an occupied-cell scan,
so a far plane that vastly exceeds the scene extent never costs more
than the underlying linear scan would.

The index is fully configurable via Render3DConfig::spatial_index —
disabling it falls back to the original linear scan, so any game that
needs the legacy behavior can opt out without code changes.

Related Issues: Fixes #678

---

Changes Made

Engine Core (goud_engine/src/)

• libs/graphics/renderer3d/spatial_index.rs (new) — sparse uniform-grid index over scene-object IDs with insert/remove/update/query and per-query stamp dedup.
• libs/graphics/renderer3d/core/spatial.rs (new) — Renderer3D helper methods that translate Object3D state into world AABB inserts/removes.
• libs/graphics/renderer3d/frustum.rs — added frustum_world_aabb helper next to Frustum::from_view_projection.
• libs/graphics/renderer3d/render/mod.rs — replaced the linear cull loop with query_aabb + frustum sphere check; added cell-size hot-tune branch when config changes.
• libs/graphics/renderer3d/config.rs — new SpatialIndexConfig { enabled, cell_size }.
• libs/graphics/renderer3d/types.rs — Renderer3DStats adds spatial_index_candidates and spatial_index_cells_visited.
• libs/graphics/renderer3d/core/mod.rs, core/object_transforms.rs, core_primitives.rs, core_models/{mod.rs,lifecycle.rs}, core_model_instances.rs — wire create/transform/remove paths to the index. Refactored instantiate_model to snapshot the source model up front so the borrow checker is happy when we mutate self.objects and self.spatial_index inside the loop.
• libs/graphics/renderer3d/tests.rs — new scene_culling_678 module with 7 scaling/parity/movement tests at 1k/10k/30k/50k objects.

FFI Layer (goud_engine/src/ffi/)

No changes. (Issue ask #3 — populate surface_present/gpu_submit/readback_stall/uniform_upload/render_pass phase counters — was already shipped by #682 + lifecycle wrappers; verified in frame_timing.rs and ffi/renderer/lifecycle.rs.)

C# / Python / TypeScript SDKs

No changes. New stats counters are reachable via Renderer3DStats; surfacing them through SDK wrappers is left for a follow-up if/when game code wants to read them directly.

Codegen / Proc Macros / Tools / WASM / Examples / Docs

No changes.

---

Architectural Compliance

• Rust-first: All logic lives in Rust.
• FFI boundary: No new exports.
• Dependency flow: Imports follow layer hierarchy (validated by cargo run -p lint-layers — no violations).
• SDK parity: No new FFI surface to wrap.
• Unsafe discipline: No new unsafe blocks introduced.
• File size: New spatial_index.rs is ~580 lines including tests; production code is well under 500. Existing files stay under their previous sizes.

---

Testing

• cargo test passes (4900 tests; one pre-existing flaky native_main_thread test that also fails on origin/main is unrelated).
• cargo clippy --all-targets -- -D warnings is clean.
• cargo fmt --all -- --check passes.
• cargo run -p lint-layers reports no violations.
• 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.
• Pre-commit hooks pass.

---

Code Quality

• No todo!() or unimplemented!() introduced.
• One #[allow(dead_code)] on SpatialIndex impl block — covers diagnostic accessors and the update/clear methods used only by tests today; documented in a comment immediately above the attribute.
• Error handling uses Result where applicable; spatial-index methods are infallible.
• Public items in the new modules have doc comments.

---

Documentation

• No AGENTS.md updates needed (no architectural shift).
• No README.md updates needed (no user-visible config change at default settings).
• New public-ish APIs (SpatialIndexConfig, Renderer3DStats::spatial_index_*) carry doc comments.

---

Breaking Changes

• API changes: None at default settings. Render3DConfig gains a spatial_index: SpatialIndexConfig field with sensible defaults; existing call sites that construct Render3DConfig via ..Default::default() keep working.
• FFI signature changes: None.
• SDK interface changes: None.

---

Version Bump

Bump type: patch
Justification: Internal performance improvement, no public API or SDK shape changes.

---

Security

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

---

Performance

Measured via cargo bench --bench scene_culling_benchmarks (NullBackend, throne-style 200x200 grid scenes, camera looking at a small visible patch):

objects	linear scan	spatial index	speedup
1 024	8.9 µs	1.5 µs	6×
10 000	80.9 µs	1.9 µs	44×
50 176	418 µs	3.2 µs	131×

The 30k checkpoint is also covered in the bench; a parity test
(spatial_index_visible_count_matches_linear_scan) ensures the
spatial-index path produces the same visible_objects count as the
legacy linear scan, and a movement test
(spatial_index_tracks_object_movement) guards the
insert/transform/remove sync hooks.

---

Deployment

• NuGet — N/A.
• Python package — N/A.
• npm package — N/A.
• Native library builds on macOS arm64 (verified locally). Linux + Windows builds rely on CI.

---

Reviewer Notes

• The big behavioral assumption is that Object3D::bounds is a uniform sphere, so rotation-only mutations skip the spatial-index refresh. Both set_object_rotation and set_model_rotation carry an INVARIANT: comment pointing this out — switching to OBB-style bounds in the future will need to revisit those skips.
• query_stamp: Vec<u64> indexed by raw u32 ID is documented as growing with next_object_id but never trimmed. Realistic worst case is ~2^32 IDs before it becomes a real RAM hog; flagged in the struct field doc comment so it is not a hidden footgun.
• The cell-size hot-tune branch in render::render rebuilds the index on the rare frame where config.spatial_index.cell_size changes; on the common path it is a single f32 epsilon comparison.
• Phase counters listed in issue ask X Y coordinates not calibrated #3 were already populated by PR feat(renderer): record begin/end frame timings and surface them via FFI #682 + lifecycle wrappers; this PR confirms but does not modify them.

[claude]

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

---

PR Review: perf(renderer): spatial-index frustum cull (#678)

Todo List:

• Gather context (CLAUDE.md, PR metadata)
• Phase 1: Context gathering — identify layers touched
• Phase 2: Architecture validation — dependency flow, Rust-first
• Phase 3: Anti-pattern scan — FFI safety, unsafe, todo!(), unwrap()
• Phase 4: Quality assessment — naming, error handling, tests, docs
• Phase 5: Security review — N/A (no new unsafe/FFI)
• Phase 6: Scope verification — PR description vs changes
• Phase 7: Produce structured review output

---

Verdict: ✅ APPROVED

No blockers found. The implementation is correct, well-tested, and follows all project conventions.

---

Phase 1 — Context

Layers touched: Layer 2 (Libs) only — libs/graphics/renderer3d/. No FFI, SDK, ECS, or example changes.

PR type: Pure performance improvement with opt-out fallback. All behavior-critical paths are guarded by the existing frustum_culling.enabled check.

---

Phase 2 — Architecture

Check	Result
Downward-only imports	✅ spatial_index.rs only imports cgmath + rustc_hash. No engine-layer imports.
Rust-first principle	✅ All logic in Rust; no SDK code path created.
No raw GPU calls outside backend/	✅ Pure CPU data-structure code.
File sizes	✅ spatial_index.rs is 579 lines (≈340 prod, ≈240 test). render/mod.rs is 481 lines. Both within limits.
Dependency direction	✅ cargo run -p lint-layers clean per PR.

---

Phase 3 — Anti-Pattern Scan

Check	Result
Missing #[no_mangle] / #[repr(C)]	✅ No new FFI exports
unsafe without // SAFETY:	✅ No new unsafe blocks
todo!() / unimplemented!()	✅ None found
unwrap() in library code	✅ No new unwrap() calls added (one pre-existing unwrap() in lifecycle.rs:21 is guarded by a contains_key check one line above — pre-existing issue, not introduced here)
Duplicated Rust/SDK logic	✅ N/A

---

Phase 4 — Quality Assessment

Strengths observed:

• Two-path query strategy is elegant: sweep vs. occupied-cell scan, selected per-call via sweep_cost <= occupied * 2. The saturating arithmetic prevents overflow on giant far-plane queries.
• Stamp dedup avoids per-query HashSet allocations. The u64 stamp + wraparound reset is correct and tested by stamp_wraparound_preserves_correctness.
• Excellent invariant documentation: INVARIANT: comments on set_object_rotation and set_model_rotation clearly flag that the spatial index skip is only valid while bounds are uniform spheres — any future OBB migration will be obvious.
• Graceful degenerate-camera fallback: frustum_world_aabb returns None on non-invertible VP; the caller falls back to a full scan rather than panicking or over-culling.
• scratch_cull_candidates reuse via std::mem::take avoids per-frame reallocation.
• Test coverage: 12 unit tests in spatial_index.rs + 7 integration-level tests in scene_culling_678 including parity, movement, disable-fallback, and stamp-wraparound. Strong.

Warnings (non-blocking):

1. Rebuild allocates O(n) on cell_size change (render/mod.rs:208–232): When config.spatial_index.cell_size changes at runtime, the code collects all object state into a Vec before rebuilding the index. For 50k objects this is one notable allocation on a normally rare operation. It's documented as a "hot-tune" path, but the SpatialIndexConfig doc comment doesn't warn that rebuilding is expensive at large scene sizes. A one-line note there (e.g. "changing this at runtime forces an O(n) index rebuild") would save future profiling surprises. Fix this →

2. query_stamp Vec grows with next_object_id (acknowledged in PR notes): The struct's doc comment already flags this well, but it's worth verifying that the bench/test scenarios don't inadvertently hide it. Object IDs wrap at u32::MAX, but the Vec doesn't shrink. This is correctly documented; no action required unless a trim-on-clear policy is desired later.

3. VisibleDrawData defined inside render() (render/mod.rs:244–254): Declaring a struct inside a method body is unusual in this codebase. It's valid Rust but reduces discoverability. If this struct is needed only in this one function it's fine, but worth noting for future readers who might expect it at the module level.

4. Scale-zero edge case in world_aabb_from_sphere: If an object has scale = (0,0,0), max_scale = 0 and the AABB degenerates to a point. The object would still be inserted into a single cell and remain queryable. This isn't a crash, but it's an unusual state that could cause visual bugs (a scaled-to-zero object technically outside its cell). Minor; only surfaced if callers actually set zero scale.

5. No test for set_model_scale → spatial index sync: spatial_index_tracks_object_movement covers position changes. Scale changes to sub-mesh objects are exercised via set_model_transform but there's no dedicated assertion that scaling an object updates its cell membership. This is a minor gap given the movement test already validates the hook pattern. Fix this →

---

Phase 5 — Security

N/A. No new unsafe blocks, no new FFI pointer parameters, no new dependencies.

---

Phase 6 — Scope

All 16 changed files match the PR description. No unrelated changes observed. Version bump marked as patch, which is appropriate for an internal perf improvement with no public API changes.

The one PR checklist item noted as shipping separately (phase counters from issue ask #3) correctly points to PR #682 — verified the scope claim is accurate.

---

Blockers

None.

Warnings Summary

#	Location	Issue	Severity
1	config.rs:35 (cell_size doc)	No mention that runtime change triggers O(n) rebuild	Minor
2	spatial_index.rs:56	query_stamp Vec unbounded growth — documented but worth revisiting if object churn is ever profiled	Acknowledged
3	render/mod.rs:244	Local struct VisibleDrawData inside method body — unusual style	Nit
4	spatial_index.rs:325	Scale-zero degenerate AABB is a silent point-insert	Nit
5	tests.rs	No scale-change → spatial-index sync test	Minor gap

---

Positive Callouts

• The occupied-cell-scan branch guard with test:occupied_cell_scan_used_for_huge_aabb is exactly the right paranoia test for a heuristic.
• INVARIANT: comments on rotation methods are textbook-quality future-proofing.
• 131× measured speedup at 50k objects with a NullBackend bench that anyone can reproduce locally.
• Zero unsafe blocks in ~350 lines of new production code. Clean.