Skip to content

fix(gl): translate vector equality to GLSL equal - #2964

Open
oxura wants to merge 2 commits into
software-mansion:mainfrom
oxura:fix/2832-glsl-vector-comparisons
Open

fix(gl): translate vector equality to GLSL equal#2964
oxura wants to merge 2 commits into
software-mansion:mainfrom
oxura:fix/2832-glsl-vector-comparisons

Conversation

@oxura

@oxura oxura commented Sep 2, 2026

Copy link
Copy Markdown

WGSL vector equality is component-wise and returns a boolean vector. GLSL’s == operator instead returns a scalar boolean for vector operands, so the fallback currently emits an expression whose type does not match std.eq.

std.eq now delegates binary emission to the active generator. The GLSL generator recognizes vector equality and emits GLSL’s equal(lhs, rhs) builtin; WGSL output remains unchanged through the base generator. A GLSL regression test covers the generated bvec3 function.

Verification:

  • focused GLSL generator suite: 32 passed
  • WGSL equality suites: 6 passed
  • TypeGPU and @typegpu/gl type tests
  • changed-file Oxlint and Oxfmt checks
  • fast unit suite on Node 24: 2,809 passed, 2 skipped
  • TypeGPU and @typegpu/gl package builds

Closes #2832.

Copilot AI lite review requested due to automatic review settings September 2, 2026 08:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Vector != (std.ne) still emits a scalar-bool GLSL comparison and should be translated (and tested) similarly to == to avoid the same type/semantic mismatch.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR fixes a GLSL backend semantic mismatch for std.eq on vectors: WGSL vector equality is component-wise and yields a boolean vector, while GLSL’s == yields a scalar bool, so GLSL must use equal(lhs, rhs) to preserve std.eq’s return type.

Changes:

  • Route std.eq codegen through the active generator’s emitBinaryOp instead of emitting lhs == rhs directly.
  • Teach the GLSL generator to translate vector == into equal(lhs, rhs).
  • Add a GLSL regression test covering vector equality translation.
File summaries
File Description
packages/typegpu/src/std/boolean.ts Changes std.eq codegen to delegate binary emission to the active generator.
packages/typegpu-gl/src/glslGenerator.ts Adds a GLSL-specific emitBinaryOp translation for vector equality to equal().
packages/typegpu-gl/tests/glslGenerator.test.ts Adds a snapshot test to lock in the equal(lhs, rhs) emission for vector equality.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread packages/typegpu-gl/src/glslGenerator.ts
Comment thread packages/typegpu/src/std/boolean.ts
Comment thread packages/typegpu-gl/tests/glslGenerator.test.ts

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The fix covers std.eq (==) but leaves std.ne (!=) and std.allEq on vectors emitting GLSL that fails to compile. Since #2832 is scoped as "component-wise vector comparison" and GLSL's notEqual is the direct analogue, please extend the fix (or explicitly narrow the issue scope) before merging.

Reviewed changes

  • std.eq delegates to the active generatorboolean.ts eq.codegenImpl now calls ctx.gen.emitBinaryOp(lhs, '==', rhs) instead of the raw stitch ==; WGSL output is unchanged (base emitBinaryOp wraps == in the same parens).
  • GLSL emitBinaryOp maps vector == to equal — a new branch in GlslGenerator.emitBinaryOp emits the GLSL equal(lhs, rhs) builtin when the lhs is a vector, producing a bvecN result that matches WGSL's component-wise semantics; all other operators fall through to the base generator.
  • Regression test — a glslGenerator.test.ts case asserting std.eq on vec3f resolves to return equal(lhs, rhs);.

⚠️ std.ne and std.allEq on vectors still emit invalid GLSL

This PR translates vector equality to GLSL's equal, but the same bug class remains for the sibling comparison helpers, and #2832's title ("component-wise vector comparison") covers them too.

  • std.ne(lhs, rhs) on vectors resolves (verified) to bvec3 compare(...) { return (lhs != rhs); }. GLSL's != on vector operands returns a scalar bool, not a bvecN, so this is a type error — the fix mechanism in emitBinaryOp handles == but not != (GLSL notEqual is the analogue).
  • std.allEq(lhs, rhs) resolves to return all(lhs == rhs);. GLSL's all() requires a bvecN argument, but lhs == rhs on vectors is a scalar bool, so this also fails to compile. allEq's codegen uses a raw stitch == and does not route through emitBinaryOp, so it is untouched by this change.
