gfx-rs / wgpu

A cross-platform, safe, pure-Rust graphics API.
https://wgpu.rs
Apache License 2.0
11.46k stars 855 forks source link

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

Closed kjarosh closed 5 days ago

kjarosh commented 1 week ago

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

Connections Fixes https://github.com/gfx-rs/wgpu/issues/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

torokati44 commented 6 days ago

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

kjarosh commented 6 days ago

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 commented 6 days ago

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 commented 5 days ago

@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:

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

teoxoy commented 5 days ago

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