impr: Better error when logging from vertex shader #1985
Conversation
|
pkg.pr.new packages benchmark commit |
There was a problem hiding this comment.
Pull request overview
This PR improves error handling when console.log is used in vertex shaders by detecting the shader stage during resolution and throwing a clear error instead of allowing invalid code generation.
Key changes:
- Replaces the
isEntryboolean flag with anentryPointstring field to distinguish between shader stages ('vertex', 'fragment', 'compute') - Tracks the current shader stage during resolution and validates
console.logusage - Removes the deprecated
doesmethod from entry point function shells
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/src/types.ts | Adds ShaderStage type definition for tracking shader stages |
| packages/typegpu/src/resolutionCtx.ts | Implements stage tracking logic and validates console.log usage based on current stage |
| packages/typegpu/src/core/function/tgpuVertexFn.ts | Replaces isEntry with entryPoint: 'vertex' and adds type guard |
| packages/typegpu/src/core/function/tgpuFragmentFn.ts | Replaces isEntry with entryPoint: 'fragment' and adds type guard |
| packages/typegpu/src/core/function/tgpuFn.ts | Removes unused isEntry: false from regular functions |
| packages/typegpu/src/core/function/tgpuComputeFn.ts | Replaces isEntry with entryPoint: 'compute' and adds type guard |
| packages/typegpu/tests/tgsl/consoleLog.test.ts | Adds test coverage for vertex shader console.log error handling |
| apps/typegpu-docs/src/examples/rendering/3d-fish/render.ts | Demonstrates the error by adding a console.log in a vertex shader |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…-logging-from-vertex
aleksanderkatan
requested review from
cieplypolar,
iwoplaza,
lursz and
reczkok
| return Object.assign(Object.assign(call, shell), { | ||
| does: call, | ||
| }) as TgpuComputeFnShell<ComputeIn>; | ||
| return Object.assign(call, shell); |
There was a problem hiding this comment.
Is there a reason, why we don't have to cast this to TgpuComputeFnShell<T> like in fragment and vertex functions?
There was a problem hiding this comment.
We don't have to. In other cases this is necessary.
Damn my reading comprehension sucks today.
I don't know, I didn't investigate it.
There was a problem hiding this comment.
From what I see, it is an issue regarding createVertexFn not being generic, as well as the OmitBuiltins not being consistent. I think it's the simplest solution to just cast.
| /** | ||
| * Holds info about the currently resolved shader stage (if there is any). | ||
| * Note that if a function is used both in vertex and fragment stage, | ||
| * then it will only go through the process during the vertex stage. | ||
| */ | ||
| private _currentStage: ShaderStage; |
There was a problem hiding this comment.
This will only allow shader code to ask which stage they're being resolved in first. If we instead create an internal slot that provides the value of the stage currently being resolved, then branches that actually use this value will be able to have branching logic based on which stage they're in.
I can definitely see someone wanting to log a value inside of a utility function that happens to execute in a compute shader and a fragment shader, and not being able to do so because of this error. If we instead emit a no-op and warn the user that a console log has been skipped because it's in a vertex shader, then it would still allow both shader stages to compile, and the other console log to run.
Before:
After:
Changes:
isEntryfrom all functions shells (it was unused),doesfrom entrypoints,entryPointto make entry functions differentiable,Currently: