-
-
Notifications
You must be signed in to change notification settings - Fork 45
impr: Better error when logging from vertex shader #1985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 12 commits
c82ea68
77c5132
8417be7
84c0399
9c108c0
ed3d0ae
10e27ab
675697a
e3e24a5
a1060a9
2f31c53
45bf547
3428f04
14e6b7a
1310713
c820cd0
2ea9223
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,12 +65,16 @@ import type { | |
| ItemLayer, | ||
| ItemStateStack, | ||
| ResolutionCtx, | ||
| ShaderStage, | ||
| TgpuShaderStage, | ||
| Wgsl, | ||
| } from './types.ts'; | ||
| import { CodegenState, isSelfResolvable, NormalState } from './types.ts'; | ||
| import type { WgslExtension } from './wgslExtensions.ts'; | ||
| import { hasTinyestMetadata } from './shared/meta.ts'; | ||
| import { isTgpuComputeFn } from './core/function/tgpuComputeFn.ts'; | ||
| import { isTgpuVertexFn } from './core/function/tgpuVertexFn.ts'; | ||
| import { isTgpuFragmentFn } from './core/function/tgpuFragmentFn.ts'; | ||
|
|
||
| /** | ||
| * Inserted into bind group entry definitions that belong | ||
|
|
@@ -334,6 +338,12 @@ export class ResolutionCtxImpl implements ResolutionCtx { | |
| readonly #namespace: NamespaceInternal; | ||
| readonly #shaderGenerator: ShaderGenerator; | ||
|
|
||
| /** | ||
| * 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; | ||
|
Comment on lines
+341
to
+346
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| private readonly _indentController = new IndentController(); | ||
| private readonly _itemStateStack = new ItemStateStackImpl(); | ||
| readonly #modeStack: ExecState[] = []; | ||
|
|
@@ -444,6 +454,11 @@ export class ResolutionCtxImpl implements ResolutionCtx { | |
| } | ||
|
|
||
| generateLog(op: string, args: Snippet[]): Snippet { | ||
| if (this._currentStage === 'vertex') { | ||
| throw new Error( | ||
| `'console.log' is not supported during vertex shader stage.`, | ||
| ); | ||
| } | ||
| return this.#logGenerator.generateLog(this, op, args); | ||
| } | ||
|
|
||
|
|
@@ -732,11 +747,24 @@ export class ResolutionCtxImpl implements ResolutionCtx { | |
| } | ||
|
|
||
| if (isMarkedInternal(item) || hasTinyestMetadata(item)) { | ||
| // if we're resolving an entrypoint function, we want to update this._currentStage | ||
| const stage = getItemStage(item); | ||
| const resolutionAction = () => { | ||
| if (stage) { | ||
| this._currentStage = stage; | ||
| } | ||
| const result = this._getOrInstantiate(item); | ||
| if (stage) { | ||
| this._currentStage = undefined; | ||
| } | ||
reczkok marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return result; | ||
| }; | ||
|
|
||
| // Top-level resolve | ||
| if (this._itemStateStack.itemDepth === 0) { | ||
| try { | ||
| this.pushMode(new CodegenState()); | ||
| const result = provideCtx(this, () => this._getOrInstantiate(item)); | ||
| const result = provideCtx(this, resolutionAction); | ||
| return snip( | ||
| `${[...this._declarations].join('\n\n')}${result.value}`, | ||
| Void, | ||
|
|
@@ -747,7 +775,7 @@ export class ResolutionCtxImpl implements ResolutionCtx { | |
| } | ||
| } | ||
|
|
||
| return this._getOrInstantiate(item); | ||
| return resolutionAction(); | ||
| } | ||
|
|
||
| // This is a value that comes from the outside, maybe we can coerce it | ||
|
|
@@ -972,3 +1000,15 @@ export function resolveFunctionHeader( | |
| } ` | ||
| : `(${argList}) `; | ||
| } | ||
|
|
||
| function getItemStage(item: unknown): ShaderStage { | ||
| if (isTgpuComputeFn(item)) { | ||
| return 'compute'; | ||
| } | ||
| if (isTgpuVertexFn(item)) { | ||
| return 'vertex'; | ||
| } | ||
| if (isTgpuFragmentFn(item)) { | ||
| return 'fragment'; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason, why we don't have to cast this to
TgpuComputeFnShell<T>like in fragment and vertex functions?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see, it is an issue regarding
createVertexFnnot being generic, as well as theOmitBuiltinsnot being consistent. I think it's the simplest solution to just cast.