Conversation
WalkthroughThe hue-preserving tonemap function in DisplayMapping.hlsli was renamed and simplified, removing bloom input. ISHDR.hlsl introduces scalar tonemap factor overloads, replaces the post-processing pipeline with an HDR-centric cinematic flow, integrates DisplayMapping-based tonemapping, and adjusts control logic for exposure, bloom, and final sRGB mapping. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant VS as Vertex Shader
participant PS as ISHDR.ps (BLEND path)
participant DM as DisplayMapping.hlsli
Note over PS: HDR-centric cinematic pipeline
VS->>PS: hdr inputs (color, bloom, params)
PS->>PS: Scale by avgValue
PS->>PS: Bloom: hdrColor += max(0, Param.x - hdrColor) * bloomColor
PS->>PS: Cinematic curve: hdrColor = pow(|hdrColor|/avg, Cinematic.z)*avg*sign
PS->>PS: Luminance & exposure/tint via lerp
PS->>DM: HuePreservingTonemap(hdrColor)
DM-->>PS: srgbColor
alt FADE enabled
PS->>PS: Apply Fade to srgbColor
end
PS-->>VS: Output srgbColor
sequenceDiagram
autonumber
participant Call as Caller
participant DM as DisplayMapping.HuePreservingTonemap
participant HBD as GetTonemapFactorHejlBurgessDawson
participant RH as GetTonemapFactorReinhard
Call->>DM: HuePreservingTonemap(col)
alt Param.z > 0.5
DM->>HBD: tonemap factor (per-pixel/chan)
HBD-->>DM: factor
else Param.z ≤ 0.5
DM->>RH: tonemap factor (per-pixel/chan)
RH-->>DM: factor
end
DM->>DM: ICTCP convert, saturation ramp, hue reintro, RGB convert
DM-->>Call: mapped color
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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 |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
package/Shaders/Common/DisplayMapping.hlsli (3)
131-154: Clamp/sanitize output to guard against minor negatives from color-space roundtripsICtCp/PQ roundtrips with matrix ops can produce tiny negative RGBs on extreme inputs. A light clamp before returning avoids negative sRGB after ToSRGBColor without changing look.
col = ICtCpToRGB(ictcpMapped); - return col; + return max(0.0.xxx, col);
136-136: Reverse-edge smoothstep is harder to readsmoothstep(1.0, 0.3, ictcp.x) intentionally inverts the ramp but is nonstandard. Consider flipping edges for clarity.
- float saturationAmount = pow(smoothstep(1.0, 0.3, ictcp.x), 1.3); + float saturationAmount = pow(1.0 - smoothstep(0.3, 1.0, ictcp.x), 1.3);
139-139: Magic threshold in Param.zParam.z > 0.5 selects the operator. Prefer an integer selector or named constants for readability and to avoid precision edge cases.
- col = Param.z > 0.5 ? GetTonemapFactorHejlBurgessDawson(col) : GetTonemapFactorReinhard(col); + const bool useHBD = (Param.z >= 1.0); + col = useHBD ? GetTonemapFactorHejlBurgessDawson(col) : GetTonemapFactorReinhard(col);package/Shaders/ISHDR.hlsl (3)
41-45: Scalar overload duplicates vector logicTo reduce duplication, implement scalar in terms of the vector version (or vice versa) so future tweaks don’t diverge.
-float GetTonemapFactorReinhard(float luminance) -{ - return (luminance * (luminance * Param.y + 1)) / (luminance + 1); -} +float GetTonemapFactorReinhard(float luminance) +{ + return GetTonemapFactorReinhard(luminance.xxx).x; +}
51-57: Same here: dedupe HBD scalar overload-float GetTonemapFactorHejlBurgessDawson(float luminance) -{ - float tmp = max(0, luminance - 0.004); - return Param.y * - pow(((tmp * 6.2 + 0.5) * tmp) / (tmp * (tmp * 6.2 + 1.7) + 0.06), Color::GammaCorrectionValue); -} +float GetTonemapFactorHejlBurgessDawson(float luminance) +{ + return GetTonemapFactorHejlBurgessDawson(luminance.xxx).x; +}
121-123: Minor: stabilize luminance mixClamping hdrLuminance once avoids recomputing saturate and keeps mix deterministic.
-float hdrLuminance = Color::RGBToLuminance(hdrColor); -hdrColor = Cinematic.w * lerp(lerp(hdrLuminance, hdrColor, Cinematic.x), lerp(hdrColor, hdrLuminance, saturate(hdrLuminance)) * Tint.xyz, Tint.w).xyz; +float hdrLuminance = saturate(Color::RGBToLuminance(hdrColor)); +hdrColor = Cinematic.w * lerp(lerp(hdrLuminance.xxx, hdrColor, Cinematic.x), + lerp(hdrColor, hdrLuminance.xxx, hdrLuminance) * Tint.xyz, Tint.w).xyz;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package/Shaders/Common/DisplayMapping.hlsli(1 hunks)package/Shaders/ISHDR.hlsl(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
package/Shaders/Common/DisplayMapping.hlslipackage/Shaders/ISHDR.hlsl
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (3)
package/Shaders/Common/DisplayMapping.hlsli (1)
139-139: Ignore include-order suggestion for non-existent DisplayMapping.hlsli
DisplayMapping.hlsli (or .hlsl) isn’t present in the repository; there’s no include-order coupling to address.Likely an incorrect or invalid review comment.
package/Shaders/ISHDR.hlsl (2)
104-104: LGTM: sampling source for HDR pathUsing BlendTex as the HDR base is consistent with the new pipeline.
124-124: LGTM: centralized tonemap + hue preservationRouting final mapping through DisplayMapping::HuePreservingTonemap simplifies the pipeline and keeps the tonemap choice centralized.
| hdrColor *= avgValue.y / avgValue.x; | ||
|
|
||
| inputColor = max(0, inputColor); | ||
| hdrColor += max(0, Param.x - hdrColor) * bloomColor; | ||
|
|
||
| float3 blendedColor; | ||
| [branch] if (Param.z > 0.5) | ||
| { | ||
| blendedColor = DisplayMapping::HuePreservingHejlBurgessDawson(inputColor, bloomColor); | ||
| } | ||
| else | ||
| { | ||
| float maxCol = Color::RGBToLuminance(inputColor); | ||
| float mappedMax = GetTonemapFactorReinhard(maxCol).x; | ||
| float3 compressedHuePreserving = inputColor * mappedMax / maxCol; | ||
| blendedColor = compressedHuePreserving; | ||
| blendedColor += saturate(Param.x - blendedColor) * bloomColor; | ||
| } | ||
|
|
||
| gameSdrColor = blendedColor; | ||
| hdrColor = pow(abs(hdrColor) / avgValue.x, Cinematic.z) * avgValue.x * sign(hdrColor); | ||
|
|
||
| float blendedLuminance = Color::RGBToLuminance(blendedColor); | ||
|
|
||
| float3 linearColor = Cinematic.w * lerp(lerp(blendedLuminance, blendedColor, Cinematic.x), blendedLuminance * Tint.xyz, Tint.w).xyz; | ||
|
|
||
| linearColor = lerp(avgValue.x, linearColor, Cinematic.z); | ||
|
|
||
| ppColor = max(0, linearColor); | ||
| } | ||
| float hdrLuminance = Color::RGBToLuminance(hdrColor); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix bloom extraction sign and harden against division-by-zero
- Bloom add should use a bright-pass: max(hdrColor - threshold, 0). Current code inverts the sign and boosts darker regions.
- Multiple divisions by avgValue.x risk INF/NaN when avgValue.x ≈ 0.
- hdrColor *= avgValue.y / avgValue.x;
-
- hdrColor += max(0, Param.x - hdrColor) * bloomColor;
-
- hdrColor = pow(abs(hdrColor) / avgValue.x, Cinematic.z) * avgValue.x * sign(hdrColor);
+ // Guard against zero average luminance
+ float avgLum = max(avgValue.x, 1e-4);
+ hdrColor *= avgValue.y / avgLum;
+
+ // Bright-pass bloom: add only energy above threshold
+ hdrColor += max(0, hdrColor - Param.x) * bloomColor;
+
+ hdrColor = pow(abs(hdrColor) / avgLum, Cinematic.z) * avgLum * sign(hdrColor);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hdrColor *= avgValue.y / avgValue.x; | |
| inputColor = max(0, inputColor); | |
| hdrColor += max(0, Param.x - hdrColor) * bloomColor; | |
| float3 blendedColor; | |
| [branch] if (Param.z > 0.5) | |
| { | |
| blendedColor = DisplayMapping::HuePreservingHejlBurgessDawson(inputColor, bloomColor); | |
| } | |
| else | |
| { | |
| float maxCol = Color::RGBToLuminance(inputColor); | |
| float mappedMax = GetTonemapFactorReinhard(maxCol).x; | |
| float3 compressedHuePreserving = inputColor * mappedMax / maxCol; | |
| blendedColor = compressedHuePreserving; | |
| blendedColor += saturate(Param.x - blendedColor) * bloomColor; | |
| } | |
| gameSdrColor = blendedColor; | |
| hdrColor = pow(abs(hdrColor) / avgValue.x, Cinematic.z) * avgValue.x * sign(hdrColor); | |
| float blendedLuminance = Color::RGBToLuminance(blendedColor); | |
| float3 linearColor = Cinematic.w * lerp(lerp(blendedLuminance, blendedColor, Cinematic.x), blendedLuminance * Tint.xyz, Tint.w).xyz; | |
| linearColor = lerp(avgValue.x, linearColor, Cinematic.z); | |
| ppColor = max(0, linearColor); | |
| } | |
| float hdrLuminance = Color::RGBToLuminance(hdrColor); | |
| // Guard against zero average luminance | |
| float avgLum = max(avgValue.x, 1e-4); | |
| hdrColor *= avgValue.y / avgLum; | |
| // Bright-pass bloom: add only energy above threshold | |
| hdrColor += max(0, hdrColor - Param.x) * bloomColor; | |
| hdrColor = pow(abs(hdrColor) / avgLum, Cinematic.z) * avgLum * sign(hdrColor); | |
| float hdrLuminance = Color::RGBToLuminance(hdrColor); |
|
✅ A pre-release build is available for this PR: |
2 participants
Summary by CodeRabbit
New Features
Refactor