fix: misc fixes#1474
Conversation
WalkthroughUpdates adjust format choices and control paths: motion-vector calculations in a shader are now conditionally compiled for DLSS/XESS; several upscaling paths switch from 10/16-bit to 8-bit formats; ScreenSpaceGI preset labels/resolutions are revised; interior detection now keys off sky mode instead of player/cell. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Game as Game Loop
participant Defer as Deferred::DeferredPasses
participant Sky as globals::game::sky
Game->>Defer: Run DeferredPasses()
Defer->>Sky: Query sky pointer and mode
alt Sky exists and mode == kFull
Defer-->>Game: interior = false
else Sky missing or mode != kFull
Defer-->>Game: interior = true
end
sequenceDiagram
autonumber
participant App as Upscaling::Setup/CreateInterop
participant D3D11 as D3D11 Device
participant D3D12 as D3D12 Device
App->>D3D11: Create SRVs/RTVs with RGBA8
Note right of D3D11: NIS sharpening texture\nDXGI_FORMAT_R8G8B8A8_UNORM
App->>D3D12: Create shared resources
App->>D3D11: Wrap interop textures (RGBA8)
Note over App: Previous RGBA10/16 paths removed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Hooks.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
src/Hooks.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Hooks.cpp
🧠 Learnings (1)
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
Applied to files:
src/Hooks.cpp
⏰ 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 (4)
src/Hooks.cpp (4)
860-866: Thunk bypasses the vanilla routine—verify completeness.Since we don’t call the original GeometrySetupConstantPointLights when LightLimitFix is absent, ensure our path writes all expected PS constants (counts, ranges, attenuation, etc.) for parity.
- Compare bound PS constant blocks with and without LightLimitFix across SE/AE/VR (focus on g_NumPointLights and any attenuation constants).
- Smoke-test: interior/exterior, >4 shadow lights, VR left/right eye separation correctness.
805-842: Verify sceneLights indexing (use of i + 1)Code in src/Hooks.cpp (lines 805–842) assumes sceneLights[0] is the directional light and point lights start at index 1; confirm this is guaranteed at runtime. If not, compute/find the first point-light index or skip the directional light to avoid misbinding per-light PS constants.
844-856: Always write ShadowLightMask and avoid unsafe cast.
- Always upload a zeroed mask even when ShadowLightCount == 0 to avoid stale PS constants.
- Use a runtime-checked cast (dynamic_cast or equivalent) before reading shadowLightIndex to avoid undefined behavior.
Location: src/Hooks.cpp:844-856
- // Set shadow light mask select for shadow-casting lights - if (ShadowLightCount > 0) { - std::array<float, 4> shadowLightMaskSelect = { 0.0f, 0.0f, 0.0f, 0.0f }; - for (uint32_t i = 0; i < ShadowLightCount && i < 4; i++) { - if (auto bsLight = Pass->sceneLights[i + 1]) { - auto* shadowLight = static_cast<RE::BSShadowLight*>(bsLight); - GET_INSTANCE_MEMBER(shadowLightIndex, shadowLight); - shadowLightMaskSelect[i] = static_cast<float>(shadowLightIndex); - } - } - shadowState->SetPSConstant(shadowLightMaskSelect, RE::BSGraphics::ConstantGroupLevel::PerGeometry, shadowLightMaskSelectIndex); - } + // Set shadow light mask select for shadow-casting lights (always write) + std::array<float, 4> shadowLightMaskSelect = { 0.f, 0.f, 0.f, 0.f }; + const uint32_t cappedShadowCount = std::min<uint32_t>(ShadowLightCount, 4u); + for (uint32_t i = 0; i < cappedShadowCount; ++i) { + if (auto bsLight = Pass->sceneLights[i + 1]) { + if (auto* shadowLight = dynamic_cast<RE::BSShadowLight*>(bsLight)) { + GET_INSTANCE_MEMBER(shadowLightIndex, shadowLight); + shadowLightMaskSelect[i] = static_cast<float>(shadowLightIndex); + } + } + } + shadowState->SetPSConstant(shadowLightMaskSelect, RE::BSGraphics::ConstantGroupLevel::PerGeometry, shadowLightMaskSelectIndex);Validate that Pass->sceneLights layout guarantees shadow casters are the first ShadowLightCount entries (post-sun), or adjust indexing accordingly.
805-842: Zero out unused light slots to prevent stale constants bleeding between draws.File: src/Hooks.cpp Lines: 805-842
If current geometry has fewer lights than a previous one, old constants persist — clear remaining slots [LightCount..6] for position(s) and color; VR uses two position registers per light.
Apply:
- for (uint32_t i = 0; i < LightCount && i < 7; i++) { + const uint32_t cappedLightCount = std::min<uint32_t>(LightCount, 7u); + for (uint32_t i = 0; i < cappedLightCount; ++i) { ... - } + } + + // Clear remaining light constants to avoid stale data + const std::array<float, 4> zero = { 0.f, 0.f, 0.f, 0.f }; + if (globals::game::isVR) { + for (uint32_t i = cappedLightCount; i < 7u; ++i) { + shadowState->SetPSConstant(zero, RE::BSGraphics::ConstantGroupLevel::PerGeometry, pointLightPositionIndex + i * 2); + shadowState->SetPSConstant(zero, RE::BSGraphics::ConstantGroupLevel::PerGeometry, pointLightPositionIndex + 1 + i * 2); + shadowState->SetPSConstant(zero, RE::BSGraphics::ConstantGroupLevel::PerGeometry, pointLightColorIndex + i); + } + } else { + for (uint32_t i = cappedLightCount; i < 7u; ++i) { + shadowState->SetPSConstant(zero, RE::BSGraphics::ConstantGroupLevel::PerGeometry, pointLightPositionIndex + i); + shadowState->SetPSConstant(zero, RE::BSGraphics::ConstantGroupLevel::PerGeometry, pointLightColorIndex + i); + } + }Confirm visually by toggling nearby lights and verifying removed lights stop affecting subsequent draws.
|
✅ A pre-release build is available for this PR: |
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 (2)
src/Features/Upscaling.cpp (1)
806-813: Avoid CopyResource format mismatches for NIS output.
ApplyNISSharpening()copiesnisSharpenerTextureinto kFRAMEBUFFER viaCopyResource, which requires identical formats. Hard-coding RGBA8 here can fail on setups where kFRAMEBUFFER isn’t RGBA8 (e.g., B8G8R8A8_UNORM, R10G10B10A2_UNORM). Derive the format from the current framebuffer at runtime to keep this resilient.Apply this diff:
D3D11_TEXTURE2D_DESC nisTexDesc = texDesc; -nisTexDesc.Format = DXGI_FORMAT_R8G8B8A8_UNORM; +// Match the NIS scratch texture format to the current framebuffer to ensure CopyResource compatibility. +DXGI_FORMAT fbFormat = DXGI_FORMAT_R8G8B8A8_UNORM; +{ + auto& framebuffer = renderer->GetRuntimeData().renderTargets[RE::RENDER_TARGETS::kFRAMEBUFFER]; + if (framebuffer.RTV) { + winrt::com_ptr<ID3D11Resource> fbRes; + framebuffer.RTV->GetResource(fbRes.put()); + if (fbRes) { + auto fbTex = static_cast<ID3D11Texture2D*>(fbRes.get()); + D3D11_TEXTURE2D_DESC fbDesc{}; + fbTex->GetDesc(&fbDesc); + fbFormat = fbDesc.Format; + } + } +} +nisTexDesc.Format = fbFormat;Please sanity-check on systems where swapchain/framebuffer formats differ from RGBA8 (e.g., 10‑bit paths) to confirm no device removal or copy failures during NIS sharpening.
features/Upscaling/Shaders/Upscaling/EncodeTexturesCS.hlsl (1)
20-21: Optional: strip unused UAV when not building DLSS/XESS.When compiling without DLSS/XESS,
MotionVectorOutputbecomes dead code and is typically stripped, but to avoid any driver warnings and to reduce bound UAV slots, you can also guard the declaration:Apply this diff:
- RWTexture2D<float2> MotionVectorOutput : register(u2); +#if defined(DLSS) || defined(XESS) +RWTexture2D<float2> MotionVectorOutput : register(u2); +#endifAnd in
Upscaling.cpp, only bindu2when the current method is DLSS/XeSS.Also applies to: 71-73
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
features/Upscaling/Shaders/Upscaling/EncodeTexturesCS.hlsl(1 hunks)src/Features/Upscaling.cpp(1 hunks)src/Features/Upscaling/DX12SwapChain.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{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:
src/Features/Upscaling/DX12SwapChain.cppsrc/Features/Upscaling.cppfeatures/Upscaling/Shaders/Upscaling/EncodeTexturesCS.hlsl
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Upscaling/DX12SwapChain.cppsrc/Features/Upscaling.cpp
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh}: Place all feature shaders under features/YourFeature/Shaders/
Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)
Files:
features/Upscaling/Shaders/Upscaling/EncodeTexturesCS.hlsl
⏰ 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 (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (2)
src/Features/Upscaling/DX12SwapChain.cpp (1)
89-91: UI buffer forced to RGBA8 — confirm no downstream assumptions about format.Switching
uiBufferWrappedto RGBA8 should be fine for UI and helps perf, but verify nothing later reads this buffer assuming it matchesswapChainDesc.Format(e.g., any blit/composite path expecting the same format).If any such path exists, gate this to RGBA8 only when the swapchain is RGBA8; otherwise, fall back to
swapChainDesc.Format.features/Upscaling/Shaders/Upscaling/EncodeTexturesCS.hlsl (1)
55-66: Good: narrower ifdef keeps depth-neighborhood TAA mask logic unconditionally active.Moving the
DLSS||XESSguard to only wrap motion‑vector calculations avoids dropping the TAA‑mask min across FSR/non‑MV builds. Looks correct.
doodlum
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Deferred.cpp (1)
411-414: Sky-mode–based interior detection: verify edge cases and consider a safer fallback.Switching to
sky->mode != RE::Sky::Mode::kFullis sensible and avoids player/cell lookups. However, defaulting to interior whenskyis null can misclassify exterior frames during transient states (map/menu transitions, camera swaps, cubemap captures) and may inadvertently disable SKYLIGHTING/IBL in those frames.
- Please sanity‑check on SE/AE/VR: world map open/close, interior cells flagged “has sky,” Blackreach/Sovngarde, and reflection/cubemap rendering.
- If you observe flicker or occasional misclassification, consider a minimal, local hysteresis: keep the last known classification when
skyis unavailable.Optional diff (confines behavior to this site and avoids extra engine calls):
- bool interior = true; - if (auto sky = globals::game::sky) - interior = sky->mode.get() != RE::Sky::Mode::kFull; + static bool lastInterior = true; + bool interior = lastInterior; + if (auto sky = globals::game::sky) + interior = sky->mode.get() != RE::Sky::Mode::kFull; + lastInterior = interior;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Deferred.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
src/Deferred.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Deferred.cpp
🧠 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-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
⏰ 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 (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
Documentation