fix(particles): support non-interleaved meshes as particle meshes#8946
Conversation
ParticleEmitter._allocate copied the source mesh assuming an interleaved vertex layout (position in the first 3 floats, single global stride). Meshes from the procedural generators (e.g. createSphere) are non-interleaved, so the copy read scrambled/out-of-bounds data. Read positions and UVs using each element's own offset and stride instead, which works for both layouts. Fixes #5637 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Build size reportThis PR changes the size of the minified bundles.
|
mvaligursky
left a comment
There was a problem hiding this comment.
🤖 Automated PR review — posted on my behalf by Claude Code (Opus 4.8). Not a human review; flagging for awareness.
Verdict: correct fix, addresses the root cause cleanly. No blocking issues.
I verified the indexing math against VertexFormat (src/platform/graphics/vertex-format.js:181-182):
- interleaved →
offset= element offset within a vertex,stride= full vertex size; - non-interleaved (
vertexCountspecified, e.g.Mesh.fromGeometry/createSphere) →offset= start of that element's planar block,stride= the element's own size.
So meshData[vert * stride + offset] (in floats) is correct for both layouts, which is exactly the bug — the old single global totalFloats/numVertices stride only worked for interleaved meshes. Reading position and UV via their own element offset/stride is the right generalization, and zero-filling missing UVs (vs. the previous NaN run-off) is a strict improvement.
Other angles:
- Breaking changes: none — interleaved meshes resolve to identical indices.
- Performance:
.find()runs twice per_allocate, not per vertex. Negligible. - Cleanliness: clearer than the prior loop; good explanatory comment.
One minor latent assumption noted inline (non-blocking).
| const elements = this.mesh.vertexBuffer.format.elements; | ||
| const posElement = elements.find(el => el.name === SEMANTIC_POSITION); | ||
| const uvElement = elements.find(el => el.name === SEMANTIC_TEXCOORD0); | ||
| posOffset = posElement.offset / 4; |
There was a problem hiding this comment.
Minor / non-blocking: posElement.offset / 4 and / 4 throughout assume (a) SEMANTIC_POSITION is always present and (b) both position and UV are 32-bit floats.
(a) — if a mesh has no position element, posElement is undefined and this throws. A particle mesh without positions is degenerate, and the old code also assumed positions existed (at offset 0), so this isn't a regression — but unlike the UV path it's unguarded. A one-line guard or assert would make the failure mode explicit.
(b) — the / 4 (bytes→floats) is correct for the procedural generators this PR targets, but would mis-index a mesh with non-float positions/UVs (e.g. normalized int16 from a compressed/quantized source). Also a pre-existing limitation, not introduced here — just worth a comment so it isn't silently assumed universal.
Description
Particle systems can now use non-interleaved meshes (e.g.
pc.createSphere(device)) as the particle mesh.ParticleEmitter._allocatecopied the source mesh assuming an interleaved vertex layout (position in the first 3 floats of each vertex, one globaltotalFloats / numVerticesstride). Meshes from the procedural generators are non-interleaved (Mesh.fromGeometrybuilds the format with avertexCount, giving a planarPPP…NNN…UUU…layout), so the copy read scrambled and out-of-bounds data — only vertex 0 landed correctly, and UVs ran off the end into NaN.It now reads positions and UVs using each element's own
offsetandstride, which is correct for both interleaved and non-interleaved meshes. UVs are zero-filled when the source mesh has none (previously NaN).Fixes #5637
Checklist