fix(VR): hmd mask while upscaling#2158
Closed
alandtse wants to merge 2 commits into
Closed
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an OpenVR hidden-area mask pipeline (mesh rasterization → per-eye R8 masks → stereo composite), exposes a stereo-mask SRV to compute shaders, inserts VR-guarded per-pixel HMDMask::IsHiddenPixel checks/early-returns in many shaders, and adds per-eye FOV caching/accessors and related util changes. Changes
Sequence DiagramsequenceDiagram
participant App as CPU
participant OpenVR as OpenVR Runtime
participant Mesh as Mesh Builder
participant Raster as Raster Shaders
participant Masks as Mask Textures (R8 / Stereo t21)
participant CS as Compute Shaders
App->>OpenVR: Request hidden-area mesh per eye
OpenVR-->>Mesh: Return mesh vertex data
Mesh->>Raster: Upload mesh verts
Raster->>Masks: Rasterize per-eye R8 masks (render & display res)
Raster->>Masks: Dispatch BuildStereoMaskCS -> produce stereo mask (t21)
App->>App: Compute per-frame dilation radius
App->>Masks: Bind stereo mask SRV (t21)
App->>CS: Dispatch compute shader
Note over CS: For each pixel
CS->>Masks: VRHMDStencil.Load(pixCoord)
alt mask > 0.5
CS-->>CS: Early return (skip reads/writes)
else visible
CS->>CS: Full shader work (depth/gathers/blends/writes)
end
App->>Masks: Unbind stereo mask SRV
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)❌ Error creating Unit Test PR.
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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
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/FidelityFX.cpp (1)
202-202:⚠️ Potential issue | 🟡 MinorFSR3 frame generation uses default (eye 0) FOV for all dispatches in VR.
Upscale()was updated to passcontextIndexper-eye (line 394), but the FSR3 frame-generation prepare path here still callsUtil::GetVerticalFOVRad()with no eye index, so it always uses eye 0's vertical FOV. If FSR3-FG is exercised on VR, this would feed an incorrect FOV for eye 1. Either gate FSR3 FG off in VR or update this call to be per-eye (matching line 394's pattern).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling/FidelityFX.cpp` at line 202, The FSR3 frame-generation prepare path sets dispatchParameters.cameraFovAngleVertical using Util::GetVerticalFOVRad() with no eye index, so VR second-eye uses eye0 FOV; update this to use the per-eye FOV (match the pattern used in Upscale(contextIndex) at the call site around line 394) by calling Util::GetVerticalFOVRad(contextIndex) or otherwise passing the same per-eye index into the prepare path that sets dispatchParameters.cameraFovAngleVertical; alternatively, if per-eye FSR3-FG is not supported, gate FSR3 frame-generation off for VR in the same code path. Ensure you modify the call that assigns dispatchParameters.cameraFovAngleVertical (and any helpers it relies on) to accept and use the eye/context index.
🧹 Nitpick comments (4)
features/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl (1)
61-64: Redundant#if defined(VR)guard.This file's body is already wrapped in
#ifdef VR(line 13), so the inner#if defined(VR)aroundHMDMask::IsHiddenPixelis unreachable when not in a VR build and adds no protection. Consider dropping the inner guard for consistency.♻️ Proposed simplification
uint eyeIndex = Stereo::GetEyeIndexFromTexCoord(uv); -# if defined(VR) if (HMDMask::IsHiddenPixel((uint2)(uv * FrameDim))) return; -# endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl around lines 61 - 64, Remove the redundant inner preprocessor guard around the HMD mask check: the file is already wrapped in `#ifdef` VR, so delete the inner `#if` defined(VR)/#endif and leave the call to HMDMask::IsHiddenPixel((uint2)(uv * FrameDim)) as the sole conditional early-return; this simplifies stereoSync.cs.hlsl and keeps the hidden-pixel check intact.features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl (1)
88-91: Redundant#if defined(VR)guard.The entire shader body is already wrapped in
#ifdef VRat line 13, so the inner#if defined(VR)…#endifaround theHMDMask::IsHiddenPixelcheck is unnecessary. The check could be unconditional within this file.♻️ Proposed simplification
if (any(dtid >= uint2(FrameDim))) return; -# if defined(VR) if (HMDMask::IsHiddenPixel(dtid)) return; -# endif🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Screen-Space` Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl around lines 88 - 91, Remove the redundant inner preprocessor guard and make the HMDMask check unconditional: the shader is already compiled inside `#ifdef` VR, so delete the inner "#if defined(VR) ... `#endif`" around the HMDMask::IsHiddenPixel(dtid) check and leave the call and early return as-is (keep the dtid parameter and return behavior intact) so the hidden-pixel test runs unconditionally within this VR-only shader.src/Features/SubsurfaceScattering.cpp (1)
230-300: LGTM — bind/unbind pairing is correct.Stencil bind/unbind brackets the SSS compute dispatches and is gated by
isVRon both ends, so no slot leaks across non-VR runs. The trailing manual reset of CS bindings (lines 305-309) also covers t0–t4 SRVs and the UAV; t21 is explicitly nulled byUnbindVRHMDStencil(), so no SRV stays bound past this feature.Optional nit: an RAII scope guard around the bind/unbind would make the pairing exception/early-return safe, but
DrawSSS()has no early returns inside the guarded region, so functionally this is fine as written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/SubsurfaceScattering.cpp` around lines 230 - 300, Bind/unbind of the VR HMD stencil is paired correctly now but would be safer with an RAII guard to guarantee UnbindVRHMDStencil() in case of future early returns/exceptions; create a small scope guard (e.g., VRHMDStencilGuard) that calls globals::features::upscaling.BindVRHMDStencil() in its constructor and globals::features::upscaling.UnbindVRHMDStencil() in its destructor, and replace the manual pair around the SSS compute block in SubsurfaceScattering.cpp (where BindVRHMDStencil and UnbindVRHMDStencil are currently called) to instantiate the guard instead.features/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlsl (1)
52-72: Make the dilation shape match the intended boundary coverage.This scans only the same row and column, so it is cross-shaped rather than separable/rhombus dilation. Diagonal boundary pixels can still keep history, while large radii add many texture reads per visible pixel.
Possible low-risk adjustment if cross-shaped dilation is intentional
- // Separable 1D dilation: scan horizontal row then vertical column. - // Equivalent to a rhombus-shaped dilation (L1 distance), cheaper than square for large r. + // Cross-shaped 1D dilation: scan horizontal row and vertical column. + // This is cheaper than 2D dilation, but does not cover diagonal neighborhoods.If the goal is true temporal-kernel coverage, consider generating a pre-dilated mask or using a bounded 2D/L1 pass only near mask edges. Based on learnings, consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlsl` around lines 52 - 72, The current loops only sample the same row and column (cross-shaped) causing missed diagonal pixels; replace the two separate 1D scans with a single rhombus (L1) scan: loop dx from -dr to dr, compute int maxDy = dr - abs(dx), then loop dy from -maxDy to maxDy and sample MaskTex at int2(clamp(id.x+dx,0,w-1), clamp(id.y+dy,0,h-1)) updating dilatedMasked and breaking early; keep using dilationRadius, exactMasked, MaskTex, id, w, h and preserve the existing early-exit logic to minimize reads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlsl:
- Around line 56-59: Threads that early-return when
HMDMask::IsHiddenPixel(pixCoord) are skipping GroupMemoryBarrierWithGroupSync()
and causing a deadlock; instead of returning, set a local flag (e.g., bool
isHidden) and write neutral/placeholder depth or radiance values into the
groupshared buffer so all threads execute the same group-shared writes and reach
GroupMemoryBarrierWithGroupSync(). Update the prefilterDepths.cs.hlsl code path
that currently returns on HMDMask::IsHiddenPixel(pixCoord) to set isHidden=true
and write neutral depth to the groupshared slot, then proceed to the existing
groupshared write and GroupMemoryBarrierWithGroupSync() call; apply the same
change in prefilterRadiance.cs.hlsl (the block around the current HMDMask check
and the groupshared write/barrier).
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/prefilterRadiance.cs.hlsl:
- Around line 39-42: The early return inside the VR guard (the
HMDMask::IsHiddenPixel(pixCoord) check in prefilterRadiance.cs.hlsl) causes
threads to leave before hitting subsequent GroupMemoryBarrierWithGroupSync()
calls; replace the early return with a per-thread "active" flag (e.g., bool
isHidden = HMDMask::IsHiddenPixel(pixCoord)) and gate all work, writes and reads
(sampling/output) behind that flag, but ensure every thread still reaches each
GroupMemoryBarrierWithGroupSync() call; apply the same change to
prefilterDepths.cs.hlsl for the HMDMask check so all threads in the group
participate in barriers while hidden threads skip actual processing.
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl:
- Around line 26-29: The shader uses HMDMask::IsHiddenPixel(dtid) inside an `#if`
defined(VR) block but doesn't include the header that defines HMDMask; add
`#include` "Common/VR.hlsli" to the top of upsample.cs.hlsl (before any
VR-dependent code or `#if` defined(VR) usage) so HMDMask::IsHiddenPixel and its
namespace are available at compile time; ensure the include is placed alongside
other common includes (e.g., near ScreenSpaceGI/common.hlsli) to fix VR
compilation errors.
In `@src/Deferred.cpp`:
- Around line 310-313: The VR HMD stencil SRV bind (the vrBuffer bind via
context->CSSetConstantBuffers and
globals::features::upscaling.BindVRHMDStencil()) is currently performed too
early and can be cleared by SSGI.DrawSSGI() cleanup; move the vrBuffer
constant-buffer bind and the BindVRHMDStencil() call out of the earlier scope
and place them immediately before the DeferredCompositeCS dispatch (the
DeferredComposite dispatch scope) so the HMD mask is still bound when
DeferredCompositeCS executes; ensure any previous binds are not duplicated and
remove the earlier bind lines around vrBuffer/context->CSSetConstantBuffers to
avoid stale/unbound state (also apply same move for the other occurrences around
lines referenced: the similar binds at the other spots).
In `@src/Features/Upscaling.cpp`:
- Around line 1465-1532: The stereo mask texture vrHMDMaskStereoRenderRes is
created before checking vrBuildStereoMaskCS but never cleared, so when
vrBuildStereoMaskCS is missing you must avoid leaving undefined R8 data bound at
t21; after creating vrHMDMaskStereoRenderRes and detecting vrBuildStereoMaskCS
is null (or if CreateBuffer/compile fails), either release/reset
vrHMDMaskStereoRenderRes or explicitly clear it via the device context (e.g.,
call context->ClearUnorderedAccessViewUint(vrHMDMaskStereoRenderRes->uav.get(),
clearZero) or context->ClearRenderTargetView on a fallback SRV) so
BindVRHMDStencil never binds uninitialized data; implement this right after the
check around vrBuildStereoMaskCS and before the warning log, referencing
vrHMDMaskStereoRenderRes, vrBuildStereoMaskCS,
context->ClearUnorderedAccessViewUint, and BindVRHMDStencil.
- Around line 1834-1852: ClearShaderCache currently nulls
vrHMDMaskStereoRenderRes but does not force EnsureVRIntermediateTextures to
rebuild VR masks, leaving BindVRHMDStencil stuck “not ready”; change
ClearShaderCache to also reset the VR-mask recreation predicate used by
EnsureVRIntermediateTextures (for example clear or zero the
vrHMDMaskStereoRenderRes size/ready flag or set vrHMDMaskStereoRenderReady =
false) so the mask is recreated on next EnsureVRIntermediateTextures call, and
update BindVRHMDStencil to handle a null vrHMDMaskStereoRenderRes gracefully;
reference functions/variables: ClearShaderCache, EnsureVRIntermediateTextures,
BindVRHMDStencil, vrHMDMaskStereoRenderRes (or the predicate/ready flag tracking
its creation).
In `@src/Utils/VRUtils.cpp`:
- Around line 263-269: The function currently copies
poses[vr::k_unTrackedDeviceIndex_Hmd] into outPose and returns true even when
that tracked device's bPoseIsValid is false; change the logic after calling
compositor->GetLastPoses to check
poses[vr::k_unTrackedDeviceIndex_Hmd].bPoseIsValid and if it is false return
false without mutating outPose, otherwise assign outPose and return true so the
function's "leave pose unchanged" contract is honored.
---
Outside diff comments:
In `@src/Features/Upscaling/FidelityFX.cpp`:
- Line 202: The FSR3 frame-generation prepare path sets
dispatchParameters.cameraFovAngleVertical using Util::GetVerticalFOVRad() with
no eye index, so VR second-eye uses eye0 FOV; update this to use the per-eye FOV
(match the pattern used in Upscale(contextIndex) at the call site around line
394) by calling Util::GetVerticalFOVRad(contextIndex) or otherwise passing the
same per-eye index into the prepare path that sets
dispatchParameters.cameraFovAngleVertical; alternatively, if per-eye FSR3-FG is
not supported, gate FSR3 frame-generation off for VR in the same code path.
Ensure you modify the call that assigns
dispatchParameters.cameraFovAngleVertical (and any helpers it relies on) to
accept and use the eye/context index.
---
Nitpick comments:
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl:
- Around line 61-64: Remove the redundant inner preprocessor guard around the
HMD mask check: the file is already wrapped in `#ifdef` VR, so delete the inner
`#if` defined(VR)/#endif and leave the call to HMDMask::IsHiddenPixel((uint2)(uv *
FrameDim)) as the sole conditional early-return; this simplifies
stereoSync.cs.hlsl and keeps the hidden-pixel check intact.
In `@features/Screen-Space` Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl:
- Around line 88-91: Remove the redundant inner preprocessor guard and make the
HMDMask check unconditional: the shader is already compiled inside `#ifdef` VR, so
delete the inner "#if defined(VR) ... `#endif`" around the
HMDMask::IsHiddenPixel(dtid) check and leave the call and early return as-is
(keep the dtid parameter and return behavior intact) so the hidden-pixel test
runs unconditionally within this VR-only shader.
In `@features/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlsl`:
- Around line 52-72: The current loops only sample the same row and column
(cross-shaped) causing missed diagonal pixels; replace the two separate 1D scans
with a single rhombus (L1) scan: loop dx from -dr to dr, compute int maxDy = dr
- abs(dx), then loop dy from -maxDy to maxDy and sample MaskTex at
int2(clamp(id.x+dx,0,w-1), clamp(id.y+dy,0,h-1)) updating dilatedMasked and
breaking early; keep using dilationRadius, exactMasked, MaskTex, id, w, h and
preserve the existing early-exit logic to minimize reads.
In `@src/Features/SubsurfaceScattering.cpp`:
- Around line 230-300: Bind/unbind of the VR HMD stencil is paired correctly now
but would be safer with an RAII guard to guarantee UnbindVRHMDStencil() in case
of future early returns/exceptions; create a small scope guard (e.g.,
VRHMDStencilGuard) that calls globals::features::upscaling.BindVRHMDStencil() in
its constructor and globals::features::upscaling.UnbindVRHMDStencil() in its
destructor, and replace the manual pair around the SSS compute block in
SubsurfaceScattering.cpp (where BindVRHMDStencil and UnbindVRHMDStencil are
currently called) to instantiate the guard instead.
🪄 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: 82ca9753-4d89-41c3-b2fa-f0f9e10218e1
📒 Files selected for processing (32)
features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterRadiance.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslfeatures/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/BuildStereoMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterPS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterVS.hlslpackage/Shaders/Common/VR.hlslipackage/Shaders/DeferredCompositeCS.hlslpackage/Shaders/VR/StereoBlendCS.hlslsrc/Deferred.cppsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceShadows.cppsrc/Features/SubsurfaceScattering.cppsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling/Streamline.cppsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/StereoBlend.cppsrc/Utils/Game.cppsrc/Utils/Game.hsrc/Utils/VRUtils.cppsrc/Utils/VRUtils.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
features/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl (1)
26-29:⚠️ Potential issue | 🔴 CriticalAdd the VR header before using
HMDMask.
HMDMask::IsHiddenPixel(dtid)is declared inCommon/VR.hlsli, but this shader does not include that header directly. VR variants can fail compilation whenScreenSpaceGI/common.hlslidoes not bring it into scope.Fix
`#include` "Common/FastMath.hlsli" +#include "Common/VR.hlsli" `#include` "ScreenSpaceGI/common.hlsli"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl around lines 26 - 29, The shader uses HMDMask::IsHiddenPixel(dtid) without including its declaration; add an explicit include of the VR header (Common/VR.hlsli) at the top of this shader (before any use of HMDMask) so VR variants compile reliably; locate the use of HMDMask::IsHiddenPixel in upsample.cs.hlsl and insert the include for Common/VR.hlsli (or the appropriate VR header) above that code path.src/Features/Upscaling.cpp (2)
1034-1047:⚠️ Potential issue | 🟠 MajorForce VR masks to rebuild after clearing the shader cache.
ClearShaderCache()dropsvrHMDMaskStereoRenderRes, butEnsureVRIntermediateTextures()only recreates resources when the intermediate color/depth textures are missing or resized. With unchanged dimensions,BindVRHMDStencil()can stay permanently not-ready after a shader-cache clear.Fix
bool needsRecreate = !vrIntermediateColorIn[0] || !vrIntermediateColorOut[0] || !vrIntermediateLinearDepth[0]; + needsRecreate = needsRecreate || + !vrHMDMaskRenderRes[0] || + !vrHMDMaskRenderRes[1] || + !vrHMDMaskDisplayRes[0] || + !vrHMDMaskDisplayRes[1] || + !vrHMDMaskStereoRenderRes; if (!needsRecreate) { needsRecreate = (vrIntermediateColorIn[0]->desc.Width != eyeWidthIn || vrIntermediateColorIn[0]->desc.Height != eyeHeightIn ||vrApplyHMDMaskWithReactiveCS = nullptr; vrDilationCB = nullptr; vrBuildStereoMaskCS = nullptr; vrHMDMaskStereoRenderRes = nullptr; + for (int eye = 0; eye < 2; ++eye) { + vrHMDMaskRenderRes[eye].reset(); + vrHMDMaskDisplayRes[eye].reset(); + } + vrRenderPerEyeW = 0; + vrRenderPerEyeH = 0; }As per coding guidelines, include proper resource management and graceful degradation for DirectX 11 resources to prevent crashes from malformed configurations.
Also applies to: 1834-1852
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 1034 - 1047, Ensure VR HMD mask resources are rebuilt after ClearShaderCache() by making EnsureVRIntermediateTextures() (the code checking vrIntermediateColorIn/Out and vrIntermediateLinearDepth) also validate vrHMDMaskStereoRenderRes and related mask state; when ClearShaderCache() is called the vrHMDMaskStereoRenderRes should be reset/marked invalid so EnsureVRIntermediateTextures() will recreate the masks (or force CreateVRIntermediateTextures to also recreate mask resources). Update BindVRHMDStencil() to gracefully handle a missing/invalid vrHMDMaskStereoRenderRes (return early and avoid dereferencing) so DirectX11 resource failures degrade safely rather than crash.
1465-1532:⚠️ Potential issue | 🟠 MajorClear or discard the stereo mask when the build pass cannot run.
vrHMDMaskStereoRenderResis created beforevrBuildStereoMaskCSand the build cbuffer are guaranteed. If shader compilation orCreateBufferfails,BindVRHMDStencil()can later bind undefined R8 data att21, causing random hidden-pixel exits.Fix
D3D11_UNORDERED_ACCESS_VIEW_DESC uavDesc = {}; uavDesc.Format = DXGI_FORMAT_R8_UNORM; uavDesc.ViewDimension = D3D11_UAV_DIMENSION_TEXTURE2D; vrHMDMaskStereoRenderRes->CreateUAV(uavDesc); + const UINT clearMask[4] = { 0, 0, 0, 0 }; + context->ClearUnorderedAccessViewUint(vrHMDMaskStereoRenderRes->uav.get(), clearMask); if (!vrBuildStereoMaskCS) { vrBuildStereoMaskCS.attach((ID3D11ComputeShader*)Util::CompileShader( L"Data/Shaders/Upscaling/BuildStereoMaskCS.hlsl", {}, "cs_5_0")); @@ D3D11_SUBRESOURCE_DATA cbInit = { &cbData }; winrt::com_ptr<ID3D11Buffer> cb; - globals::d3d::device->CreateBuffer(&cbDesc, &cbInit, cb.put()); + if (FAILED(globals::d3d::device->CreateBuffer(&cbDesc, &cbInit, cb.put())) || !cb) { + logger::warn("[Upscaling] BuildStereoMaskCS cbuffer creation failed — t21 mask left all-visible"); + return; + } @@ } else { - logger::warn("[Upscaling] BuildStereoMaskCS compile failed — t21 binding unavailable"); + logger::warn("[Upscaling] BuildStereoMaskCS compile failed — t21 mask left all-visible"); }As per coding guidelines, include proper resource management and graceful degradation for DirectX 11 resources to prevent crashes from malformed configurations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 1465 - 1532, The stereo texture vrHMDMaskStereoRenderRes is created unconditionally but may remain uninitialized if vrBuildStereoMaskCS compile or CreateBuffer for BuildCB fails; modify the code around vrBuildStereoMaskStereoRenderRes, vrBuildStereoMaskCS and the CreateBuffer call to detect failures (check vrBuildStereoMaskCS and the HRESULT from CreateBuffer) and in the failure path explicitly clear/discard the resource instead of leaving garbage — e.g. obtain the UAV (vrHMDMaskStereoRenderRes->uav.get()) and call context->ClearUnorderedAccessViewUint(uav, uint[4]{0,0,0,0}) or copy a zero-initialized buffer into the texture so BindVRHMDStencil() later never binds random R8 data at t21; also ensure you unbind any SRV/UAV/CB when aborting the build pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@features/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlsl`:
- Around line 74-76: The current branch that handles dilatedMasked sets
ReactiveOut and also clears TransparencyOut, but per the contract
TransparencyOut should be cleared only for exact-masked pixels; update the logic
so ReactiveOut[id.xy] = 1.0 remains inside the dilatedMasked branch, but remove
TransparencyOut[id.xy] = 0.0 from that branch and instead clear TransparencyOut
only when the exact-mask condition is true (use the exact-mask boolean/variable
used elsewhere in this shader, e.g. exactMasked or masked) so visible boundary
pixels preserve their existing transparency data.
In `@src/Features/Upscaling.cpp`:
- Around line 1269-1283: VirtualQuery + MEM_COMMIT check can still return pages
that are not readable (PAGE_NOACCESS or PAGE_GUARD); update the validation
around mesh.pVertexData (the VirtualQuery call and MEMORY_BASIC_INFORMATION mbi
usage that computes validBytes) to reject regions whose mbi.Protect indicates
unreadable/guard pages. Concretely, after the existing mbi.State/mbi.Type checks
also ensure (mbi.Protect & (PAGE_NOACCESS | PAGE_GUARD)) == 0 (or equivalently
mbi.Protect != PAGE_NOACCESS and no PAGE_GUARD bit set), allowing only readable
protections (read/read-write/execute-read variants) before computing validBytes
and dereferencing mesh.pVertexData. Ensure the new protect check is applied in
the same block that sets validBytes to prevent any access to inaccessible
memory.
---
Duplicate comments:
In `@features/Screen` Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl:
- Around line 26-29: The shader uses HMDMask::IsHiddenPixel(dtid) without
including its declaration; add an explicit include of the VR header
(Common/VR.hlsli) at the top of this shader (before any use of HMDMask) so VR
variants compile reliably; locate the use of HMDMask::IsHiddenPixel in
upsample.cs.hlsl and insert the include for Common/VR.hlsli (or the appropriate
VR header) above that code path.
In `@src/Features/Upscaling.cpp`:
- Around line 1034-1047: Ensure VR HMD mask resources are rebuilt after
ClearShaderCache() by making EnsureVRIntermediateTextures() (the code checking
vrIntermediateColorIn/Out and vrIntermediateLinearDepth) also validate
vrHMDMaskStereoRenderRes and related mask state; when ClearShaderCache() is
called the vrHMDMaskStereoRenderRes should be reset/marked invalid so
EnsureVRIntermediateTextures() will recreate the masks (or force
CreateVRIntermediateTextures to also recreate mask resources). Update
BindVRHMDStencil() to gracefully handle a missing/invalid
vrHMDMaskStereoRenderRes (return early and avoid dereferencing) so DirectX11
resource failures degrade safely rather than crash.
- Around line 1465-1532: The stereo texture vrHMDMaskStereoRenderRes is created
unconditionally but may remain uninitialized if vrBuildStereoMaskCS compile or
CreateBuffer for BuildCB fails; modify the code around
vrBuildStereoMaskStereoRenderRes, vrBuildStereoMaskCS and the CreateBuffer call
to detect failures (check vrBuildStereoMaskCS and the HRESULT from CreateBuffer)
and in the failure path explicitly clear/discard the resource instead of leaving
garbage — e.g. obtain the UAV (vrHMDMaskStereoRenderRes->uav.get()) and call
context->ClearUnorderedAccessViewUint(uav, uint[4]{0,0,0,0}) or copy a
zero-initialized buffer into the texture so BindVRHMDStencil() later never binds
random R8 data at t21; also ensure you unbind any SRV/UAV/CB when aborting the
build pass.
🪄 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: f63d7e84-9d88-4f8e-a76c-34d3294bf001
📒 Files selected for processing (30)
features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterRadiance.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslfeatures/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/BuildStereoMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterPS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterVS.hlslpackage/Shaders/Common/VR.hlslipackage/Shaders/VR/StereoBlendCS.hlslsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceShadows.cppsrc/Features/SubsurfaceScattering.cppsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling/Streamline.cppsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/StereoBlend.cppsrc/Utils/Game.cppsrc/Utils/Game.hsrc/Utils/VRUtils.cppsrc/Utils/VRUtils.h
✅ Files skipped from review due to trivial changes (6)
- features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl
- features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl
- features/Upscaling/Shaders/Upscaling/HMDMaskRasterPS.hlsl
- features/Upscaling/Shaders/Upscaling/HMDMaskRasterVS.hlsl
- src/Features/ScreenSpaceShadows.cpp
🚧 Files skipped from review as they are similar to previous changes (16)
- features/Screen Space GI/Shaders/ScreenSpaceGI/prefilterRadiance.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlsl
- features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
- src/Features/ScreenSpaceGI.cpp
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
- src/Features/SubsurfaceScattering.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlsl
- package/Shaders/VR/StereoBlendCS.hlsl
- src/Features/Upscaling/FidelityFX.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl
- src/Features/Upscaling/Streamline.cpp
- src/Features/VR.h
- src/Features/VR.cpp
- src/Utils/VRUtils.cpp
- src/Features/VR/StereoBlend.cpp
- src/Utils/Game.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Features/Upscaling.cpp (2)
1830-1838:⚠️ Potential issue | 🟠 MajorBind a null SRV when the stereo mask is unavailable.
Returning without touching slot
t21can leave a stale SRV bound for shaders usingHMDMask::IsHiddenPixel, causing false hidden-pixel skips. Bindnullptras the fallback and rate-limit the warning. As per coding guidelines, include graceful degradation for DirectX 11 resources.Proposed fix
void Upscaling::BindVRHMDStencil() { - if (!vrHMDMaskStereoRenderRes) { - logger::warn("[Upscaling] BindVRHMDStencil: stereo mask not ready"); - return; - } - ID3D11ShaderResourceView* srv = vrHMDMaskStereoRenderRes->srv.get(); + ID3D11ShaderResourceView* srv = vrHMDMaskStereoRenderRes ? vrHMDMaskStereoRenderRes->srv.get() : nullptr; + if (!srv) { + static bool warned = false; + if (!warned) { + logger::warn("[Upscaling] BindVRHMDStencil: stereo mask not ready; binding null mask"); + warned = true; + } + } globals::d3d::context->CSSetShaderResources(21, 1, &srv); }#!/bin/bash # Verify the missing-mask path binds a null SRV to CS slot 21 instead of returning early. rg -n -C6 'void Upscaling::BindVRHMDStencil|CSSetShaderResources\(21' --iglob 'Upscaling.cpp'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 1830 - 1838, The early return in Upscaling::BindVRHMDStencil leaves slot t21 (CS slot 21) possibly bound to a stale SRV; change the fallback to call globals::d3d::context->CSSetShaderResources(21, 1, nullptr) when vrHMDMaskStereoRenderRes is null so the shader sees an unbound SRV, and replace the unconditional logger::warn with a rate-limited/log-once or throttled warning to avoid spamming logs; locate these changes in the Upscaling::BindVRHMDStencil function (reference vrHMDMaskStereoRenderRes, CSSetShaderResources(21, 1, ...), and logger::warn).
1276-1285:⚠️ Potential issue | 🟠 MajorWhitelist readable page protections before dereferencing
pVertexData.This now rejects
PAGE_NOACCESS/PAGE_GUARD, butMEM_COMMITcan still be non-readable, e.g.PAGE_EXECUTE. Keep the explicit readable-protection whitelist so a hooked runtime cannot hand back an execute-only private page that still passes this check. As per coding guidelines, include proper resource management and graceful degradation for DirectX 11 resources and malformed configurations.Proposed fix
MEMORY_BASIC_INFORMATION mbi{}; size_t validBytes = 0; + auto isReadableProtect = [](DWORD protect) { + protect &= 0xff; + return protect == PAGE_READONLY || + protect == PAGE_READWRITE || + protect == PAGE_WRITECOPY || + protect == PAGE_EXECUTE_READ || + protect == PAGE_EXECUTE_READWRITE || + protect == PAGE_EXECUTE_WRITECOPY; + }; if (VirtualQuery(mesh.pVertexData, &mbi, sizeof(mbi)) && mbi.State == MEM_COMMIT && mbi.Type != MEM_IMAGE && // MEM_IMAGE = code/data section of a PE; not valid mesh data - (mbi.Protect & (PAGE_NOACCESS | PAGE_GUARD)) == 0) // unreadable pages would crash on access + (mbi.Protect & (PAGE_NOACCESS | PAGE_GUARD)) == 0 && // unreadable pages would crash on access + isReadableProtect(mbi.Protect)) {#!/bin/bash # Verify whether the current VirtualQuery guard has a readable-protection whitelist. rg -n -C5 'VirtualQuery\(mesh\.pVertexData|PAGE_NOACCESS|PAGE_GUARD|isReadableProtect' --iglob 'Upscaling.cpp'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 1276 - 1285, The current VirtualQuery check for mesh.pVertexData allows MEM_COMMIT but doesn't ensure the page protection is actually readable (e.g., PAGE_EXECUTE is non-readable); update the check in Upscaling.cpp around the VirtualQuery/MEMORY_BASIC_INFORMATION logic to whitelist only explicitly readable protections (e.g., PAGE_READONLY, PAGE_READWRITE, PAGE_WRITECOPY, and their EXECUTE+READ variants) before dereferencing mesh.pVertexData, and add graceful degradation when the page is not readable by setting validBytes = 0 and failing back safely (avoid dereference), plus ensure any DirectX 11 resource handling (where mesh.pVertexData is used) checks for nullptr/invalid buffers and releases or skips processing instead of crashing; reference the symbols VirtualQuery, MEMORY_BASIC_INFORMATION, mesh.pVertexData, and validBytes when making these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/Upscaling.cpp`:
- Around line 1295-1336: The runtime-provided mesh counts (mesh.unTriangleCount)
must be validated and capped before any multiplication or allocation: introduce
a constant kMaxHMDMaskVerts (e.g. 65536), use uint64_t to compute products (e.g.
static_cast<uint64_t>(mesh.unTriangleCount) * 3) to detect overflow, and replace
checks that set lineLoopByMemory and compute vertCount/polyVerts with safe
capped values (polyVerts = std::min<uint32_t>(mesh.unTriangleCount,
kMaxHMDMaskVerts) and vertCount =
std::min<uint32_t>(static_cast<uint32_t>(safeProduct), kMaxHMDMaskVerts) or
compute vertCount from capped polyVerts for line-loop case); ensure
ndcVerts.resize(...) is passed the capped vertCount and that all multiplications
use the 64-bit pre-check before casting back to uint32_t (update references:
mesh.unTriangleCount, lineLoopByMemory, polyVerts, vertCount, ndcVerts.resize).
---
Duplicate comments:
In `@src/Features/Upscaling.cpp`:
- Around line 1830-1838: The early return in Upscaling::BindVRHMDStencil leaves
slot t21 (CS slot 21) possibly bound to a stale SRV; change the fallback to call
globals::d3d::context->CSSetShaderResources(21, 1, nullptr) when
vrHMDMaskStereoRenderRes is null so the shader sees an unbound SRV, and replace
the unconditional logger::warn with a rate-limited/log-once or throttled warning
to avoid spamming logs; locate these changes in the Upscaling::BindVRHMDStencil
function (reference vrHMDMaskStereoRenderRes, CSSetShaderResources(21, 1, ...),
and logger::warn).
- Around line 1276-1285: The current VirtualQuery check for mesh.pVertexData
allows MEM_COMMIT but doesn't ensure the page protection is actually readable
(e.g., PAGE_EXECUTE is non-readable); update the check in Upscaling.cpp around
the VirtualQuery/MEMORY_BASIC_INFORMATION logic to whitelist only explicitly
readable protections (e.g., PAGE_READONLY, PAGE_READWRITE, PAGE_WRITECOPY, and
their EXECUTE+READ variants) before dereferencing mesh.pVertexData, and add
graceful degradation when the page is not readable by setting validBytes = 0 and
failing back safely (avoid dereference), plus ensure any DirectX 11 resource
handling (where mesh.pVertexData is used) checks for nullptr/invalid buffers and
releases or skips processing instead of crashing; reference the symbols
VirtualQuery, MEMORY_BASIC_INFORMATION, mesh.pVertexData, and validBytes when
making these changes.
🪄 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: 3ffaa3cc-5d7b-424c-98a7-5eb60a405b09
📒 Files selected for processing (31)
features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterRadiance.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Subsurface Scattering/Shaders/Features/SubsurfaceScattering.inifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslfeatures/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/BuildStereoMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterPS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterVS.hlslpackage/Shaders/Common/VR.hlslipackage/Shaders/VR/StereoBlendCS.hlslsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceShadows.cppsrc/Features/SubsurfaceScattering.cppsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling/Streamline.cppsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/StereoBlend.cppsrc/Utils/Game.cppsrc/Utils/Game.hsrc/Utils/VRUtils.cppsrc/Utils/VRUtils.h
✅ Files skipped from review due to trivial changes (9)
- features/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl
- features/Subsurface Scattering/Shaders/Features/SubsurfaceScattering.ini
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlsl
- features/Upscaling/Shaders/Upscaling/HMDMaskRasterPS.hlsl
- src/Features/ScreenSpaceGI.cpp
- features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl
- features/Upscaling/Shaders/Upscaling/HMDMaskRasterVS.hlsl
- package/Shaders/Common/VR.hlsli
🚧 Files skipped from review as they are similar to previous changes (17)
- features/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl
- features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl
- src/Utils/Game.h
- src/Features/ScreenSpaceShadows.cpp
- src/Features/Upscaling/Streamline.cpp
- src/Features/VR.h
- features/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlsl
- src/Features/SubsurfaceScattering.cpp
- src/Features/VR/StereoBlend.cpp
- src/Utils/Game.cpp
- src/Features/Upscaling/FidelityFX.cpp
- src/Utils/VRUtils.cpp
- src/Features/VR.cpp
- features/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlsl
- src/Features/Upscaling.h
- features/Screen Space GI/Shaders/ScreenSpaceGI/prefilterRadiance.cs.hlsl
|
✅ A pre-release build is available for this PR: |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Utils/Game.cpp (1)
146-155:⚠️ Potential issue | 🟠 MajorUse eye-local aspect ratio in the VR fallback path.
When
GetPerEyeVerticalFOVRad()returns0, this fallback still derives vertical FOV from the full side-by-sidescreenSize.x. VR upscaling dispatches per-eye resources using half width, so the fallback vertical FOV is too narrow if OpenVR projection data is unavailable.🐛 Proposed fix
if (globals::game::isVR) { float vfov = Util::GetPerEyeVerticalFOVRad(eyeIndex); if (vfov > 0.0f) return vfov; } static float& cameraFOVDeg = (*(float*)(REL::RelocationID(513786, 388785).address())); // FOV degrees float hFOVRad = cameraFOVDeg * (3.14159265359f / 180.0f); float unitHalfWidth = tan(hFOVRad / 2); // This is same as camera frustum RL - float unitHalfHeight = unitHalfWidth / (globals::state->screenSize.x / globals::state->screenSize.y); // frustum TB + const float aspectWidth = globals::game::isVR ? globals::state->screenSize.x * 0.5f : globals::state->screenSize.x; + float unitHalfHeight = unitHalfWidth / (aspectWidth / globals::state->screenSize.y); // frustum TB float vFOVRad = 2.0f * atan(unitHalfHeight); return vFOVRad;As per coding guidelines, ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and
REL::RelocateMember()patterns for compatibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/Game.cpp` around lines 146 - 155, The VR fallback computes vertical FOV using the full side-by-side screen width, making the FOV too narrow in VR; update the calculation in the block after globals::game::isVR so the aspect ratio uses per-eye width (i.e., use screenSize.x/2 when globals::game::isVR) instead of the full screen width when computing unitHalfHeight, and ensure the camera FOV field retrieval uses the REL relocation pattern for cross-variant compatibility (replace the hardcoded REL::RelocationID usage with a REL::RelocateMember/relocation lookup for the cameraFOVDeg symbol so it works across SE/AE/VR).
🧹 Nitpick comments (1)
src/Features/Upscaling.h (1)
182-186: Optional: state the t21 contract at the API boundary.The Doxygen comment mentions t21 but not that callers must ensure no other CS SRV is bound at slot 21 for the duration of the dispatch (and that
UnbindVRHMDStencilonly clears that one slot). Noting this contract here makes accidental t21 collisions easier to catch at review time.💡 Suggested clarification
- /// Binds vrHMDMaskStereoRenderRes as a CS SRV at t21 so deferred compute shaders can - /// early-exit for HMD hidden pixels via HMDMask::IsHiddenPixel. + /// Binds vrHMDMaskStereoRenderRes as a CS SRV at t21 so deferred compute shaders can + /// early-exit for HMD hidden pixels via HMDMask::IsHiddenPixel. + /// Contract: slot 21 is reserved project-wide for this mask in VR CS passes; callers + /// must pair this with UnbindVRHMDStencil() after the dispatch. void BindVRHMDStencil(); - /// Unbinds t21. + /// Clears CS SRV slot 21 only. Does not touch other SRV/UAV/sampler bindings. void UnbindVRHMDStencil();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.h` around lines 182 - 186, Update the Doxygen for BindVRHMDStencil and UnbindVRHMDStencil to explicitly state the t21 contract: callers must ensure no other compute-shader SRV is bound to slot t21 for the duration of any dispatch that relies on vrHMDMaskStereoRenderRes, that BindVRHMDStencil binds only that SRV into CS slot t21, and that UnbindVRHMDStencil clears only slot t21 (it does not restore previous bindings)—document these requirements next to the function declarations (BindVRHMDStencil, UnbindVRHMDStencil) so reviewers and callers know to avoid t21 collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Utils/Game.cpp`:
- Around line 146-155: The VR fallback computes vertical FOV using the full
side-by-side screen width, making the FOV too narrow in VR; update the
calculation in the block after globals::game::isVR so the aspect ratio uses
per-eye width (i.e., use screenSize.x/2 when globals::game::isVR) instead of the
full screen width when computing unitHalfHeight, and ensure the camera FOV field
retrieval uses the REL relocation pattern for cross-variant compatibility
(replace the hardcoded REL::RelocationID usage with a
REL::RelocateMember/relocation lookup for the cameraFOVDeg symbol so it works
across SE/AE/VR).
---
Nitpick comments:
In `@src/Features/Upscaling.h`:
- Around line 182-186: Update the Doxygen for BindVRHMDStencil and
UnbindVRHMDStencil to explicitly state the t21 contract: callers must ensure no
other compute-shader SRV is bound to slot t21 for the duration of any dispatch
that relies on vrHMDMaskStereoRenderRes, that BindVRHMDStencil binds only that
SRV into CS slot t21, and that UnbindVRHMDStencil clears only slot t21 (it does
not restore previous bindings)—document these requirements next to the function
declarations (BindVRHMDStencil, UnbindVRHMDStencil) so reviewers and callers
know to avoid t21 collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ecab769-ca25-467f-aabf-44c07a4a0b77
📒 Files selected for processing (31)
features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterRadiance.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Subsurface Scattering/Shaders/Features/SubsurfaceScattering.inifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslfeatures/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/BuildStereoMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterPS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterVS.hlslpackage/Shaders/Common/VR.hlslipackage/Shaders/VR/StereoBlendCS.hlslsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceShadows.cppsrc/Features/SubsurfaceScattering.cppsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling/Streamline.cppsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/StereoBlend.cppsrc/Utils/Game.cppsrc/Utils/Game.hsrc/Utils/VRUtils.cppsrc/Utils/VRUtils.h
✅ Files skipped from review due to trivial changes (10)
- features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
- features/Subsurface Scattering/Shaders/Features/SubsurfaceScattering.ini
- features/Upscaling/Shaders/Upscaling/HMDMaskRasterPS.hlsl
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
- features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlsl
- src/Features/ScreenSpaceGI.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlsl
- features/Upscaling/Shaders/Upscaling/HMDMaskRasterVS.hlsl
- features/Upscaling/Shaders/Upscaling/BuildStereoMaskCS.hlsl
🚧 Files skipped from review as they are similar to previous changes (13)
- package/Shaders/VR/StereoBlendCS.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl
- src/Features/ScreenSpaceShadows.cpp
- src/Features/Upscaling/Streamline.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlsl
- src/Features/SubsurfaceScattering.cpp
- src/Utils/VRUtils.h
- src/Utils/VRUtils.cpp
- src/Utils/Game.h
- features/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlsl
- src/Features/Upscaling.cpp
Collaborator
Author
|
I'm seeing a performance drop per tracey so don't merge. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/Features/Upscaling.cpp (1)
1278-1287:⚠️ Potential issue | 🟠 MajorReject
PAGE_GUARDbefore accepting the mesh pointer.Line 1286 still accepts guarded readable pages because
PAGE_READWRITE | PAGE_GUARDhas a readable bit set. Explicitly rejectPAGE_GUARD/PAGE_NOACCESS, then compare the base protection.🛡️ Proposed fix
- // Whitelist only explicitly readable page protections. - // PAGE_EXECUTE alone is not readable on x64 (DEP); PAGE_NOACCESS/PAGE_GUARD trap. - constexpr DWORD kReadableProtect = - PAGE_READONLY | PAGE_READWRITE | PAGE_WRITECOPY | - PAGE_EXECUTE_READ | PAGE_EXECUTE_READWRITE | PAGE_EXECUTE_WRITECOPY; + auto isReadableProtect = [](DWORD protect) { + if (protect & (PAGE_NOACCESS | PAGE_GUARD)) + return false; + + switch (protect & 0xff) { + case PAGE_READONLY: + case PAGE_READWRITE: + case PAGE_WRITECOPY: + case PAGE_EXECUTE_READ: + case PAGE_EXECUTE_READWRITE: + case PAGE_EXECUTE_WRITECOPY: + return true; + default: + return false; + } + }; if (VirtualQuery(mesh.pVertexData, &mbi, sizeof(mbi)) && mbi.State == MEM_COMMIT && mbi.Type != MEM_IMAGE && // code/data section of a PE: not valid mesh data - (mbi.Protect & kReadableProtect)) // must be explicitly readable + isReadableProtect(mbi.Protect)) // must be explicitly readableVerify the current guard-page acceptance with:
#!/bin/bash # Description: Show the current VirtualQuery protection predicate in BuildHMDMesh. # Expected after the fix: explicit PAGE_GUARD/PAGE_NOACCESS rejection is present. rg -n -C6 'kReadableProtect|PAGE_GUARD|isReadableProtect|VirtualQuery\(mesh\.pVertexData' --iglob 'Upscaling.cpp'As per coding guidelines,
src/**/*.{h,hpp,cpp}: Include proper resource management and graceful degradation for DirectX 11 resources and user input validation to prevent crashes from malformed configurations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Upscaling.cpp` around lines 1278 - 1287, The current VirtualQuery check in BuildHMDMesh accepts pages with PAGE_GUARD or PAGE_NOACCESS because those flags can coexist with readable bits; update the if condition around VirtualQuery(mesh.pVertexData, &mbi, ...) to explicitly reject guarded or no‑access pages by ensuring (mbi.Protect & (PAGE_GUARD | PAGE_NOACCESS)) == 0 before testing readability (kReadableProtect), e.g. add that explicit test to the compound if that references mbi.Protect and kReadableProtect so guarded/noaccess pages are declined.
🧹 Nitpick comments (1)
src/Utils/VRUtils.h (1)
39-40: Optional: add doxygen comments for the new per-eye FOV accessors.The rest of the header uses
@brief/@param/@returndoxygen for public APIs. The newGetPerEyeVerticalFOVRad/GetPerEyeHorizontalFOVRaddeclarations have no docs — in particular, the expectedeyeIndexconvention (0 = left, 1 = right?), units (radians, confirmed by name), and behavior outside VR / for invalid indices are worth documenting here to matchTryGetLastHMDPose's inline contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/VRUtils.h` around lines 39 - 40, Add Doxygen comments for the two new accessors GetPerEyeVerticalFOVRad(uint32_t eyeIndex) and GetPerEyeHorizontalFOVRad(uint32_t eyeIndex): include an `@brief` describing that they return the per-eye vertical/horizontal field-of-view in radians, an `@param` describing the eyeIndex convention (e.g., 0 = left eye, 1 = right eye) and how out-of-range indices are handled, and an `@return` describing the units (radians) and what is returned when no HMD/VR is active or the index is invalid (e.g., 0.0 or std::optional/false path). Ensure the wording matches the style and contract used by TryGetLastHMDPose (brief/param/return).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/Upscaling.cpp`:
- Around line 1106-1116: The per-eye mask dispatchs (ApplyHMDMaskToEyeInputs and
ClearHMDMask) are causing extra GPU work every VR upscaling frame; instead,
avoid separate dispatches by (1) gating the display-resolution pass so
ClearHMDMask/ApplyHMDMaskToEyeInputs only run when truly needed (derive a
needsMasking bool from vrHMDMeshVertCount[i] and vrHMDMaskRenderRes[i] and skip
if false) and (2) fusing the mask work into the existing per-eye
encode/copy-back pipeline where vrIntermediateColorIn[i] is produced/consumed
(integrate mask writes into that shader or append them to the same command
list/dispatch that already writes vrIntermediateColorIn[i] to eliminate an extra
dispatch). Use the existing depthTexture.depthSRV/depthOffset logic when merging
and only mark reactive when the mesh path indicates it (vrHMDMeshVertCount[i] >
0).
- Around line 1358-1373: The CreateBuffer/CreateShaderResourceView calls in
BuildHMDMesh currently use DX::ThrowIfFailed which will crash on optional HMD
mesh failures; change them to check HRESULTs and gracefully degrade to the
depth-fallback path: call globals::d3d::device->CreateBuffer(...) and store the
result in an HRESULT, if it fails then clear/release vrHMDMeshVB[eye] and
vrHMDMeshVBSRV[eye], optionally log the failure, and return/continue so the
existing depth fallback is used instead of throwing; do the same for
globals::d3d::device->CreateShaderResourceView(...) (replace DX::ThrowIfFailed
usage with HRESULT checks) and ensure no partially-created resources are left
behind before exiting the function.
---
Duplicate comments:
In `@src/Features/Upscaling.cpp`:
- Around line 1278-1287: The current VirtualQuery check in BuildHMDMesh accepts
pages with PAGE_GUARD or PAGE_NOACCESS because those flags can coexist with
readable bits; update the if condition around VirtualQuery(mesh.pVertexData,
&mbi, ...) to explicitly reject guarded or no‑access pages by ensuring
(mbi.Protect & (PAGE_GUARD | PAGE_NOACCESS)) == 0 before testing readability
(kReadableProtect), e.g. add that explicit test to the compound if that
references mbi.Protect and kReadableProtect so guarded/noaccess pages are
declined.
---
Nitpick comments:
In `@src/Utils/VRUtils.h`:
- Around line 39-40: Add Doxygen comments for the two new accessors
GetPerEyeVerticalFOVRad(uint32_t eyeIndex) and
GetPerEyeHorizontalFOVRad(uint32_t eyeIndex): include an `@brief` describing that
they return the per-eye vertical/horizontal field-of-view in radians, an `@param`
describing the eyeIndex convention (e.g., 0 = left eye, 1 = right eye) and how
out-of-range indices are handled, and an `@return` describing the units (radians)
and what is returned when no HMD/VR is active or the index is invalid (e.g., 0.0
or std::optional/false path). Ensure the wording matches the style and contract
used by TryGetLastHMDPose (brief/param/return).
🪄 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: d486a650-b8a8-4e06-b647-3dc8aa97d5d7
📒 Files selected for processing (31)
features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterDepths.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/prefilterRadiance.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlslfeatures/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlslfeatures/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlslfeatures/Subsurface Scattering/Shaders/Features/SubsurfaceScattering.inifeatures/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlslfeatures/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/BuildStereoMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterPS.hlslfeatures/Upscaling/Shaders/Upscaling/HMDMaskRasterVS.hlslpackage/Shaders/Common/VR.hlslipackage/Shaders/VR/StereoBlendCS.hlslsrc/Features/ScreenSpaceGI.cppsrc/Features/ScreenSpaceShadows.cppsrc/Features/SubsurfaceScattering.cppsrc/Features/Upscaling.cppsrc/Features/Upscaling.hsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling/Streamline.cppsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/StereoBlend.cppsrc/Utils/Game.cppsrc/Utils/Game.hsrc/Utils/VRUtils.cppsrc/Utils/VRUtils.h
✅ Files skipped from review due to trivial changes (10)
- features/Subsurface Scattering/Shaders/Features/SubsurfaceScattering.ini
- features/Upscaling/Shaders/Upscaling/HMDMaskRasterPS.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/stereoSync.cs.hlsl
- features/Screen Space GI/Shaders/ScreenSpaceGI/upsample.cs.hlsl
- features/Subsurface Scattering/Shaders/SubsurfaceScattering/SeparableSSSCS.hlsl
- features/Upscaling/Shaders/Upscaling/ClearHMDMaskCS.hlsl
- features/Upscaling/Shaders/Upscaling/HMDMaskRasterVS.hlsl
- src/Features/VR/StereoBlend.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/blur.cs.hlsl
- src/Utils/VRUtils.cpp
🚧 Files skipped from review as they are similar to previous changes (16)
- features/Screen Space GI/Shaders/ScreenSpaceGI/radianceDisocc.cs.hlsl
- src/Features/ScreenSpaceShadows.cpp
- features/Screen Space GI/Shaders/ScreenSpaceGI/gi.cs.hlsl
- src/Features/SubsurfaceScattering.cpp
- features/Screen-Space Shadows/Shaders/ScreenSpaceShadows/StereoSyncCS.hlsl
- src/Utils/Game.h
- src/Features/VR.h
- src/Features/ScreenSpaceGI.cpp
- src/Features/Upscaling/Streamline.cpp
- src/Utils/Game.cpp
- src/Features/Upscaling/FidelityFX.cpp
- src/Features/Upscaling.h
- src/Features/VR.cpp
- package/Shaders/VR/StereoBlendCS.hlsl
- features/Upscaling/Shaders/Upscaling/BuildStereoMaskCS.hlsl
- features/Upscaling/Shaders/Upscaling/ApplyHMDMaskCS.hlsl
Comment on lines
+1358
to
+1373
| D3D11_BUFFER_DESC bd = {}; | ||
| bd.ByteWidth = vertCount * sizeof(float2); | ||
| bd.Usage = D3D11_USAGE_IMMUTABLE; | ||
| bd.BindFlags = D3D11_BIND_SHADER_RESOURCE; | ||
| bd.MiscFlags = D3D11_RESOURCE_MISC_BUFFER_STRUCTURED; | ||
| bd.StructureByteStride = sizeof(float2); | ||
|
|
||
| D3D11_SUBRESOURCE_DATA init = { ndcVerts.data(), 0, 0 }; | ||
| DX::ThrowIfFailed(globals::d3d::device->CreateBuffer(&bd, &init, vrHMDMeshVB[eye].put())); | ||
|
|
||
| D3D11_SHADER_RESOURCE_VIEW_DESC srvDesc = {}; | ||
| srvDesc.Format = DXGI_FORMAT_UNKNOWN; | ||
| srvDesc.ViewDimension = D3D11_SRV_DIMENSION_BUFFER; | ||
| srvDesc.Buffer.FirstElement = 0; | ||
| srvDesc.Buffer.NumElements = vertCount; | ||
| DX::ThrowIfFailed(globals::d3d::device->CreateShaderResourceView(vrHMDMeshVB[eye].get(), &srvDesc, vrHMDMeshVBSRV[eye].put())); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect D3D resource creation inside BuildHMDMesh.
# Expected after the fix: no DX::ThrowIfFailed around vrHMDMeshVB/vrHMDMeshVBSRV creation.
rg -n -C5 'BuildHMDMesh|vrHMDMeshVB|vrHMDMeshVBSRV|ThrowIfFailed\(globals::d3d::device->Create(Buffer|ShaderResourceView)' --iglob 'Upscaling.cpp'Repository: doodlum/skyrim-community-shaders
Length of output: 10671
Use graceful degradation instead of throwing for optional HMD mesh GPU resource creation.
The OpenVR mesh is optional—buffer/SRV creation failures should fall back to the existing depth fallback instead of crashing. Lines 1366 and 1373 use DX::ThrowIfFailed on resource creation, violating the pattern already established elsewhere in BuildHMDMesh (lines 1224, 1248, 1257, 1339) and the coding guideline requiring graceful degradation for DirectX 11 resources.
Proposed fix
- D3D11_SUBRESOURCE_DATA init = { ndcVerts.data(), 0, 0 };
- DX::ThrowIfFailed(globals::d3d::device->CreateBuffer(&bd, &init, vrHMDMeshVB[eye].put()));
+ D3D11_SUBRESOURCE_DATA init = { ndcVerts.data(), 0, 0 };
+ winrt::com_ptr<ID3D11Buffer> meshVB;
+ HRESULT hr = globals::d3d::device->CreateBuffer(&bd, &init, meshVB.put());
+ if (FAILED(hr) || !meshVB) {
+ logger::warn("[Upscaling] BuildHMDMesh: eye {} vertex buffer creation failed ({:`#x`}), using depth fallback", eye, static_cast<uint32_t>(hr));
+ continue;
+ }
D3D11_SHADER_RESOURCE_VIEW_DESC srvDesc = {};
srvDesc.Format = DXGI_FORMAT_UNKNOWN;
srvDesc.ViewDimension = D3D11_SRV_DIMENSION_BUFFER;
srvDesc.Buffer.FirstElement = 0;
srvDesc.Buffer.NumElements = vertCount;
- DX::ThrowIfFailed(globals::d3d::device->CreateShaderResourceView(vrHMDMeshVB[eye].get(), &srvDesc, vrHMDMeshVBSRV[eye].put()));
+ winrt::com_ptr<ID3D11ShaderResourceView> meshSRV;
+ hr = globals::d3d::device->CreateShaderResourceView(meshVB.get(), &srvDesc, meshSRV.put());
+ if (FAILED(hr) || !meshSRV) {
+ logger::warn("[Upscaling] BuildHMDMesh: eye {} SRV creation failed ({:`#x`}), using depth fallback", eye, static_cast<uint32_t>(hr));
+ continue;
+ }
+
+ vrHMDMeshVB[eye] = std::move(meshVB);
+ vrHMDMeshVBSRV[eye] = std::move(meshSRV);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Features/Upscaling.cpp` around lines 1358 - 1373, The
CreateBuffer/CreateShaderResourceView calls in BuildHMDMesh currently use
DX::ThrowIfFailed which will crash on optional HMD mesh failures; change them to
check HRESULTs and gracefully degrade to the depth-fallback path: call
globals::d3d::device->CreateBuffer(...) and store the result in an HRESULT, if
it fails then clear/release vrHMDMeshVB[eye] and vrHMDMeshVBSRV[eye], optionally
log the failure, and return/continue so the existing depth fallback is used
instead of throwing; do the same for
globals::d3d::device->CreateShaderResourceView(...) (replace DX::ThrowIfFailed
usage with HRESULT checks) and ensure no partially-created resources are left
behind before exiting the function.
Restore 4 blank lines incidentally removed and drop a dead #include "Globals.h" from ScreenSpaceShadows.cpp that was not required by the feature changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
Contributor
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
Collaborator
Author
|
Closing as this didn't have performance improvements. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Performance Improvements