chore(rendering): remove the Clamp Lighting setting - #5370
Conversation
#5152: Clamp Lighting was reported to visibly do nothing. The maintainer who investigated confirmed why and asked for it to be removed outright, not just fixed or hidden: "remove clamplightning in both ui and Rendering Config" / "remove clamp lightning from UI, rendering config and any related code you can find." It clamps the deferred lighting pass's output color to [0.0, 1.0] before it reaches later HDR-aware post-processing (bloom, eye adaptation, tone mapping) - see the paired CoreRendering PR. Most framebuffer formats already clamp on write, so the setting only has a visible effect in specific HDR configurations, which is why toggling it does nothing for most players - not a bug to fix, a setting whose value never showed up where a player would look for it. Removed the RenderingConfig field, getter/setter and property-change constant, the GLSLShader #define, the checkbox and its label from videoMenuScreen.ui, the VideoSettingsScreen binding, the default.cfg entry, and the base i18n string. Other locale .lang files keep the now- orphaned key; that is the normal, harmless state for a translation string whose base entry has been retired, and Weblate's own sync will stop requesting it going forward rather than needing 25 files hand-edited here. Compiles clean (:engine:compileJava, :engine-tests:compileTestJava). default.cfg, videoMenuScreen.ui and menu_en.lang all still parse as valid JSON after the edits. Fixes #5152 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions
Bot
added
the
Type: Chore
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (6)
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change removes the clamp lighting option from rendering configuration, shader source assembly, default settings, localization, and video settings UI. ChangesClamp lighting removal
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR removes the unused Clamp Lighting setting and its related configuration and UI entries. No actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Ran a visual/log test with this change (plus the rest of a larger merge-train batch) using the full Omega module set, then compared its Both runs produce the exact same 231 So this DAG noise is pre-existing in |
|
Split out into its own ticket: #5371 (Flickering lights flicker in unison per chunk instead of independently per block) - still open, tracked separately from this PR. |
2 participants
Fixes #5152 (the "Clamp Lighting" part of it - see below for the rest).
Why removal, not a fix
Clamp Lighting was reported to visibly do nothing. The maintainer who investigated confirmed it and asked for outright removal, not a fix: "remove clamplightning in both ui and Rendering Config" / "remove clamp lightning from UI, rendering config and any related code you can find." That's a still-open TODO from the issue thread - unimplemented.
It clamps the deferred lighting pass's output color to
[0.0, 1.0]before it reaches later HDR-aware post-processing (bloom, eye adaptation, tone mapping). Most framebuffer formats already clamp on write regardless, so the setting only has a visible effect in specific HDR configurations - which is why toggling it does nothing for most players. Not a bug, a setting whose effect never showed up where a player looking for it would see it.What's removed
RenderingConfig: theclampLightingfield,isClampLighting()/setClampLighting(), and theCLAMP_LIGHTINGproperty-change constantGLSLShader: the#define CLAMP_LIGHTINGemissionvideoMenuScreen.ui: the checkbox and its labelVideoSettingsScreen: the checkbox bindingdefault.cfg: the persisted entrymenu_en.lang: the base i18n stringOther locale
.langfiles keep the now-orphaned key - that's the normal state for a retired translation string; Weblate stops requesting it going forward rather than needing 25 files hand-edited here.Companion PR
The shader side (the actual
#if defined(CLAMP_LIGHTING)branch) lives in a separate repo: Terasology/CoreRendering#85Verification
:engine:compileJavaand:engine-tests:compileTestJavaboth clean.default.cfg,videoMenuScreen.ui, andmenu_en.langstill parse as valid JSON after the edits (verified withpython3 -m json.tool, not just by eye). Couldn't launch the actual settings screen to confirm the UI renders without the removed rows - no display in this environment.What's still open on #5152
The issue's other item - separating the flickering-light offset per block instead of per chunk - is explicitly not addressed here. The maintainer thread flagged it as performance-sensitive ("we need to be considerate of the performance impact here... we probably don't want to merge that until we improved performance a bunch first") and it needs shader-level profiling I can't do in this environment, so I left it out rather than guess at a fix. Eye-adaptation, bloom, and vignette were already fixed in earlier, already-merged CoreRendering PRs (#77, #78, #80).