fix: misc fixes#1480
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis PR removes NVIDIA Image Scaling (NIS) shaders, configs, and runtime; deletes NIS sharpening UI/serialization; forces DLSS preset K; enables FidelityFX sharpening flag; adjusts frame-limiter timing; changes volumetric/reflection interior detection; adds FXAA/IBLF disable hooks; and makes Load() always return true. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Loader
participant Hooks
participant FXAA_Init as BSImageSpace_Init_FXAA
participant IBLF_Init as BSImageSpace_Init_IBLF
Loader->>Hooks: Install detours (non‑VR)
Hooks->>FXAA_Init: Detour FXAA init
Hooks->>IBLF_Init: Detour IBLF init
Note right of FXAA_Init: FXAA flag forced off, then call original
Loader->>FXAA_Init: Initialize FXAA
FXAA_Init-->>Loader: Forward to original
Note right of IBLF_Init: IBLF flag forced false, then call original
Loader->>IBLF_Init: Initialize IBLF
IBLF_Init-->>Loader: Forward to original
sequenceDiagram
autonumber
participant RenderLoop
participant Upscaling
participant Streamline
participant FFX as FidelityFX
RenderLoop->>Upscaling: Upscale()
Upscaling->>Streamline: Upscale(..., preset=K)
Streamline-->>Upscaling: Upscaled frame
alt FFX path active
Upscaling->>FFX: DispatchUpscale(enableSharpening=true)
FFX-->>Upscaling: Output
end
Note over Upscaling: NIS sharpening path removed
sequenceDiagram
autonumber
participant DXSwap as DX12SwapChain::Present
participant Limiter as FrameLimiter
DXSwap->>DXSwap: Present(SyncInterval)
alt SyncInterval == 0 (VSync off)
DXSwap->>Limiter: FrameLimiter()
else VSync on
Note over DXSwap: Skip limiter call
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
✅ A pre-release build is available for this PR: |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/Upscaling.cpp (1)
798-812: Leftover NIS sharpener allocation — dead resource and memory waste.This creates
nisSharpenerTexturebut NIS paths were removed. It’s never used nor destroyed.Apply:
- // Create NIS sharpener texture with swapchain format and UAV access - D3D11_TEXTURE2D_DESC nisTexDesc = texDesc; - nisTexDesc.Format = DXGI_FORMAT_R8G8B8A8_UNORM; - nisTexDesc.BindFlags = D3D11_BIND_SHADER_RESOURCE | D3D11_BIND_UNORDERED_ACCESS; - - D3D11_SHADER_RESOURCE_VIEW_DESC nisSrvDesc = srvDesc; - nisSrvDesc.Format = nisTexDesc.Format; - - D3D11_UNORDERED_ACCESS_VIEW_DESC nisUavDesc = uavDesc; - nisUavDesc.Format = nisTexDesc.Format; - - nisSharpenerTexture = new Texture2D(nisTexDesc); - nisSharpenerTexture->CreateSRV(nisSrvDesc); - nisSharpenerTexture->CreateUAV(nisUavDesc);Also remove the
nisSharpenerTexturemember fromUpscaling.hto avoid drift.
🧹 Nitpick comments (10)
src/Features/VolumetricLighting.cpp (1)
202-206: Protect Min1 fields against underflow; keep them consistent with clamped dims.If
width/heightever hit 0,width - 1underflows. After clamping to ≥1 above, reflect that here.- vlData.screenX = width; - vlData.screenY = height; - vlData.screenXMin1 = width - 1; - vlData.screenYMin1 = height - 1; + vlData.screenX = width; + vlData.screenY = height; + vlData.screenXMin1 = width > 0 ? width - 1 : 0; + vlData.screenYMin1 = height > 0 ? height - 1 : 0;src/State.cpp (1)
129-134: Reflections shader deactivation looks fine; consider setting once instead of every Reset.Setting
BSImagespaceShaderApplyReflections->active = falseeachReset()is harmless but redundant. If you want to avoid touching it every frame, gate it behind a one‑time apply (static flag) or move it toSetup()after verifying the instance is available.src/Features/Upscaling/FidelityFX.cpp (1)
290-292: Enable sharpening only when nonzero and wire to a settingCurrently the pass runs with sharpness 0.0f (cost without effect). Gate by a user-configured sharpness and clamp.
- dispatchUpscale.reset = false; - dispatchUpscale.enableSharpening = true; - dispatchUpscale.sharpness = 0.0f; + dispatchUpscale.reset = false; + // Drive CAS from settings; clamp to [0,1] + float sharpness = std::max(0.0f, std::min(1.0f, globals::features::upscaling.settings.casSharpness)); + dispatchUpscale.enableSharpening = sharpness > 0.0f; + dispatchUpscale.sharpness = sharpness;src/XSEPlugin.cpp (1)
205-205: Unconditional Load() success may mask hard dependency failuresReturning true even when required DLLs are missing changes prior behavior and can surprise loaders/diagnostics. Consider returning errors.empty() to fail fast, or explicitly document the new behavior.
- return true; + return errors.empty();Is the unconditional success intentional for UX (show message boxes but keep plugin loaded)?
src/Hooks.cpp (1)
808-824: Type-safe write to IBLF toggleWrite a float 0.0f using REL helpers to avoid raw casts and make intent explicit.
- auto enableIBLF = (float*)(REL::RelocationID(513510, 391362).address()); - *enableIBLF = false; + auto enableIBLF = REL::Relocation<float*>(REL::RelocationID(513510, 391362)); + *enableIBLF = 0.0f;src/Features/Upscaling/DX12SwapChain.cpp (1)
169-171: Also gate frame limiter on tearing flag (optional)Helps avoid double pacing when some drivers/apps pass SyncInterval==0 without allowing tearing.
- if (SyncInterval == 0) + if (SyncInterval == 0 && (Flags & DXGI_PRESENT_ALLOW_TEARING)) upscaling.FrameLimiter();src/Features/Upscaling.h (3)
133-133: Remove dead NIS API surfaceNIS was removed in this PR; keeping this declaration risks drift/confusion.
- void ApplyNISSharpening();
139-140: Remove unused NIS texture memberPart of the NIS path; safe to drop with the rest.
- Texture2D* nisSharpenerTexture = nullptr;
50-59: Add CAS sharpness setting to drive FFX sharpeningNeeded by the FidelityFX change to avoid a no‑op sharpening pass. Default to a conservative value.
struct Settings { uint upscaleMethod = (uint)UpscaleMethod::kDLSS; uint upscaleMethodNoDLSS = (uint)UpscaleMethod::kFSR; uint qualityMode = 1; // Default to Quality (1=Quality, 2=Balanced, 3=Performance, 4=Ultra Performance, 0=Native AA) uint frameLimitMode = 1; uint frameGenerationMode = 1; uint frameGenerationForceEnable = 0; uint streamlineLogLevel = 0; // 0=Off, 1=Default, 2=Verbose + float casSharpness = 0.2f; // 0..1 for FSR3 CAS };I can follow up with SaveSettings/LoadSettings JSON glue if you want.
src/Features/Upscaling.cpp (1)
167-169: Hard-coded “NVIDIA DLSS 4 Preset K” label is brittle.Prefer building the label from runtime version info (like FSR/XeSS) and avoid baking “4” in UI.
Apply:
- std::string dlssLabel = "NVIDIA DLSS 4 Preset K"; + std::string dlssLabel = "NVIDIA DLSS"; + if (!streamline.versionInfo.empty()) { + dlssLabel += " " + streamline.versionInfo; + } + dlssLabel += " Preset K";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
features/Upscaling/Shaders/Upscaling/NIS/NIS_Scaler.h(0 hunks)features/Upscaling/Shaders/Upscaling/NIS/NIS_Sharpen.hlsl(0 hunks)src/Features/Upscaling.cpp(5 hunks)src/Features/Upscaling.h(1 hunks)src/Features/Upscaling/DX12SwapChain.cpp(1 hunks)src/Features/Upscaling/FidelityFX.cpp(1 hunks)src/Features/Upscaling/NIS/NIS.cpp(0 hunks)src/Features/Upscaling/NIS/NIS.h(0 hunks)src/Features/Upscaling/NIS/NIS_Config.h(0 hunks)src/Features/VolumetricLighting.cpp(1 hunks)src/Hooks.cpp(2 hunks)src/State.cpp(1 hunks)src/XSEPlugin.cpp(1 hunks)
💤 Files with no reviewable changes (5)
- src/Features/Upscaling/NIS/NIS.h
- src/Features/Upscaling/NIS/NIS.cpp
- features/Upscaling/Shaders/Upscaling/NIS/NIS_Scaler.h
- features/Upscaling/Shaders/Upscaling/NIS/NIS_Sharpen.hlsl
- src/Features/Upscaling/NIS/NIS_Config.h
🧰 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/Features/Upscaling/DX12SwapChain.cppsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling.hsrc/XSEPlugin.cppsrc/State.cppsrc/Features/VolumetricLighting.cppsrc/Hooks.cppsrc/Features/Upscaling.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/Features/Upscaling/DX12SwapChain.cppsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling.hsrc/XSEPlugin.cppsrc/State.cppsrc/Features/VolumetricLighting.cppsrc/Hooks.cppsrc/Features/Upscaling.cpp
🧠 Learnings (2)
📓 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.
📚 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/State.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 (8)
src/Features/VolumetricLighting.cpp (1)
192-196: Round and clamp dynamic render size to avoid mismatch/underflow.Truncation via static_cast can make the RT one texel too small (partial dispatch) and allow 0→-1 underflow; round then clamp to >=1. Shaders use integer sizes (cbuffer VLData in package/Shaders/ISVolumetricLightingBlurHCS.hlsl / ISVolumetricLightingBlurVCS.hlsl declare int2 screenSize and int2 screenSizeMin1). ConvertToDynamic and RT creation/resize weren't found by the repo search — verify the rounding strategy matches how RT/UAV/SRV extents are computed so vlData.screenSize equals the actual bound extents during VL passes.
File: src/Features/VolumetricLighting.cpp (around lines 192–196)
Apply this diff:
- auto renderSize = Util::ConvertToDynamic(globals::state->screenSize); - - int32_t width = static_cast<int32_t>(renderSize.x); - int32_t height = static_cast<int32_t>(renderSize.y); + auto renderSize = Util::ConvertToDynamic(globals::state->screenSize); + const int32_t width = std::max<int32_t>(1, static_cast<int32_t>(std::lround(renderSize.x))); + const int32_t height = std::max<int32_t>(1, static_cast<int32_t>(std::lround(renderSize.y)));Add the header once (outside this hunk):
#include <cmath>src/State.cpp (1)
136-139: Guard GetINISetting nullptr and avoid per-frame INI stomping.RE::GetINISetting("bEnableImprovedSnow:Display") can be nullptr (dereference crash); also avoid forcing the INI every Reset/frame — apply once.
- // Disable "improved" snow shader, unsupported - if (!globals::game::isVR) { - RE::GetINISetting("bEnableImprovedSnow:Display")->data.b = false; - } + // Disable "Improved Snow" on non‑VR (unsupported). Guard against missing INI and apply once. + static bool improvedSnowForcedOff = false; + if (!globals::game::isVR && !improvedSnowForcedOff) { + if (auto* improvedSnow = RE::GetINISetting("bEnableImprovedSnow:Display"); improvedSnow) { + if (improvedSnow->GetBool()) { + improvedSnow->data.b = false; + logger::info("Forced bEnableImprovedSnow:Display = false."); + } + } else { + logger::warn("INI setting 'bEnableImprovedSnow:Display' not found; skipping."); + } + improvedSnowForcedOff = true; // avoid touching this every frame + }src/State.cpp not found in the repository — confirm file path or apply the patch where this code lives.
src/Hooks.cpp (1)
840-844: Hook install guard LGTMNon‑VR guard and detour placement look correct.
src/Features/Upscaling.h (2)
59-59: Streamline log level field LGTMField and comment are clear.
1-254: Verify repo for lingering NIS referencesAutomated regex scan returned no matches; manually confirm there are no stale NIS symbols/assets remaining.
- Check src/Features/Upscaling.h for declarations/usages (e.g., ApplyNISSharpening(), nisSharpenerTexture).
- Search repo for: NIS, NIS_Sharpen, NIS_Scaler, ApplyNISSharpening, nisSharpenerTexture, Upscaling/NIS.
- Inspect shader/asset directories, CMake/project files, resource manifests and packaging/installer for leftover references.
src/Features/Upscaling.cpp (3)
1428-1430: Always forcing DLSS Preset K — add fallback for older Streamline/GPUs.If Preset K isn’t supported everywhere you target, consider a runtime fallback to F (or default) to avoid hard failures or quality regressions.
Would you like a small helper in Streamline to probe preset support and fall back automatically?
13-21: Confirm no stale NIS references remain after WITH_DEFAULT change.Sandbox search failed (ripgrep: "No files were searched"). Run locally: rg -n -C2 -S --hidden -uu -g '!/build/' 'enableNISSharpening|nisSharpness|ApplyNISSharpening|NIS' and confirm zero hits. Check src/Features/Upscaling.cpp (lines 13–21) mapping; if any hits exist, remove or update those references and related config keys.
394-396: Verify RelocationIDs and thunk target for SSE/AEDetour placement OK — I couldn't find src/Features/Upscaling.cpp in this branch and searches returned no matches for REL::RelocationID(98974|105626) or stl::detour_thunk (there is a features/Upscaling directory). Confirm the actual file path and that the detour target (BSImageSpace_Init_FXAA) and FXAA flag relocation IDs (98974, 105626) are correct for both SSE and AE (provide per-runtime IDs if they differ).
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: 2
🧹 Nitpick comments (2)
src/Utils/Game.h (1)
171-172: Clarify IsInterior() semantics in the header.Add a brief comment that this returns true for interior cells and for exteriors with NoSky worldspaces, false otherwise, so downstream users don’t misread it as “strict interior only.”
Apply this diff:
- bool IsInterior(); + // True for interior cells or exteriors whose worldspace has NoSky; false for skyed exteriors. + bool IsInterior();src/Features/Skylighting.cpp (1)
498-499: Switch to Util::IsInterior()—align Prepass() gating for consistency.Good change. Prepass() (Lines 213–219) still uses sky->mode-based detection; please update it to Util::IsInterior() to avoid mismatches on NoSky exteriors.
Suggested update for Prepass():
bool interior = Util::IsInterior(); if (interior) return;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Deferred.cpp(2 hunks)src/Features/DynamicCubemaps.cpp(1 hunks)src/Features/Skylighting.cpp(1 hunks)src/State.cpp(2 hunks)src/State.h(1 hunks)src/Utils/Game.cpp(1 hunks)src/Utils/Game.h(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/State.hsrc/Utils/Game.cppsrc/Utils/Game.hsrc/Features/Skylighting.cppsrc/Deferred.cppsrc/State.cppsrc/Features/DynamicCubemaps.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/State.hsrc/Utils/Game.cppsrc/Utils/Game.hsrc/Features/Skylighting.cppsrc/Deferred.cppsrc/State.cppsrc/Features/DynamicCubemaps.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). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (7)
src/Deferred.cpp (2)
414-414: LGTM: unified interior check via Util::IsInterior().This simplifies and de-duplicates interior logic across passes.
234-238: activeReflections set during reflections prepass — confirm Reset() is called once per frameRepository scan found no src/Deferred.cpp or any State::Reset (or equivalent) references; cannot verify Reset() runs after the cubemap render path. Ensure State::Reset() (or an explicit per-frame clear) runs once per frame after cubemap rendering to clear activeReflections, or explicitly set activeReflections = false at frame end.
src/State.cpp (3)
136-140: LGTM: disable unsupported improved snow via INI.Matches the feature set and prevents unintended shader paths.
141-142: LGTM: reset activeReflections on frame reset.Keeps reflection sequencing bounded to the frame.
130-134: Confirm BSImagespaceShaderApplyReflections member type before calling .get().
src/State.cpp:130-134
Repository search returned no matches; ensure the member is a smart pointer that exposes .get() (std::shared_ptr/std::unique_ptr). If it's a NiPointer or a raw pointer, use an appropriate null-check (if (ptr) / ptr != nullptr) and avoid calling .get().src/State.h (1)
167-168: Confirm State::Reset() runs once per frame (prevent activeReflections leaking).activeReflections is set true in ReflectionsPrepasses and cleared in State::Reset(); ensure Reset() is invoked exactly once per frame so the flag cannot persist across frames — if not guaranteed, clear activeReflections at end-of-frame or make it frame-local.
Location: src/State.h:167-168.src/Utils/Game.cpp (1)
303-314: IsInterior() logic LGTM — verify VR runtime accessorUses tes->GetRuntimeData2() for worldSpace. src/Utils/Game.cpp (lines 303–314) not present in this branch; confirm VR builds don't require an alternate accessor (e.g., GetVRRuntimeData2) or VR-specific build flags. Quick local check: rg -n -S 'GetVRRuntimeData2|GetRuntimeData2|SKYRIMVR|VR_BUILD' -C2
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Features/DynamicCubemaps.cpp (1)
651-655: Avoid TOCTOU on globals::game::sky; compute once.Grab the pointer into a local before checking flags to remove the double read and narrow the race window. Also simplifies the flow.
Apply:
- if (globals::game::sky) - fakeReflections = activeReflections && globals::game::sky->flags.any(RE::Sky::Flags::kHideSky); - else - fakeReflections = false; + const auto sky = globals::game::sky; + const bool hideSky = sky && sky->flags.any(RE::Sky::Flags::kHideSky);…and then fold assignment as below (see next comment).
🧹 Nitpick comments (2)
src/Features/DynamicCubemaps.cpp (2)
656-659: Fold branches to a single, brace-safe block.Combine the fallback and fakeReflections computation; add braces to avoid future slip-ups.
- if (!activeReflections && !Util::IsInterior()) { - activeReflections = true; - fakeReflections = true; - } + if (!activeReflections && !Util::IsInterior()) { + activeReflections = true; + fakeReflections = true; + } else { + fakeReflections = activeReflections && hideSky; + }
656-659: Confirm perf intent of forced exterior fallback.Forcing reflections on exteriors even when state says “off” could add work on low-end/VR. If intentional, consider a toggle (e.g., “ExteriorFakeReflectionsFallback”) or log once to aid profiling.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/DynamicCubemaps.cpp(1 hunks)src/State.cpp(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/State.cpp
🧰 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/Features/DynamicCubemaps.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/Features/DynamicCubemaps.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). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (1)
src/Features/DynamicCubemaps.cpp (1)
649-659: Null-deref fixed; logic reads clearer.The added sky null-check prevents early-frame crashes and the exterior fallback keeps reflections alive when the dedicated pass didn’t run.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Improvements
Refactor
Chores