Skip to content

fix(ishdr): gate bloom for legacy weather mods#2297

Merged
doodlum merged 2 commits into
community-shaders:devfrom
SkrubbySkrubInAShrub:legacy-weather-fix
May 8, 2026
Merged

fix(ishdr): gate bloom for legacy weather mods#2297
doodlum merged 2 commits into
community-shaders:devfrom
SkrubbySkrubInAShrub:legacy-weather-fix

Conversation

@SkrubbySkrubInAShrub
Copy link
Copy Markdown
Collaborator

@SkrubbySkrubInAShrub SkrubbySkrubInAShrub commented May 8, 2026

Fixes old weather mods looking blown out in SDR. Merge when verified by users on discord.

Summary by CodeRabbit

  • Bug Fixes
    • Improved bloom effect rendering to properly distinguish between HDR and standard dynamic range displays, ensuring consistent visual quality across different display configurations.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 8, 2026

No actionable suggestions for changed features.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 8, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

The BLEND shader path now branches its bloom addition logic based on HDR mode. For HDR, it applies a soft-saturation mask using exponential decay; for SDR, it applies a hard-cutoff mask. Both paths multiply the resulting bloomMask by bloomColor before adding to the blended result.

Changes

HDR-dependent bloom masking

Layer / File(s) Summary
HDR-dependent bloom mask calculation
package/Shaders/ISHDR.hlsl
Bloom masking branches on isHDR: HDR uses saturate(Param.x - (1.0 - exp2(-blendedColor))) while SDR uses saturate(Param.x - blendedColor). Both multiply the resulting mask by bloomColor and add to blendedColor.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • alandtse
  • doodlum

Poem

🐰 A bloom so bright, now split in two—
One path for HDR's softer hue,
One sharp and clear for SDR's tone,
The shader's heart now fully known!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(ishdr): gate bloom for legacy weather mods' clearly and accurately describes the main change: gating bloom in the ISHDR shader to address legacy weather mods appearing blown out in SDR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
package/Shaders/ISHDR.hlsl (1)

171-176: Consider adding an issue-closing keyword in the PR description (if this maps to a tracked bug).

Using Fixes #<id> / Closes #<id> will improve traceability and auto-close behavior after merge.

As per coding guidelines "Suggest adding appropriate GitHub keywords: 'Fixes #123' or 'Closes #123' for bug fixes".

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@package/Shaders/ISHDR.hlsl` around lines 171 - 176, The PR lacks an
issue-closing keyword for traceability; update the pull request description to
include a GitHub keyword like "Fixes #<id>" or "Closes #<id>" that references
the tracked bug this change addresses (e.g., the HDR/SDR bloom behavior code
around bloomMask, isHDR, Param.x, blendedColor and bloomColor in ISHDR.hlsl) so
the issue is auto-closed on merge.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@package/Shaders/ISHDR.hlsl`:
- Around line 171-176: The PR lacks an issue-closing keyword for traceability;
update the pull request description to include a GitHub keyword like "Fixes
#<id>" or "Closes #<id>" that references the tracked bug this change addresses
(e.g., the HDR/SDR bloom behavior code around bloomMask, isHDR, Param.x,
blendedColor and bloomColor in ISHDR.hlsl) so the issue is auto-closed on merge.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 86075eed-9b90-497b-afca-05db9bed5e29

📥 Commits

Reviewing files that changed from the base of the PR and between 4294716 and 131756e.

📒 Files selected for processing (1)
  • package/Shaders/ISHDR.hlsl

@doodlum doodlum merged commit 79b0e40 into community-shaders:dev May 8, 2026
13 checks passed
alandtse added a commit to alandtse/open-shaders that referenced this pull request May 9, 2026
Catches up with 4 commits from upstream/dev:
  * refactor(hlsl): use Game constants (community-shaders#2298)
  * fix: mesh external emittance in interiors (community-shaders#2294) -- adds
    Utils/ExternalEmittance, hooks LLF's BSEffectShader_SetupGeometry
    via ExternalEmittance::UpdatePermutation(Pass)
  * fix(grass-collision): avoid sorting actor smart pointers (community-shaders#2299)
  * fix(ishdr): gate bloom for legacy weather mods (community-shaders#2297)

Conflicts resolved:
  * features/Inverse Square Lighting/Shaders/Features/InverseSquareLighting.ini
    Took upstream's 1-3-0 (their refactor; we hadn't bumped ISL).
  * features/Light Limit Fix/Shaders/Features/LightLimitFix.ini
    Kept our 3-1-0 -- supersedes upstream's 3-0-3 because our UI
    consolidation is the larger structural change. Upstream's
    external-emittance hook is folded in alongside.
  * src/Features/LightLimitFix.cpp
    Kept BOTH include sets: our Deferred.h plus upstream's
    Menu/ThemeManager.h and Utils/ExternalEmittance.h. The
    BSEffectShader_SetupGeometry hook gained
    ExternalEmittance::UpdatePermutation(Pass) cleanly via the
    auto-merger -- no manual reconciliation needed there.

Build verified: ALL preset compiles cleanly post-merge.
alandtse added a commit that referenced this pull request May 9, 2026
Catches up with 4 commits from upstream/dev:
  * refactor(hlsl): use Game constants (#2298)
  * fix: mesh external emittance in interiors (#2294) -- adds
    Utils/ExternalEmittance, hooks LLF's BSEffectShader_SetupGeometry
    via ExternalEmittance::UpdatePermutation(Pass)
  * fix(grass-collision): avoid sorting actor smart pointers (#2299)
  * fix(ishdr): gate bloom for legacy weather mods (#2297)

Conflicts resolved:
  * features/Inverse Square Lighting/Shaders/Features/InverseSquareLighting.ini
    Took upstream's 1-3-0 (their refactor; we hadn't bumped ISL).
  * features/Light Limit Fix/Shaders/Features/LightLimitFix.ini
    Kept our 3-1-0 -- supersedes upstream's 3-0-3 because our UI
    consolidation is the larger structural change. Upstream's
    external-emittance hook is folded in alongside.
  * src/Features/LightLimitFix.cpp
    Kept BOTH include sets: our Deferred.h plus upstream's
    Menu/ThemeManager.h and Utils/ExternalEmittance.h. The
    BSEffectShader_SetupGeometry hook gained
    ExternalEmittance::UpdatePermutation(Pass) cleanly via the
    auto-merger -- no manual reconciliation needed there.

Build verified: ALL preset compiles cleanly post-merge.
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.

3 participants