Skip to content
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

Accept only vec3 (not vecN) for the cross built-in #6171

Conversation

ErichDonGubler
Copy link
Member

@ErichDonGubler ErichDonGubler commented Aug 27, 2024

Connections

Resolves #6153.

Description

Previously, we only checked that a single type of vector of floats was being used as input for the cross built-in. Only vec3 should be permitted; so, narrow our checking of the vector type to 3 elements.

Testing

Tests have been added to ensure coverage of what we should and should not accept for the cross built-in.

Open questions:

  • Do we need to permit more than just vec3 from language frontends other than WGSL? Nope!

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@ErichDonGubler ErichDonGubler added area: validation Issues related to validation, diagnostics, and error handling area: correctness We're behaving incorrectly area: naga middle-end Intermediate representation labels Aug 27, 2024
@ErichDonGubler ErichDonGubler self-assigned this Aug 27, 2024
@ErichDonGubler ErichDonGubler force-pushed the erichdongubler/push-rnptotqoozpt branch 2 times, most recently from 144288e to c728ede Compare August 27, 2024 21:09
@ErichDonGubler ErichDonGubler marked this pull request as ready for review August 27, 2024 21:09
@ErichDonGubler ErichDonGubler requested a review from a team as a code owner August 27, 2024 21:09
@ErichDonGubler ErichDonGubler force-pushed the erichdongubler/push-rnptotqoozpt branch 3 times, most recently from 89796ae to 969f806 Compare August 27, 2024 21:15
@ErichDonGubler
Copy link
Member Author

@teoxoy: Thoughts on this question in the OP?

Do we need to permit more than just vec3 from language frontends other than WGSL?

@ErichDonGubler ErichDonGubler force-pushed the erichdongubler/push-rnptotqoozpt branch from 969f806 to 1300c54 Compare August 28, 2024 13:32
@teoxoy
Copy link
Member

teoxoy commented Aug 28, 2024

I don't know, I'd have to check.

@teoxoy
Copy link
Member

teoxoy commented Aug 28, 2024

GLSL and SPIR-V say they must be vec3s as well.

@ErichDonGubler
Copy link
Member Author

ErichDonGubler commented Aug 28, 2024

Ah, yep, all these sources state only accepting 3-component vectors:

There seems to be some divergence in the float types accepted on these platforms, but that's orthogonal to this PR. 😀 :shipit:

Thanks to @teoxoy for onboarding me onto the language spec. resources I cited above. 🍻

@ErichDonGubler ErichDonGubler merged commit 0ce5996 into gfx-rs:trunk Aug 28, 2024
25 checks passed
@ErichDonGubler ErichDonGubler deleted the erichdongubler/push-rnptotqoozpt branch August 28, 2024 17:26
@ErichDonGubler ErichDonGubler mentioned this pull request Sep 4, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: correctness We're behaving incorrectly area: naga middle-end Intermediate representation area: validation Issues related to validation, diagnostics, and error handling
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Vec4 cross product does not check types and crashes the program
2 participants