fix(deferred-composite): use linear space for math#2064
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors SampleSSGISpecular to convert environment and reflection samples to linear irradiance earlier, compute luminance from linear ambient, adjust DALC and non‑DALC IBL specular flows to operate in linear space, and remove redundant gamma/linear conversions across skylighting and fallback paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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. |
![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
🤖 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/DeferredCompositeCS.hlsl`:
- Around line 220-223: The code normalizes a gamma-space cubemap sample by a
luminance computed after linearization; update the sequence so
EnvTexture.SampleLevel results (envSample and fullSample) are converted with
Color::IrradianceToLinear before computing envLum and before the division for
envSpecular and skySpecular. Specifically, call Color::IrradianceToLinear on the
sampled colors from EnvTexture.SampleLevel, compute envLum =
Color::RGBToLuminance(linearEnvSample), then compute envSpecular by dividing the
linear envSample by max(envLum, 0.001) and multiplying by
directionalAmbientColorSpecular and SharedData::iblSettings.DALCAmount, and
compute skySpecular using the linearized fullSample - envSample multiplied by
SharedData::iblSettings.SkyIBLScale.
🪄 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: 3bc953b5-e330-4e67-8517-dea89034d448
📒 Files selected for processing (1)
package/Shaders/DeferredCompositeCS.hlsl
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
package/Shaders/DeferredCompositeCS.hlsl (2)
254-301: Consider extracting normalized cubemap sampling into a helper.The fallback branches repeat the same sample-linearize-luminance-normalize pattern. A small helper would reduce duplication and lower drift risk in future edits.
♻️ Proposed refactor
+float3 SampleDALCNormalized(TextureCube<float3> tex, float3 dir, float level, float dalcScale) +{ + float3 s = Color::IrradianceToLinear(tex.SampleLevel(LinearSampler, dir, level)); + float lum = Color::RGBToLuminance(Color::IrradianceToLinear(tex.SampleLevel(LinearSampler, dir, 15))); + return s / max(lum, 0.001) * dalcScale; +} + ... - float3 specularIrradiance = Color::IrradianceToLinear( - EnvTexture.SampleLevel(LinearSampler, R, level) - ); - float specularIrradianceLuminance = Color::RGBToLuminance( - Color::IrradianceToLinear( - EnvTexture.SampleLevel(LinearSampler, R, 15) - ) - ); - finalIrradiance = specularIrradiance / max(specularIrradianceLuminance, 0.001) * directionalAmbientColorSpecular; + finalIrradiance = SampleDALCNormalized(EnvTexture, R, level, directionalAmbientColorSpecular);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/DeferredCompositeCS.hlsl` around lines 254 - 301, The code repeats a sample->linearize->luminance->normalize pattern for cubemap lookups (seen around EnvTexture/EnvReflectionsTexture, Color::IrradianceToLinear and Color::RGBToLuminance, and use of directionalAmbientColorSpecular/skylightingSpecular), so extract that logic into a small helper (e.g., SampleAndNormalizeIrradiance or NormalizeCubemapIrradiance) that takes a texture, sampler, direction R and level and returns the normalized irradiance and/or its luminance; then replace the duplicated blocks in the INTERIOR, SKYLIGHTING and default branches to call the helper and apply the dalc scaling and lerp with skylightingSpecular.
1-1: PR metadata: add an issue-closing reference if applicable.If this fix maps to a tracked issue, add
Fixes #<id>orCloses #<id>in the PR description for traceability.As per coding guidelines: “Issue References… Suggest adding appropriate GitHub keywords: ‘Fixes
#123’ or ‘Closes#123’ for bug fixes.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/DeferredCompositeCS.hlsl` at line 1, Update the PR description to include an issue-closing reference (e.g., "Fixes `#123`" or "Closes `#123`") if this change addresses a tracked bug or ticket; mention the relevant file DeferredCompositeCS.hlsl in the PR body for traceability and add the appropriate GitHub keyword so the issue is automatically closed when the PR is merged.
🤖 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/DeferredCompositeCS.hlsl`:
- Around line 254-301: The code repeats a
sample->linearize->luminance->normalize pattern for cubemap lookups (seen around
EnvTexture/EnvReflectionsTexture, Color::IrradianceToLinear and
Color::RGBToLuminance, and use of
directionalAmbientColorSpecular/skylightingSpecular), so extract that logic into
a small helper (e.g., SampleAndNormalizeIrradiance or
NormalizeCubemapIrradiance) that takes a texture, sampler, direction R and level
and returns the normalized irradiance and/or its luminance; then replace the
duplicated blocks in the INTERIOR, SKYLIGHTING and default branches to call the
helper and apply the dalc scaling and lerp with skylightingSpecular.
- Line 1: Update the PR description to include an issue-closing reference (e.g.,
"Fixes `#123`" or "Closes `#123`") if this change addresses a tracked bug or ticket;
mention the relevant file DeferredCompositeCS.hlsl in the PR body for
traceability and add the appropriate GitHub keyword so the issue is
automatically closed when the PR is merged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ec48b5cd-175f-481a-b08f-29845744b8e6
📒 Files selected for processing (1)
package/Shaders/DeferredCompositeCS.hlsl
|
✅ A pre-release build is available for this PR: |
1 participant
Summary by CodeRabbit