fix(playground): report GLSL compiler errors#72
Conversation
devallibus
left a comment
There was a problem hiding this comment.
LGTM. The switch from gl.CURRENT_PROGRAM to renderer.debug.onShaderError is the right call — hooks into Three.js's own error reporting instead of relying on brittle WebGL state queries. The diagnostic extraction helper is clean, well-tested, and properly handles edge cases (dedup, fallback chain, both throw and non-throw paths).
Two minor nits left as inline comments (redundant | null type, optional missing test for link-only fallback). Neither blocks merge.
| vertexShader?: unknown | null | ||
| fragmentShader?: unknown | null |
There was a problem hiding this comment.
unknown | null is redundant since unknown already includes null. Stylistically fine — communicates intent — but worth knowing it's a no-op at the type level.
| vertexShader?: unknown | null | |
| fragmentShader?: unknown | null | |
| vertexShader?: unknown | |
| fragmentShader?: unknown |
| return [{ kind: 'glsl-compile', message: 'GLSL shader compilation failed.' }] | ||
| } | ||
|
|
||
| return [{ kind: 'glsl-link', message: 'GLSL program linking failed.' }] |
There was a problem hiding this comment.
Nice fallback chain. One path that isn't covered by tests: if onShaderError fires but neither shader reports a compile failure and getProgramInfoLog returns empty, this line is reached. Easy to add:
runTest('collectShaderDiagnostics falls back to a generic link message', () => {
const diagnostics = collectShaderDiagnostics({
gl: mockGl,
program: { log: null },
vertexShader: { ok: true, log: null },
fragmentShader: { ok: true, log: null },
})
assert.deepEqual(diagnostics, [
{ kind: 'glsl-link', message: 'GLSL program linking failed.' },
])
})nit / optional
| const shaderDiagnostics: ReturnType<typeof collectShaderDiagnostics> = [] | ||
| const previousShaderError = renderer.debug.onShaderError | ||
|
|
||
| renderer.debug.onShaderError = (gl, program, glVertexShader, glFragmentShader) => { | ||
| shaderDiagnostics.splice( | ||
| 0, | ||
| shaderDiagnostics.length, | ||
| ...collectShaderDiagnostics({ | ||
| gl, | ||
| program, | ||
| vertexShader: glVertexShader, | ||
| fragmentShader: glFragmentShader, | ||
| }), | ||
| ) | ||
| } | ||
|
|
||
| if (errors.length > 0) { | ||
| props.onError(errors) | ||
| return | ||
| } | ||
| try { | ||
| renderer.render(scene, camera) | ||
| } catch (renderError) { | ||
| const fallbackMessage = renderError instanceof Error | ||
| ? renderError.message | ||
| : 'Shader compilation failed' | ||
|
|
||
| props.onError( | ||
| shaderDiagnostics.length > 0 ? diagnosticsToMessages(shaderDiagnostics) : [fallbackMessage], | ||
| ) | ||
| return | ||
| } finally { | ||
| renderer.debug.onShaderError = previousShaderError | ||
| } | ||
|
|
||
| if (shaderDiagnostics.length > 0) { | ||
| props.onError(diagnosticsToMessages(shaderDiagnostics)) | ||
| return | ||
| } |
There was a problem hiding this comment.
Good use of renderer.debug.onShaderError — this is the official Three.js hook and much more reliable than querying gl.CURRENT_PROGRAM after the fact. The try/catch/finally ensures the hook is always restored, and the post-render shaderDiagnostics.length > 0 check correctly handles the case where Three.js calls onShaderError without throwing (it may continue with a fallback shader). Well done.
Summary
gl.CURRENT_PROGRAMcheck in the playground with Three'srenderer.debug.onShaderErrorhooktest:webVerification
bun run testbun run build:webcompilationErrorsCloses #68.