Technical details
# Extend vector-equality translation to ne / allEq

## Affected sites
- packages/typegpu/src/std/boolean.ts:109 — `ne.codegenImpl` still emits `stitch\`(${lhs} != ${rhs})\`` (no generator delegation).
- packages/typegpu/src/std/boolean.ts:67 — `allEq.codegenImpl` still emits `stitch\`all(${lhs} == ${rhs})\`` (no generator delegation).
- packages/typegpu-gl/src/glslGenerator.ts:826 — the new `==` branch does not handle `!=`.

## Required outcome
- `std.ne` on vectors must emit a `bvecN` result on GLSL (e.g. `notEqual(lhs, rhs)`) and unchanged `(${lhs} != ${rhs})` on WGSL.
- `std.allEq` on vectors must emit `all(equal(lhs, rhs))` on GLSL (WGSL `all(${lhs} == ${rhs})` unchanged).

## Suggested approach
- Extend the `emitBinaryOp` override's equality branch to also map `!=``notEqual` for vector operands, and delegate `ne.codegenImpl` to `emitBinaryOp(lhs, '!=', rhs)` (mirroring the `eq` change).
- For `allEq`, either route through `emitBinaryOp`/a generator hook or emit `all(equal(...))` conditionally on the GLSL generator; add GLSL regression tests for both.

## Verification
- Reproduced via a scratch GLSL resolve: `std.ne``(lhs != rhs)` and `std.allEq``all(lhs == rhs)` for `vec3f` inputs (both invalid GLSL for a `bvec3`/scalar-bool context).

Pullfrog  | Fix it ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The delta correctly fixes std.ne, but std.allEq on vectors — the other half of the prior finding — still emits GLSL that fails to compile. Please route it through the same mechanism (or explicitly narrow the issue scope) before merging.

Reviewed changes

Since the prior pullfrog review (9679612), commit 34edcac7 ("fix(gl): translate vector inequality"):

  • Extended GLSL emitBinaryOp's equality branch to also translate vector != to GLSL's notEqual builtin, now requiring both operands to carry known vec* types.
  • Routed std.ne codegen through ctx.gen.emitBinaryOp(lhs, '!=', rhs); WGSL output stays byte-identical ((lhs != rhs)) while GLSL emits notEqual(lhs, rhs).
  • Added a GLSL regression snapshot pinning std.ne on vec3f to return notEqual(lhs, rhs);.

⚠️ std.allEq on vectors still emits non-compiling GLSL

The delta addresses std.ne but not std.allEq, the other helper the prior review flagged. allEq's codegen (boolean.ts:67) still uses a raw stitch `all(${lhs} == ${rhs})`, so on GLSL it emits return all(lhs == rhs); — verified — where the vector == yields a scalar bool while GLSL's all() requires a bvecN. Any shader calling std.allEq on vectors fails to compile on the WebGL fallback; the WGSL path is unaffected (valid there).

Technical details
# std.allEq: route inner == through emitBinaryOp

## Affected sites
- packages/typegpu/src/std/boolean.ts:67 — `allEq.codegenImpl` emits raw `stitch` `` `all(${lhs} == ${rhs})` ``, bypassing the generator hook added for eq/ne. (This line is not in the no diff; it is the untouched sibling of the changed `eq`/`ne` lines 90/109.)

## Required outcome
- GLSL: `all(equal(lhs, rhs))` (compiles; `equal` returns `bvecN`).
- WGSL: unchanged semantics — `all(lhs == rhs)` stays valid (an extra paren via `emitBinaryOp` is harmless, but existing inline snapshots would need regenerating).

## Suggested approach
- `codegenImpl: (ctx, [lhs, rhs]) => stitch` `` `all(${ctx.gen.emitBinaryOp(lhs, '==', rhs)})` `` ` (or a generator hook), mirroring the eq/ne delegation. Add a GLSL snapshot test (e.g. `std.allEq(vec3f, vec3f)``all(equal(lhs, rhs))`).

## Open questions for the human
- `std.lt` / `std.le` / `std.gt` / `std.ge` on vectors have the same scalar-bool type error on GLSL (raw `stitch` `` `(a < b)` `` etc.); GLSL provides `lessThan` / `lessThanEqual` / `greaterThan` / `greaterThanEqual`. If the intent is full component-wise comparison parity they belong in this fix; otherwise explicitly narrow issue #2832's scope.

Pullfrog  | Fix it ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(@typegpu/gl): Translate component-wise vector comparison to GLSL's equal

2 participants