feat(@typepgu/gl): Generate GLSL function signature#2360
Open
iwoplaza wants to merge 1 commit intorefactor/naming-cleanupfrom
Open
feat(@typepgu/gl): Generate GLSL function signature#2360iwoplaza wants to merge 1 commit intorefactor/naming-cleanupfrom
iwoplaza wants to merge 1 commit intorefactor/naming-cleanupfrom
Conversation
|
pkg.pr.new packages benchmark commit |
📊 Bundle Size Comparison
👀 Notable resultsStatic test results:No major changes. Dynamic test results:
📋 All resultsClick to reveal the results table (349 entries).
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu. |
iwoplaza
force-pushed
the
feat/generate-glsl-signature
branch
5 times, most recently
from
fcc485d to
03aadb1
Compare
iwoplaza
force-pushed
the
feat/generate-glsl-signature
branch
3 times, most recently
from
584c220 to
45077a0
Compare
iwoplaza
force-pushed
the
feat/generate-glsl-signature
branch
from
45077a0 to
9a20acd
Compare
iwoplaza
requested review from
aleksanderkatan,
cieplypolar,
Copilot and
reczkok
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds GLSL codegen capabilities on top of the existing TGSL/WGSL pipeline by moving function “header” generation into the generator layer and extending the GL generator to emit GLSL function signatures, structs, and basic entry-point out handling.
Changes:
- Renamed/expanded function-resolution API (
fnToWgsl→resolveFunction) to include function name and workgroup size. - Updated WGSL generator to emit full function declarations (attributes +
fn name(...)) and refactored return/var-decl emission. - Extended GLSL generator to generate GLSL function signatures, struct declarations, and entry-point output assignment/declarations; updated tests + added
GLOptions()helper export.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/src/types.ts | Renames options/interface and exposes function name/workgroup size to resolution. |
| packages/typegpu/src/tgsl/wgslGenerator.ts | Emits full WGSL function declarations and refactors return + var-declaration helpers. |
| packages/typegpu/src/tgsl/shaderGenerator_members.ts | Extends generator option types (name, workgroupSize) and re-exports helpers. |
| packages/typegpu/src/resolutionCtx.ts | Renames ctx API to resolveFunction and forwards new options to generator. |
| packages/typegpu/src/core/function/fnCore.ts | Switches call site to resolveFunction and stops double-prepending function headers. |
| packages/typegpu-gl/tests/glslGenerator.test.ts | Updates tests to use GLOptions() and adds coverage for function signatures/structs/entry output. |
| packages/typegpu-gl/src/index.ts | Exports GLOptions. |
| packages/typegpu-gl/src/glslGenerator.ts | Implements GLSL function signature + struct generation and entry-point return rewriting. |
| packages/typegpu-gl/src/glOptions.ts | Adds convenience helper to configure the GLSL generator. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+865
to
+872
| let attributes = ''; | ||
| if (options.functionType === 'compute') { | ||
| attributes = `@compute @workgroup_size(${options.workgroupSize?.join(', ')}) `; | ||
| } else if (options.functionType === 'vertex') { | ||
| attributes = `@vertex `; | ||
| } else if (options.functionType === 'fragment') { | ||
| attributes = `@fragment `; | ||
| } |
There was a problem hiding this comment.
For compute, @workgroup_size(...) must be valid; with options.workgroupSize undefined this will emit @workgroup_size(undefined) which is invalid WGSL. Make workgroupSize required when functionType === 'compute', or provide a safe default (e.g., [1,1,1]) or throw a targeted error when missing.
| const location = (dataType.attribs as d.AnyAttribute[]).find( | ||
| (a) => a.type === '@location', | ||
| )?.params[0]; | ||
| this.ctx.addDeclaration(`layout(location = ${location}) out ${varName};`); |
There was a problem hiding this comment.
The generated GLSL out declarations are missing the variable type (e.g. layout(location=0) out vec2 uv_1;), and location can become undefined, producing invalid GLSL. Include the resolved underlying data type in the declaration, and only emit the declaration when location is a concrete number (otherwise throw or handle a default consistently).
Suggested change
| this.ctx.addDeclaration(`layout(location = ${location}) out ${varName};`); | |
| if (typeof location !== 'number') { | |
| throw new Error( | |
| `Missing or invalid @location for output property "${String(propName)}"`, | |
| ); | |
| } | |
| const resolvedDataType = this.ctx.resolve(dataType); | |
| if (resolvedDataType === UnknownData) { | |
| throw new Error( | |
| `Unable to resolve GLSL type for output property "${String(propName)}"`, | |
| ); | |
| } | |
| this.ctx.addDeclaration( | |
| `layout(location = ${location}) out ${resolvedDataType.value} ${varName};`, | |
| ); |
Comment on lines
+118
to
+123
| if (this.#functionType !== 'normal') { | ||
| // oxlint-disable-next-line no-non-null-assertion | ||
| const entryFnState = this.#entryFnState!; | ||
| const expectedReturnType = this.ctx.topFunctionReturnType; | ||
|
|
||
| if (typeof exprNode === 'object' && exprNode[0] === NODE.objectExpr) { |
There was a problem hiding this comment.
Entry-point return rewriting assigns any returned vec* to gl_Position for all non-normal stages, which is incorrect for fragment (and compute). Restrict gl_Position assignment to the vertex stage, and for fragment returns emit a proper fragment output (e.g., layout(location=0) out vec4 outColor; outColor = ...; return;).
Comment on lines
+174
to
+181
| if (expr.dataType.type.startsWith('vec')) { | ||
| const block = super._block( | ||
| [NODE.block, [[NODE.assignmentExpr, 'gl_Position', '=', exprNode], [NODE.return]]], | ||
| { gl_Position: gl_PositionSnippet.$ }, | ||
| ); | ||
|
|
||
| return `${this.ctx.pre}${block}`; | ||
| } |
There was a problem hiding this comment.
Entry-point return rewriting assigns any returned vec* to gl_Position for all non-normal stages, which is incorrect for fragment (and compute). Restrict gl_Position assignment to the vertex stage, and for fragment returns emit a proper fragment output (e.g., layout(location=0) out vec4 outColor; outColor = ...; return;).
Comment on lines
+54
to
+65
| function resolveStruct(ctx: ResolutionCtx, struct: d.WgslStruct) { | ||
| const id = ctx.makeUniqueIdentifier(ShaderGenerator.getName(struct), 'global'); | ||
|
|
||
| ctx.addDeclaration(`\ | ||
| struct ${id} { | ||
| ${Object.entries(struct.propTypes) | ||
| .map(([prop, type]) => ` ${ctx.resolve(type).value} ${prop};\n`) | ||
| .join('')}\ | ||
| };`); | ||
|
|
||
| return id; | ||
| } |
There was a problem hiding this comment.
As written, resolving a struct type may add the same struct declaration multiple times (e.g., if the struct is referenced in multiple places), which will cause GLSL compilation failures due to duplicate struct definitions. Add memoization (e.g., a WeakMap<d.WgslStruct, string> in GlslGenerator, or a context-level cache) so each struct is declared once and subsequent resolutions reuse the same emitted name.
| @@ -0,0 +1,7 @@ | |||
| import glslGenerator from './glslGenerator.ts'; | |||
|
|
|||
| export function GLOptions() { | |||
There was a problem hiding this comment.
Exported functions are typically lowerCamelCase in TS/JS APIs; GLOptions() reads like a type/namespace rather than a factory. Consider renaming to glOptions() (or createGLOptions()) to make callsites clearer and align with common conventions.
Suggested change
| export function GLOptions() { | |
| export function glOptions() { |
Comment on lines
+267
to
274
| public _emitVarDecl( | ||
| keyword: 'var' | 'let' | 'const', | ||
| name: string, | ||
| _dataType: wgsl.BaseData | UnknownData, | ||
| rhsStr: string, | ||
| ): string { | ||
| return `${pre}${keyword} ${name} = ${rhsStr};`; | ||
| return `${this.ctx.pre}${keyword} ${name} = ${rhsStr};`; | ||
| } |
There was a problem hiding this comment.
This changes a previously protected helper into a public underscore-prefixed method, increasing the effective public API surface. If this is only intended for subclass overrides, prefer protected to avoid external callers depending on an internal API; if it must be public, consider a non-underscored, documented method name.
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.
Blocked by #2363
This PR builds upon the previous
GlslGeneratorfunctionality by adding:outinter-stage variables in entry functions.