Fix preserving layout(std140) directives with -sGL_EXPLICIT_UNIFORM_BINDING #25839
Conversation
| source = source.replace(/layout\s*\([^,]*?binding\s*=\s*([-\d]+)[^,]*?\)/g, ''); // "layout(binding = 3)" -> "" | ||
| source = source.replace(/(layout\s*\((.*?)),\s*binding\s*=\s*([-\d]+)\)/g, '$1)'); // "layout(std140, binding = 1)" -> "layout(std140)" | ||
| source = source.replace(/layout\s*\(\s*binding\s*=\s*([-\d]+)\s*,(.*?)\)/g, 'layout($2)'); // "layout(binding = 1, std140)" -> "layout(std140)" | ||
| source = source.replace(/layout\s*\(\s*binding\s*=\s*([-\d]+)\s*,\s*(.*?)\)/g, 'layout($2)'); // "layout(binding = 1, std140)" -> "layout(std140)" |
There was a problem hiding this comment.
This \s* addition is not part of the fix itself, but just to improve cleanliness of the string replace for the assert() in the test below. Without this, layout(binding = 1, std140) was converted to form layout( std140), so the \s* here swallows the stray space before std140.
src/lib/libwebgl.js
Outdated
| // their way to the actual WebGL shader compiler. These regexes get quite | ||
| // hairy, check against https://regex101.com/ when working on these. | ||
| source = source.replace(/layout\s*\(.*?binding\s*=\s*([-\d]+).*?\)/g, ''); // "layout(binding = 3)" -> "" | ||
| source = source.replace(/layout\s*\([^,]*?binding\s*=\s*([-\d]+)[^,]*?\)/g, ''); // "layout(binding = 3)" -> "" |
There was a problem hiding this comment.
The actual fix here is to change the regex match to use [^,] (capture anything else than a , character), instead of a . (capture any character).
This way the first replace won't catch on layout(binding = 3, ...) or layout(..., binding = 3)
There was a problem hiding this comment.
Why not just use \s* here? i.e. what chars are allowed between the opening paren and the word binding?
There was a problem hiding this comment.
It could be layout(std140, binding=5) for example.
There was a problem hiding this comment.
I you don't want to match those is that right?
Why doesn't \s* work then? Otherwise won't you end up matching things like layout(whatever binding = 0)?
I guess its not valid but I don't see why you would want to match anything except space?
There was a problem hiding this comment.
Also, can you remove the ? that follow the * operators? Isn't it redundant to say [^,]*? since the * already includes matching zero things.
There was a problem hiding this comment.
Err, yeah, I somehow read your first question as "why not use .* here?".
Now I see what you meant. Definitely using \s* is simpler than [^,]*?. Updated to that form. Thanks!
…the Emscripten -sGL_EXPLICIT_UNIFORM_BINDING WebGL extension. Improve the existing test to catch the reported scenario. Fixes emscripten-core#25828.
juj
force-pushed
the
fix_layout_std140_preserve
branch
from
79e9661 to
fbbd974
Compare
kainino0x
left a comment
kainino0x
left a comment
There was a problem hiding this comment.
FWIW @juj I discussed the issue more with @SetoKaiba on chat and determined that the root issue, that exposed this bug, was actually in the non-browser runtime they're using, which doesn't implement one of the WebGL special cases: in WebGL (unlike GLES), uniform buffers always have std140 layout.
Because of that, for WebGL I don't think it really matters if the layout(std140) is preserved or not. I don't know if this changes what you want to do in Emscripten.
Yes.
|
|
Yeah, that makes sense. It is still good to land this, given that it can otherwise produce red herrings to developers (like happened in this case) |
4 participants
Fix preserving layout(std140) directives in WebGL shaders when using
-sGL_EXPLICIT_UNIFORM_BINDINGWebGL extension. Improve the existing test to catch the reported scenario.Fixes #25828.