fix(ISHDR): guard contrastedColorModified against zero avgValue#2126
Conversation
Replace raw avgValue.x with max(avgValue.x, 1e-6) in the contrast modification path to prevent INF/NaN when the average luminance is zero. The earlier exposure path already guards this value; this brings the contrast path in line. Fixes community-shaders#2104
📝 WalkthroughWalkthroughThe shader Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
|
No actionable suggestions for changed features. |
|
✅ A pre-release build is available for this PR: |
Reviewer flagged the hardcoded 1e-6 as a magic number. Replace with the existing EPSILON_DIVISION constant from Common/Math.hlsli, matching how BRDF.hlsli and StencilCS.hlsl guard divisions elsewhere in the codebase.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
package/Shaders/ISHDR.hlsl (1)
1-1: Optional: tighten PR title to match repo convention limits.Suggested title:
fix(ishdr): guard contrast path for zero avg value
As per coding guidelines, commit titles should followtype(scope): description, use lowercase description, and stay within 50 characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISHDR.hlsl` at line 1, Update the PR title to follow the repo convention "type(scope): description" in lowercase and within ~50 characters; replace the current title with the suggested one "fix(ishdr): guard contrast path for zero avg value" so it matches the required format and succinctly describes the change (scope: ISHDR).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package/Shaders/ISHDR.hlsl`:
- Line 1: Update the PR title to follow the repo convention "type(scope):
description" in lowercase and within ~50 characters; replace the current title
with the suggested one "fix(ishdr): guard contrast path for zero avg value" so
it matches the required format and succinctly describes the change (scope:
ISHDR).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7307c682-e4a1-431d-8c1d-931e48ce8e82
📒 Files selected for processing (1)
package/Shaders/ISHDR.hlsl
…unity-shaders#2126) Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>
Summary
Guard the contrast modification path in ISHDR.hlsl against division by zero when
avgValue.xis 0.Why this matters
Line 178 divides
tintedColorbyavgValue.xwithout checking for zero. WhenavgValue.xis 0, the division producesINF, which propagates throughpow()and contaminatesoutputColor. The earlier exposure path (lines ~168-169) already guards this same value withif (avgValue.x != 0), but the contrast modification skips the check. Flagged by @alandtse during review of PR #2103.Changes
package/Shaders/ISHDR.hlslline 178: Replace rawavgValue.xwithmax(avgValue.x, 1e-6)(epsilon clamp). Both the divisor and the multiplier use the clamped value so the result stays mathematically consistent.Testing
HLSL static analysis. The change is a single float substitution with no control flow changes.
Fixes #2104
This contribution was developed with AI assistance (Claude Code).
Summary by CodeRabbit