Skip to content

fix(ssgi): ensure ssgiil values off#2372

Merged
alandtse merged 1 commit into
devfrom
fixssgiilspec
May 18, 2026
Merged

fix(ssgi): ensure ssgiil values off#2372
alandtse merged 1 commit into
devfrom
fixssgiilspec

Conversation

@jiayev
Copy link
Copy Markdown
Collaborator

@jiayev jiayev commented May 18, 2026

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Refined specular illumination processing in deferred rendering to improve visual accuracy and consistency.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 18, 2026

📝 Walkthrough

Walkthrough

The PR adds a conditional guard in the dynamic cubemaps + SSGI specular composition path. Instead of unconditionally converting ssgiIlSpecular from YCoCg to RGB, the shader now checks whether the YCoCg x component is positive before performing the conversion and clamping; otherwise it sets ssgiIlSpecular to zero. The rest of the irradiance accumulation logic remains unchanged.

Changes

SSGI Specular Conditional Guard

Layer / File(s) Summary
SSGI specular irradiance conditional processing
package/Shaders/DeferredCompositeCS.hlsl
Conditional guard added to the YCoCg→RGB reconstruction of ssgiIlSpecular in the dynamic cubemaps + SSGI path: conversion and clamping occur only when ssgiIlSpecular.x > 0.0; otherwise ssgiIlSpecular is zeroed.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related PRs

Suggested reviewers

  • doodlum

Poem

🐰 A specular guard stands tall and true,
checking the YCoCg as shaders do—
when x shines bright, the colors blend,
but zeros wait when lights don't send! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'fix(ssgi): ensure ssgiil values off' is vague and incomplete, using grammatically awkward phrasing that doesn't clearly convey what the fix accomplishes. Revise the title to be more specific and grammatically clear, such as 'fix(ssgi): conditionally process ssgiIlSpecular only when active' or similar wording that better describes the actual change.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fixssgiilspec

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

No actionable suggestions for changed features.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
package/Shaders/DeferredCompositeCS.hlsl (1)

302-306: Run targeted hlslkit validation for SSGI cubemap permutations before merge.

Please validate/compile this shader with at least SSGI + DYNAMIC_CUBEMAPS enabled (and relevant SKYLIGHTING/IBL variants), since this branch-specific guard only executes under those defines. Also, suggested PR title: fix(ssgi): zero specular il when ssgi is off. If there is a tracking bug, add Fixes #<id> in the PR description.

As per coding guidelines "**/*.hlsl: Use hlslkit for shader validation and compilation during targeted development testing and before deployment" and "When reviewing PRs, please provide suggestions for Conventional Commit Titles ... [and] Issue References".

🤖 Prompt for 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.

In `@package/Shaders/DeferredCompositeCS.hlsl` around lines 302 - 306, Run
targeted hlslkit shader validation/compilation for the SSGI + DYNAMIC_CUBEMAPS
permutation (and also SKYLIGHTING/IBL variants) to exercise the branch that uses
ssgiIlSpecular; confirm that when the SSGI path is disabled the code path sets
ssgiIlSpecular to zero and that the Color::YCoCgToRGB/Color::RGBToYCoCg
conversion call remains valid under those defines; if the validation fails,
adjust the branch so ssgiIlSpecular is explicitly zeroed in all compilation
permutations where SSGI may be off and re-run hlslkit until the
SSGI+DYNAMIC_CUBEMAPS builds succeed, then use PR title "fix(ssgi): zero
specular il when ssgi is off" and add a Fixes #<id> reference if applicable.
🤖 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.

Nitpick comments:
In `@package/Shaders/DeferredCompositeCS.hlsl`:
- Around line 302-306: Run targeted hlslkit shader validation/compilation for
the SSGI + DYNAMIC_CUBEMAPS permutation (and also SKYLIGHTING/IBL variants) to
exercise the branch that uses ssgiIlSpecular; confirm that when the SSGI path is
disabled the code path sets ssgiIlSpecular to zero and that the
Color::YCoCgToRGB/Color::RGBToYCoCg conversion call remains valid under those
defines; if the validation fails, adjust the branch so ssgiIlSpecular is
explicitly zeroed in all compilation permutations where SSGI may be off and
re-run hlslkit until the SSGI+DYNAMIC_CUBEMAPS builds succeed, then use PR title
"fix(ssgi): zero specular il when ssgi is off" and add a Fixes #<id> reference
if applicable.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: a9670d5a-9082-4778-9cf5-84efb92a92bf

📥 Commits

Reviewing files that changed from the base of the PR and between 142ed75 and 2581fbc.

📒 Files selected for processing (1)
  • package/Shaders/DeferredCompositeCS.hlsl

@alandtse alandtse changed the title fix(ssgi): make sure ssgiil not producing value when off fix(ssgi): ensure ssgiil values off May 18, 2026
@alandtse alandtse merged commit c22b906 into dev May 18, 2026
16 checks passed
@jiayev jiayev deleted the fixssgiilspec branch May 19, 2026 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants