perf(renderer3d): InstantiatePlane collapses identical plane primitives into one instanced draw (#679)

[aram-devdocs]

Overview

Type: perf

Summary:
CreatePlane() primitives bypass the engine's instancing path: a per-tile terrain (40k planes / 9 materials in throne_ge) produces 11k+ draw calls and 7 fps. InstantiatePlane(sourcePlaneId) mirrors InstantiateModel(sourceModelId) for primitives so every instance that shares the same source plane renders through one instanced draw call. A 9-material terrain collapses to ~9 instanced draws regardless of tile count.

Related Issues: Closes #679

---

Changes Made

Engine Core (goud_engine/src/)

• New core_plane_instances.rs: per-source-plane PlaneInstancePool over the existing instanced_meshes map. instantiate_plane(source_plane_id) -> Option<u32> allocates a slot in the pool's InstanceTransform vec and returns a stable handle.
• flush_dirty_plane_instance_pools() re-uploads the GPU instance buffer once per frame before the instanced pass. Capacity grows with next_power_of_two; in-place updates use a split-borrow with a SAFETY-commented unsafe slice (no per-frame Vec clone). Persistent scratch_dirty_plane_pool_ids avoids per-frame allocation of the dirty list.
• set_object_position / set_object_rotation / set_object_scale dispatch to the pool first via try_update_plane_instance_transform, then fall through to the dense objects map. remove_object does the same and additionally cascades pool teardown when called on a source plane (frees buffers, invalidates instance handles).
• add_object_to_scene resolves a plane-instance handle to its source plane so existing scene APIs work unchanged. render_instanced_and_particles filters pool draws by source-plane scene membership.

FFI Layer (goud_engine/src/ffi/)

• New goud_renderer3d_instantiate_plane(context_id, source_plane_id) -> u32 in ffi/renderer3d/primitives.rs. Doc comment covers lifecycle (cascading source-plane destruction) and the interaction with set_static_batching_enabled / set_instancing_enabled / set_min_instances_for_batching (issue ask 3).

C# SDK (sdks/csharp/)

• Auto-generated GoudGame.InstantiatePlane(uint sourcePlaneId) and matching NativeMethods entry.

Python SDK (sdks/python/)

• Auto-generated instantiate_plane(source_plane_id) plus _ffi.py argtype/restype declarations.

TypeScript SDK (sdks/typescript/)

• Node napi binding changes (auto-generated)
• Web WASM binding changes
• Type definition changes
• Tests updated

Codegen Pipeline (codegen/)

• Schema changes (instantiatePlane method + ffi mapping entry)
• Generator changes
• Validator changes
• ffi_mapping changes

Proc Macros (goud_engine_macros/)

• #[goud_api] attribute changes

Tools (tools/)

• lint-layers changes

WASM (goud_engine/src/wasm/)

• wasm-bindgen exports
• Sprite renderer changes
• Texture loader changes

Examples (examples/)

No changes.

Documentation

FFI doc + schema doc explain the interaction between set_static_batching_enabled / set_instancing_enabled / set_min_instances_for_batching and primitives + plane instances.

---

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: cargo run -p lint-layers is clean
• SDK parity: Auto-generated for C#, Python, TypeScript, Go, Swift, Kotlin, Lua, C, C++
• Unsafe discipline: One // SAFETY: block in flush_dirty_plane_instance_pools covers the disjoint-field split borrow
• File size: All files under 500 lines

---

Testing

• cargo test --lib passes (4 787 tests, including 5 new plane-instance tests)
• cargo clippy --all-targets -- -D warnings is clean
• cargo fmt --all -- --check passes
• Python SDK tests pass — codegen regenerated only, no behavior change
• C# SDK tests pass — codegen regenerated only, no behavior change
• TypeScript SDK tests pass — codegen regenerated only, no behavior change

New tests in goud_engine/src/libs/graphics/renderer3d/tests.rs:

• test_instantiate_plane_collapses_many_tiles_to_single_draw_call — 10 000 tiles → 1 instanced draw.
• test_instantiate_plane_per_instance_transform_updates_pool_only — per-instance position/rotation/scale + remove keep the pool dense.
• test_instantiate_plane_two_source_planes_two_draw_calls — 1 000 + 2 000 instances over two source planes → 2 instanced draws.
• test_instantiate_plane_respects_current_scene — pool only draws when its source plane is in the current scene.
• test_remove_source_plane_cascades_pool_teardown — destroying the source plane frees the pool and invalidates instance handles.

Pre-existing failure: cargo test --test native_main_thread requires a real display and fails locally on main too. Not introduced by this PR; CI skips it via the CI env var.

SDK interface changes:

• New method InstantiatePlane(sourcePlaneId) exposed in C#, Python, TypeScript, Go, Swift, Kotlin, Lua, C, C++. No existing method signatures changed.

---

Version Bump

Bump type: minor
Justification: Adds a new public FFI export and SDK method without changing any existing signatures.

---

Security

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

---

Performance

• A 10 000-instance pool collapses to 1 instanced draw call (verified by test). For the 40 000-tile / 9-material throne_ge terrain in perf: CreatePlane() primitives bypass instancing — 40k terrain planes = 11k draw calls, only 43 instanced #679 this should drop draw calls from 11 000+ to ~9.
• Per-frame steady-state cost: zero allocations (scratch dirty-id Vec reused, instance Vec is uploaded as a slice, no clone). Grow path uses next_power_of_two capacity to bound buffer reallocations.

---

Deployment

• NuGet package version updated — separate release task
• Python package version updated — separate release task
• npm package version updated — separate release task
• Native library builds on all targets (codegen + cargo check pass)

---

Reviewer Notes

• Per-instance materials are intentionally not supported. Throne creates one source plane per material (9 in their case), which collapses to 9 instanced draws — matches the issue's expected outcome.
• set_object_static, set_static_batching_enabled, set_instancing_enabled, and set_min_instances_for_batching do not gate plane-instance pools — pools always use the instancing path. The FFI doc comment on goud_renderer3d_instantiate_plane and the schema doc string both spell this out (issue ask 3).
• The unsafe slice in flush_dirty_plane_instance_pools relies on self.instanced_meshes and self.backend being disjoint fields; the SAFETY comment documents the invariant.

🤖 Generated with Claude Code

[claude]

Claude finished @aram-devdocs's task in 4m 34s —— View job

---

PR Review: perf(renderer3d): InstantiatePlane (#685)

Verdict: CHANGES REQUESTED

---

Phase 1 — Context

Layers touched: Libs/Engine (renderer3d core), FFI (ffi/renderer3d/primitives.rs), SDKs (C#, Python, TypeScript, Go, Swift, Kotlin, Lua — all auto-generated), codegen (schema + FFI mapping).

PR type: perf — new public API that collapses plane-primitive draw calls into instanced batches.

---

Phase 2 — Architecture Validation ✅

• Dependency flow is downward-only. core_plane_instances.rs imports from within the renderer3d module only. No upward imports detected.
• No raw GPU calls outside backend/ — upload_buffer, upload_instance_buffer, update_instance_buffer, destroy_buffer all go through the backend trait. ✅
• All SDKs are thin wrappers with no logic. ✅
• File sizes: all under 500 lines. ✅

---

Phase 3 — Anti-Pattern Scan

Item	Result
#[no_mangle] extern "C" on new export	✅ present
unsafe blocks with // SAFETY:	✅ one block, comment present
FFI changes → C# + Python updated	✅ all SDKs regenerated
todo!() / unimplemented!() in new code	✅ none
unwrap() / expect() in library code	✅ none (all errors surface via ? or log::error!)
Duplicated logic between Rust and SDKs	✅ no duplication

---

Phase 4 — Quality Assessment

Tests are thorough — five new unit tests in tests.rs cover the stated assertions:

• Collapse to 1 draw call
• Per-instance transform isolation
• Two source planes → two draw calls
• Scene filtering
• Cascade teardown

Documentation on goud_renderer3d_instantiate_plane is excellent — lifecycle, batching-flag interaction, and per-material guidance are clearly documented.

Naming is consistent with the existing instantiate_model mirror.

---

Phase 5 — Security Review

• goud_renderer3d_instantiate_plane takes no pointer parameters — no null-check required. ✅
• Memory ownership is documented: the pool's GPU buffers are freed on source-plane destruction. ✅
• The single unsafe block is in flush_dirty_plane_instance_pools. The SAFETY comment accurately identifies the disjoint-field invariant. ✅

---

BLOCKERS

None.

---

WARNINGS

W1 — Unnecessary vertex clone on every instantiate_plane call after the first

File: goud_engine/src/libs/graphics/renderer3d/core_plane_instances.rs:51-56

pub fn instantiate_plane(&mut self, source_plane_id: u32) -> Option<u32> {
    let source = self.objects.get(&source_plane_id)?;
    let texture_id = source.texture_id;
    let source_vertices = source.vertices.clone();  // ← allocated unconditionally
    let vertex_count = source.vertex_count;

    let pool_mesh_id = if let Some(&existing) = self.source_plane_to_pool.get(&source_plane_id)
    {
        existing  // source_vertices never used in this branch
    } else {
        // source_vertices used here only
        let mesh_buffer = match upload_buffer(self.backend.as_mut(), &source_vertices) { …

source.vertices.clone() is called on every invocation of instantiate_plane, but source_vertices is only consumed inside the else branch (pool creation — first call per source plane). For the throne_ge terrain with ~40,000 instances per 9 source planes, this produces ~39,991 unnecessary Vec<f32> allocations and immediate drops during scene setup.

Fix: move the clone inside the else branch. Since texture_id and vertex_count are Copy, they can be read before the branch without keeping the borrow alive:

pub fn instantiate_plane(&mut self, source_plane_id: u32) -> Option<u32> {
    if !self.objects.contains_key(&source_plane_id) {
        return None;
    }

    let pool_mesh_id = if let Some(&existing) = self.source_plane_to_pool.get(&source_plane_id) {
        existing
    } else {
        let source = self.objects.get(&source_plane_id)?;
        let texture_id = source.texture_id;
        let source_vertices = source.vertices.clone();  // only when creating pool
        let vertex_count = source.vertex_count;
        // … pool creation unchanged …
    };
    // …
}

Fix this →

---

W2 — Unsafe block may be avoidable with safe split-field borrows

File: goud_engine/src/libs/graphics/renderer3d/core_plane_instances.rs:300-316

The in-place update path captures a raw pointer + length to sidestep the borrow checker, then reconstructs a slice. This is sound (the SAFETY comment correctly identifies the disjoint backend / instanced_meshes fields). However, the same effect can be achieved safely because Rust NLL can prove field disjointness when fields are accessed via distinct named paths:

// Safe alternative (no unsafe needed):
if let Some(m) = self.instanced_meshes.get(&mesh_id) {
    // m borrows self.instanced_meshes; self.backend is a separate field —
    // NLL can prove disjointness here
    if let Err(e) = update_instance_buffer(self.backend.as_mut(), buffer_handle, &m.instances) {
        log::error!("…");
        continue;
    }
}

If the safe version compiles, prefer it: it removes the need for the SAFETY comment and can't silently break if update_instance_buffer's internals ever touch instanced_meshes. Not a blocker — the current unsafe is correct.

---

W3 — Source plane drawn twice when no scene is active

The source plane object is stored in self.objects and is rendered by the per-object pass on every frame (as noted in the test comment at tests.rs:413). Without a scene filter, a caller using instantiate_plane for all tiles will get:

• 1 per-object draw for the source plane (from self.objects)
• 1 instanced draw for all instances (from the pool)

This is documented in the FFI doc-comment, but it's easy to miss. A usage note in the SDK wrappers or a more prominent callout in the FFI doc (e.g., "to suppress the source-plane's own draw call, add it to a scene but not to the current scene") would help callers avoid unexpected draw calls in production.

---

Positive Callouts

• scratch_dirty_plane_pool_ids pattern is excellent — reusing a persistent Vec for the dirty-pool scan is the right call and is already used consistently elsewhere in this renderer.
• Grow path using mem::take in flush_dirty_plane_instance_pools is clean: no extra allocation, the live Vec is restored with correct length after the padded upload.
• Cascade teardown (destroy_plane_pool_for_source) is thorough — all three maps (plane_instance_pools, instanced_meshes, plane_instance_index, source_plane_to_pool) are cleaned up correctly, and GPU buffers are freed.
• Test test_remove_source_plane_cascades_pool_teardown is a strong regression guard — it verifies both the pool teardown and handle invalidation paths.
• 5 new tests, all pure (no GL context) — correct use of NullBackend, consistent with the testing conventions.

---

[aram-devdocs]

@claude-review — addressed all three warnings from your earlier review on commit 3eddac6:

W1 — vertex clone moved into pool-creation branch (eac844e)
source.vertices.clone() now happens only inside the else branch where a new pool is created. The hot path for instances 2..N skips the clone entirely. For a 40 000-tile / 9-source-plane terrain this is ~39 991 fewer Vec allocations. See core_plane_instances.rs:51-69.

W2 — unsafe block removed in favor of safe split-borrow (eac844e)
The in-place update path now uses NLL field disjointness directly:

if let Some(mesh) = self.instanced_meshes.get(&mesh_id) {
    let buffer_handle = mesh.instance_buffer;
    if let Err(e) = update_instance_buffer(self.backend.as_mut(), buffer_handle, &mesh.instances) {
        ...
    }
}

cargo check --workspace --all-targets accepts it. No unsafe, no SAFETY comment needed.

W3 — source-plane visibility now documented prominently (eac844e)
Added a "Source plane visibility" section to the FFI doc explaining that the source plane itself still costs one per-object draw (e.g., 9 sources + 9 pools = 18 draws, not 9), with two recommended workarounds: place the source plane outside the camera frustum, or use it as the first visible tile.

Verification: cargo fmt --all -- --check, cargo clippy --all-targets -- -D warnings, and cargo test --lib all clean. 5 plane-instance tests pass; 4787 lib tests pass.

Auto-merge (squash) is enabled — please re-review.

[claude]

Claude finished @aram-devdocs's task in 1m 28s —— View job

---

PR Re-Review: perf(renderer3d): InstantiatePlane (#685)

Verdict: APPROVED ✅

All three warnings from the previous review have been correctly addressed. No new issues introduced.

---

Todo

• Load tools
• Read core_plane_instances.rs — verify W1 (clone moved) and W2 (unsafe removed)
• Read ffi/renderer3d/primitives.rs — verify W3 (visibility doc added)
• Scan for any new issues introduced
• Post final verdict

---

W1 — Vertex clone moved into pool-creation branch ✅

core_plane_instances.rs:51-113 — Fix is correct. The function now begins with a cheap contains_key existence check at line 55, and source.vertices.clone() at line 65 is inside the else branch (pool-creation path only). The hot path for instances 2..N hits the if let Some(&existing) branch and skips the clone entirely. The comment at lines 52-54 also explains the optimization clearly for future readers.

W2 — Unsafe block removed in favor of safe split-borrow ✅

core_plane_instances.rs:305-325 — Fix is correct. The in-place update path now uses:

if let Some(mesh) = self.instanced_meshes.get(&mesh_id) {
    let buffer_handle = mesh.instance_buffer;
    if let Err(e) = update_instance_buffer(self.backend.as_mut(), buffer_handle, &mesh.instances) { ... }
}

No unsafe block. NLL proves field disjointness between self.instanced_meshes (immutable borrow) and self.backend (mutable borrow) since they are distinct named struct fields. The comment on lines 307-308 briefly documents why this is safe without being a heavyweight SAFETY block.

W3 — Source plane visibility prominently documented ✅

ffi/renderer3d/primitives.rs:122-135 — A dedicated # Source plane visibility rustdoc section now leads with the concrete draw-call count (9 sources + 9 pools = 18 draws, not 9) and provides two actionable workarounds. Callers cannot miss this.

---

No New Issues

The grow path (lines 274-303) correctly uses mem::take → pad → upload → truncate → restore. If the upload fails, the live instance vec is restored before the error is handled — no data loss. The dirty_ids.drain(..) loop correctly returns the drained (empty) vec to scratch_dirty_plane_pool_ids after the loop, preserving the allocation for next frame. ID wrapping at lines 120-124 correctly skips 0 as an invalid handle.

---

Summary

Five new pure tests, clean clippy, no unsafe, no redundant allocations on the hot path. The three warnings are fully resolved. This is ready to merge.
| Branch