Repository navigation
GLEmulation: fix texture-load merging when a texture matrix is in use - #28061
TheTomBritton wants to merge 1 commit into
Conversation
CTexUnit.genPassLines merges repeated texture loads into one variable, finding them with /(texture.*?\(.*?\))/g. With a texture matrix in use the generated sample expression is texture2D(u_texUnit0, (u_textureMatrix0 * v_texCoord0).xy) and the non-greedy match stops at the `)` that closes the matrix product, so any GL_COMBINE environment that uses the texture more than once was rewritten into invalid GLSL and nothing was drawn. (The regex used after the first merge was greedy instead, and could run to the last `)` on a line.) Mark the end of every generated texture load with a /*endload*/ comment and match up to it, as suggested in emscripten-core#27946. The comments left behind after merging are ordinary GLSL comments. Adds a browser test that draws with two GL_COMBINE environments (one with two loads on a single line, one with GL_INTERPOLATE loading the texture on two lines) while a texture matrix is set, and checks the colours. It fails before this change (the shader does not compile) and passes after it. Fixes emscripten-core#27946 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UR3bCLyLBQiGtevGJx2ARt
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue in LEGACY_GL_EMULATION where invalid fixed-function fragment shaders were generated when a texture matrix was in use and a texture was referenced multiple times in GL_COMBINE environments. The fix appends an /*endload*/ sentinel to texture load expressions and updates the regex to match it, preventing premature matching at internal parentheses. A regression test has also been added. The review feedback highlights a potential issue where the non-greedy regex could backtrack across multiple texture calls if a sentinel is missing, and suggests a more robust regex pattern using negative lookaheads to prevent incorrect merging.
|
|
||
| var texLoadLines = ''; | ||
| var texLoadRegex = /(texture.*?\(.*?\))/g; | ||
| var texLoadRegex = /(texture.*?\(.*?\))\/\*endload\*\//g; |
There was a problem hiding this comment.
If any texture load expression (such as texture2DProj, as noted in the PR description) does not have the /*endload*/ sentinel, the non-greedy .*? in the regex /(texture.*?\(.*?\))\/\*endload\*\//g can backtrack and match across multiple texture calls to find the next available /*endload*/. This would incorrectly merge multiple distinct texture calls into a single invalid expression, leading to shader compilation failures. Using (?:(?!texture).)*? instead of .*? ensures that the match cannot cross another texture keyword, making the regex robust against missing sentinels or other occurrences of the word texture on the same line.
var texLoadRegex = /(texture(?:(?!texture).)*?\((?:(?!texture).)*?\))\/\*endload\*\//g;| lines = lines.split(texLoadExpr).join(texLoadVar); | ||
| // Reset regex search, since we modified the string. | ||
| texLoadRegex = /(texture.*\(.*\))/g; | ||
| texLoadRegex = /(texture.*?\(.*?\))\/\*endload\*\//g; |
There was a problem hiding this comment.
If any texture load expression (such as texture2DProj, as noted in the PR description) does not have the /*endload*/ sentinel, the non-greedy .*? in the regex /(texture.*?\(.*?\))\/\*endload\*\//g can backtrack and match across multiple texture calls to find the next available /*endload*/. This would incorrectly merge multiple distinct texture calls into a single invalid expression, leading to shader compilation failures. Using (?:(?!texture).)*? instead of .*? ensures that the match cannot cross another texture keyword, making the regex robust against missing sentinels or other occurrences of the word texture on the same line.
texLoadRegex = /(texture(?:(?!texture).)*?\((?:(?!texture).)*?\))\/\*endload\*\//g;
|
Feel free to take Gemini's comments with a grain of salt. We are currently experimenting with various review agents. |
Fixes #27946.
The duplicate-texture-load merge in
genPassLinesextracted sample expressions with a lazy regex that stopped at the first), so with a texture matrix active it captured a truncatedtexture2D(u_texUnit0, (u_textureMatrix0 * v_texCoord0and the GL_COMBINE shader failed to compile. This is @juj's sentinel diff from the issue, verbatim:genTexUnitSampleExprappends/*endload*/and the regex matches up to it.Adds
test_gl_texenv_combine_texmatrix: two GL_COMBINE setups that use the texture more than once (two loads on a single line, and INTERPOLATE), drawn with a texture matrix set, checked texel by texel. Fails before the change (shader compile error), passes after it on Chrome/ANGLE (Metal and SwiftShader). The other LEGACY_GL_EMULATION browser tests pass locally.Note: this and #27947's PR both touch
genTexUnitSampleExpr; whichever lands second needs a one-line rebase (thetexture2DProjreturn also wants the/*endload*/sentinel).🤖 Generated with Claude Code
https://claude.ai/code/session_01AFhyBALrPm8CSkyHzQizDH