feat: Use @typegpu/gl as a fallback when @typegpu/three is made to generate GLSL - #2794
feat: Use @typegpu/gl as a fallback when @typegpu/three is made to generate GLSL#2794iwoplaza wants to merge 1 commit into
Conversation
|
pkg.pr.new packages benchmark commit |
Resolution Time Benchmark---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Random Branching (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.64, 1.32, 2.65, 4.39, 4.92, 8.43, 13.80, 18.56]
line [0.68, 1.39, 2.82, 4.29, 5.12, 8.72, 16.14, 16.09]
line [0.65, 1.35, 2.85, 4.72, 5.36, 8.69, 16.67, 18.75]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Linear Recursion (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.26, 0.34, 0.45, 0.51, 0.70, 0.80, 0.93, 1.01]
line [0.22, 0.37, 0.50, 0.58, 0.80, 0.88, 1.04, 1.09]
line [0.22, 0.37, 0.54, 0.64, 0.87, 0.79, 1.00, 1.11]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Full Tree (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.65, 1.41, 2.54, 4.47, 8.47, 17.90, 36.50, 76.07]
line [0.58, 1.43, 2.96, 5.15, 8.75, 17.88, 37.79, 75.85]
line [0.69, 1.45, 2.73, 4.41, 8.73, 18.58, 38.52, 78.52]
|
Bundle size comparison (
|
| 🟢 Decreased (max -0.25%) | ➖ Unchanged | 🔴 Increased (max 2.79%) | ❔ Unknown |
|---|---|---|---|
| 6 | 129 | 187 | 1 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
| Test | tsdown |
|---|---|
| std_isBeingTranspiled.ts | 15.71 kB ( |
| std_getTargetShaderLanguage.ts | 15.77 kB ( |
| STATIC_std.ts | 110.08 kB ( |
| std_getShaderStage.ts | 15.76 kB |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 268.26 kB ( |
| tgpu_initFromDevice.ts | 267.73 kB ( |
| tgpu_resolve.ts | 168.97 kB ( |
| tgpu_resolveWithContext.ts | 168.90 kB ( |
| tgpu_bindGroupLayout.ts | 73.91 kB ( |
| tgpu_mutableAccessor.ts | 68.64 kB ( |
| tgpu_accessor.ts | 68.63 kB ( |
| tgpu_privateVar.ts | 67.33 kB ( |
| tgpu_workgroupVar.ts | 67.32 kB ( |
| tgpu_const.ts | 66.74 kB ( |
| tgpu_lazy.ts | 66.54 kB ( |
| tgpu_fragmentFn.ts | 38.92 kB ( |
| tgpu_fn.ts | 38.86 kB ( |
| tgpu_vertexFn.ts | 38.73 kB ( |
| tgpu_computeFn.ts | 38.44 kB ( |
| tgpu_vertexLayout.ts | 27.57 kB ( |
| tgpu_comptime.ts | 15.18 kB ( |
| tgpu_unroll.ts | 1.75 kB ( |
| tgpu_slot.ts | 1.70 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
There was a problem hiding this comment.
Pull request overview
This PR updates @typegpu/three to support Three.js’s WebGL backend path by switching TypeGPU shader generation to GLSL via @typegpu/gl when WebGL is detected.
Changes:
- Add
@typegpu/glas a peer dependency and wire it into the workspace lockfile. - Detect WebGL backend in the node builder and apply
glOptions({ shaderStage: 'none' })totgpu.resolve(...)calls. - Adjust function-start detection to handle GLSL-style function declarations.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds @typegpu/gl to the workspace install graph for packages/typegpu-three. |
| packages/typegpu-three/src/typegpu-node.ts | Adds WebGL detection + GLSL generation options via glOptions, and updates function-start detection. |
| packages/typegpu-three/package.json | Declares @typegpu/gl as a peer dependency for @typegpu/three. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
The WebGL fallback path now generates GLSL, but forceExplicitVoidReturn is WGSL-only: it appends -> void after the first ), corrupting every GLSL function signature. This prevents the fallback from working. See the inline comment on packages/typegpu-three/src/typegpu-node.ts:193.
Reviewed changes
- Declared
@typegpu/glas a peer dependency and wiredglOptions({ shaderStage: 'none' })into the threetgpu.resolvecalls when the Three.js builder targets WebGL. - Added
isWebGL()backend detection andfindFunctionStart()to handle both WGSL (fn name) and GLSL (type name() function declaration syntax. - Refactored the dependency-assignment resolution in
generate()into a localcodevariable. - Updated
pnpm-lock.yamlto link the new peer dependency.
ℹ️ Docs will need updating once WebGL works
The @typegpu/three docs currently include a caution block stating that the WebGL fallback does not work. Once this feature branch makes it work, that block and the install instructions should be updated.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
da61438 to
a010b4f
Compare
|
Follow-up to the review above with line-specific notes:
|
a010b4f to
2cc2d21
Compare
There was a problem hiding this comment.
Important
The @typegpu/gl delta improves GLSL emission, but the WebGL fallback path is still blocked because forceExplicitVoidReturn corrupts GLSL signatures.
Reviewed changes
Reviewed the delta that removed the WORKAROUND_wgslToGlslFixups post-processor from @typegpu/gl and replaced it with native GLSL generation.
- Removed
WORKAROUND_wgslToGlslFixupsfrompackages/typegpu-gl/src/tgpuRootWebGL.tsand its export inpackages/typegpu-gl/src/index.ts. - Added native array support in
packages/typegpu-gl/src/glslGenerator.ts:resolveArraySizeSuffix, array-awaredeclareGlobalConst,declareGlobalVar,typeAnnotation,typeInstantiation, and_emitVarDeclnow emit GLSLtype name[N]declarations andtype[N](...)constructors. - Added a
numericLiteraloverride that emits GLSL literals without WGSLf/hsuffixes, keepsuforu32, and chooses decimal or exponential form for floats. - Updated
@typegpu/gltest snapshots; all 35@typegpu/gltests pass.
⚠️ WebGL fallback remains broken for GLSL signatures
packages/typegpu-three/src/typegpu-node.ts:193 still applies forceExplicitVoidReturn(fnCode) unconditionally. The helper inserts -> void after the first ), which corrupts GLSL function signatures such as float foo() into float foo()-> void. This prevents the WebGL fallback from working even though @typegpu/gl now emits valid GLSL.
Technical details
# forceExplicitVoidReturn corrupts GLSL signatures
## Affected sites
- packages/typegpu-three/src/typegpu-node.ts:193 — `forceExplicitVoidReturn(fnCode)` is applied to GLSL code
## Required outcome
- GLSL function code must not be passed through the WGSL-specific `forceExplicitVoidReturn` helper.
## Suggested approach
Skip `forceExplicitVoidReturn` when `webgl` is true, or detect whether the resolved code already has a return type in a language-agnostic way.Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
2cc2d21 to
7b889ba
Compare
7b889ba to
961d532
Compare
961d532 to
6b3dea9
Compare
6b3dea9 to
24d8fe3
Compare
24d8fe3 to
75504e6
Compare
There was a problem hiding this comment.
Caution
The upstream @typegpu/gl fixes in this rebase are correct, but the PR's own @typegpu/three integration still applies a WGSL-only helper to GLSL code. The WebGL fallback therefore remains broken.
Reviewed changes
Reviewed the delta from the previous pullfrog review head (6b3dea9) to the current head (75504e6). The @typegpu/three changes are byte-for-byte identical to the previous iteration; the new commits come from the upstream feat/make-triangle-work-with-gl-fallback feature branch.
- Eagerly reserved
gl_Positionandsampleinpackages/typegpu-gl/src/glslGenerator.tson the firstfunctionDefinitioncall, so theshaderStage: 'none'path used by@typegpu/threecannot accidentally use these GLSL keywords as identifiers. - Updated
@typegpu/glinline snapshots to match improved numeric-literal and zero-constructor emission (1i→1,0i→0,1.5f→1.5,vec3()→vec3(0)). - The
@typegpu/threefiles are unchanged from the previous iteration.
🚨 WebGL fallback still blocked by forceExplicitVoidReturn
packages/typegpu-three/src/typegpu-node.ts:193 still passes the resolved function code through forceExplicitVoidReturn. That helper assumes WGSL syntax: it looks for -> to detect an explicit return type and otherwise appends -> void after the first ). GLSL function declarations do not contain ->, so a signature such as float foo() is corrupted into float foo()-> void, which is invalid GLSL. This was raised in the previous pullfrog review and has not been addressed in this rebase.
Technical details
# forceExplicitVoidReturn corrupts GLSL signatures
## Affected sites
- packages/typegpu-three/src/typegpu-node.ts:193 — `forceExplicitVoidReturn(fnCode)` is applied to GLSL code
## Required outcome
- GLSL function code must not be passed through the WGSL-specific `forceExplicitVoidReturn` helper.
## Suggested approach
Skip `forceExplicitVoidReturn` when `webgl` is true, or detect whether the resolved code already has a return type in a language-agnostic way.Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
b4129ca to
fb2b7ae
Compare
There was a problem hiding this comment.
✅ No new issues found.
The prior
forceExplicitVoidReturnconcern is fixed, and the new GLSL implicit-pointer support is well covered by tests.
Reviewed changes
Reviewed the delta since the prior pullfrog review (75504e6). The latest commits add native GLSL handling for implicit const pointers and fix the WGSL-only return-type helper that was blocking the WebGL fallback.
- Fixed
forceExplicitVoidReturncorrupting GLSL signatures.packages/typegpu-three/src/typegpu-node.tsnow skips the WGSL-specific helper when the Three.js builder targets WebGL, so GLSL functions no longer get-> voidappended. - Refactored implicit-pointer
constemission.packages/typegpu/src/tgsl/wgslGenerator.tsextracted the alias logic into a protected_aliasConstStatementmethod so language-specific generators can override it. - Added GLSL implicit-pointer support.
packages/typegpu-gl/src/glslGenerator.tsoverrides_aliasConstStatement: immutable memory is copied, mutable memory is aliased by inlining the original expression, and runtime index expressions are hoisted intoletvariables to guarantee single evaluation. - Added test coverage.
packages/typegpu-gl/tests/implicitPointer.test.tscovers immutable copies, mutable aliases, nested member/index accesses, aliases of aliases, and local-variable aliases.
All @typegpu/gl and packages/typegpu tests pass.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
5e1ad8a to
c98cc33
Compare
c98cc33 to
c4ac089
Compare
There was a problem hiding this comment.
Important
The new isWebGL helper dereferences builder.renderer without a null check, which crashes with TypeError: Cannot read properties of undefined (reading 'backend') whenever a TgpuFnNode is built against a renderer-less NodeBuilder. This breaks the base branch's own @typegpu/three test suite (packages/typegpu-three/tests/typegpu-node.test.ts, 3 failures) — those tests construct new WGSLNodeBuilder() without a renderer and pass on the base branch, so this is a regression introduced by this PR.
Reviewed changes
Reviewed the PR head c98cc339 against base ce1619a9. The branch was force-pushed/rebuilt since the prior pullfrog review (fb2b7ae9, unreachable), so this compares the current iteration against the advanced base.
- Adapted the WebGL fallback to the new base APIs.
typegpu-node.tsnow uses a freshglOptions()(noshaderStagearg) in#getNodeFunction,#analyzeFunctionandgenerate, matchescrossShaderStagestate semantics, and resolves the function declaration fromresolveWithContext'sdeclarationsviaexistingDeclarations. - Added
isWebGLbackend detection and preserved thewebgl ? fnDeclaration : forceExplicitVoidReturn(fnDeclaration)guard. - GLSL implicit-pointer support (
_aliasConstStatementoverride, immutable-copy / mutable-alias semantics,#hoistIndexAccesses) carried over to the new base;wgslGenerator.tsretainsthe extracted_aliasConstStatementhook. @typegpu/glpeer dependency wiring and lockfile unchanged in substance.
Verified: packages/typegpu-gl suite fully passes (46 passed, 2 skipped, incl. implicitPointer.test.ts 6/6). packages/typegpu-three fails only on the isWebGL null-renderer path.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
The renderer-less crash fix is a test-only patch. isWebGL still dereferences builder.renderer.backend unguarded, and it now trips the production fromTSL path: fromTSL(toTSL(...), type) resolves through the module-level renderer-less sharedBuilder = new WGSLNodeBuilder(), so isWebGL throws, the try/catch swallows it, and TypeGPU silently skips its TSL-vs-TypeGPU type comparison — while emitting two fromTSL: failed to infer node type via getNodeType; skipping type comparison. warnings in the PR's own test run (base emits zero). Guarding the access (e.g. !!builder.renderer &&) removes both warnings and also fixes the 3 test failures without needing the renderer mock.
Reviewed changes
Reviewed the single new commit c4ac0890 (head changed from c98cc339). The PR was also restructured: the implicit-pointer GLSL work now lives on the base branch feat/glsl-implicit-pointer-definitions, so this PR's own surface shrank to the @typegpu/three integration plus the test fix.
- Added renderer/backend mocks to
typegpu-node.test.tsand assignedbuilder.rendererinbuilderFor, fixing the renderer-lessWGSLNodeBuildercrash that previously broke the 3 base tests. isWebGL+glOptions()wiring unchanged from the prior iteration;forceExplicitVoidReturnskip for WebGL preserved.@typegpu/glpeer dependency wiring unchanged in substance.
Verified: packages/typegpu-gl suite still fully green (46 passed, 2 skipped). packages/typegpu-three tests now pass, but emit two fromTSL: failed to infer node type warnings that a guarded isWebGL eliminates.
Technical details
# isWebGL unguarded deref degrades fromTSL type comparison
## Affected sites
- packages/typegpu-three/src/typegpu-node.ts:113-115 — `isWebGL` reads `builder.renderer.backend`
- packages/typegpu-three/src/typegpu-node.ts:394-395 — `fromTSL` caches a renderer-less `new WGSLNodeBuilder()` as `sharedBuilder`, then calls `node.getNodeType(sharedBuilder)` in try/catch (lines 398-403)
## Required outcome
- `isWebGL` must not throw on builders without a `renderer` (both the tests' bare builders and `fromTSL`'s `sharedBuilder`).
## Suggested approach
- Guard the access: `return !!builder.renderer && 'isWebGLBackend' in builder.renderer.backend && !!builder.renderer.backend.isWebGLBackend;`
- Verified: with that one-line guard, `packages/typegpu-three/tests/typegpu-node.test.ts` passes with zero `fromTSL` warnings.
## Open questions for the human
- Keep the renderer mock in the test (it makes the setup more faithful) while adding the guard, or replace the mock with the guard alone?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
c4ac089 to
aee322e
Compare
aee322e to
6e3e8c2
Compare
6e3e8c2 to
cc1f6fb
Compare
2e4aefa to
0bf429b
Compare
0bf429b to
5fa70c0
Compare
5fa70c0 to
96e6104
Compare
There was a problem hiding this comment.
Important
A new test in this delta fails: WebGL storage arrays > lowers array reads through a typed TSL helper and writes to the current element. The generated compute code is direct GLSL subscripting (void item() { vec3 value = storageValue[2]; storageValue[2] = value; }), which never contains the asserted typegpuReadStorage marker. That identifier does not exist anywhere in the codebase — it's referenced only by the test itself — so the assertion cannot pass as written. This blocks the merge until either the typed-helper lowering (or whatever the test is meant to pin) is implemented, or the expectation is corrected to match actual codegen.
Reviewed changes
Reviewed the delta since the prior pullfrog review (c4ac0890):
- Guarded
isWebGLagainst renderer-less builders.packages/typegpu-three/src/typegpu-node.ts:128now readsbuilder.renderer?.backendand returnsfalsewhen there is no renderer, fixing the open unguarded-deref concern. The newuses the active WebGL builder to infer nested toTSL return typestest pins the fix (passes with zeroconsole.warncalls). - Added a WebGL storage-array test that builds a fake storage node through setup/analyze/generate and asserts GLSL codegen — this test currently fails.
- Added
SetupStageData/getSetupStageDataplumbing to the builder data so WebGL builders can be exercised through thesetupbuild stage.
⚠️ New WebGL storage-array test fails
packages/typegpu-three/tests/typegpu-node.test.ts:169 asserts the generated compute code contains typegpuReadStorage, but the generator emits plain array indexing instead. Since typegpuReadStorage is defined nowhere in the repo, this test is red on a clean run (verified locally: 4/5 pass, this one fails). Either the storage-array lowering through the typed helper is intended-but-not-implemented (then it needs implementing), or the assertion is stale and should match the real storageValue[2] codegen.
Technical details
# New WebGL storage-array test fails
## Affected sites
- packages/typegpu-three/tests/typegpu-node.test.ts:169 — expects `typegpuReadStorage` in `builder.getCodes('compute')`
- packages/typegpu-three/tests/typegpu-node.test.ts:153-171 — the failing test itself
## Required outcome
- The test must pass on a clean run. Either implement the typed-helper lowering it describes (or confirm it isn't needed and drop the marker assertion), or fix the expectation to the actual generated code.
## Suggested approach
- Run `pnpm vitest run --project=!browser packages/typegpu-three/tests/typegpu-node.test.ts` to reproduce the failure, then decide whether the feature or the assertion is in error.ℹ️ Nitpicks
packages/typegpu-three/src/typegpu-node.ts:84-91—getSetupStageData/SetupStageData/setupStageDataMapare added but never called anywhere (onlygetGenerateStageData/getAnalyzeStageDatahave call sites). If thesetupbuild stage doesn't need its own stage data yet, this is dead code added by the delta.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| builder.setBuildStage('generate'); | ||
|
|
||
| expect(() => fn.build(builder)).not.toThrow(); | ||
| expect(builder.getCodes('compute')).toContain('typegpuReadStorage'); |
There was a problem hiding this comment.
This test fails on a clean run: getCodes('compute') returns direct GLSL indexing (vec3 value = storageValue[2]; storageValue[2] = value;) and never contains typegpuReadStorage, which is defined nowhere in the repo. Either the typed-helper lowering this pins is missing (implement it) or the expectation is stale (match the actual codegen). Reproduce with pnpm vitest run --project=!browser packages/typegpu-three/tests/typegpu-node.test.ts.
96e6104 to
f33e6db
Compare
f33e6db to
68c6f79
Compare
68c6f79 to
f1a4dba
Compare
f1a4dba to
7a52920
Compare
|
|
||
| function isWebGL(builder: THREE.NodeBuilder): boolean { | ||
| const backend = builder.renderer?.backend; | ||
| return !!backend && 'isWebGLBackend' in backend && !!backend.isWebGLBackend; |
There was a problem hiding this comment.
| return !!backend && 'isWebGLBackend' in backend && !!backend.isWebGLBackend; | |
| return !!backend && (backend as unknown as { isWebGLBackend}).isWebGLBackend === true; |
No description provided.