Skip to content

fix: fix effect blending#1490

Merged
doodlum merged 1 commit into
devfrom
burley-fix-2
Sep 18, 2025
Merged

fix: fix effect blending#1490
doodlum merged 1 commit into
devfrom
burley-fix-2

Conversation

@doodlum
Copy link
Copy Markdown
Collaborator

@doodlum doodlum commented Sep 18, 2025

Summary by CodeRabbit

  • New Features
    • Improved deferred rendering for multi-blend materials/decals, enabling an additional output channel to enhance visual fidelity in complex surfaces.
  • Bug Fixes
    • Resolved a rendering issue that could occur in non–multi-blend configurations by preventing unintended writes to an unavailable output, reducing potential artifacts and stability problems.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Sep 18, 2025

Walkthrough

The shader PS_OUTPUT struct in package/Shaders/Effect.hlsl was modified to conditionally include an Albedo render target in deferred mode when MULTBLEND or MULTBLEND_DECAL are defined. Corresponding write logic was removed from non-MULTBLEND paths to avoid referencing an undeclared field.

Changes

Cohort / File(s) Summary
Deferred G-Buffer outputs
package/Shaders/Effect.hlsl
Added conditional member float4 Albedo : SV_Target3; within #ifdef DEFERRED when MULTBLEND or MULTBLEND_DECAL are defined. Removed Albedo writes from non-MULTBLEND code path in PS main to match conditional declaration.

Sequence Diagram(s)

sequenceDiagram
    participant Engine
    participant Compiler as HLSL Compiler
    participant PS as Pixel Shader (PS_OUTPUT)

    Engine->>Compiler: Compile Effect.hlsl with defines
    alt DEFERRED && (MULTBLEND || MULTBLEND_DECAL)
        note over PS: PS_OUTPUT includes Albedo (SV_Target3)
        Engine->>PS: Execute fragment
        rect rgba(200,230,255,0.3)
        PS->>Engine: Outputs NormalGlossiness, Albedo, Specular, ...
        end
    else
        note over PS: PS_OUTPUT excludes Albedo
        Engine->>PS: Execute fragment
        rect rgba(255,230,200,0.3)
        PS->>Engine: Outputs NormalGlossiness, Specular, ... (no Albedo)
        end
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • alandtse

Poem

A whisk of light on render night,
I blend my hues with gentle might—
When flags are set, Albedo gleams,
Else, I hop past deferred streams.
Carrots compiled, targets aligned,
A shader bunny, color-defined. 🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix: fix effect blending" correctly indicates the intent to repair blending-related shader behavior and matches the changes that add a conditional Albedo SV_Target3 and prevent writing it when not declared; therefore it is directly related to the changeset. The phrasing is redundant ("fix: fix") and not as specific as it could be about the DEFERRED/MULTBLEND context, but it still conveys the primary purpose. Overall the title is acceptable for reviewers scanning history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch burley-fix-2

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

Using provided base ref: 3f462fa
Using base ref: 3f462fa
Base commit date: 2025-09-18T00:36:38+01:00 (Thursday, September 18, 2025 12:36 AM)
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.

Actionable comments posted: 0

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

858-863: Confirm intent: MULTBLEND path mirrors Diffuse into Specular/Reflectance/Masks.

If downstream only needs Albedo for effect blending, consider keeping Specular/Reflectance/Masks neutral (zeros) to reduce bandwidth and avoid contaminating G‑buffer channels. If mirroring is required, a short comment here would prevent future regressions.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f462fa and b767f8f.

📒 Files selected for processing (1)
  • package/Shaders/Effect.hlsl (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Do not include TODO/FIXME placeholders; provide complete, working solutions

Files:

  • package/Shaders/Effect.hlsl
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
  • GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
  • GitHub Check: Build plugin and addons
🔇 Additional comments (1)
package/Shaders/Effect.hlsl (1)

430-432: Verify RTV slot 3 is bound for MULTBLEND permutations

Automated sweep failed (ripgrep: "unrecognized file type: hlsl"); cannot confirm bindings. Confirm that when MULTBLEND or MULTBLEND_DECAL + DEFERRED are compiled the render pass binds an RTV at SV_Target3 and that Albedo's color space (linear vs sRGB) matches the compositor.

File: package/Shaders/Effect.hlsl lines 430-432

Run these locally to verify:

# 1) find Albedo declarations using SV_Target3
rg -nP -C2 --glob '**/*.hlsl' '\bAlbedo\s*:\s*SV_Target3\b' || true

# 2) locate engine code that binds multiple RTVs (inspect results for >=4 RTVs)
rg -nP -C3 --glob '**/*.{cpp,c,h}' '(OMSetRenderTargets\(|OMSetRenderTargetsAndUnorderedAccessViews\(|SetRenderTargets\(|CreateRenderTargetView\(|BindRenderTargets)' || true

# 3) search for engine-side Albedo attachment names or color-space hints
rg -nP -C2 --glob '**/*.{cpp,hlsl,h}' '\bAlbedo\b.*(RTV|RenderTarget|Attachment|SV_Target3|sRGB|SRGB|gamma|linear)' || true

@doodlum doodlum merged commit 9dcff08 into dev Sep 18, 2025
15 checks passed
@github-actions
Copy link
Copy Markdown

✅ A pre-release build is available for this PR:
Download

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.

2 participants