fix(core): bound the pie-chart label atlas to the device texture limit - #458
Merged
Conversation
…mits The pie-chart atlas grows with the point count and nothing checks it against gl.MAX_TEXTURE_SIZE, which is queried nowhere in the tree. An over-size texImage2D raises INVALID_VALUE — not a JS exception — but labelTextureInitialized is set regardless, so every later update runs texSubImage2D against storage that was never allocated. A device reporting the WebGL2 floor of 2048 cannot allocate the atlas for the shipped 573,649-protein bundle, and shows black marks with a clean console. Prerequisite for #456: raising MAX_POINTS_DIRECT_RENDER to 2,000,000 on today's code makes the atlas 7813 rows, fatal on every 4096 device. Refs #457 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USFuj7M2Ls1hpJXKaX2uJp
The stub had neither, and texImage2D/texSubImage2D/uniform1i were bare noops, so no test could observe what geometry the renderer hands the driver or detect an allocation that failed without throwing. Adding either call to the renderer would have thrown TypeError across every renderer suite. Lands alone, ahead of any renderer change, so a mock regression bisects separately. All 23 renderer suites (115 tests) pass unchanged. Refs #457 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USFuj7M2Ls1hpJXKaX2uJp
The pie-chart atlas is sized from point capacity, so its height silently made gl.MAX_TEXTURE_SIZE the point ceiling: a device reporting the WebGL2 floor of 2048 could not allocate it for the shipped 573,649-protein bundle. The failure was invisible and permanent — an over-size texImage2D raises INVALID_VALUE rather than throwing, but labelTextureInitialized was set regardless, so every later update ran texSubImage2D against storage that never existed. - Probe gl.MAX_TEXTURE_SIZE once per context and plan geometry in a new pure module. Full stride is preferred over a wider texture, so every device at 4096 or above allocates byte-identical geometry to before (2048x2241 at Swiss-Prot); width widens to 4096/8192 first, and only then does stride fall to 4, then 2. Points are never dropped to fit a decoration. - Bound planned capacity by MAX_POINTS_DIRECT_RENDER. The constant capped maxPoints but not capacity, so 900k then 950k planned 1,350,144 -> 5274 rows and failed on a 4096 device at a point count well under a million. - Check gl.getError() once per capacity change, never per frame: after the allocating bufferData (before any texture call, so a buffer failure is not misattributed) and after the atlas texImage2D. Mark initialised only on NO_ERROR; fall back to a 1x1 placeholder that keeps the sampler complete. - Gate the shader's pie branch on a new u_labelAtlasCapacity, so a point outside the atlas paints its dominant colour instead of black. - Surface reductions through the host-message channel, latched once per reason, and route the equally silent gamma fallback through it. Three defects found while verifying, none of them in the report: - stage-point wrote labelCounts unclamped while at most MAX_LABELS texels were filled, so a >8-colour point sampled the NEXT protein's texels. - The shader clamped neither count nor sliceIndex; atan(+0, x<0) is exactly +PI, so the sweep reached 1.0 and overran by one on well-formed points too. - POINT_FRAGMENT_SHADER declared only highp float, leaving v_pointIndex and the atlas index mediump — undefined above 32,767 on any driver honouring the 16-bit minimum, i.e. the low-end hardware this change is about. Export gets the same treatment: its own probe, the stride inherited from the live view so a figure matches the screen, an error check, and a MAX_DIMENSION that is min(8192, device limit) rather than a constant its message called "the browser limit". Refs #457 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USFuj7M2Ls1hpJXKaX2uJp
A gl.MAX_TEXTURE_SIZE stub alone proves nothing: it changes what the app believes, while the real driver on the test machine accepts the allocation regardless. And the reported failure is silent — nothing called gl.getError() — so "no console errors" passes on the broken code too. So the driver's refusal is simulated as well: texImage2D past the simulated limit does not call through and arms getError, exactly as a real driver does (no exception, nothing unwinds, texture left unallocated), and a later texSubImage2D against it raises INVALID_OPERATION. The assertion is then that the renderer never *issues* an allocation the device would refuse. Verified red on the parent commit: "renderer issued texture allocations the device refuses: [[2048,31]]", and no warning surfaced. Green after the fix. The demo dataset is ~7.8K proteins, so its atlas is 2048x31 and no realistic limit forces a stride reduction at that size; this spec covers the no-atlas-fits path. The reduced-stride path needs the 573K fixture. Refs #457 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USFuj7M2Ls1hpJXKaX2uJp
- Pin the atlas byte layout against the shader's own index arithmetic at every supported width and stride. A width change would otherwise be a silent, invisible corruption: the CPU writes at a linear texel index and the shader recovers the texel from that index and the width. - Assert the new capacity uniform is actually pushed, and that zero is pushed when no atlas is allocated — the value that makes the pie branch unreachable. - Cover export stride inheritance (device limit and live stride, smaller wins; null means no atlas) and the export dimension guard now naming the limit it enforces rather than a constant. - Add the reduced-stride case to the opt-in 573K spec. That is the size the issue actually reported: 2048x2241 against a 2048 device. The demo dataset's atlas is 2048x31, so the default suite can only reach the no-atlas path. Extract the GL simulation into helpers/gl-simulation.ts, now shared by both. Refs #457 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USFuj7M2Ls1hpJXKaX2uJp
Merges the renderer-capability-limits capability and the point-visibility delta into openspec/specs/, and replaces the archive tool's TBD Purpose placeholder with the real one — it validates clean either way, so nothing else catches it. Refs #457 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01USFuj7M2Ls1hpJXKaX2uJp
This was referenced Aug 16, 2026
…derers The pure geometry half of the atlas work already lived in one place (`label-atlas-plan.ts`); the GL half did not. This adds its sibling, `label-atlas-texture.ts`, and routes both the live and export paths through it. - `MIN_MAX_TEXTURE_SIZE` was declared in both renderers and hardcoded a third time as the narrowest atlas width. It now lives once, in the plan module, with the width ladder derived from it. - The probe-and-sanitize ternary was spelled out twice; both call `readMaxTextureSize`. The export dimension check keeps its own fallback on purpose — an unknown limit must not tighten the bound to 2048 — and now says so. - The allocate / check / fall-back-to-placeholder sequence was written twice, with the 1x1 placeholder block copy-pasted three times (twice in adjacent branches of one function). One `allocateLabelAtlas` + `uploadPlaceholderAtlas`; the live path's self-recursion goes with it. Drain the error flag before an allocation that is about to be checked. It is sticky and context-wide, and nothing else drains it, so the check after the point-buffer upload was reporting the first error raised anywhere in the context's lifetime and disabling the atlas over someone else's failure. The mock's `glErrors` queue could not express this — a fixture keyed on "the Nth getError() call" describes the mock, not the driver — so it is replaced by driver-shaped limits that raise from the offending call into a faithfully sticky flag. Refresh only the rows the drawn points occupy. Storage is sized from capacity, which overshoots after a 1.5x grow, and the refresh runs on every recolor: at 573k-then-700k that was ~5 MB of never-sampled texels per legend click. Also: fold the plan's backing array into the plan field, so the two cannot disagree; pass the plan to `bindPointDrawState` rather than four independently-defaulted uniforms; drop `reducedDetail`, which stored `stride < MAX_LABELS`; drop the inert atlas resets in `expandCapacity`, which `syncLabelAtlas` overwrites one line later; state the capacity clamp as one clamp-then-snap; stop re-exporting `MAX_LABELS` through `stage-point.ts`; and surface the gamma fallback's cause, which was collected into `context.detail` and then never read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZrGRWs8oSZRNtHhv46xxK
… session `render()` with an empty dataset — nothing loaded yet, or a viewport cull that matched nothing — reaches `syncLabelAtlas` with capacity 0. `planLabelAtlas` rejects that as un-plannable, so the null return latched `labelAtlasDisabled`, killing pie markers for the rest of the renderer's life and toasting the user that their device "cannot hold a colour table for 0 points". Reachable in the app: the zoom/pan controller's `renderWebGL` callback bypasses `_renderPlot`'s empty-data guard. Two more on the point-buffer OOM path, which is the one that cannot retry its way out: - It called `disableLabelAtlas` to free the atlas "so the retry has a chance", but that only drops the CPU-side texels — the GPU kept its storage, and since `buffersInitialized` stays false every later populate took the same early return, so the placeholder was never installed and the memory was never handed back. It now uploads the placeholder before returning, which is the only pass that can. - It reported two reasons: the real failure, then a fabricated `label-atlas-out-of-memory` naming an allocation that was never attempted. `disableLabelAtlas` now takes a nullable reason, and the copy no longer promises a retry "with a smaller footprint" that never happens — capacity is unchanged. Also: bound `drainGlErrors` at 32 iterations, since a context that keeps reporting the same code would freeze the tab in a `while`; stop the Playwright `texImage2D` patch from reading `format`/`type` as width/height on the 6-argument DOM-source overload, which recorded refusals the renderer never issued; and raise the simulated limit in `label-atlas-limit.spec.ts` from 1024 to 2047 — still below the narrowest atlas width, but clear of the canvas-sized gamma framebuffer texture the prototype patch also sees, which coupled the assertions to the viewport. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZrGRWs8oSZRNtHhv46xxK
`drainGlErrors`, added in the previous commit, cleared the flag before the
export's atlas upload — which was the point, but it also swallowed the
only signal the seven `bufferData` uploads above it had. Before the drain
a failed vertex-buffer allocation at least surfaced as a spurious atlas
degradation; after it, a 1M-point export that exhausts GPU memory renders
from storage that was never allocated and returns a blank or
half-populated PNG, saved and published with nothing logged anywhere.
The export now drains before those uploads and checks after them, and
throws. That matches how `renderToCanvas` already rejects an over-size
request: an export is a discrete user action with an error path, and a
figure that is quietly wrong is worse than one that failed. The throw is
inside the `try` whose `finally` releases the context, so nothing leaks.
Also from the same review pass:
- `planRendererCapacity`'s `maxCapacity` is required rather than
defaulting to Infinity. An unbounded plan is the bug the parameter
exists to prevent, so a caller that forgets it should not silently get
one. The seven original cases now pass `UNBOUNDED` explicitly, which is
what they were always testing; the "is inert when omitted" case is gone,
since both of its sides are now the same call.
- `StagePointStyleArrays` was documented as "the style channels a staged
point writes" but also carries `maxLabels`, which is an input.
The mock's `getExtension` returns a shaped `WEBGL_lose_context` instead of
`{}`: the export's `finally` calls `loseContext()` on it, and a bare
object throws a TypeError that replaces whatever the test was asserting.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AZrGRWs8oSZRNtHhv46xxK
`CI - Code Quality` failed once on this branch with
`this._webglRenderer?.setStyleSignature is not a function`, while every
assertion passed and the same commit went green on two sibling branches
minutes later.
`_scheduleNumericAnnotationRefresh` queues a requestAnimationFrame. All
three tests that call it assert only its synchronous effects — the
requestUpdate count, the absence of a dispatched event, the busy mirror —
and none wants the frame's body. But jsdom runs that body after the test
returns, against the deliberately minimal `_plotData` (`{ length: 3 }`,
no xs/ys) and the two-method renderer stub this file sets up. It throws
from inside the frame callback, where no test can catch it: an unhandled
error, which `vitest --run` treats as a failure even with every assertion
green. Whether the frame lands before teardown is a timing race, which is
why it surfaced intermittently rather than consistently.
Holding the callbacks unrun keeps the file to the synchronous,
never-connected contract its own header describes. Filling in the missing
stub methods instead would only move the throw further down the same path
— verified: it reaches `createScales` and fails on the fake `_plotData`.
Verified by forcing the race with a temporary 150ms teardown delay: six
unhandled errors before, zero after, and 8/8 clean full-suite runs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AZrGRWs8oSZRNtHhv46xxK
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.
Closes #457. Prerequisite for #456 — see Ordering below.
What was wrong
The pie-chart label atlas is sized from the renderer's point capacity, so its height silently made
gl.MAX_TEXTURE_SIZEthe point ceiling.gl.MAX_TEXTURE_SIZEwas queried nowhere in the tree, andgl.getError()was called nowhere in anysrc/.MAX_TEXTURE_SIZEA device reporting 2048 could not allocate the atlas for the shipped 573,649-protein Swiss-Prot bundle — it needs height 2241. The failure was invisible and permanent: an over-size
texImage2DraisesGL_INVALID_VALUE, which is not a JS exception, so nothing unwound and the texture was left unallocated — butlabelTextureInitialized = truewas set regardless, so every later update rantexSubImage2Dagainst storage that never existed. The user saw black pie marks and a clean console.The 4096 tier looked safe against
MAX_POINTS_DIRECT_RENDER = 1_000_000until the 1.5× geometric growth inplanRendererCapacityis accounted for: the constant cappedmaxPointsbut notcapacity, so loading 900k then 950k planned 1,350,144 → height 5274 → over the limit at a point count well under a million.Three more defects found while verifying, none of them in the report
stage-point.tswrotelabelCountsunclamped whilefillLabelColorTexelswrites at mostMAX_LABELStexels. The shader'scountwas likewise unclamped, so a point with more than 8 distinct colours read the next protein's texels for its surplus slices — an unrelated protein's colours, presented as this one's data.countnorsliceIndexwas clamped.atan(+0, x<0)is exactly+PI, so the normalised sweep reaches 1.0 on the middle pixel row of any odd-height sprite andsliceIndexreachedcount— the same overrun on well-formed points.POINT_FRAGMENT_SHADERdeclared onlyprecision highp float. ES 3.00 defaults fragmentinttomediump, whose guaranteed range is 16 bits, soflat in int v_pointIndexand the atlas index derived from it were undefined above 32,767 on any driver honouring the minimum — precisely the low-end hardware this change is about.What changed
eat-annotation-overlayrequires a two-label cell to render both hues in live and exported markers. Points are never dropped to fit a decoration.MAX_POINTS_DIRECT_RENDER, bounding the 1.5× overshoot. This alone rescues every device at 4096+: 1,000,192 points is height 3907. The clamp is floored at the snapped requirement, so it can never starve a larger load.gl.getError()once per capacity change, never per frame: after the allocatingbufferData(before any texture call, so a buffer failure is not misattributed) and after the atlastexImage2D. Initialised is recorded only onNO_ERROR; a 1×1 placeholder keeps the sampler complete otherwise.u_labelAtlasCapacityuniform gates the pie branch, so a point outside the atlas paints its dominant colour — never black.MAX_DIMENSIONthat ismin(8192, device limit)rather than a constant whose message called it "the browser limit".updateStyleswhile the reorder branch rewrites all of them, so a positions-only populate left the GPU holding the previous permutation. Masked today only because an unchanged depth vector re-sorts identically.Verification
A
getParameterstub alone proves nothing — it changes what the app believes while the real driver still accepts the allocation — and the reported failure is silent, so a console-error assertion passes on broken code too. The Playwright layer therefore simulates the driver's refusal as well, and asserts the renderer never issues an allocation the device would refuse.Verified red on the parent commit:
renderer issued texture allocations the device refuses: [[2048,31]], and no warning surfaced. Green after.pnpm test— 2,270 unit tests green (all 7 pre-existingcapacity-plannercases unchanged, as a regression guard)pnpm test:e2e— 121 passed, no regressionspnpm precommit+pnpm format:checkgreenload-large-bundlespec; the demo dataset's atlas is 2048×31, so the default suite can only reach the no-atlas pathOrdering
This must land before #456. Raising
MAX_POINTS_DIRECT_RENDERto 2,000,000 on today's code makes the atlas 7813 rows — fatal on every 4096 device, a tier that works today — and a 1.5M→2M sequence on an 8192 device plans capacity 2,250,240 → height 8790, failing on mainstream hardware. #456 first would convert a minority-device bug into a mainstream one.After both, the maximum drawable point count at full 8-slice fidelity becomes 2,097,152 on a 4096 device and 8,388,608 on an 8192 device; above ~4M the binding constraint is GPU memory, not texture size.
Not in this PR
Deferring allocation entirely for single-label annotations — the 42% GPU-residency win — is a follow-up. It only adds a second reason to enter the "no atlas" state this PR builds and tests on the error path, so it is small and cannot regress to corruption.
Please merge or rebase, not squash (repo-wide
allow_squash_merge: false).🤖 Generated with Claude Code
https://claude.ai/code/session_01USFuj7M2Ls1hpJXKaX2uJp