Repository navigation
Fix GLES type errors in PBR probe blending - #3001
riccardobl merged 3 commits into
Conversation
| while (assignments.find()) { | ||
| String expression = assignments.group(2); | ||
| if (expression.contains("sumNdf")) { | ||
| weights.add(assignments.group(1)); | ||
| assertTrue(expression.contains("/ float(NB_PROBES - 1)"), | ||
| "weight" + assignments.group(1) + " must not divide a float by an int"); | ||
| } | ||
| } | ||
| assertEquals(new HashSet<>(Arrays.asList("1", "2", "3")), weights); | ||
| } | ||
| } |
There was a problem hiding this comment.
Small nit on the test: it pins the exact spacing of / float(NB_PROBES - 1), so a perfectly valid equivalent (/float(NB_PROBES - 1), or hoisting float invProbeCount = float(NB_PROBES - 1); into a local) would break the build even though the shader compiles fine on GLES. Asserting that the divisor is a float expression rather than an exact substring keeps the regression signal without the false alarms.
There was a problem hiding this comment.
Updated in a82e2fa: the divisor check now allows whitespace between its tokens, including tabs and line breaks. Formatting-only variants pass; removing any one of the three float casts, or replacing them with int, still fails. This remains a focused source contract for the inline expression, so a future refactor to a local divisor should update the test too.
I also checked the probe-count divisions in the core and terrain lighting paths. The other probe-count fallback already uses float(NB_PROBES), and the NDF sums and weight sums are floats. I found no further uncast probe-count denominator in that bounded scan.
jaime-jmebot
left a comment
jaime-jmebot
left a comment
There was a problem hiding this comment.
Nice, tightly scoped fix — thanks for keeping it to the type cast and leaving the blending math and the zero/one-probe paths alone.
float(NB_PROBES - 1)is the right minimal change: it survives the jME GLSL preprocessor (float(2 - 1)), is valid GLSL ES, and is a no-op for desktop drivers.- One nit on the new test: it matches an exact substring with specific spacing, so harmless reformatting of the shader would fail it. Loosening that check would keep the regression signal without future false alarms (inline comment has details).
- Out of curiosity, did you also scan the rest of the probe/light code for the same float-divided-by-int pattern while you were in here? Not blocking either way.
|
@toaster0123 please remove the test from this PR , it doesn't add anything useful |
|
@riccardobl Removed the test in eeb322c and updated the description. The PR now contains only the three shader-line changes. I missed your comment before the previous update, sorry. |
3 participants
PBRLighting fails to compile on GLES when two or three environment light probes are active. The blending formula divides a floating-point value by the integer expression NB_PROBES - 1. The GLES compiler rejects this mixed-type arithmetic.
This change explicitly converts that denominator to float in all three blend weights. The formula and the zero/one-probe paths are unchanged. The PR contains only these three shader-line changes.
Native rendering through the production LWJGL3 desktop and GLES backends confirms the compilation failure before the fix and successful rendering afterward. The 48-case matrix includes zero/one-probe controls, two/three probes, different light batch sizes, and positive direct lights. Float framebuffer results match the desktop baseline within 1e-6.
The combined patches from #2999 and #3000 also pass the same 48 native cases. These are Mesa correctness checks.
Core build (including Javadoc), plus desktop, effects, plugins and terrain checks passed on the unchanged production patch. The shader change was independently reviewed.