TSL Bug: TempNode sibling-branch caching + NodeBuilder.addFlowCodeHierarchy() crash - #34339
Open
bhouston wants to merge 9 commits into
Open
TSL Bug: TempNode sibling-branch caching + NodeBuilder.addFlowCodeHierarchy() crash#34339bhouston wants to merge 9 commits into
bhouston wants to merge 9 commits into
Conversation
📦 Bundle sizeFull ESM build, minified and gzipped.
🌳 Bundle size after tree-shakingMinimal build including a renderer, camera, empty scene, and dependencies.
|
🖼️ E2E screenshot tests❌ 2 example(s) failed (full artifacts).
|
…y reads as zero A node promoted to a cached temp variable (anything deriving from TempNode) had its assignment statement flowed into whichever code-block was active the first time it was referenced. A later reference from a sibling conditional block (an If/Else that doesn't share an ancestor with the first reference's block) never actually executed that assignment, so the 'cached' variable silently read its default-initialized value (0) there -- no compile error, no runtime warning. NodeBuilder.addFlowCodeHierarchy() exists specifically to re-flow a cached node's assignment into a new block on repeat reference, and the generic (non-TempNode) caching path in Node.build() already calls it -- this adds the same call to TempNode.build()'s own specialized propertyName-based caching path. Adds a TSL unit test (TSLMatrixInverseRoundTrip.tests.js) whose harness writes each mat4 column from its own If branch, all referencing the same shared M*inverse(M) expression -- before this fix, only the diagonal entries came out correct and the rest silently read zero. Built on top of the tsl-unit-tests branch (three.js PR mrdoob#34331).
… fixed at the source The .toVar() call on value1/value2 in AssertWriteNode.setup() existed specifically to paper over the TempNode sibling-branch caching bug this branch fixes. Now that the bug is fixed at the source, the workaround is unneeded -- and keeping the harness routing every asserted value through an extra .toVar() would mask a regression of this exact fix instead of exercising it. Verified: the full addons suite (55 assertions) still passes clean with the workaround removed and only this branch's TempNode.js fix applied -- no other existing test relied on it.
…nditional temp node reference
addFlowCodeHierarchy() read flowCodeBlock off the node's builder data and
immediately called .get() on it. flowCodeBlock is created lazily, by
addLineFlowCodeBlock(), and only when the node's first build happened
inside some enclosing conditional block. If a cached temp node's first
build instead happened at the top level of a function body (no enclosing
block at all), flowCodeBlock was never initialized -- and if that same
node was later referenced again from inside an If(), TempNode's cached-value
fast path (fixed in the prior commit on this branch) would call
addFlowCodeHierarchy() and crash outright dereferencing undefined.
When flowCodeBlock is undefined, needsFlowCode is now set to false
directly instead of dereferencing it -- the mathematically correct answer,
not just a crash-avoidance guard: flowCodeBlock being undefined means the
node's assignment line was flowed unconditionally at the top level, which
is in scope from every block nested inside it, so no re-flow is ever
needed.
Depends on this branch's prior commit (the TempNode.js fix): without it,
TempNode's cached-value path never calls addFlowCodeHierarchy() at all, so
this crash is unreachable -- confirmed empirically, which is why this PR
is built on top of fix/tempnode-sibling-branch-caching rather than directly
on tsl-unit-tests.
Adds a TSL unit test (TSLNeutralToneMapping.tests.js) using the real,
unmodified neutralToneMapping() library function, which hits exactly this
pattern and reliably crashed on both [webgpu] and [webgl] before this fix.
Verified: reverting just this commit's NodeBuilder.js change (keeping the
TempNode.js fix) reproduces the exact crash ("Cannot read properties of
undefined (reading 'get')") and wrong values described above; restoring it
fixes both, full addons suite passes clean (61/61) either way this commit
is or isn't the one under test.
Built on top of fix/tempnode-sibling-branch-caching, which is itself built
on top of the tsl-unit-tests branch (three.js PR mrdoob#34331).
bhouston
force-pushed
the
fix/tempnode-sibling-branch-caching
branch
from
August 22, 2026 02:55
b165cbb to
b36edea
Compare
Collaborator
|
I felt confident to review the other TSL Unit Tests PR but this one changes |
Collaborator
|
I am still working on the PR review regarding unit tests; another unit test would also be needed to check for duplicate snippets, for example. These are not correctly verified solely by analyzing the results, that might be the next step. Everything that has been done by @bhouston already a huge step forward 🙏 The code bellow was a simple reproduction I created to test the latest commit using const { Fn, If, vec4, uv, mat4, mul, inverse } = await import( 'three/tsl' );
output = Fn( () => {
const m = mat4(
1, 0, 0, 0,
0.5, 1, 0, 0,
0, 0.25, 1, 0,
3, - 2, 1, 1
);
const product = mul( m, inverse( m ) );
const color = vec4( 0, 0, 0, 1 ).toVar( 'colorOutput' );
If( uv().x.lessThan( 0.33 ), () => {
color.assign( vec4( product.element( 0 ).xyz, 1.0 ) );
} ).ElseIf( uv().x.lessThan( 0.66 ), () => {
color.assign( vec4( product.element( 1 ).xyz, 1.0 ) );
} ).Else( () => {
color.assign( vec4( product.element( 2 ).xyz, 1.0 ) );
If( uv().x.lessThan( 0.33 ), () => {
color.assign( vec4( product.element( 0 ).xyz, 1.0 ) );
} ).ElseIf( uv().x.lessThan( 0.66 ), () => {
color.assign( vec4( product.element( 1 ).xyz, 1.0 ) );
} ).Else( () => {
color.assign( vec4( product.element( 2 ).xyz, 1.0 ) );
} );
} );
return color;
} )(); |
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.






This PR combines two fixes that turned out to be inseparable in practice --
see "Why these two are one PR" below for the CI evidence that made that
clear.
Fix 1: TempNode sibling-branch caching
A node promoted to a cached temp variable (anything deriving from TempNode)
had its assignment statement flowed into whichever code-block was active
the first time it was referenced. A later reference from a sibling
conditional block (an If/Else that doesn't share an ancestor with the
first reference's block) never actually executed that assignment, so the
'cached' variable silently read its default-initialized value (0) there --
no compile error, no runtime warning.
NodeBuilder.addFlowCodeHierarchy() exists specifically to re-flow a cached
node's assignment into a new block on repeat reference, and the generic
(non-TempNode) caching path in Node.build() already calls it -- this adds
the same call to TempNode.build()'s own specialized propertyName-based
caching path.
Adds a TSL unit test (TSLMatrixInverseRoundTrip.tests.js) whose harness
writes each mat4 column from its own If branch, all referencing the same
shared M*inverse(M) expression -- before this fix, only the diagonal
entries came out correct and the rest silently read zero.
Fix 2: NodeBuilder.addFlowCodeHierarchy() crash
addFlowCodeHierarchy() read flowCodeBlock off the node's builder data and
immediately called .get() on it. flowCodeBlock is created lazily, by
addLineFlowCodeBlock(), and only when the node's first build happened
inside some enclosing conditional block. If a cached temp node's first
build instead happened at the top level of a function body (no enclosing
block at all), flowCodeBlock was never initialized -- and if that same
node was later referenced again from inside an If(), Fix 1's own
TempNode.build() cached-value path would call addFlowCodeHierarchy() and
crash outright dereferencing undefined.
When flowCodeBlock is undefined, needsFlowCode is now set to false
directly instead of dereferencing it -- the mathematically correct answer,
not just a crash-avoidance guard: flowCodeBlock being undefined means the
node's assignment line was flowed unconditionally at the top level, which
is in scope from every block nested inside it, so no re-flow is ever
needed.
Adds a TSL unit test (TSLNeutralToneMapping.tests.js) using the real,
unmodified neutralToneMapping() library function, which hits exactly this
pattern and reliably crashed on both [webgpu] and [webgl] before this fix.
Why these two are one PR
These started as two separate PRs (this one, and #34336). CI on the
TempNode-only version of this PR caught something Fix 1's own unit test
couldn't: 48 of 110 e2e example screenshots crashed outright with
Cannot read properties of undefined (reading 'get')-- shadows,postprocessing, skinning, sky, reflections, and more
(failing run).
Fix 1 makes
TempNode.build()'s cached path calladdFlowCodeHierarchy()far more often in real-world TSL graphs, which immediately exposes Fix 2's
pre-existing bug well beyond the one synthetic test case. Shipping Fix 1
alone would have been a serious regression to the live example gallery.
Re-running the exact same commit with Fix 2 included:
all green.
Since Fix 1 is unsafe to merge without Fix 2, they're combined here as a
single PR rather than left as two independently-mergeable ones. (#34336,
which carried Fix 2 alone stacked on an earlier version of this branch, is
superseded by this PR and closed.)
Also included: removing
gpu-test-utils.js's.toVar()workaroundThe shared test harness (
AssertWriteNode.setup()) previously routed everyasserted value through
.toVar()specifically to sidestep the Fix 1 bug,before any per-column
Ifbranching. That workaround is why an earlierversion of this PR's regression test passed even with the bug still
present -- verified directly: with the workaround in place, reverting Fix 1
left the test green; with the workaround removed, reverting Fix 1 reliably
reproduces the original failure (only the diagonal entries wrong) on both
[webgpu]and[webgl], and restoring it passes again. Removed theworkaround now that the underlying bug is fixed at the source, so the
harness exercises the real fix instead of masking a regression of it.
Confirmed this doesn't destabilize anything else: the full addons suite
(61 assertions, all unrelated tests included) passes clean with the
workaround gone and both fixes applied.
Built on top of the tsl-unit-tests branch (three.js PR #34331).
CC: @sunag