fix(IBL): glowing metals in shadow#2088
Draft
davo0411 wants to merge 1 commit into
Draft
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR applies the computed Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adjusts IBL + skylighting specular composition so the environment-specular contribution is attenuated by skylighting, preventing overly bright (“glowing”) metallic reflections in shadowed areas.
Changes:
- Apply
skylightingSpecularmultiplier toenvSpecularin the IBL path (in addition toskySpecular). - Mirror the same fix in both the deferred composite shader and the dynamic cubemap shader include to keep behavior consistent across paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
package/Shaders/DeferredCompositeCS.hlsl |
Multiplies IBL envSpecular by skylightingSpecular in both DALC mode 2 and ratio-based modes under SKYLIGHTING. |
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli |
Applies the same envSpecular *= skylightingSpecular correction in the dynamic cubemap IBL calculations (both relevant code paths). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli (1)
76-76: PR metadata follow-up: use a stricter Conventional Commit title + issue keyword.Suggested title:
fix(ibl): apply skylighting to env specular
If there’s a tracked bug, add:Closes #<issue_number>in the PR description.As per coding guidelines, PRs should include Conventional Commit titles and issue references when applicable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Dynamic` Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli at line 76, Update the PR title to follow Conventional Commits by using a stricter title like "fix(ibl): apply skylighting to env specular" (referencing the change to envSpecular *= skylightingSpecular;) and, if this change addresses a tracked bug, add "Closes #<issue_number>" to the PR description so the issue is automatically linked and closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@features/Dynamic` Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlsli:
- Line 76: Update the PR title to follow Conventional Commits by using a
stricter title like "fix(ibl): apply skylighting to env specular" (referencing
the change to envSpecular *= skylightingSpecular;) and, if this change addresses
a tracked bug, add "Closes #<issue_number>" to the PR description so the issue
is automatically linked and closed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aae4340b-ffde-495e-9817-cdf5f05d9bcf
📒 Files selected for processing (2)
features/Dynamic Cubemaps/Shaders/DynamicCubemaps/DynamicCubemaps.hlslipackage/Shaders/DeferredCompositeCS.hlsl
doodlum
approved these changes
Apr 10, 2026
|
✅ A pre-release build is available for this PR: |
Collaborator
|
Wow if we needed DRY this is definitely one of those parts. @coderabbitai identify a minimal change to DRY this code in a maintainable way. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. The PR description images show:
What’s duplicated
Minimal change
Proposed code
// Utility: apply skylighting visibility uniformly to IBL specular terms.
void applySkylightingSpecular(inout float3 envSpecular, inout float3 skySpecular, float skylightingSpecular)
{
envSpecular *= skylightingSpecular;
skySpecular *= skylightingSpecular;
}
`#if` defined(SKYLIGHTING)
skySpecular *= skylightingSpecular;
envSpecular *= skylightingSpecular;
`#endif`with: `#if` defined(SKYLIGHTING)
Skylighting::applySkylightingSpecular(envSpecular, skySpecular, skylightingSpecular);
`#endif`(Do this in both DALCMode branches of GetDynamicCubemapSpecularIrradiance(...) and GetDynamicCubemap(...).) (github.com)
`#if` defined(SKYLIGHTING)
Skylighting::applySkylightingSpecular(envSpecular, skySpecular, skylightingSpecular);
`#elif` defined(INTERIOR)
skySpecular = 0;
`#endif`(Apply in both branches where it appears.) (github.com) Why this is minimal and safe
If you want, I can prep a tiny follow‑up PR with these edits. 🧠 Learnings used |
Collaborator
|
@coderabbitai if we allow a slightly larger refactor, is there a better way to handle this then the helper you proposed? We want to support maintainability in a sensible way. |
Contributor
|
Tip For best results, initiate chat on the files or code changes. The PR description images show:
Why this shape
Proposed code
namespace IBL }
include "IBL/IBL.hlsli"include "IBL/SpecularCompose.hlsli"Then replace each duplicated composition block with: (Apply in both DALCMode branches inside GetDynamicCubemapSpecularIrradiance(...) and GetDynamicCubemap(...).) (github.com)
include "IBL/IBL.hlsli"include "IBL/SpecularCompose.hlsli"Replace both composition blocks with: This preserves the existing INTERIOR sky=0 behavior through the helper’s macro guard. (github.com) Optional extra DRY (same file)
Why this is a sensible step up from a tiny helper
If you like this direction, I can sketch a tiny follow‑up patch with these exact edits. (github.com) Learnings referenced- Non‑IBL skylighting fallback double‑weighting is intentional; the refactor leaves that path untouched. (Saved learnings from PR `#1947`.)🧠 Learnings used |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
adds a envspecular mult to the skylighting calculations with ibl. previously this was missing causing glowing metals.
Summary by CodeRabbit
Release Notes