Conversation
📝 WalkthroughWalkthroughThis PR inverts the conditional gamma conversion in the HDR shader path and tunes default lighting multiplier values in the Linear Lighting settings struct. The changes affect HDR color output when ENABLE_LL is active and adjust baseline diffuse, ambient, and glow lighting constants. ChangesLinear Lighting Adjustments
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.20.0)OpenGrep fatal error (exit code 2): [00.25][ERROR]: Error: exception Unix_error: No such file or directory stat src/Features/LinearLighting.h 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 |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@package/Shaders/ISHDR.hlsl`:
- Around line 219-221: The current SDR path may gamma-encode twice because when
ENABLE_LL is true you call Color::LinearToGammaSafe(outputColor) then
FrameBuffer::ToSRGBColor(outputColor) which uses FrameParams.x ("inverse
fGamma"); verify and fix by ensuring only one transfer is applied: either remove
the LinearToGammaSafe call when FrameBuffer::ToSRGBColor already performs the
SDR gamma (or change ToSRGBColor to be a no-op for already-encoded input), or
gate ToSRGBColor behind !ENABLE_LL. Update the logic around outputColor,
referencing ENABLE_LL, Color::LinearToGammaSafe, FrameBuffer::ToSRGBColor and
FrameParams.x to ensure SDR path applies a single gamma conversion consistent
with the HDR (!ENABLE_LL) logic.
🪄 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 Plus
Run ID: 57f0bba5-ecec-42b0-ad8e-0554fd052aee
📒 Files selected for processing (2)
package/Shaders/ISHDR.hlslsrc/Features/LinearLighting.h
|
✅ A pre-release build is available for this PR: |
This pull request introduces adjustments to the Linear Lighting feature's multipliers and corrects color space conversion handling in the shader pipeline. These changes aim to improve lighting accuracy and ensure proper color output when linear lighting is enabled.
Linear Lighting configuration adjustments:
vanillaDiffuseColorMultto 1.0 (was 1.5), reducing diffuse color intensity for more accurate lighting.ambientMultto 1.0 (was 0.67), increasing ambient light contribution.glowmapMultto 0.66 (was 0.5), increasing the glowmap effect.Shader pipeline improvements:
PS_OUTPUT main(PS_INPUT input)inISHDR.hlsl, added a conditional step to applyColor::LinearToGammaSafeconversion whenENABLE_LLis enabled, ensuring correct gamma correction for linear lighting.Summary by CodeRabbit
Bug Fixes
Improvements