Skip to content

fix early type checks for comparison/logical ops in binaryOpNode - #9163

Open
skyyash wants to merge 11 commits into
processing:mainfrom
skyyash:warn/invalid-strands-comparisons
Open

skyyash wants to merge 11 commits into
processing:mainfrom
skyyash:warn/invalid-strands-comparisons

Conversation

@skyyash

@skyyash skyyash commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Resolves #8445

Changes:

  • add early type validation for comparison and logical operators in binaryOpNode.
  • ordering operators (<, <=, >, >=) are now rejected when either operand is a vector, matching glsl rules.
  • equality and logical operators also get matching dimension/base-type checks.
  • errors are thrown via the existing fes helpers.

PR Checklist

@skyyash skyyash changed the title Warn/invalid strands comparisons fix early type checks for comparison/logical ops in binaryOpNode Sep 11, 2026
@p5-bot

p5-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown

@perminder-17 perminder-17 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hi, thanks for your work on this! Do you think it would be worth adding two more cases?

  1. Comparing two boolean values using <, etc. such as a < b where both a and b are booleans.
  2. Comparing a boolean with a number using equalTo() or notEqual().

Both are invalid in GLSL, so catching them early with a strands error would be helpful? what do you think

@skyyash

skyyash commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

hi @perminder-17 , thanks for suggesting both cases. yes we should catch them early to log friendly errors.

happy to push follow up commit.

@perminder-17 perminder-17 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If you look at lines 177 to 183 in ir_builders.js

if (leftType.dimension === 1 && rightType.dimension > 1) {
  cast.node = leftStrandsNode;
  cast.toType = rightType;
} else if (rightType.dimension === 1 && leftType.dimension > 1) {
  cast.node = rightStrandsNode;
  cast.toType = leftType;

This code does type casting, where the dimensions get equalized. Because of this, the error message can become confusing.

For example:

if (color < 2) {
  // ...some operations
}

Currently we get: < is only defined for scalars. Got float4 < float4. But this is misleading, we should instead get Got float4 < float1, since that reflects the user's original types, not the post-cast types.

Can we take a snapshot of the original types before the typecasting happens, and use that snapshot in all the error messages, so this confusion doesn't happen? @skyyash Let me know what you think?

@skyyash

skyyash commented Sep 19, 2026

Copy link
Copy Markdown
Contributor Author

@perminder-17 good catch, (i didnt noticed it).

when the pre-cast type is DEFER, i think the error message should fall back to the resolved post-cast types? what do you think?

preCastLeftType.baseType === BaseType.DEFER ? leftBase : preCastLeftType.baseType

since DEFER is an internal placeholder and Got defer4 < defer1 would mean nothing to the user...

@perminder-17

Copy link
Copy Markdown
Collaborator

i think the error message should fall back to the resolved post-cast types? what do you think?

Sounds good to me!

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.

[p5.strands] Some comparisons are allowed by strands but disallowed by GLSL

2 participants