Skip to content

Fix PBR sunlight exposure inputs - #3000

Open
toaster0123 wants to merge 6 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/pbr-sunlight-exposure-inputs-20261002
Open

toaster0123 wants to merge 6 commits into
jMonkeyEngine:masterfrom
toaster0123:fix/pbr-sunlight-exposure-inputs-20261002

Conversation

@toaster0123

@toaster0123 toaster0123 commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Three sunlight controls in PBRLighting were not working:

  • StaticSunIntensity and UseVertexColorsAsSunIntensity enabled defines that the shared shader did not check, so their values were ignored
  • SunLightExposureMap used adjusted texture coordinates before their declaration, so enabling it failed shader compilation

The shader now recognizes the material's intensity defines alongside the existing exposure-style names. The adjusted coordinates are declared at module scope and initialized before parallax adjustment. A shader using the exposure helper without the surface reader uses its raw texture coordinates.

Four source/define contract tests cover both intensity inputs, declaration order, and the existing define names. Three fail before the fix; all four pass afterward.

A new integration test renders seven variants and checks pixels: static and vertex intensity, a two-tone exposure map, parallax with and without tangents, and the standalone exposure helper with and without a map. The existing desktop OpenGL and ANGLE CI jobs select it. Its render assertions passed locally through the production LWJGL3 desktop and GLES backends; the full display wrapper and ANGLE execution await CI.

The existing 28 native rendering cases also pass, covering zero, one, quarter intensity, disabled vertex exposure, combined inputs and ordinary directional lighting. For example, quarter exposure reads back as 64/255 instead of 255/255. These are Mesa correctness checks.

Core build (including Javadoc), plus desktop, effects, plugins and terrain checks passed: 564 tests passed, two existing skips, no failures. The new integration test compiles and has no Checkstyle warnings. The final shader and test were independently reviewed.




#ifdef ENABLE_PBRLightingUtils_readPBRSurface

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch moving this declaration up, but it now sits inside the ENABLE_PBRLightingUtils_readPBRSurface guard while the sun-exposure helper (which lives in its own ENABLE_... block) is what reads it — so a shader that enables the exposure API without readPBRSurface would fail to compile again, same class of bug as the one this PR fixes. Could you hoist it to true module scope (right below #define __PBR_LIGHT_UTILS_MODULE__) or into the exposure API's own guard, and give it an explicit default (newTexCoord = texCoord;) before the parallax branch so it is never read uninitialized?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Confirmed the modular case: enabling the direct-light helper and an exposure map without readPBRSurface failed compilation on both desktop GL and GLES. Fixed in ea7a042 by moving the declaration to module scope and using raw UVs when the surface reader is disabled. The standard reader now assigns its default UV before the parallax branch, retaining the adjusted coordinates when parallax is used.

I also added a pixel-asserting integration test selected by the existing OpenGL and ANGLE jobs. It covers seven variants, including a two-tone map, parallax with/without tangents, and the standalone helper. The exact render assertions pass locally on production desktop/GLES backends; the published shader fails the standalone-map case. The full display wrapper and ANGLE run remain for CI. Core and related checks passed with 564 passing tests and two existing skips, and the new test compiles with clean Checkstyle.

@jaime-jmebot jaime-jmebot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for this — the core diagnosis is right: the intensity parameters were silently ignored and the exposure-map path never compiled.

  • Nice: the intensity defines are now recognized alongside the old exposure-style names, and the declaration order issue is fixed.
  • One scoping nit worth a follow-up (inline): newTexCoord now lives inside the ENABLE_PBRLightingUtils_readPBRSurface guard even though the exposure helper is what consumes it.
  • On tests: the source/define contract tests are cheap and useful, but they assert on shader text, so they stay green even when the GLSL doesn't compile — which is exactly how the exposure map broke. Your 28 local rendering cases are the real proof; adding at least one of them to CI (or a small shader-compile smoke test) would keep this from regressing unnoticed.

Happy to re-review once the declaration is moved.

@riccardobl
riccardobl marked this pull request as ready for review October 2, 2026 17:46

@jaime-jmebot jaime-jmebot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The scoping nit is resolved — newTexCoord is declared at module scope now, so the exposure helper can read it without readPBRSurface being enabled, and both paths assign it before use (parallax-adjusted when parallax is on, raw UVs otherwise).

Thanks for also folding in the test feedback:

  • The new integration test asserts real pixels, which is exactly what the shader-text tests couldn't catch — PbrSunExposureModule.frag reproduces the failing modular combination (exposure API + map, no readPBRSurface) that the old code wouldn't compile.
  • Seven variants across static/vertex intensity, two-tone map, parallax with and without tangents, and the standalone helper is solid coverage for a fairly small shader change.
  • Noting for the record: the full display-wrapper and ANGLE runs are still outstanding, so this PR lands on local desktop/GLES evidence plus CI. Fine for me, just worth watching when CI reports back.

Approved.

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.

2 participants