Skip to content

Commit 7ac30db

Browse files
aram-devdocsclaude
andauthored
perf(renderer3d): InstantiatePlane collapses identical plane primitives into one instanced draw (#679) (#685)
* perf(renderer3d): add InstantiatePlane to collapse identical plane primitives into one instanced draw (#679) `CreatePlane` primitives previously bypassed the instancing path so a per-tile terrain (40k planes / 9 materials) produced 11k+ draw calls. `InstantiatePlane` mirrors `InstantiateModel` for primitives: every instance that shares the same source plane renders through a single instanced draw call -- ~9 draws for a 9-material terrain regardless of tile count. - New `instantiate_plane(source_plane_id) -> u32` engine method backed by a per-source-plane pool (`PlaneInstancePool`) over the existing `instanced_meshes` map. Pool grows with `next_power_of_two` capacity, dirty flush re-uploads once per frame. - `set_object_position` / rotation / scale and `remove_object` dispatch to the pool first; `add_object_to_scene` resolves an instance handle to its source plane so the existing scene API works unchanged. - Scene filtering for plane-instance pools: a pool only draws when its source plane is in the current scene. - New FFI export `goud_renderer3d_instantiate_plane` and schema entry; codegen regenerated all 9 SDK wrappers. - 4 new tests: 10k tiles collapse to 1 instanced draw; per-instance transforms + remove keep the pool dense; two source planes -> two draw calls; current scene gates the pool. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(renderer3d): cascade plane-instance pool teardown + skip avoidable clone (#679 review) Addresses code-quality, security, and spec review feedback on the InstantiatePlane PR: - Removing the source plane via destroy_object now tears down its pool (frees GPU buffers, invalidates every instance handle) so a future object id reusing the same value cannot route into a stale pool. - flush_dirty_plane_instance_pools no longer clones the live instances Vec on the steady-state in-place upload path; uses a split-borrow with a SAFETY-commented unsafe slice plus a persistent scratch buffer for the dirty-id list. Grow path uses mem::take/swap to pad without cloning. - instantiate_plane drops the .expect() panic; lookup falls back to None gracefully and reserves the object id only after the pool lookup succeeds. - New regression test test_remove_source_plane_cascades_pool_teardown asserts pool teardown + handle invalidation when the source plane is destroyed. - FFI doc + schema doc explain interaction with set_static_batching_enabled, set_instancing_enabled, and set_min_instances_for_batching for primitives and InstantiatePlane (spec ask 3 of #679). All SDK wrappers regenerated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor(tests): split plane-instance tests into sibling file (#679 CI) scripts/check-rs-line-limit.sh enforces a 500-line cap on .rs files. The new plane-instance tests pushed renderer3d/tests.rs to 615 lines, which made CI's `Rust File Line Limit` job fail. Moved the 5 plane-instance tests into a sibling `tests_plane_instances.rs` so both files stay under the cap. No test logic changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(ffi): re-export goud_renderer3d_instantiate_plane from renderer3d/mod.rs (#679 CI) The Lua codegen imports `crate::ffi::renderer3d::goud_renderer3d_instantiate_plane`, which requires it to be re-exported from `ffi/renderer3d/mod.rs` like its sibling primitive exports. Added it to the `pub use primitives::{...}` list so `cargo check --workspace --all-targets` (the Preflight CI gate) passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: ignore .claude/scheduled_tasks.lock runtime artifact This file is a per-machine lockfile created by Claude Code's ScheduleWakeup runtime; it should not be tracked in git. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * perf(renderer3d): drop redundant clones + unsafe in plane-instance pool (#679 review) Addresses Claude PR review warnings on #685: - W1: source.vertices.clone() now happens only when creating a new pool (the else branch). Previously it ran on every instantiate_plane call, costing ~39_991 redundant Vec<f32> allocations during a 40k-tile setup over 9 source planes. - W2: replace the unsafe split-borrow + raw pointer slice in flush_dirty_plane_instance_pools' in-place update path with a safe field-disjoint borrow. NLL proves self.instanced_meshes and self.backend don't overlap; the SAFETY comment is no longer needed. - W3: FFI doc on goud_renderer3d_instantiate_plane now spells out that the source plane itself still costs one per-object draw on top of the instanced draw, and recommends frustum exclusion or first-tile reuse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(ts-web-sdk): add instantiatePlane stub to satisfy IGoudGame (#679 CI) The web TypeScript SDK doesn't have a real backend for renderer3d, so codegen/gen_ts_web.py emits hardcoded stubs returning 0/false for every 3D method. The IGoudGame interface requires instantiatePlane (added in this PR), so the web GoudGame class needs a matching stub or TS2420 fails the wasm build. Added the stub next to instantiateModel; regenerated all SDKs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 8e9795e commit 7ac30db

36 files changed

Lines changed: 795 additions & 9 deletions

File tree

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,3 +132,4 @@ examples/go/*/sandbox
132132
# Coverage artifacts
133133
.coverage
134134
**/coverage/
135+
.claude/scheduled_tasks.lock

codegen/ffi_manifest.json

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3695,6 +3695,15 @@
36953695
"return_type": "i32",
36963696
"is_unsafe": true
36973697
},
3698+
"goud_renderer3d_instantiate_plane": {
3699+
"source_file": "ffi/renderer3d/primitives.rs",
3700+
"params": [
3701+
"context_id: GoudContextId",
3702+
"source_plane_id: u32"
3703+
],
3704+
"return_type": "u32",
3705+
"is_unsafe": false
3706+
},
36983707
"goud_renderer3d_is_animation_playing": {
36993708
"source_file": "ffi/renderer3d/animation.rs",
37003709
"params": [
@@ -6593,5 +6602,5 @@
65936602
"is_unsafe": false
65946603
}
65956604
},
6596-
"total_count": 669
6605+
"total_count": 670
65976606
}

codegen/ffi_mapping.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -636,6 +636,7 @@
636636
"renderer_3d": {
637637
"goud_renderer3d_create_cube": {},
638638
"goud_renderer3d_create_plane": {},
639+
"goud_renderer3d_instantiate_plane": {},
639640
"goud_renderer3d_create_sphere": {},
640641
"goud_renderer3d_create_cylinder": {},
641642
"goud_renderer3d_set_object_position": {},

codegen/gen_ts_web.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1162,6 +1162,7 @@ def gen_web_wrapper():
11621162
lines.append(" loadModel(_path: string): number { return 0; }")
11631163
lines.append(" destroyModel(_modelId: number): boolean { return false; }")
11641164
lines.append(" instantiateModel(_sourceModelId: number): number { return 0; }")
1165+
lines.append(" instantiatePlane(_sourcePlaneId: number): number { return 0; }")
11651166
lines.append(" setModelMaterial(_modelId: number, _meshIndex: number, _materialId: number): boolean { return false; }")
11661167
lines.append(" getModelMeshCount(_modelId: number): number { return 0; }")
11671168
lines.append(" getModelBoundingBox(_modelId: number): { minX: number; minY: number; minZ: number; maxX: number; maxY: number; maxZ: number } { return { minX: 0, minY: 0, minZ: 0, maxX: 0, maxY: 0, maxZ: 0 }; }")

codegen/generated/goud_engine.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2728,6 +2728,11 @@ uint32_t goud_renderer3d_create_cube(struct GoudContextId context_id, uint32_t t
27282728
*/
27292729
uint32_t goud_renderer3d_create_plane(struct GoudContextId context_id, uint32_t texture_id, float width, float depth);
27302730

2731+
/**
2732+
* Creates an instance of a source plane primitive.
2733+
*/
2734+
uint32_t goud_renderer3d_instantiate_plane(struct GoudContextId context_id, uint32_t source_plane_id);
2735+
27312736
/**
27322737
* Creates a 3D sphere object.
27332738
*/

codegen/goud_sdk.schema.json

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5055,6 +5055,17 @@
50555055
],
50565056
"returns": "u32"
50575057
},
5058+
{
5059+
"name": "instantiatePlane",
5060+
"doc": "Creates an instanced plane that shares geometry with a source plane (issue #679). All instances of the same source plane render through one instanced draw call regardless of setStaticBatchingEnabled / setInstancingEnabled / setMinInstancesForBatching (those flags govern non-instanced primitives and skinned models, not plane-instance pools). Use one source plane per material to draw multiple materials. Destroying the source plane cascades: the pool is freed and existing instance handles are invalidated.",
5061+
"params": [
5062+
{
5063+
"name": "sourcePlaneId",
5064+
"type": "u32"
5065+
}
5066+
],
5067+
"returns": "u32"
5068+
},
50585069
{
50595070
"name": "createSphere",
50605071
"doc": "Creates a 3D sphere",
@@ -13032,6 +13043,9 @@
1303213043
"createPlane": {
1303313044
"ffi": "goud_renderer3d_create_plane"
1303413045
},
13046+
"instantiatePlane": {
13047+
"ffi": "goud_renderer3d_instantiate_plane"
13048+
},
1303513049
"createSphere": {
1303613050
"ffi": "goud_renderer3d_create_sphere"
1303713051
},

goud_engine/src/ffi/renderer3d/mod.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,9 @@ pub use postprocess::{
6262
pub use primitives::{
6363
goud_renderer3d_create_cube, goud_renderer3d_create_cylinder, goud_renderer3d_create_plane,
6464
goud_renderer3d_create_sphere, goud_renderer3d_destroy_object,
65-
goud_renderer3d_set_object_position, goud_renderer3d_set_object_rotation,
66-
goud_renderer3d_set_object_scale, goud_renderer3d_set_object_static,
65+
goud_renderer3d_instantiate_plane, goud_renderer3d_set_object_position,
66+
goud_renderer3d_set_object_rotation, goud_renderer3d_set_object_scale,
67+
goud_renderer3d_set_object_static,
6768
};
6869
pub use scene::{
6970
goud_renderer3d_add_light_to_scene, goud_renderer3d_add_model_to_scene,

goud_engine/src/ffi/renderer3d/primitives.rs

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,92 @@ pub extern "C" fn goud_renderer3d_create_plane(
106106
.unwrap_or(GOUD_INVALID_OBJECT)
107107
}
108108

109+
/// Creates an instance of a source plane primitive.
110+
///
111+
/// Mirrors `goud_renderer3d_instantiate_model` for primitives. Every instance
112+
/// of the same source plane renders through one instanced draw call, so a
113+
/// terrain of identical-geometry tiles collapses to one batch per source plane
114+
/// (issue #679).
115+
///
116+
/// The returned id behaves like an object id for transform updates
117+
/// (`set_object_position`, `set_object_rotation`, `set_object_scale`,
118+
/// `destroy_object`). Per-instance materials are not supported -- the source
119+
/// plane's material/texture is captured when the first instance is created.
120+
/// Use one source plane per material to draw multiple materials.
121+
///
122+
/// # Source plane visibility
123+
///
124+
/// The source plane is a regular `Object3D`, so it is also drawn by the
125+
/// per-object pass. Each pool therefore costs **one extra per-object draw on
126+
/// top of the instanced draw** (e.g., 9 source planes + 9 pools = 18 draws,
127+
/// not 9). The pool's draw call collapse is still the dominant win for
128+
/// large terrains, but if the source plane is camera-visible callers should
129+
/// either:
130+
///
131+
/// * Position the source plane outside the camera frustum / view region so
132+
/// frustum culling drops it (recommended for terrain templates), or
133+
/// * Use it as the first visible tile (place it where one of the pool's
134+
/// instances would have been) so the per-object draw is not "extra".
135+
///
136+
/// # Lifecycle
137+
///
138+
/// Destroying the source plane via `destroy_object` cascades: the underlying
139+
/// pool's GPU buffers are freed, every existing instance handle is invalidated,
140+
/// and subsequent calls to `instantiate_plane` with the same id return
141+
/// `GOUD_INVALID_OBJECT`. Adding the source plane to a scene via
142+
/// `add_object_to_scene` makes the entire pool visible in that scene; passing
143+
/// an instance handle to `add_object_to_scene` resolves to the source plane.
144+
///
145+
/// # Interaction with batching flags
146+
///
147+
/// Plane-instance pools always render through the instanced path. The
148+
/// renderer's batching flags do **not** gate them:
149+
///
150+
/// - `set_static_batching_enabled` controls the static-batch VBO that combines
151+
/// non-instanced primitives marked with `set_object_static`. It does **not**
152+
/// apply to plane-instance pools and does not need to be enabled to benefit
153+
/// from `instantiate_plane`.
154+
/// - `set_instancing_enabled` enables instanced rendering for skinned model
155+
/// instances above the `min_instances_for_batching` threshold. It does
156+
/// **not** apply to plane-instance pools, which always batch regardless of
157+
/// the flag or instance count.
158+
/// - `set_min_instances_for_batching` is the threshold for skinned model
159+
/// instancing. It is ignored by plane-instance pools.
160+
///
161+
/// Primitives created via `create_plane` (without `instantiate_plane`) still
162+
/// follow the legacy paths: `set_object_static` + `set_static_batching_enabled`
163+
/// places them in the static-batch VBO; otherwise they render via the
164+
/// per-object pass.
165+
///
166+
/// # Arguments
167+
/// * `context_id` - The windowed context
168+
/// * `source_plane_id` - Object id returned by `goud_renderer3d_create_plane`
169+
///
170+
/// # Returns
171+
/// Plane-instance handle on success, GOUD_INVALID_OBJECT on failure.
172+
#[no_mangle]
173+
pub extern "C" fn goud_renderer3d_instantiate_plane(
174+
context_id: GoudContextId,
175+
source_plane_id: u32,
176+
) -> u32 {
177+
if context_id == GOUD_INVALID_CONTEXT_ID {
178+
set_last_error(GoudError::InvalidContext);
179+
return GOUD_INVALID_OBJECT;
180+
}
181+
182+
if let Err(e) = ensure_renderer3d_state(context_id) {
183+
set_last_error(e);
184+
return GOUD_INVALID_OBJECT;
185+
}
186+
187+
with_renderer(context_id, |renderer| {
188+
renderer
189+
.instantiate_plane(source_plane_id)
190+
.unwrap_or(GOUD_INVALID_OBJECT)
191+
})
192+
.unwrap_or(GOUD_INVALID_OBJECT)
193+
}
194+
109195
/// Creates a 3D sphere object.
110196
#[no_mangle]
111197
pub extern "C" fn goud_renderer3d_create_sphere(

goud_engine/src/jni/generated.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3242,6 +3242,26 @@ pub extern "system" fn Java_com_goudengine_internal_GoudGameNative_createPlane<'
32423242
})
32433243
}
32443244

3245+
#[allow(non_snake_case)]
3246+
#[no_mangle]
3247+
pub extern "system" fn Java_com_goudengine_internal_GoudGameNative_instantiatePlane<'local>(
3248+
mut env: jni::JNIEnv<'local>,
3249+
_class: jni::objects::JClass<'local>,
3250+
contextId: jni::sys::jlong,
3251+
sourcePlaneId: jni::sys::jint,
3252+
) -> jni::sys::jint {
3253+
crate::jni::helpers::catch_jni_panic(&mut env, "Java_com_goudengine_internal_GoudGameNative_instantiatePlane", 0, |env| -> crate::jni::helpers::JniCallResult<jni::sys::jint> {
3254+
crate::jni::helpers::prepare_call(env)?;
3255+
crate::jni::helpers::clear_last_error();
3256+
let result = crate::ffi::renderer3d::goud_renderer3d_instantiate_plane(goud_context_id_from_jlong(contextId), sourcePlaneId as _);
3257+
if crate::jni::helpers::last_error_code() != 0 {
3258+
let _ = crate::jni::helpers::throw_engine_error(env, "goud_renderer3d_instantiate_plane", Some(result as i64));
3259+
return Err(());
3260+
}
3261+
Ok(result as i32)
3262+
})
3263+
}
3264+
32453265
#[allow(non_snake_case)]
32463266
#[no_mangle]
32473267
pub extern "system" fn Java_com_goudengine_internal_GoudGameNative_createSphere<'local>(

