Conversation
|
No actionable suggestions for changed features. |
📝 WalkthroughWalkthroughReplaces the in-loop bright-pass with Karis-weighted 2×2 sampling and a soft‑knee bright-pass, moves avg luminance computation outside the blur loop, adds a new Color helper and Math include, and updates tests for the Karis-weighted average. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
@copilot can you please review the following comments and address if valid?
use the bloom firefly pass from jiaye pp
this is not really doing anything correct
you need to check other pixels basically, which you can do in the first downscale pass yeah
https://github.com/jiayev/skyrim-community-shaders/blob/pp/features/Post%20Processing/Shaders/PostProcessing/common.hlsli
DownsampleCODFirstMip if that's what u mean
![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/ISBlur.hlsl`:
- Around line 103-106: The bright-pass branch bypasses GetImageColor so
BlurScale.y is not applied, changing HDR cutoff and bloom energy; restore
BlurScale.y influence by either (A) using BlurScale.y when computing sampleUV
(e.g., apply dynamic-resolution adjustment that incorporates BlurScale.y before
calling SampleKarisFireflySuppress) or (B, preferred if vertical blur should
affect Karis weights) extend SampleKarisFireflySuppress to accept BlurScale.y
and apply it per tap inside SampleKarisFireflySuppress (instead of averaging
then scaling) so ApplyBrightPass receives the correctly scaled HDR sample;
update the call site that currently passes sampleUV to pass the BlurScale.y
parameter if you choose option B.
🪄 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: c7dec34b-4c03-486e-bb34-ca364d15ab18
📒 Files selected for processing (1)
package/Shaders/ISBlur.hlsl
| { | ||
| float2 sampleUV = (BlurScale.x < 0.5) ? FrameBuffer::GetDynamicResolutionAdjustedScreenPosition(screenPosition) : screenPosition; | ||
| imageColor.rgb = ApplyBrightPass(SampleKarisFireflySuppress(sampleUV, texelSize)); | ||
| } |
There was a problem hiding this comment.
Keep blurScale.y in the BRIGHTPASS sample path.
This branch no longer goes through GetImageColor, so any non-1 blurScale.y stops affecting the HDR sample before bright-pass. That changes the cutoff and bloom energy relative to the previous path in COLORRANGE builds.
Suggested fix
{
float2 sampleUV = (BlurScale.x < 0.5) ? FrameBuffer::GetDynamicResolutionAdjustedScreenPosition(screenPosition) : screenPosition;
- imageColor.rgb = ApplyBrightPass(SampleKarisFireflySuppress(sampleUV, texelSize));
+ float3 hdrColor = SampleKarisFireflySuppress(sampleUV, texelSize) * blurScale.y;
+ imageColor.rgb = ApplyBrightPass(hdrColor);
}If blurScale.y is supposed to influence the Karis weights too, thread it into SampleKarisFireflySuppress and apply it per tap instead of after the average.
📝 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.
| { | |
| float2 sampleUV = (BlurScale.x < 0.5) ? FrameBuffer::GetDynamicResolutionAdjustedScreenPosition(screenPosition) : screenPosition; | |
| imageColor.rgb = ApplyBrightPass(SampleKarisFireflySuppress(sampleUV, texelSize)); | |
| } | |
| { | |
| float2 sampleUV = (BlurScale.x < 0.5) ? FrameBuffer::GetDynamicResolutionAdjustedScreenPosition(screenPosition) : screenPosition; | |
| float3 hdrColor = SampleKarisFireflySuppress(sampleUV, texelSize) * blurScale.y; | |
| imageColor.rgb = ApplyBrightPass(hdrColor); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package/Shaders/ISBlur.hlsl` around lines 103 - 106, The bright-pass branch
bypasses GetImageColor so BlurScale.y is not applied, changing HDR cutoff and
bloom energy; restore BlurScale.y influence by either (A) using BlurScale.y when
computing sampleUV (e.g., apply dynamic-resolution adjustment that incorporates
BlurScale.y before calling SampleKarisFireflySuppress) or (B, preferred if
vertical blur should affect Karis weights) extend SampleKarisFireflySuppress to
accept BlurScale.y and apply it per tap inside SampleKarisFireflySuppress
(instead of averaging then scaling) so ApplyBrightPass receives the correctly
scaled HDR sample; update the call site that currently passes sampleUV to pass
the BlurScale.y parameter if you choose option B.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
package/Shaders/ISBlur.hlsl (1)
89-93:⚠️ Potential issue | 🟠 MajorRestore
blurScale.ybeforeApplyBrightPass().Line 92 still feeds unscaled HDR into the bright-pass threshold, while the non-
BRIGHTPASSpath appliesblurScale.yinGetImageColor(). WhenblurScale.y != 1, that changes both the cutoff and bloom energy.Suggested fix
{ float2 sampleUV = (BlurScale.x < 0.5) ? FrameBuffer::GetDynamicResolutionAdjustedScreenPosition(screenPosition) : screenPosition; - imageColor.rgb = ApplyBrightPass(SampleKarisFireflySuppress(sampleUV, texelSize)); + float3 hdrColor = SampleKarisFireflySuppress(sampleUV, texelSize) * blurScale.y; + imageColor.rgb = ApplyBrightPass(hdrColor); }If
blurScale.yis also supposed to influence the Karis weights, pass it intoSampleKarisFireflySuppress()and scale each tap there instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/ISBlur.hlsl` around lines 89 - 93, The BRIGHTPASS branch is applying ApplyBrightPass to HDR color sampled without restoring blurScale.y, so when blurScale.y != 1 the bright-pass cutoff and bloom energy are wrong; in the BRIGHTPASS block restore or apply blurScale.y to the sampled color before calling ApplyBrightPass (mirroring the non-BRIGHTPASS GetImageColor path), or alternatively modify SampleKarisFireflySuppress to accept blurScale.y and scale each Karis tap there; update the call in the BRIGHTPASS branch (SampleKarisFireflySuppress(...)) and/or scale imageColor by blurScale.y prior to ApplyBrightPass so behavior matches non-BRIGHTPASS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@package/Shaders/ISBlur.hlsl`:
- Around line 89-93: The BRIGHTPASS branch is applying ApplyBrightPass to HDR
color sampled without restoring blurScale.y, so when blurScale.y != 1 the
bright-pass cutoff and bloom energy are wrong; in the BRIGHTPASS block restore
or apply blurScale.y to the sampled color before calling ApplyBrightPass
(mirroring the non-BRIGHTPASS GetImageColor path), or alternatively modify
SampleKarisFireflySuppress to accept blurScale.y and scale each Karis tap there;
update the call in the BRIGHTPASS branch (SampleKarisFireflySuppress(...))
and/or scale imageColor by blurScale.y prior to ApplyBrightPass so behavior
matches non-BRIGHTPASS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9818b545-942c-40c9-ac22-00fac65f3216
📒 Files selected for processing (3)
package/Shaders/Common/Color.hlslipackage/Shaders/ISBlur.hlslpackage/Shaders/Tests/TestColor.hlsl
| return Color::KarisWeightedAverage(s0, s1, s2, s3); | ||
| } | ||
|
|
||
| float3 ApplyBrightPass(float3 hdrColor) |
There was a problem hiding this comment.
shouldn't be needed now. probably isn't very effective
There was a problem hiding this comment.
I don't understand what you're saying.
2 participants
Meant to address this issue:
Summary by CodeRabbit