fix: fix effect shader decals#1186
Conversation
WalkthroughThe changes refactor decal rendering control flow by centralizing "in-world" state management with a global flag, introducing a global vector for deferred blended decal render passes, and consolidating decal rendering logic into a new Changes
Sequence Diagram(s)sequenceDiagram
participant GameEngine
participant Deferred
participant State
participant Hooks
GameEngine->>Deferred: Start world rendering
Deferred->>State: Set inWorld = true
loop Render Passes
GameEngine->>Hooks: RenderPassImmediately
Hooks->>State: Check inWorld
alt Blended Decal Pass
Hooks->>State: Append RenderPass to blendedDecalRenderPasses
else
Hooks->>GameEngine: Call original render pass
end
end
GameEngine->>Deferred: End world rendering
Deferred->>Deferred: RenderBlendedDecals()
Deferred->>State: For each blendedDecalRenderPass, render via hook
Deferred->>State: Clear blendedDecalRenderPasses
Deferred->>State: Set inWorld = false
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/Hooks.cpp (1)
874-876: Note: Inconsistency in hook logic.The second hook includes an additional call to
UpdateRasterStateCullModefor interior sun shadows, while the other hooks don't. This appears intentional based on the feature check, but worth noting for maintainability.Consider adding a comment explaining why only this hook handles interior sun shadows.
src/Deferred.cpp (1)
778-792: Consider defining the depth bias mode as a named constant.The implementation correctly manages the deferred blended decal render passes. However, the magic number
10forrasterStateDepthBiasModeshould be defined as a named constant for better maintainability and clarity.+ static constexpr int BLENDED_DECAL_DEPTH_BIAS_MODE = 10; + void Deferred::RenderBlendedDecals() { if (!globals::state->blendedDecalRenderPasses.empty()) { auto& runtimeData = globals::game::shadowState->GetRuntimeData(); auto runtimeDataCopy = runtimeData; - runtimeData.rasterStateDepthBiasMode = 10; + runtimeData.rasterStateDepthBiasMode = BLENDED_DECAL_DEPTH_BIAS_MODE; for (auto& renderPass : globals::state->blendedDecalRenderPasses) ::Hooks::BSBatchRenderer_RenderPassImmediately1::func(renderPass.a_pass, renderPass.a_technique, renderPass.a_alphaTest, renderPass.a_renderFlags); globals::state->blendedDecalRenderPasses.clear(); runtimeData = runtimeDataCopy; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Deferred.cpp(4 hunks)src/Deferred.h(1 hunks)src/Features/TerrainBlending.cpp(1 hunks)src/Hooks.cpp(1 hunks)src/Hooks.h(1 hunks)src/State.h(2 hunks)src/TruePBR.cpp(0 hunks)
💤 Files with no reviewable changes (1)
- src/TruePBR.cpp
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.777Z
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.
src/Features/TerrainBlending.cpp (2)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.777Z
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: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
src/Deferred.h (3)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: features/Wetness Effects/Shaders/WetnessEffects/WetnessEffects.hlsli:57-61
Timestamp: 2025-06-17T05:40:22.785Z
Learning: Default parameter values are supported in the HLSL compiler used by the skyrim-community-shaders project, contrary to standard HLSL (FXC/DXC) limitations.
src/Hooks.cpp (1)
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.777Z
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.
src/Deferred.cpp (1)
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
src/State.h (3)
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent size, alignment, and cross-platform compatibility when passing data to shader constant buffers.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#577
File: src/Features/WetnessEffects.h:36-36
Timestamp: 2025-06-08T11:25:14.536Z
Learning: In the skyrim-community-shaders project, boolean flags in C++ structs that interface with HLSL shaders use `uint` type instead of `bool` for compatibility reasons. This ensures consistent 4-byte size, proper 16-byte alignment in constant buffers, and cross-platform compatibility when passing data between C++ and HLSL shaders.
🧬 Code Graph Analysis (1)
src/Features/TerrainBlending.cpp (2)
src/Upscaling.h (1)
state(169-185)src/FrameAnnotations.cpp (1)
state(224-231)
⏰ 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). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (12)
src/Features/TerrainBlending.cpp (1)
263-263: LGTM: Global state refactor correctly implemented.The change from
deferred->inWorldtoglobals::state->inWorldaligns with the centralized state management approach described in the PR summary.src/State.h (2)
13-13: LGTM: Include added for RenderPass struct.The include is necessary for the
RenderPassstruct used in theblendedDecalRenderPassesvector.
151-152: LGTM: Global state variables for decal rendering refactor.The new members support the centralized decal rendering approach:
blendedDecalRenderPassesstores deferred render passesinWorldcentralizes world rendering state trackingNote that
RenderPasscontains raw pointers (RE::BSRenderPass*,RE::BSShaderProperty*), so ensure these remain valid until the deferred passes are processed.src/Deferred.h (1)
82-82: LGTM: New method for centralized decal rendering.The
RenderBlendedDecals()method aligns with the refactor to centralize decal rendering logic and process deferred render passes stored in the global state.src/Hooks.h (2)
16-20: LGTM: New hook structure follows established pattern.The
BSBatchRenderer_RenderPassImmediately1hook structure is consistent with other hooks in the file and supports the deferred decal rendering system.
22-28: LGTM: RenderPass struct appropriately designed.The struct encapsulates the necessary parameters for deferred render pass processing, with appropriate member types for the rendering pipeline.
src/Hooks.cpp (1)
858-861: Verify blended decal identification logic in Hooks.cpp (src/Hooks.cpp:858–861)Please confirm that using
a_pass->accumulationHint == 3together with!a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)reliably captures all and only the intended decal passes (both deferred and forward). In particular:
- Ensure that the engine’s accumulation‐hint value 3 indeed represents decal rendering.
- Confirm that checking for absence of the
kZBufferWriteflag correctly differentiates forward blended decals.Consider extracting this into a named helper (e.g.
IsBlendedDecalPass(a_pass)) or referring directly to well‐named enum constants for clarity and future maintenance.src/Deferred.cpp (5)
16-16: LGTM!The addition of the
Hooks.hheader is appropriate given the new usage of hook functions in theRenderBlendedDecals()method.
524-524: LGTM!Correctly updated to use the global
inWorldflag, maintaining the same early return logic while centralizing state management.
759-761: LGTM!Properly manages the global
inWorldstate by setting it before and clearing it after the world rendering phase.
769-772: LGTM!Consistently uses the global
inWorldflag for conditional logic in the deferred rendering path.
800-800: LGTM!The changes correctly integrate the new deferred blended decal rendering approach:
- Uses the global
inWorldflag for consistency- Calls the new
RenderBlendedDecals()method instead of the original function, consolidating the decal rendering logicAlso applies to: 815-815
| // Separate deferred and forward blended decals | ||
| if (globals::state->inWorld && a_pass->accumulationHint == 3 && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) { | ||
| RenderPass call{ a_pass, a_technique, a_alphaTest, a_renderFlags }; | ||
| globals::state->blendedDecalRenderPasses.push_back(call); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Add null pointer check for shader property access.
Same issue as in the other hooks - missing null check for shaderProperty.
Apply this diff to add safety:
// Separate deferred and forward blended decals
-if (globals::state->inWorld && a_pass->accumulationHint == 3 && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) {
+if (globals::state->inWorld && a_pass->accumulationHint == 3 && a_pass->shaderProperty && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) {🤖 Prompt for AI Agents
In src/Hooks.cpp around lines 896 to 901, the code accesses
a_pass->shaderProperty without checking if shaderProperty is null, which can
cause a crash. Add a null pointer check for a_pass->shaderProperty before
accessing its flags to ensure safety. Only proceed with the condition if
shaderProperty is not null.
| // Separate deferred and forward blended decals | ||
| if (globals::state->inWorld && a_pass->accumulationHint == 3 && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) { | ||
| RenderPass call{ a_pass, a_technique, a_alphaTest, a_renderFlags }; | ||
| globals::state->blendedDecalRenderPasses.push_back(call); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Add null pointer check for shader property access.
Same issue as in the first hook - missing null check for shaderProperty.
Apply this diff to add safety:
// Separate deferred and forward blended decals
-if (globals::state->inWorld && a_pass->accumulationHint == 3 && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) {
+if (globals::state->inWorld && a_pass->accumulationHint == 3 && a_pass->shaderProperty && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) {🤖 Prompt for AI Agents
In src/Hooks.cpp around lines 877 to 882, the code accesses
a_pass->shaderProperty without checking if shaderProperty is null, which can
cause a crash. Add a null pointer check for a_pass->shaderProperty before
accessing its flags to ensure safety. Only proceed with the condition if
shaderProperty is not null.
| void BSBatchRenderer_RenderPassImmediately1::thunk(RE::BSRenderPass* a_pass, uint32_t a_technique, bool a_alphaTest, uint32_t a_renderFlags) | ||
| { | ||
| static void thunk(RE::BSRenderPass* pass, uint32_t technique, bool alphaTest, uint32_t renderFlags) | ||
| { | ||
| if (globals::features::lightLimitFix->loaded && !globals::features::lightLimitFix->CheckParticleLights(pass, technique)) | ||
| return; | ||
| if (globals::features::lightLimitFix->loaded && !globals::features::lightLimitFix->CheckParticleLights(a_pass, a_technique)) | ||
| return; | ||
|
|
||
| func(pass, technique, alphaTest, renderFlags); | ||
| // Separate deferred and forward blended decals | ||
| if (globals::state->inWorld && a_pass->accumulationHint == 3 && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) { | ||
| RenderPass call{ a_pass, a_technique, a_alphaTest, a_renderFlags }; | ||
| globals::state->blendedDecalRenderPasses.push_back(call); | ||
| return; | ||
| } | ||
| static inline REL::Relocation<decltype(thunk)> func; | ||
| }; | ||
|
|
||
| func(a_pass, a_technique, a_alphaTest, a_renderFlags); | ||
| } |
There was a problem hiding this comment.
Add null pointer check for shader property access.
The code accesses a_pass->shaderProperty->flags without checking if shaderProperty is null, which could cause crashes.
Apply this diff to add safety:
// Separate deferred and forward blended decals
-if (globals::state->inWorld && a_pass->accumulationHint == 3 && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) {
+if (globals::state->inWorld && a_pass->accumulationHint == 3 && a_pass->shaderProperty && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) {📝 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.
| void BSBatchRenderer_RenderPassImmediately1::thunk(RE::BSRenderPass* a_pass, uint32_t a_technique, bool a_alphaTest, uint32_t a_renderFlags) | |
| { | |
| static void thunk(RE::BSRenderPass* pass, uint32_t technique, bool alphaTest, uint32_t renderFlags) | |
| { | |
| if (globals::features::lightLimitFix->loaded && !globals::features::lightLimitFix->CheckParticleLights(pass, technique)) | |
| return; | |
| if (globals::features::lightLimitFix->loaded && !globals::features::lightLimitFix->CheckParticleLights(a_pass, a_technique)) | |
| return; | |
| func(pass, technique, alphaTest, renderFlags); | |
| // Separate deferred and forward blended decals | |
| if (globals::state->inWorld && a_pass->accumulationHint == 3 && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) { | |
| RenderPass call{ a_pass, a_technique, a_alphaTest, a_renderFlags }; | |
| globals::state->blendedDecalRenderPasses.push_back(call); | |
| return; | |
| } | |
| static inline REL::Relocation<decltype(thunk)> func; | |
| }; | |
| func(a_pass, a_technique, a_alphaTest, a_renderFlags); | |
| } | |
| void BSBatchRenderer_RenderPassImmediately1::thunk(RE::BSRenderPass* a_pass, uint32_t a_technique, bool a_alphaTest, uint32_t a_renderFlags) | |
| { | |
| if (globals::features::lightLimitFix->loaded && !globals::features::lightLimitFix->CheckParticleLights(a_pass, a_technique)) | |
| return; | |
| // Separate deferred and forward blended decals | |
| if (globals::state->inWorld | |
| && a_pass->accumulationHint == 3 | |
| && a_pass->shaderProperty | |
| && !a_pass->shaderProperty->flags.all(RE::BSShaderProperty::EShaderPropertyFlag::kZBufferWrite)) | |
| { | |
| RenderPass call{ a_pass, a_technique, a_alphaTest, a_renderFlags }; | |
| globals::state->blendedDecalRenderPasses.push_back(call); | |
| return; | |
| } | |
| func(a_pass, a_technique, a_alphaTest, a_renderFlags); | |
| } |
🤖 Prompt for AI Agents
In src/Hooks.cpp around lines 852 to 865, the code accesses
a_pass->shaderProperty->flags without verifying if shaderProperty is null,
risking a crash. Add a null pointer check for a_pass->shaderProperty before
accessing its flags to ensure safety. Modify the condition to first confirm
a_pass->shaderProperty is not null, then proceed with the existing flag checks.
|
✅ A pre-release build is available for this PR: |
davo0411
left a comment
davo0411
left a comment
There was a problem hiding this comment.
Looks good to me. good work
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Deferred.cpp (1)
778-805: Consider refactoring to reduce code duplication.The
RenderBlendedDecals()method correctly handles deferred blended decal rendering with proper state management and cleanup. However, the VR and non-VR paths have significant code duplication.Consider extracting the common logic into a helper function:
+void Deferred::RenderBlendedDecalsImpl(auto& runtimeData) { + auto runtimeDataCopy = runtimeData; + runtimeData.rasterStateDepthBiasMode = 10; + + for (auto& renderPass : globals::state->blendedDecalRenderPasses) + ::Hooks::BSBatchRenderer_RenderPassImmediately1::func(renderPass.a_pass, renderPass.a_technique, renderPass.a_alphaTest, renderPass.a_renderFlags); + + globals::state->blendedDecalRenderPasses.clear(); + + runtimeData = runtimeDataCopy; +} + void Deferred::RenderBlendedDecals() { if (!globals::state->blendedDecalRenderPasses.empty()) { if (globals::game::isVR) { - auto& runtimeData = globals::game::shadowState->GetVRRuntimeData(); - auto runtimeDataCopy = runtimeData; - runtimeData.rasterStateDepthBiasMode = 10; - - for (auto& renderPass : globals::state->blendedDecalRenderPasses) - ::Hooks::BSBatchRenderer_RenderPassImmediately1::func(renderPass.a_pass, renderPass.a_technique, renderPass.a_alphaTest, renderPass.a_renderFlags); - - globals::state->blendedDecalRenderPasses.clear(); - - runtimeData = runtimeDataCopy; + RenderBlendedDecalsImpl(globals::game::shadowState->GetVRRuntimeData()); } else { - auto& runtimeData = globals::game::shadowState->GetRuntimeData(); - auto runtimeDataCopy = runtimeData; - runtimeData.rasterStateDepthBiasMode = 10; - - for (auto& renderPass : globals::state->blendedDecalRenderPasses) - ::Hooks::BSBatchRenderer_RenderPassImmediately1::func(renderPass.a_pass, renderPass.a_technique, renderPass.a_alphaTest, renderPass.a_renderFlags); - - globals::state->blendedDecalRenderPasses.clear(); - - runtimeData = runtimeDataCopy; + RenderBlendedDecalsImpl(globals::game::shadowState->GetRuntimeData()); } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Deferred.cpp(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.777Z
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.
src/Deferred.cpp (1)
Learnt from: davo0411
PR: doodlum/skyrim-community-shaders#1070
File: src/State.cpp:79-83
Timestamp: 2025-05-30T11:44:15.542Z
Learning: In the Skyrim Community Shaders project, the `smoothDrawCalls` array in the `State` class is declared as type `double` in `src/State.h`, which is the correct floating-point type for performing exponential moving average smoothing calculations.
⏰ 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). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (6)
src/Deferred.cpp (6)
16-16: LGTM: Include addition aligns with new functionality.The new include for
Hooks.his necessary for theRenderBlendedDecals()method that uses hook functions.
524-524: LGTM: State centralization is consistent.The change from
deferred->inWorldtoglobals::state->inWorldaligns with the PR objective of centralizing "in-world" state management.
759-761: LGTM: Proper global state management.The global state is correctly set to
trueat the start of world rendering and reset tofalseat the end, ensuring proper state lifecycle management.
769-769: LGTM: Consistent with global state refactoring.The change maintains consistency with the centralized state management approach throughout the codebase.
813-813: LGTM: Consistent state management.The change to use
globals::state->inWorldmaintains consistency with the global state refactoring.
828-828: LGTM: Integration of new decal rendering method.The call to
RenderBlendedDecals()properly integrates the new centralized decal rendering logic, replacing the previous implementation.
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
2 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes