⏳ Use InstancedMesh for generated clones and pedestrians

[kfarr]

🧑‍🦲 Status:

• pending user feedback, temporarily deployed to dev server
• issue: some 3d objects when instanced appear as oversized and distorted versions, possibly due to complicated transforms in the affected models
• since instanced objects aren't selectable (which is ok, kinda by design), there is no detach button visible; instead the managed street segment should have a detach option (or the street itself)

🤖 Summary

• Replace individual A-Frame entity creation with THREE.InstancedMesh in street-generated-clones and street-generated-pedestrians components
• Each unique model is now rendered as a single instanced draw call instead of one entity per clone
• detach() still works — stored placement specs are used to recreate individual entities on demand
• New shared utility src/lib/instanced-mesh-helper.js handles model loading and instanced mesh creation

Performance Results (East Ave scene)

Metric	Production (before)	With Instancing (after)	Improvement
Draw calls	2,397	1,954	-18%
Entities	5,525	2,112	-62%
Autocreated entities	5,083	1,670	-67%
Three.js Objects	39,274	10,036	-74%
JS Heap	2,466 MB	1,708 MB	-31%
Triangles	5.68M	5.57M	~same
FPS (avg)	1.3	36.8	28x faster
Frames in 10s	12	358	30x more

The remaining 1,670 autocreated entities are all from stencil components (left unchanged per plan).

Test plan

• npm test — all 309 tests pass
• npm run lint — no new errors
• Performance profiling — 28x FPS improvement on freezing scene
• Visual check — models appear in correct positions/rotations - FAIL for some models such as streetplan foliage and buildings
• Editor detach() — select segment, detach generated-clones, verify entities created -- FAIL do not have ability to trigger since instanced objects aren't selectable which is how the current detach button is accessed by user
• Test with various street templates (Streetmix import, StreetPlan import)

[claude]

PR Review: Use InstancedMesh for generated clones and pedestrians

This is a significant, well-motivated optimization. The 28x FPS improvement on the East Ave scene is impressive, and the architecture is clean overall. Here are my findings organized by severity.

---

Bugs / Correctness Issues

1. Race condition in update() when async model loading is in-flight

Affects: street-generated-clones.js and street-generated-pedestrians.js

update() calls this.remove() synchronously (resetting this.instancedGroups = []), then kicks off async model loading. If update() is called a second time before the first batch of promises resolves (e.g., a rapid segment-length-changed event), the first batch of promises still resolves and does:

this.el.object3D.add(group);      // orphaned — added to scene…
this.instancedGroups.push(group); // …into a fresh array that a second remove() won't see

This can permanently leak instanced meshes into the scene that can never be removed. Suggestion: cancel/ignore stale async operations by storing a generation counter and checking it before resolving.

---

2. removeEventListener in loadMixinModel removes the wrong reference

Affects: src/lib/instanced-mesh-helper.js lines 54–57 vs. line 42

The anonymous arrow function is registered as the listener:

tempEntity.addEventListener('model-loaded', () => {
  clearTimeout(timeout);
  onModelLoaded();
});

But cleanup() tries to remove onModelLoaded (the named function), which was never registered directly:

tempEntity.removeEventListener('model-loaded', onModelLoaded); // no-op

In practice this is harmless because the entity is immediately removed from the DOM, but it's technically dead code and could cause double-execution in edge cases.

---

3. Undefined position when pedestrian count exceeds available Z slots

Affects: street-generated-pedestrians.js lines 137–143

zPositions has floor((length / 1.5) + 1) entries. If totalPedestrians > zPositions.length, zPositions[i] is undefined, setting position.z = undefined (becomes NaN). For dense streets (density factor 0.25) this can happen for shorter segments. A guard like Math.min(totalPedestrians, zPositions.length) would fix it.

---

4. model-error not handled in loadMixinModel

If a GLTF fails to load, model-loaded never fires, and the 30-second timeout is the only escape hatch. Listening on model-error and rejecting immediately would give faster failure feedback and avoid 30-second hangs.

---

5. this.data accessed before null-check in street-generated-pedestrians.js

update: function (oldData) {
  const data = this.data;    // line 96 — assigned before the guard
  if (!this.length) { return; }
  if (!this.data) { return; } // line 101 — check comes after use

const data = this.data executes before the !this.data guard. This won't cause a runtime error in practice (A-Frame always initializes data before calling update), but the ordering is confusing and the check should precede any use.

---

Design / Architecture Observations

6. AFRAME.INSPECTOR required for detach()

Both detach() implementations call AFRAME.INSPECTOR.execute(). If detach() is ever invoked without the inspector loaded (e.g., a programmatic call outside the editor), this will throw. Worth guarding or documenting.

---

7. Hardcoded building dimensions in generateFit

The buildingWidths and buildingDepths lookup tables (~30 entries each) live inside a hot function that runs on every update() call. These should be module-level constants. Also, new building assets will require code changes rather than catalog metadata updates — worth noting as a maintenance consideration.

---

8. Instanced meshes don't receive shadows (receiveShadow = false)

instancedMesh.castShadow = true;
instancedMesh.receiveShadow = false;

The non-instanced fallback path (buildEntityClones) uses A-Frame's default mixin settings, which may set receiveShadow = true. This creates a visual difference between instanced and non-instanced modes. If the instancing-parity test ever runs with shadow-casting lights enabled, it may fail.

---

9. Documentation signature mismatch in docs/instanced-mesh.md

Line 16 of the doc describes:

createInstancedGroup(sourceObject, instances)

But the actual function signature is createInstancedGroup(sourceObject, mixinScale, instances) (3 params, not 2).

---

10. buildInstancedMeshes is duplicated verbatim between both components

The function is ~40 lines and nearly identical in both files (only the event name differs: clones-generated vs pedestrians-generated). This isn't a blocking issue, but it's worth extracting to instanced-mesh-helper.js for DRY maintenance, especially since any future changes (e.g., error handling improvements) would need to be made in both places.

---

Minor Nits

• getModelMixin() comment: // this is a hack but it works for now (line 399) — the fallback to modelsArray[0] when this.rng is null silently loses randomness. At minimum, a console.warn would surface this.
• new URLSearchParams(window.location.search) is constructed on every buildInstancedMeshes() call. Cache it once or move to module-level constant.
• The visual test in instancing-parity.spec.js compares each screenshot against a stored baseline but doesn't compare instanced vs. non-instanced directly. The draw call reduction is only logged, not asserted.

---

Summary

The performance gains are real and the architecture is sound. The most important issue to address before merging is #1 (race condition) — rapid updates during editing could leak geometry into the scene indefinitely. Issues #3 (undefined Z position) and #4 (no model-error handler) are also worth fixing. The rest are minor improvements.

Great work on the performance optimization overall!

[claude]

Code Review: InstancedMesh for generated clones and pedestrians

The performance numbers are impressive (28x FPS improvement!) and the architecture is sound. There are a few bugs and design concerns worth addressing before merging, particularly since the PR already flags two failing test items.

---

Critical Bugs

1. Event listener is never removed in loadMixinModel (src/lib/instanced-mesh-helper.js lines 55-64)

cleanup() tries to remove onModelLoaded, but the registered listener is an anonymous arrow function that wraps it. removeEventListener won't find a match, so the listener leaks on the temp entity:

The fix: either register onModelLoaded directly (and call clearTimeout inside it), or store the anonymous arrow wrapper in a variable so cleanup() can remove it.

---

2. Race condition: groups added after remove() is called

buildInstancedMeshes fires async promises. If the component is updated (which calls this.remove() then re-generates) before those promises resolve, this.instancedGroups.push(group) runs after cleanup has already cleared the array. The orphaned groups stay in this.el.object3D forever.

Fix: capture a generation ID at the start of each buildInstancedMeshes call and skip adding/pushing if the ID no longer matches this.currentGenerationId.

---

Significant Issues

3. buildEntityClones drops editor metadata attributes

The original createClone set four attributes used by the editor layer panel and filtering system. buildEntityClones (the ?instancing=off fallback) sets none of them:

• clone.classList.add('autocreated')
• clone.setAttribute('data-no-transform', '')
• clone.setAttribute('data-layer-name', 'Cloned Model / ' + mixinId)
• clone.setAttribute('data-parent-component', this.attrName)

The fallback path will produce a broken editor experience (entities shown without names, wrong layer counts, etc.).

---

4. Duplicate buildInstancedMeshes / buildEntityClones between the two components

Both components now have near-identical implementations of buildInstancedMeshes, buildEntityClones, remove, and detach (~100 lines of duplicated logic). Consider extracting a shared helper in instanced-mesh-helper.js to avoid future divergence.

---

Minor Issues

5. URLSearchParams re-parsed on every update

window.location.search does not change during a session. The new URLSearchParams(window.location.search) call inside buildInstancedMeshes (called on every update()) should be parsed once at init() or as a module-level constant.

---

6. Known transform/scale bug with complex models

The PR flags this as a failing visual check. In createInstancedGroup, meshLocalMatrix is computed using matrixWorld relative to sourceObject, but for multi-level hierarchies (e.g. vehicles with separate body/wheel meshes that have their own parent transforms) this may incorrectly bake in world-space transforms. At minimum, document which model types are affected as a condition for merge.

---

7. detach() loses 'select created entity' behavior

The original code put the selected entity's spec last in the multi-command array so it remained selected after detach. This is removed in both components. Since instanced objects aren't selectable this can't be replicated exactly, but consider selecting the first created entity after the multi-command executes.

---

8. Docs say 'puppeteer must be installed' but script uses Playwright

docs/instanced-mesh.md at the bottom reads: 'puppeteer must be installed (npm install puppeteer --save-dev)'. scripts/perf-profile.js actually uses require('playwright'). Update the docs.

---

Minor Notes

• The ?importer=managed flag in json-utils_1.1.js looks like a debug shortcut. Is this meant to be long-lived or temporary scaffolding?
• The visual test waitForStreetLoaded settling heuristic (3s of no DOM mutations) could be flaky on slow CI. A deterministic scene-level event would be more reliable.
• Playwright baseline screenshots aren't committed -- the first npm run test:visual run will auto-generate them. A note in the README about this would help new contributors.

---

Summary

The core instancing approach is well-designed and the performance win is real. Issues to resolve before merge: the race condition (groups leaking after rapid updates), missing editor metadata in buildEntityClones, and the event listener not being removed (low-impact but trivial to fix). The known transform bug with complex models is already tracked -- resolving or documenting affected model types should also be a merge condition given the failing visual check.