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

[naga] Add packed as a keyword for GLSL #5855

Merged
merged 1 commit into from
Jun 24, 2024
Merged

Conversation

kjarosh
Copy link
Contributor

@kjarosh kjarosh commented Jun 21, 2024

Turns out that sometimes packed is a keyword, and the produced GLSL had syntax errors due to that.

Connections
Fixes #5853.

Description
Sometimes packed is a keyword in GLSL, and naga was preserving variables named that way, which caused syntax errors.

Testing
I executed the following command and observed that the packed variable name was suffixed with an underscore. The input file was the original file that caused syntax errors for some users (https://github.com/ruffle-rs/ruffle/blob/036839fb1f382852711e0c1e270fa3e69a3a540a/render/wgpu/shaders/filter/displacement_map.wgsl). Others have confirmed that renaming the identifier to something else than packed fixes those issues.

cargo run --package naga-cli displacement_map.wgsl output.vert --entry-point main_vertex

Checklist

  • Run cargo fmt.
  • Run cargo clippy (it produces warnings but my patch does not introduce any). If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
    • The same set of ~45 tests fail both before and after this PR, so... it's stable at least?
  • Add change to CHANGELOG.md. See simple instructions inside file.

@kjarosh kjarosh requested a review from a team as a code owner June 21, 2024 19:53
Turns out that sometimes `packed` is a keyword,
and the produced GLSL had syntax errors due to that.
@torokati44
Copy link
Contributor

I'm wondering whether there are any other similar keywords that should also be added. Such as the std### ones.

@kjarosh
Copy link
Contributor Author

kjarosh commented Jun 22, 2024

I was wondering that too, but I couldn't find any evidence on the internet that any of them should be treated as a keyword. However, we may prefer falsely claiming that something is a keyword to falsely claiming that something isn't.

There's also an option to test those suspected keywords on a platform where packed is a keyword.

@torokati44
Copy link
Contributor

I tried looking at the documents linked in the comments within this list, and search for packed in them, seeing what's next to it in there.

@kjarosh
Copy link
Contributor Author

kjarosh commented Jun 24, 2024

@sombraguerrero has confirmed that whilst packed is a keyword on their platform, other similar identifiers (std140,std430,row_major,location,triangles,index,r8) are not.

I did some tests myself (but not using wgpu, as OpenGL does not work for me in wgpu). And packed on my platform:

  • is not a keyword in OpenGL,
  • is not a keyword in OpenGL ES,
  • is a keyword in WebGL 1.0,
  • is not a keyword in WebGL 2.0.

I also tried other similar identifiers in WebGL, but it really seems that packed is the only one.

Copy link
Member

@teoxoy teoxoy left a comment

Choose a reason for hiding this comment

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

Thanks!

@teoxoy teoxoy merged commit 3556133 into gfx-rs:trunk Jun 24, 2024
25 checks passed
@teoxoy
Copy link
Member

teoxoy commented Jun 24, 2024

I looked at the specs and we seem to be missing a few more, I will open a PR for the remaining ones.

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.

Naga produces a reserved keyword when translating variable name packed to GLSL
3 participants