Store immediate line batches in retained typed arrays - #9286
Merged
Conversation
ImmediateBatch accumulated line data in two plain arrays and truncated both in clear() every frame, so each frame re-grew them element by element through push(). For a batch of any size that dominates the cost of submitting lines. The storage is now a pair of Float32Arrays with an explicit vertex count: - appends are index writes rather than push, with no capacity check per value - clear() resets the count and keeps the backing store, so a steady workload never reallocates - capacity grows by doubling, so a batch filled over many calls reallocates O(log n) times rather than per call - Mesh#setPositions and Mesh#setColors already accept a numVertices argument, so the mesh consumes only the written part of an over-sized buffer Measured on the graphics/lines example with a 60000 segment payload, comparing this against the previous implementation in the same browser: median time in addLinesArrays drops from 6.4ms to 1.3ms. The median gains more than the minimum because the old path re-grew its arrays every frame, and that allocation churn is what retaining the storage removes. Storage is handed back once a batch stops needing it: if no frame in the last 100 has used more than half the capacity, it shrinks to fit the largest frame seen while waiting, never below a 256 vertex floor. Shrinking to the recent peak rather than to the last frame keeps a batch whose usage varies from having to grow again immediately. Float32 storage is not a precision change, as the vertex buffer these feed is TYPE_FLOAT32 either way; the conversion simply happens earlier. Also sets cull = false on the batch mesh instance. It is injected straight into the visible list and update() is called with updateBoundingBox false, so its bounding box is never computed and it must not take part in culling. This matches WideLineRenderer, which already does it. The debug asserts on mismatched color array lengths are new. The contract was already documented but never checked, and a short array previously either threw a TypeError deep inside addLines or silently produced a colors array shorter than the positions. Adds unit tests for the batch, which had none: data layout for both add paths, uniform and per-vertex colors, growth, mid-frame growth preserving earlier data, storage retention across clear, and each branch of the shrink policy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Build size reportThis PR changes the size of the minified bundles.
|
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The focused optimization preserves existing behavior and is thoroughly covered by targeted tests.
Pull request overview
Optimizes immediate line batching by retaining typed-array storage across frames while safely managing capacity.
Changes:
- Adds grow-and-shrink capacity management for line data.
- Disables culling for immediate batch meshes.
- Adds comprehensive batching and capacity tests.
File summaries
| File | Description |
|---|---|
src/scene/immediate/immediate-batch.js |
Implements retained typed-array batching and capacity management. |
test/scene/immediate/immediate-batch.test.mjs |
Tests layout, growth, retention, assertions, and shrinking. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
Those should have probably been separate PRs, but still a great change. |
Contributor
Author
What do you mean? It's an isolated single optimisation. |
Contributor
|
I've misunderstood the purpose of culling. All good. |
mvaligursky
added a commit
that referenced
this pull request
Sep 3, 2026
…egory (#9288) * Add WireRenderer for wireframe debug shapes, and a debug examples category Resolves #6024. `AppBase` grew a set of `@ignore` debug drawing methods over time - `drawWireSphere` and `drawWireAlignedBox` among them - which are used by a good number of examples despite never being public. This moves that functionality into extras as `WireRenderer`, and fills it out into the shape set that debugging actually calls for. ```javascript const wire = new WireRenderer(app); wire.color = Color.RED; wire.sphere(entity.getPosition(), 2); wire.frustum(otherCamera.camera); ``` ## API State lives on the renderer - `color`, `layer`, `depthTest`, `segments`, `transform` - rather than in a per-call options object, so drawing a thousand shapes with the same settings allocates nothing. A second set of state is simply a second instance: instances hold no GPU resources, and those sharing a layer and depth test mode submit into the same batch, so using several costs nothing extra. This mirrors how `Immediate` already keys its batches on (layer, depthTest). Shapes: `line`, `lines`, `linesPacked`, `polyline`, `loop`, `box`, `boxMinMax`, `sphere`, `circle`, `cylinder`, `capsule`, `cone`, `plane`, `point`, `arrow`, `axes`, `frustum`, `light`. Everything is thin lines. Solid shapes and arbitrary mesh submission are left out deliberately: they need the mesh path, which allocates a MeshInstance and a GraphNode per call, and they belong with whatever replaces `drawMesh` rather than here. `WideLineRenderer` remains the home for thick production line geometry, and both classes now cross-reference the other. `frustum` accepts a camera or a view-projection matrix. It derives the view matrix from the camera's entity transform rather than reading `CameraComponent#viewMatrix`, which is only refreshed for a camera that is actually rendering - and visualizing a camera you are not looking through is the whole point. `light` follows the light's own axis. A light shines along the negative y-axis of its entity, not the negative z-axis that `lookAt` aims, so anything using `forward` here is 90 degrees wrong. ## Writing straight into the batch Shapes are generated directly into the immediate batch's storage rather than built in an array and copied in. `Immediate#allocateLines` claims an exact number of vertices, accounts for them up front and returns a reused `LineWriter` cursor. Because the space is accounted for immediately there is no commit call, and because the cursor is reused there is no allocation per shape. It is therefore only valid inside the function that allocated it. `_arc` writes the storage itself instead of going through `LineWriter#segment`. It backs sphere, circle, cylinder, capsule, cone and arrow, so nearly every segment passes through it. The other shapes use the checked `segment` and `vertex` methods. Measured on 1000 spheres, 60000 segments: 2.4ms to 1.4ms. Most of that came from hoisting the center and tangent reads out of the arc loop rather than from avoiding the call, which was worth only about 14 percent once the hoisting was in. Stacked on #9286 the same workload went from 13.6ms to 1.4ms. ## Examples Adds a `debug` category, since debug visualization examples were scattered: - `debug/wire-shapes` - every shape, animated across the range of values it takes, with segments, depth test and animation controls - `debug/frustum-culling` - an orbiting camera whose volume is drawn with `frustum`, every object's bounds colored by `Frustum#containsAabb`, and arrows showing direction of travel - `debug/lines` - moved from `graphics`, and rewritten onto WireRenderer. Uses all five line functions at once - `debug/mini-stats` - moved from `misc` `graphics/clustered-lighting` gains a toggle that draws every light's shape in its own color. ## Tests 76 new tests. `WireRenderer` is covered for exact allocation per shape, the degenerate input paths, real geometry rather than just counts, the color and transform paths, passthrough versus copy, batch routing, and `light` aiming along the correct axis. `LineWriter` is covered for offsets, bounds asserts and seeing reallocated storage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Address review: frustum matrices, packed length validation, LineWriter type `frustum` now builds a camera's matrices the way the renderer does, so the shape drawn is the volume the camera would actually render. The view transform is scale-free, and calculateProjection and calculateTransform are both honored. Previously the full entity world transform was inverted, so a camera under a scaled parent, or an oblique or reflection camera, drew the wrong frustum. `linesPacked` asserts its position count is a multiple of six. Anything else either leaves a partial position, which made the vertex count fractional and corrupted the batch, or an unpaired vertex, which mispairs every later line. Imports the `LineWriter` type, which was annotated but never declared. Adds tests for all three, and both frustum tests fail against the previous derivation. --------- Co-authored-by: Martin Valigursky <mvaligursky@snapchat.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ImmediateBatchaccumulated line data in two plain arrays and truncated both inclear()every frame, so each frame re-grew them element by element throughpush(). For a batch of any size that dominates the cost of submitting lines — it is the single hottest thing in the immediate line path.The storage is now a pair of
Float32Arrays with an explicit vertex count:push, with no capacity check per valueclear()resets the count and keeps the backing store, so a steady workload never reallocatesMesh#setPositionsandMesh#setColorsalready accept anumVerticesargument, so the mesh consumes only the written part of an over-sized bufferMeasured
Same payload (60000 segments), same browser, this branch vs
main:pushinto plain arrays)The median gains more than the minimum because the old path re-grew its arrays every frame; that allocation churn is precisely what retention removes.
Float32 storage is not a precision change — the vertex buffer these feed is
TYPE_FLOAT32either way, so the conversion just happens earlier.Giving storage back
If no frame in the last 100 has needed more than half the capacity, the batch shrinks to fit the largest frame seen while waiting, never below a 256 vertex floor. Shrinking to the recent peak rather than to the last frame stops a batch whose usage varies from having to grow again immediately.
Verified live through the engine's own
onPostRender()ongraphics/lines: after inflating a batch to 131072 vertices, it shrank to 16384 — exactly fitting the observed per-frame peak of 13728 — reclaiming ~3.1 MB.One known limit:
Immediate#onPostRenderonly clears layers touched that frame, so batches in a layer that stops being used entirely keep their storage. It is bounded by that layer's past peak and cannot grow, and clearing every layer every frame would cost more than it saves. Batches within a used layer do tick, which covers the common case (for example thedepthTest: falsebatch when nothing draws x-ray lines).Also in here
cull = falseon the batch mesh instance. It is injected straight into the visible list andupdate()is called withupdateBoundingBoxfalse, so its bounding box is never computed and it must not take part in culling.WideLineRendereralready does this;ImmediateBatchdid not.Debug asserts on mismatched color array lengths. The contract was already documented but never checked. A short array previously either threw a
TypeErrordeep insideaddLinesor silently produced a colors array shorter than the positions. The assert now names the problem first. Flagging it explicitly since it is adjacent to, rather than required by, the perf work — happy to drop it if you would rather keep this change pure.Tests
The batch had none. Adds 19 covering data layout for both add paths, uniform and per-vertex colors, typed-array input, growth by doubling, mid-frame growth preserving earlier data, storage retention across
clear(), and every branch of the shrink policy. Full suite 2566 passing.Verified visually on
graphics/lines, which exercises all three entry points:addLinesArrayswith a per-vertex color array (the gradient grid),drawLinewith a single color (the magenta links), anddrawLineswith aColor[](the grey spokes).Landing this first, ahead of a
WireRendererchange that builds on the same path.🤖 Generated with Claude Code