goud_engine/src/libs/graphics/renderer3d/core/mod.rs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,18 @@ pub struct Renderer3D {
165165
/// Object IDs that were actually included in the static batch (not overflowed).
166166
/// Used to distinguish batched objects from overflow objects that need individual draws.
167167
pub(in crate::libs::graphics::renderer3d) static_batched_ids: FxHashSet<u32>,
168+
/// Pools of instanced plane primitives, keyed by `instanced_mesh_id`.
169+
/// One pool per source plane; every instance through `instantiate_plane`
170+
/// goes into the corresponding pool's `InstancedMesh`, collapsing to a
171+
/// single instanced draw call per source plane (#679).
172+
pub(in crate::libs::graphics::renderer3d) plane_instance_pools:
173+
FxHashMap<u32, super::core_plane_instances::PlaneInstancePool>,
174+
/// Reverse lookup from a plane-instance handle to `(instanced_mesh_id, slot)`.
175+
pub(in crate::libs::graphics::renderer3d) plane_instance_index: FxHashMap<u32, (u32, usize)>,
176+
/// Reverse lookup from a source plane object id to its pool's `instanced_mesh_id`.
177+
pub(in crate::libs::graphics::renderer3d) source_plane_to_pool: FxHashMap<u32, u32>,
178+
/// Reusable scratch buffer of dirty pool ids (avoids per-frame alloc).
179+
pub(in crate::libs::graphics::renderer3d) scratch_dirty_plane_pool_ids: Vec<u32>,
168180
}
169181

170182
// StaticBatchGroup is defined in core_static_batch.rs
@@ -364,6 +376,10 @@ impl Renderer3D {
364376
static_batch_groups: Vec::new(),
365377
static_batch_vertex_count: 0,
366378
static_batched_ids: FxHashSet::default(),
379+
plane_instance_pools: FxHashMap::default(),
380+
plane_instance_index: FxHashMap::default(),
381+
source_plane_to_pool: FxHashMap::default(),
382+
scratch_dirty_plane_pool_ids: Vec::new(),
367383
})
368384
}
369385
}

0 commit comments

Comments
 (0)