Skip to content

Include hasWarn and textures in the shader rebuild key#59

Closed
kmatzen wants to merge 1 commit into
fix-gpu-resource-leakfrom
fix-shader-rebuild-key
Closed

Include hasWarn and textures in the shader rebuild key#59
kmatzen wants to merge 1 commit into
fix-gpu-resource-leakfrom
fix-shader-rebuild-key

Conversation

@kmatzen

@kmatzen kmatzen commented Jul 18, 2026

Copy link
Copy Markdown
Owner

Fixes #55. Stacked on #57 — base is fix-gpu-resource-leak, so this shows only its own diff and will retarget to main when #57 merges.

The rebuild decision compared only glsl and paramCount:

if (sdf.glsl !== this.lastGlsl || sdf.paramCount !== this.lastParamCount) {

but rebuild() bakes two more inputs into the material — hasWarn selects the fragment source via buildFrag(sdf.glsl, sdf.paramCount, sdf.hasWarn), and each texture becomes a uniform. An evaluation producing identical GLSL and param count but a different hasWarn therefore never reached the GPU, leaving the thin-wall highlight showing the previous evaluation's state.

Correction to the issue as I filed it

The textures half is latent, not live. codegen.ts declares textures (:6), resets it (:324), and returns a copy (:352), but nothing ever pushes to it:

$ grep -nE 'textures\s*(=|\.push|\.splice|\[)' src/worker/sdf/codegen.ts
324:  textures = [];

Nothing else in src/worker/sdf/ mentions textures. So sdf.textures is always [] at runtime and a texture-only change cannot currently produce a stale render. I over-claimed a user-visible symptom that can't presently occur, and have said so on the issue rather than quietly narrowing the fix.

Only hasWarn is a live bug. The texture key is included anyway — it costs nothing while the array is empty, and it means the key is already correct whenever textures get wired up, rather than being a trap waiting for that change. It hashes content (FNV-1a) rather than just dimensions, so a content edit at unchanged width/height still invalidates.

Verification

check result
tsc --noEmit exit 0
SdfMesh tests, this branch 7/7 pass
new tests, base branch 2 fail, 1 pass

The test that passes on both asserts an identical display does not rebuild. That guards the opposite direction: each rebuild recompiles a shader and (after #57) disposes the previous one, so an over-eager key would trade a stale-render bug for a churn bug.

🤖 Generated with Claude Code

Fixes #55.

The rebuild decision compared only glsl and paramCount, but rebuild() bakes
two more inputs into the material: hasWarn selects the fragment source via
buildFrag, and each texture becomes a uniform. An evaluation producing
identical GLSL and param count but a different hasWarn therefore never
reached the GPU, leaving the thin-wall highlight showing the previous
evaluation's state.

Correction to the issue as I filed it: the textures half is latent, not
live. codegen declares `textures`, resets it, and returns it, but nothing
ever pushes to it, and nothing else in src/worker/sdf references textures --
so sdf.textures is always [] at runtime and cannot currently go stale. Only
hasWarn is a live bug. The texture key is included anyway: it costs nothing
while the array is empty, and it means the key is already correct when
textures get wired up rather than being a trap waiting for that change.

The texture key hashes content (FNV-1a) rather than just dimensions, so a
content edit at unchanged width/height still invalidates.

Tests fail 2/3 against the base branch. The third asserts that an identical
display does NOT rebuild, which both satisfy -- it guards against
over-invalidating, since each rebuild recompiles a shader and disposes the
previous one.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kmatzen
kmatzen force-pushed the fix-gpu-resource-leak branch from 0501983 to bef0825 Compare July 25, 2026 16:38
@kmatzen
kmatzen force-pushed the fix-shader-rebuild-key branch from c6ebbdd to 405f63f Compare July 25, 2026 16:38
@kmatzen
kmatzen deleted the branch fix-gpu-resource-leak July 25, 2026 16:48
@kmatzen kmatzen closed this Jul 25, 2026
@kmatzen

kmatzen commented Jul 25, 2026

Copy link
Copy Markdown
Owner Author

Can't reopen this — GitHub refuses (HTTP 422) because the head branch was force-pushed (rebased onto main) after the PR auto-closed. Re-filed as #67 with the same commit and body. Closing the loop here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant