Include hasWarn and textures in the shader rebuild key#67
Merged
Conversation
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>
|
🚀 Preview deployed: https://fix-shader-rebuild-key.sinter.pages.dev (updates on every push to this PR) |
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
1 participant
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.
Fixes #55. Supersedes #59 (that PR was auto-closed when its base branch
fix-gpu-resource-leakwas deleted on merging #57; reopen wasn't possible, so re-filing againstmain). Same commit, rebased onto current main (which now includes #57).The rebuild decision compared only
glslandparamCount:but
rebuild()bakes two more inputs into the material —hasWarnselects the fragment source viabuildFrag(sdf.glsl, sdf.paramCount, sdf.hasWarn), and each texture becomes a uniform. An evaluation producing identical GLSL and param count but a differenthasWarntherefore never reached the GPU, leaving the thin-wall highlight showing the previous evaluation's state.Scope note (carried from #59)
The textures half is latent, not live.
codegen.tsdeclares/resets/returns atexturesarray but nothing ever pushes to it, sosdf.texturesis always[]at runtime and a texture-only change cannot currently produce a stale render. OnlyhasWarnis a live bug. The texture key (FNV-1a over content) is included anyway — it costs nothing while the array is empty and is already correct for when textures get wired up.Verification
tsc --noEmit— exit 0SdfMeshtests, this branch — 7/7 pass (2 fail on pre-fix base)🤖 Generated with Claude Code