chore(hdr): rewrite hdr sun#2090
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClamps volumetric lighting at multiple stages and refactors HDR sun handling: Sky.hlsl now delegates HDR sun gain computation to a new HDRSun.hlsli (guarded by Changes
Sequence Diagram(s)sequenceDiagram
participant Sky as Sky.hlsl
participant HDR as HDRSun.hlsli
participant Shared as SharedData
participant Tex as sunTex
Sky->>Shared: read HDRData (x,z,w)
Sky->>HDR: call GetHdrSunGain(texCoord, baseColor, sunTex, samp, alpha)
HDR->>Tex: sample coarse/fine mip(sunTex)
HDR->>Shared: use HDRData values to compute boost/weight
HDR-->>Sky: return hdrGain
Sky->>Sky: apply hdrGain to baseColor and gate glare/cloud-shadow via IsHdrSunActive()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Pull request overview
This PR adjusts HDR rendering behavior to prevent excessive brightness, specifically by refining the HDR sun glare/disc shaping logic and clamping volumetric lighting output to a 0–1 range to avoid blowouts with certain weather mods.
Changes:
- Refines HDR sun scaling by adding radial/alpha/luminance gating so HDR gain is concentrated toward the sun core instead of the entire quad.
- Clamps volumetric lighting sampling, adjusted value, and final output to 0–1 to prevent extreme brightness spikes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| package/Shaders/Sky.hlsl | Updates HDR sun glare/disc scaling with radial and alpha/luminance-based tapering to reduce HDR blowout. |
| package/Shaders/ISApplyVolumetricLighting.hlsl | Clamps volumetric lighting sample and output to [0, 1] to prevent overbright volumetric contribution. |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package/Shaders/Sky.hlsl (1)
231-236: Consider naming the new tuning constants.This block adds several coupled knobs (
1.85,0.32,0.62,0.22,0.88,0.94, etc.). Hoisting them into namedstatic constvalues would make the next round of visual tuning much safer.Also applies to: 247-251, 282-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Sky.hlsl` around lines 231 - 236, This change introduces several magic-number "tuning knobs" in the Sky shader (e.g., the constants 1.41421356 and 1.85 used with sunUvRadial, sunRadialR, and sunRadialHdr inside the TEX block), so hoist those literals into descriptive static const float names (for example: SUN_UV_CENTER_SCALE, SUN_RADIAL_POWER, etc.) and replace the inline numbers in the expressions that compute sunUvRadial, sunRadialR, sunRadialHdr; do the same for the other magic numbers referenced in the nearby blocks (lines around 247-251 and 282-293) so each numeric tuning value has a single named constant defined near the top of the shader and reused where needed to simplify future visual tuning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/Shaders/ISApplyVolumetricLighting.hlsl`:
- Around line 48-49: The current code clamps vlSample immediately (vlSample =
min(...,1.0)) which squashes values >1.0 before applying the intensity scale;
instead sample the texture without the min (use VLTex.SampleLevel(VLSampler,
float3(input.TexCoord, repartition), 0).x directly into vlSample) and then apply
the intensity scale and clamp on vl (vl = min(g_IntensityX_TemporalY.x *
vlSample, 1.0)); update the code that sets vlSample and vl (variables vlSample,
vl, and the VLTex.SampleLevel call) so the clamp happens after scaling, leaving
the later caps at lines 53/86/88 intact.
---
Nitpick comments:
In `@package/Shaders/Sky.hlsl`:
- Around line 231-236: This change introduces several magic-number "tuning
knobs" in the Sky shader (e.g., the constants 1.41421356 and 1.85 used with
sunUvRadial, sunRadialR, and sunRadialHdr inside the TEX block), so hoist those
literals into descriptive static const float names (for example:
SUN_UV_CENTER_SCALE, SUN_RADIAL_POWER, etc.) and replace the inline numbers in
the expressions that compute sunUvRadial, sunRadialR, sunRadialHdr; do the same
for the other magic numbers referenced in the nearby blocks (lines around
247-251 and 282-293) so each numeric tuning value has a single named constant
defined near the top of the shader and reused where needed to simplify future
visual tuning.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1519f0fb-6dd9-4ca9-9d52-29936c510206
📒 Files selected for processing (2)
package/Shaders/ISApplyVolumetricLighting.hlslpackage/Shaders/Sky.hlsl
There was a problem hiding this comment.
🧹 Nitpick comments (3)
package/Shaders/Sky.hlsl (3)
220-230: PR metadata polish: normalize title casing and link issue.Suggested title:
fix(hdr): clamp volumetric lighting and sun tuning
If this resolves a tracked bug, add a keyword line in the PR body (e.g.,Fixes #<issue-id>).As per coding guidelines: "Conventional Commit Titles... Format: type(scope): description" and "Issue References... Suggest adding appropriate GitHub keywords."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Sky.hlsl` around lines 220 - 230, Update the PR metadata: change the commit/PR title to follow Conventional Commits format (e.g., "fix(hdr): clamp volumetric lighting and sun tuning") and, if this change fixes a tracked bug, add an issue reference line in the PR body such as "Fixes #<issue-id>"; this is purely a metadata change and does not modify code like SharedData::HDRData, SUN_HDR_MENU_TARGET_NITS, hdrScale, ENABLE_LL, or the HDR sun scaling logic in Sky.hlsl.
222-236: Consider centralizing new sun-tuning literals.The added constants/exponents are understandable, but grouping them as named
static constvalues would make iteration and regression tracking easier.Also applies to: 248-253, 283-292
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Sky.hlsl` around lines 222 - 236, Centralize the new literal tuning values by introducing descriptive static const names and replacing magic numbers in the Sun/HDR calculations: replace SUN_REF_PAPER_WHITE_NITS, SUN_HDR_MENU_TARGET_NITS, the exponent 1.85 used in computing sunRadialHdr, the radial scale 1.41421356, the gamma reciprocal 2.2 (used in rcp(2.2)), and the small threshold 1e-3 with named constants; declare these static const floats near the top of the shader or the SharedData/HDR block so hdrScale, hdrSunMenuMul, sunRadialR and sunRadialHdr computations (and the related blocks around SUN_REF_PAPER_WHITE_NITS, hdrSunMenuMul, hdrScale, and the TEX branch computing sunRadialHdr) reference those constants instead of raw literals for easier tuning and regression tracking.
296-297: Consider quantizing screen coordinates for consistency with other IGN uses in the codebase.At Line 296, while using
input.Position.xydirectly is valid, other shaders in the codebase (e.g., StereoSyncCS.hlsl) use integer pixel/thread IDs for the IGN hash. For consistency and to ensure pixel-perfect stability with temporal effects, consider quantizing:- float ign = frac(52.9829189 * frac(dot(input.Position.xy, float2(0.06711056, 0.00583715)))); + float2 ignPixel = floor(input.Position.xy); + float ign = frac(52.9829189 * frac(dot(ignPixel, float2(0.06711056, 0.00583715))));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Sky.hlsl` around lines 296 - 297, The IGN calculation uses input.Position.xy directly; to match other shaders (e.g., the IGN hash in StereoSyncCS) and ensure pixel-stable results, quantize the screen coordinates before hashing: replace the direct use of input.Position.xy in the dot() that feeds frac(...) with integer/rounded pixel coordinates (e.g., floor or round of input.Position.xy or SV_Position.xy) so ign is computed from the quantized pixel position; keep the rest of the expression (the constants, frac chain, and use of discScale) unchanged and ensure the variable name ign is preserved.
🤖 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/Sky.hlsl`:
- Around line 220-230: Update the PR metadata: change the commit/PR title to
follow Conventional Commits format (e.g., "fix(hdr): clamp volumetric lighting
and sun tuning") and, if this change fixes a tracked bug, add an issue reference
line in the PR body such as "Fixes #<issue-id>"; this is purely a metadata
change and does not modify code like SharedData::HDRData,
SUN_HDR_MENU_TARGET_NITS, hdrScale, ENABLE_LL, or the HDR sun scaling logic in
Sky.hlsl.
- Around line 222-236: Centralize the new literal tuning values by introducing
descriptive static const names and replacing magic numbers in the Sun/HDR
calculations: replace SUN_REF_PAPER_WHITE_NITS, SUN_HDR_MENU_TARGET_NITS, the
exponent 1.85 used in computing sunRadialHdr, the radial scale 1.41421356, the
gamma reciprocal 2.2 (used in rcp(2.2)), and the small threshold 1e-3 with named
constants; declare these static const floats near the top of the shader or the
SharedData/HDR block so hdrScale, hdrSunMenuMul, sunRadialR and sunRadialHdr
computations (and the related blocks around SUN_REF_PAPER_WHITE_NITS,
hdrSunMenuMul, hdrScale, and the TEX branch computing sunRadialHdr) reference
those constants instead of raw literals for easier tuning and regression
tracking.
- Around line 296-297: The IGN calculation uses input.Position.xy directly; to
match other shaders (e.g., the IGN hash in StereoSyncCS) and ensure pixel-stable
results, quantize the screen coordinates before hashing: replace the direct use
of input.Position.xy in the dot() that feeds frac(...) with integer/rounded
pixel coordinates (e.g., floor or round of input.Position.xy or SV_Position.xy)
so ign is computed from the quantized pixel position; keep the rest of the
expression (the constants, frac chain, and use of discScale) unchanged and
ensure the variable name ign is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a7a210c-0907-4899-80cc-959d7bd607a6
📒 Files selected for processing (2)
package/Shaders/Sky.hlslsrc/Features/HDRDisplay.h
✅ Files skipped from review due to trivial changes (1)
- src/Features/HDRDisplay.h
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
@coderabbitai analyse this pull request and be as mean as possible to @davo0411 |
🧩 Analysis chainThe PR description images show:
If you want this merged without me gnawing the chair leg:
References: The PR description claims these scope changes and shows the before/after HDR graphs that match the “narrow spike” result; my critiques above are exactly about how you achieve that without collateral damage. (github.com) 🏁 Script executed: Length of output: 403 The PR description images show:
🏁 Script executed: Length of output: 10925 The PR description images show:
Volumetric lighting (package/Shaders/ISApplyVolumetricLighting.hlsl)
Concrete fix sketch: // sample + scale (preserve headroom here)
float vlSample = VLTex.SampleLevel(VLSampler, float3(input.TexCoord, repartition), 0).x;
float vl = g_IntensityX_TemporalY.x * vlSample;
// threshold, then clamp once
float adjustedVl = saturate(noiseGrad + vl - 0.0078125);
float temporalContribution = g_IntensityX_TemporalY.y * (1 - smoothstep(0, 1, min(1, 100 * abs(depth - previousDepth))));
psout.VL = saturate(lerp(adjustedVl, previousVl, temporalContribution * isValid));Sky/HDR sun (package/Shaders/Sky.hlsl)
HDR intent and constants
Safety and build matrix
Tests you owe us before merge
Nits
If you clean up the clamp spam, tighten the HDRSun API, and prove the non‑HDR/VR paths are inert, I’m out of things to throw. Until then, this is an over‑eager fix that trades one problem (blowouts) for two (flattened VL and a leaky sky abstraction). 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
package/Shaders/ISApplyVolumetricLighting.hlsl (1)
49-50:⚠️ Potential issue | 🟠 MajorClamp after intensity scaling, not on the raw sample.
Line 49 still changes the curve before
g_IntensityX_TemporalY.xis applied: a raw sample of2.0with intensity0.4becomes0.4instead of0.8. Keep the cap on the scaled value only.🐛 Proposed fix
- float vlSample = min(VLTex.SampleLevel(VLSampler, float3(input.TexCoord, repartition), 0).x, 1.0); + float vlSample = VLTex.SampleLevel(VLSampler, float3(input.TexCoord, repartition), 0).x; float vl = min(g_IntensityX_TemporalY.x * vlSample, 1.0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISApplyVolumetricLighting.hlsl` around lines 49 - 50, The sample is being clamped before intensity is applied: remove the min(...) clamp around vlSample (the value produced by VLTex.SampleLevel using VLSampler and repartition) so vlSample stores the raw sampled x value, then apply the intensity scale with g_IntensityX_TemporalY.x and clamp the result when computing vl (i.e., clamp only the scaled value stored in vl); update references to vlSample and vl in ISApplyVolumetricLighting.hlsl accordingly.
🧹 Nitpick comments (3)
package/Shaders/ISApplyVolumetricLighting.hlsl (2)
89-89: Drop the redundant non-temporal upper clamp.
adjustedVlis alreadysaturate(...)from Line 54, so thismindoes not change behavior.♻️ Proposed cleanup
- psout.VL = min(adjustedVl, 1.0); + psout.VL = adjustedVl;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISApplyVolumetricLighting.hlsl` at line 89, psout.VL is being clamped again with min(adjustedVl, 1.0) even though adjustedVl is already produced via saturate(...) (from the earlier computation), so remove the redundant clamp by assigning psout.VL = adjustedVl; update the assignment that currently reads psout.VL = min(adjustedVl, 1.0) to simply use adjustedVl (keep references to adjustedVl and psout.VL intact).
1-1: Add an issue-closing reference if this fix has a tracked bug.The PR is a bug fix, but the provided description does not include
Fixes #.../Closes #.... Add one if there is a corresponding issue. As per coding guidelines, “Suggest adding appropriate GitHub keywords:Fixes#123or `Closes `#123for bug fixes.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISApplyVolumetricLighting.hlsl` at line 1, This PR fixes a bug but the PR description lacks an issue-closing keyword; update the PR description to include a GitHub keyword referencing the tracked issue (e.g., "Fixes `#123`" or "Closes `#123`") so the fix will auto-close the corresponding issue—mention the impacted file ISApplyVolumetricLighting.hlsl (the added include "Common/DummyVSTexCoord.hlsl") in the description for clarity.package/Shaders/Sky.hlsl (1)
242-243: Replace255.0fwith a named constant.The
/ 255.0fencodes an assumed 8-bit quantization step for the dither amplitude; past review feedback on this PR has repeatedly called out magic numbers. Please pull it into a named constant (e.g.kHdrSunDitherQuantum = 1.0f / 255.0f) alongside the other HDR sun constants inHDRSun.hlsli, and usesaturate(hdrSunGain - 1.0f) * kHdrSunDitherQuantum. Also worth a brief comment that the-0.5recenter assumesInterleavedGradientNoisereturns [0,1].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/Sky.hlsl` around lines 242 - 243, Replace the magic constant 255.0f with a named constant: add kHdrSunDitherQuantum = 1.0f / 255.0f to HDRSun.hlsli alongside the other HDR sun constants, then update the usage in Sky.hlsl (the expression in the baseColor.xyz update that currently uses (saturate(hdrSunGain - 1.0f) / 255.0f)) to use saturate(hdrSunGain - 1.0f) * kHdrSunDitherQuantum; also add a short comment next to the Random::InterleavedGradientNoise(...) -0.5 recenter noting that it assumes InterleavedGradientNoise returns [0,1].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/Shaders/ISApplyVolumetricLighting.hlsl`:
- Line 87: The temporal output currently only caps the upper bound using
min(..., 1.0) which still allows negative values from previousVl to pass
through; change the assignment for psout.VL (the lerp(adjustedVl, previousVl,
temporalContribution * isValid) expression) to clamp/saturate the final result
to [0,1] so both lower and upper bounds are enforced (i.e., wrap the lerp result
with clamp(..., 0.0, 1.0) or saturate(...)).
In `@package/Shaders/Sky.hlsl`:
- Around line 239-246: When HDRSun::IsHdrSunActive() is true we must make DITHER
and non-DITHER permutations behave the same except for the added noise; update
the block around baseColor/skyScale so that either (A) skyScale is set to 0 and
the small quantization dither via Random::InterleavedGradientNoise is applied
for both DITHER and non-DITHER paths (hoist skyScale = 0 and the dither
injection out of the `#if` !defined(DITHER) block), or (B) skip applying
hdrSunGain to baseColor for permutations that carry DITHER so hdrSunGain
multiplication and the subsequent additive sky tint occur identically — modify
the logic referencing hdrSunGain, baseColor, skyScale, HDRSun::IsHdrSunActive()
and Random::InterleavedGradientNoise to ensure both code paths converge except
for the actual noise injection controlled by the DITHER macro.
---
Duplicate comments:
In `@package/Shaders/ISApplyVolumetricLighting.hlsl`:
- Around line 49-50: The sample is being clamped before intensity is applied:
remove the min(...) clamp around vlSample (the value produced by
VLTex.SampleLevel using VLSampler and repartition) so vlSample stores the raw
sampled x value, then apply the intensity scale with g_IntensityX_TemporalY.x
and clamp the result when computing vl (i.e., clamp only the scaled value stored
in vl); update references to vlSample and vl in ISApplyVolumetricLighting.hlsl
accordingly.
---
Nitpick comments:
In `@package/Shaders/ISApplyVolumetricLighting.hlsl`:
- Line 89: psout.VL is being clamped again with min(adjustedVl, 1.0) even though
adjustedVl is already produced via saturate(...) (from the earlier computation),
so remove the redundant clamp by assigning psout.VL = adjustedVl; update the
assignment that currently reads psout.VL = min(adjustedVl, 1.0) to simply use
adjustedVl (keep references to adjustedVl and psout.VL intact).
- Line 1: This PR fixes a bug but the PR description lacks an issue-closing
keyword; update the PR description to include a GitHub keyword referencing the
tracked issue (e.g., "Fixes `#123`" or "Closes `#123`") so the fix will auto-close
the corresponding issue—mention the impacted file ISApplyVolumetricLighting.hlsl
(the added include "Common/DummyVSTexCoord.hlsl") in the description for
clarity.
In `@package/Shaders/Sky.hlsl`:
- Around line 242-243: Replace the magic constant 255.0f with a named constant:
add kHdrSunDitherQuantum = 1.0f / 255.0f to HDRSun.hlsli alongside the other HDR
sun constants, then update the usage in Sky.hlsl (the expression in the
baseColor.xyz update that currently uses (saturate(hdrSunGain - 1.0f) / 255.0f))
to use saturate(hdrSunGain - 1.0f) * kHdrSunDitherQuantum; also add a short
comment next to the Random::InterleavedGradientNoise(...) -0.5 recenter noting
that it assumes InterleavedGradientNoise returns [0,1].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d186f738-fd68-4245-a040-8b89bfdb659c
📒 Files selected for processing (3)
features/HDR Display/Shaders/HDRDisplay/HDRSun.hlslipackage/Shaders/ISApplyVolumetricLighting.hlslpackage/Shaders/Sky.hlsl
🚧 Files skipped from review as they are similar to previous changes (1)
- features/HDR Display/Shaders/HDRDisplay/HDRSun.hlsli
|
note that artistic changes to sun shape are outside of project scope |
my whole goal has been to increase brightness without size. its proved very tricky to increase nits to peak without also increasing some of the low alpha parts of the textures, or having VL increase slightly. |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
Visually it looks good to me |
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: doodlum <15017472+doodlum@users.noreply.github.com>

fixes for HDR sun shape code, and clamps volumetric lighting to 0-1 to prevent huge blowout with some weather mods, like Astralite which was unusable.
Sets sun brightness in menus to 100 nits to match sdr, instead of the weird 0.58x brightness I had before.
Summary by CodeRabbit
Bug Fixes
New Features
Documentation