feat(VR): add NVAPI Variable Rate Shading #2093
Conversation
|
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:
📝 WalkthroughWalkthroughA new Variable Rate Shading feature for VR is added, integrating NVIDIA's NVAPI library through CMake. The implementation includes a core VRS feature with per-eye foveal control, shading-rate pattern generation, UV-based subrect cropping utilities, and configuration persistence via JSON. Hook installation during PostPostLoad detects the runtime environment and applies render pipeline modifications. Changes
Sequence Diagram(s)sequenceDiagram
participant PostLoad as VRS::PostPostLoad
participant HookInst as Hook Installation
participant JitterHook as Main_UpdateJitter Hook
participant VRS_Feature as VRS Feature
participant Controller as NvVrsController
participant D3D as D3D11 Device/Context
participant NVAPI as NVAPI
PostLoad->>HookInst: Detect Steam/non-Steam runtime
HookInst->>JitterHook: Install Main_UpdateJitter thunk
HookInst->>HookInst: Install Main_FinishAccumulatingDispatch hook
rect rgba(100, 150, 255, 0.5)
Note over JitterHook,NVAPI: Per-Frame Rendering Path
JitterHook->>VRS_Feature: Call UpdateVRShadingRateState()
VRS_Feature->>Controller: Build Settings from VRS::Settings
VRS_Feature->>Controller: Call Update(settings, frameInfo, device, context)
Controller->>Controller: EnsureSurface()
Controller->>Controller: UpdateSurfaceData()
Controller->>D3D: Upload shading-rate pattern via UpdateSubresource
Controller->>D3D: BindSurface() - Set SRRV
Controller->>D3D: BindRateTable() - Set pixel shading rates
D3D->>NVAPI: Apply VRS via NvAPI calls
end
rect rgba(200, 100, 100, 0.5)
Note over JitterHook,NVAPI: UI Pass Disabling
HookInst->>HookInst: Main_FinishAccumulatingDispatch detects renderMode==24
HookInst->>VRS_Feature: Call DisableVRShadingRateState()
VRS_Feature->>Controller: Call Disable(context)
Controller->>D3D: Clear viewport pixel shading rates
D3D->>NVAPI: Disable VRS via NvAPI
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 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)
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. |
There was a problem hiding this comment.
Pull request overview
Adds a new VR-only Variable Rate Shading (VRS) feature using NVIDIA NVAPI to implement foveated shading via a per-tile shading-rate surface, with a shared Subrect utility for defining per-eye regions and optional diagnostics/visualization.
Changes:
- Introduces a new
VRSfeature (registration + VR-only hook installation) that updates NVAPI VRS state each frame and disables VRS for UI passes. - Implements CPU-side SRS (shading rate surface) pattern generation + NVAPI LUT management + a DX11 NVAPI controller that creates/binds the shading-rate surface and per-viewport rate tables.
- Adds a reusable
Subrect::Controllereditor/serializer for per-eye UV regions and preset management, plus build-system wiring for NVAPI headers/lib.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Globals.h | Declares global VRS feature instance. |
| src/Globals.cpp | Instantiates globals::features::vrs and includes VRS header. |
| src/Feature.cpp | Registers vrs in the feature list so it participates in lifecycle/UI. |
| src/Features/VRS.h | Defines the new VRS feature API, settings, and hook thunks. |
| src/Features/VRS.cpp | Installs VR-only hooks and drives per-frame VRS enable/disable + UI. |
| src/Features/VRS/NvVrsController.h | Declares NVAPI DX11 controller for shading-rate surface + LUT binding + diagnostics. |
| src/Features/VRS/NvVrsController.cpp | Implements NVAPI initialization, surface creation/upload, and per-viewport rate binding. |
| src/Features/VRS/VrsSrsBuilder.h | Declares CPU SRS ring-pattern builder and SrsLevel/RingRate types. |
| src/Features/VRS/VrsSrsBuilder.cpp | Implements ring-pattern generation, boundary dithering, and debug visualization mapping. |
| src/Features/VRS/VrsLutManager.h | Declares LUT fill helpers for NVAPI shading-rate tables. |
| src/Features/VRS/VrsLutManager.cpp | Implements LUT presets/mapping from SrsLevel indices to NVAPI rates. |
| src/Features/Subrect/Subrect.h | Adds Subrect controller + preset + UV/pixel region types. |
| src/Features/Subrect/Subrect.cpp | Implements Subrect preset IO and ImGui editor/drag-crop UI. |
| features/VRS/Shaders/Features/VRS.ini | Adds feature shader INI metadata (version). |
| extern/nvapi/developer/* | Vendors NVAPI “lite” headers (and related files) into the repo. |
| CMakeLists.txt | Adds NVAPI include path + links nvapi64.lib for VRS support. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Feature.cpp (1)
212-240:⚠️ Potential issue | 🟠 MajorKeep
vrsout of the flat-screen feature list.
featuresis returned unchanged for non-VR builds, so adding&globals::features::vrshere makes this VR-only feature participate in load/settings/UI on SE/AE too. TheSupportsVR()filtering only happens in the VR branch.Suggested fix
static std::vector<Feature*> features = { &globals::features::volumetricShadows, @@ &globals::features::extendedTranslucency, &globals::features::upscaling, - &globals::features::vrs, &globals::features::renderDoc, @@ static auto BuildVRList = []() -> std::vector<Feature*> { auto v = features; + v.push_back(&globals::features::vrs); v.push_back(&globals::features::vr);As per coding guidelines, "Ensure code changes work across SE/AE/VR Skyrim variants using runtime detection and
REL::RelocateMember()patterns for compatibility".Also applies to: 249-276
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Feature.cpp` around lines 212 - 240, The flat-screen static features list currently includes &globals::features::vrs which causes the VR-only feature to appear in SE/AE; remove &globals::features::vrs from the static std::vector<Feature*> features and instead register or push it only in the VR-specific branch where SupportsVR() is checked (or via the existing VR-only initialization path referenced later around the VR branch), using the runtime detection/REL::RelocateMember() pattern the project uses to ensure compatibility across SE/AE/VR; also apply the same removal/fix for the other occurrence noted (around lines 249–276) so vrs is never added to non-VR builds.
🧹 Nitpick comments (4)
src/Features/Subrect/Subrect.cpp (1)
267-278: Integer division may truncate on odd texture widths.
fullTextureWidth / 2truncates if the width is odd. This is unlikely in practice (VR textures are typically power-of-two or even), but you could use(fullTextureWidth + 1) / 2for ceiling division if pixel-perfect accuracy matters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/Subrect/Subrect.cpp` around lines 267 - 278, The code in Controller::GetLeftEyePixelRegion and Controller::GetStereoPixelRegions passes fullTextureWidth / 2 to UVToPixelRegion which truncates for odd widths; change both calls to use a ceiling half calculation such as (fullTextureWidth + 1) / 2 (keeping types as uint32_t) so the left/right eye pixel regions computed from currentLeftEyeUV and currentRightEyeUV are pixel-accurate for odd texture widths.src/Features/VRS/NvVrsController.cpp (1)
167-171: Inconsistent resource management: raw pointer vs ComPtr.
shadingRateResourceViewis a raw pointer requiring manualRelease(), whileshadingRateSurfaceusesComPtr. Consider usingComPtr<ID3D11NvShadingRateResourceView>for consistency and safer cleanup, or document why the raw pointer is necessary (e.g., NVAPI interface requirements).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRS/NvVrsController.cpp` around lines 167 - 171, The code mixes raw pointer cleanup for shadingRateResourceView with ComPtr usage for shadingRateSurface; change shadingRateResourceView to a ComPtr<ID3D11NvShadingRateResourceView> (e.g., Microsoft::WRL::ComPtr) where it is declared, include the appropriate header, and replace the manual Release()/nullptr logic in NvVrsController.cpp with shadingRateResourceView.Reset(); alternatively, if a raw pointer is required by an NVAPI call, add a concise comment documenting that requirement and ensure the raw pointer is only used transiently while owning storage remains a ComPtr.src/Features/VRS.h (1)
47-70: Consider usingboolfor enable flags anduint32_tfor consistency.
vrEnableVRSisuintwith values 0/1, which could bebool. Similarly, otheruintmembers could beuint32_tfor explicit sizing. However, if this matches existing JSON/INI serialization patterns in the codebase, the current approach may be intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRS.h` around lines 47 - 70, Change the integer-typed enable/enum members in Settings to explicit types: make vrEnableVRS a bool (instead of uint) and change vrVRSSrsPreset and vrVRSLutPreset from uint to uint32_t to ensure fixed-width semantics; keep existing bool members (vrEnableDirectionalRates, vrEnableBoundaryDither, vrEnableDiagnostics) unchanged. Update any serialization/read/write code that references Settings::vrEnableVRS, Settings::vrVRSSrsPreset, or Settings::vrVRSLutPreset to handle the new types (e.g., treat vrEnableVRS as a boolean and cast/parse uint32_t presets where needed) so external JSON/INI round-trips remain compatible. Ensure includes for <cstdint> are present if not already.src/Features/VRS/NvVrsController.h (1)
96-98: Raw pointer for NVAPI resource view.The
ID3D11NvShadingRateResourceView*is a raw pointer while other D3D resources useComPtr. This requires careful manual lifetime management inReleaseResources()and the destructor. Document this choice or consider if NVAPI supports ComPtr wrapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRS/NvVrsController.h` around lines 96 - 98, The field shadingRateResourceView is a raw pointer while other D3D objects use ComPtr; either convert it to a COM smart pointer (e.g., Microsoft::WRL::ComPtr<ID3D11NvShadingRateResourceView>) so automatic reference counting manages its lifetime, or if NVAPI/this interface cannot be wrapped, explicitly release and null it in ReleaseResources() and the NvVrsController destructor and add a short comment explaining why a raw pointer is used; update the code that assigns/releases shadingRateResourceView to call Release() and reset to nullptr (or replace those sites to use ComPtr::Reset()) to prevent leaks and dangling pointers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 78-86: The plugin currently links nvapi64.lib directly which
creates a hard load-time dependency; instead keep the include
(target_include_directories for NvAPI) but change the linking so nvapi64 is
delay-loaded: remove the direct PRIVATE link that forces load, add the import
library as a normal link but add the MSVC linker flag "/DELAYLOAD:nvapi64.dll"
via target_link_options (or set_target_properties LINK_FLAGS) and also link
delayimp.lib (e.g., target_link_libraries(${PROJECT_NAME} PRIVATE delayimp) so
the delay-load machinery works); keep the runtime checks in
NvVrsController::EnsureInitialized() to gracefully handle absent NVAPI.
In `@src/Features/VRS/VrsSrsBuilder.cpp`:
- Around line 179-182: The paint loops use renderWidth/renderHeight to iterate
but compute idx = ty * width + tx against the destination surface
(width/height), risking out-of-bounds writes; clamp the loop bounds to the
destination surface before indexing: compute tyEnd = std::min(renderHeight,
height) and txStart/txEnd = clamp(eyeOffsetX, 0, width) / std::min(eyeOffsetX +
eyeRenderWidth, width) (or similar) and use those for the for-loops (and keep
eyeMaxX reference), then compute idx only for tx/ty within the clamped bounds so
idx = ty * width + tx is always safe. Ensure you update references to eyeMaxX /
eyeOffsetX / eyeRenderWidth / renderWidth / renderHeight accordingly so the
loops never iterate past the destination surface.
---
Outside diff comments:
In `@src/Feature.cpp`:
- Around line 212-240: The flat-screen static features list currently includes
&globals::features::vrs which causes the VR-only feature to appear in SE/AE;
remove &globals::features::vrs from the static std::vector<Feature*> features
and instead register or push it only in the VR-specific branch where
SupportsVR() is checked (or via the existing VR-only initialization path
referenced later around the VR branch), using the runtime
detection/REL::RelocateMember() pattern the project uses to ensure compatibility
across SE/AE/VR; also apply the same removal/fix for the other occurrence noted
(around lines 249–276) so vrs is never added to non-VR builds.
---
Nitpick comments:
In `@src/Features/Subrect/Subrect.cpp`:
- Around line 267-278: The code in Controller::GetLeftEyePixelRegion and
Controller::GetStereoPixelRegions passes fullTextureWidth / 2 to UVToPixelRegion
which truncates for odd widths; change both calls to use a ceiling half
calculation such as (fullTextureWidth + 1) / 2 (keeping types as uint32_t) so
the left/right eye pixel regions computed from currentLeftEyeUV and
currentRightEyeUV are pixel-accurate for odd texture widths.
In `@src/Features/VRS.h`:
- Around line 47-70: Change the integer-typed enable/enum members in Settings to
explicit types: make vrEnableVRS a bool (instead of uint) and change
vrVRSSrsPreset and vrVRSLutPreset from uint to uint32_t to ensure fixed-width
semantics; keep existing bool members (vrEnableDirectionalRates,
vrEnableBoundaryDither, vrEnableDiagnostics) unchanged. Update any
serialization/read/write code that references Settings::vrEnableVRS,
Settings::vrVRSSrsPreset, or Settings::vrVRSLutPreset to handle the new types
(e.g., treat vrEnableVRS as a boolean and cast/parse uint32_t presets where
needed) so external JSON/INI round-trips remain compatible. Ensure includes for
<cstdint> are present if not already.
In `@src/Features/VRS/NvVrsController.cpp`:
- Around line 167-171: The code mixes raw pointer cleanup for
shadingRateResourceView with ComPtr usage for shadingRateSurface; change
shadingRateResourceView to a ComPtr<ID3D11NvShadingRateResourceView> (e.g.,
Microsoft::WRL::ComPtr) where it is declared, include the appropriate header,
and replace the manual Release()/nullptr logic in NvVrsController.cpp with
shadingRateResourceView.Reset(); alternatively, if a raw pointer is required by
an NVAPI call, add a concise comment documenting that requirement and ensure the
raw pointer is only used transiently while owning storage remains a ComPtr.
In `@src/Features/VRS/NvVrsController.h`:
- Around line 96-98: The field shadingRateResourceView is a raw pointer while
other D3D objects use ComPtr; either convert it to a COM smart pointer (e.g.,
Microsoft::WRL::ComPtr<ID3D11NvShadingRateResourceView>) so automatic reference
counting manages its lifetime, or if NVAPI/this interface cannot be wrapped,
explicitly release and null it in ReleaseResources() and the NvVrsController
destructor and add a short comment explaining why a raw pointer is used; update
the code that assigns/releases shadingRateResourceView to call Release() and
reset to nullptr (or replace those sites to use ComPtr::Reset()) to prevent
leaks and dangling pointers.
🪄 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: 66969e9d-abe2-4833-b1c7-b5beeb09c6dc
📒 Files selected for processing (24)
CMakeLists.txtextern/nvapi/developer/amd64/nvapi64.libextern/nvapi/developer/nvapi.hextern/nvapi/developer/nvapi_lite_common.hextern/nvapi/developer/nvapi_lite_d3dext.hextern/nvapi/developer/nvapi_lite_salend.hextern/nvapi/developer/nvapi_lite_salstart.hextern/nvapi/developer/nvapi_lite_sli.hextern/nvapi/developer/nvapi_lite_stereo.hextern/nvapi/developer/nvapi_lite_surround.hfeatures/VRS/Shaders/Features/VRS.inisrc/Feature.cppsrc/Features/Subrect/Subrect.cppsrc/Features/Subrect/Subrect.hsrc/Features/VRS.cppsrc/Features/VRS.hsrc/Features/VRS/NvVrsController.cppsrc/Features/VRS/NvVrsController.hsrc/Features/VRS/VrsLutManager.cppsrc/Features/VRS/VrsLutManager.hsrc/Features/VRS/VrsSrsBuilder.cppsrc/Features/VRS/VrsSrsBuilder.hsrc/Globals.cppsrc/Globals.h
|
Thanks. I'll review more in depth but as an initial matter dependencies should be using submodules or vcpkg with ports and only a direct check in as a last resort. We don't want to be in the business of maintaining dependencies. Also, please respect the features boundaries. I'm not sure subrect should live as it's own feature. We also have a feature constraints system to identify conflicting settings. Please see if that's needed for terrain blending. And thanks again for contributing. I saw the other PRs too. |
doodlum
left a comment
doodlum
left a comment
There was a problem hiding this comment.
sounds like a really cool addition. please fix the issues, otherwise you might be better off doing the generic form of VRS for VR.
| /// | ||
| /// Known conflicts: | ||
| /// - Terrain Blending: visual artifacts when both active. | ||
| /// - VR DepthBuffer Culling: incorrect cull combined with VRS cull ring. |
There was a problem hiding this comment.
Removed the DBC conflict note — after in-game testing, it's not a conflict at all.
In fact, VRS and DBC complement each other well:
DBC benefits from the VRS cull ring (0x pixel shading rate) — objects in the cull zone are culled more aggressively, saving additional draw overhead.
Default DBC settings (value=10) work fine with VRS without any visible artifacts.
Together they compound savings: VRS reduces shading cost in periphery, DBC culls invisible objects entirely.
I also have a follow-up idea: retrieve the HMD hidden area mesh via OpenVR and use it to shape the cull ring.
The key advantage is we don't need to know how the headset's distortion warp works — the mask boundary is already the ground truth of what's visible after warping.
By scaling the mask inward to form concentric rings, the foveal zones would match the actual post-warp visible area regardless of headset model, enabling more aggressive rate reduction. That's for a future PR though.
There was a problem hiding this comment.
To the extent it helps, I had some code pulling the mask here..
I didn't see performance gains for my PR so probably won't merge any of it in, but might save you time.
|
I went ahead and tested it to see how it looked This meant I fixed compilation errors and moved nvapi to a submodule. Some initial issues: Upscaling completely breaks the image. VRS apparently only works with DLAA, but no DLSS. We should address that. I did start a VRS port using fholger's vrperftoolkit and puredark's upscaler code. https://github.com/alandtse/skyrim-community-shaders/tree/vr_ports_vrperf_puredark. It's from 2 months ago but I believe the VRS was working in the center with upscaling (but we had massive foliage shimmering outside the center). It may be a good reference particularly if you look at the other attempts. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmake/NVAPI/CMakeLists.txt (1)
1-17: PR metadata polish: normalize title casing and add tracker reference.Suggested title:
feat(vr): add nvapi variable rate shading
If this maps to an issue, add:Implements #<id>orAddresses #<id>in the PR body.As per coding guidelines: "Conventional Commit Titles ... type(scope): description ... lowercase description" and "Issue References ... Implements/Addresses/Related to
#123".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/NVAPI/CMakeLists.txt` around lines 1 - 17, The PR metadata needs normalization: change the PR title to use conventional commit format with lowercase description and scope, e.g. "feat(vr): add nvapi variable rate shading", and update the PR body to include an issue tracker reference using "Implements #<id>" or "Addresses #<id>" as appropriate; ensure this change is reflected in any CI or changelog metadata that reads the PR title (search for NVAPI, target_link_libraries, or the CMakeLists target NVAPI to locate related PR context if needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmake/NVAPI/CMakeLists.txt`:
- Around line 5-11: The CMake rule adding NVAPI headers and a hardcoded library
path (target_include_directories and target_link_libraries for target NVAPI)
must fail fast and be platform-aware: check the host OS/architecture (e.g. WIN32
and pointer size or CMAKE_HOST_SYSTEM_PROCESSOR) and construct or select the
correct nvapi library filename (amd64 vs x86), verify the computed library file
exists before calling target_link_libraries, and if it does not exist emit a
clear message(FATAL_ERROR) explaining the missing/unsupported NVAPI binary;
alternatively use find_library to locate the library and error out with a
descriptive message if not found.
---
Nitpick comments:
In `@cmake/NVAPI/CMakeLists.txt`:
- Around line 1-17: The PR metadata needs normalization: change the PR title to
use conventional commit format with lowercase description and scope, e.g.
"feat(vr): add nvapi variable rate shading", and update the PR body to include
an issue tracker reference using "Implements #<id>" or "Addresses #<id>" as
appropriate; ensure this change is reflected in any CI or changelog metadata
that reads the PR title (search for NVAPI, target_link_libraries, or the
CMakeLists target NVAPI to locate related PR context if needed).
🪄 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: 5c339be3-52f1-43f0-8e71-d56740131474
📒 Files selected for processing (1)
cmake/NVAPI/CMakeLists.txt
|
✅ A pre-release build is available for this PR: |
|
@YtzyFvra you wanna take a stab at converting cs shaders to pixel shaders when possible? then those shaders can benefit from VRS and perform better in general |
NVIDIA VRS via NVAPI for VR. Applies foveated shading rates based on a configurable center region, reducing pixel shader overhead at high resolutions. Includes Subrect utility for VR viewport definition and LUT-based shading rate assignment. Known: enabling VRS together with Terrain Blending causes distant terrain artifacts. VRS does not affect compute shader cost. VR-only for now; flat-screen adaptation is straightforward.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
The previous commit added add_subdirectory(cmake/NVAPI) to CMakeLists.txt but never committed the cmake module itself, causing CI to fail with "not an existing directory". Also suppresses C4828 (illegal char in Latin-1 NVAPI headers) via /wd4828 on the INTERFACE target to fix C2220 build errors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
CMakeLists.txt (1)
153-153:⚠️ Potential issue | 🔴 CriticalDelay-load NVAPI so non-NVIDIA systems can still load the plugin.
NVAPIstill linksnvapi64.libthroughcmake/NVAPI/CMakeLists.txt, so this target can introduce a load-time dependency onnvapi64.dll. Runtime graceful degradation inNvVrsControllerwill not run if the DLL import prevents the plugin from loading.🔧 Suggested fix
target_link_libraries( ${PROJECT_NAME} PRIVATE @@ Tracy::TracyClient Streamline NVAPI + delayimp.lib d3d12.lib Microsoft::DirectX-Headers ${DETOURS_LIBRARY} ) + +if(MSVC) + target_link_options(${PROJECT_NAME} PRIVATE "/DELAYLOAD:nvapi64.dll") +endif()Verify the final link options include delay-load:
#!/bin/bash # Description: Check whether NVAPI is linked with MSVC delay-load support. rg -n --hidden -S 'NVAPI|nvapi64\.lib|nvapi64\.dll|DELAYLOAD|delayimp' CMakeLists.txt cmake🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 153, The NVAPI target currently forces a load-time dependency by linking nvapi64.lib; modify the cmake/NVAPI/CMakeLists.txt to apply MSVC delay-load so the plugin can load on non-NVIDIA systems: when configuring the NVAPI target (the NVAPI target and its linkage to nvapi64.lib), add the linker option /DELAYLOAD:nvapi64.dll (via target_link_options or set_target_properties LINK_FLAGS) and ensure delayimp.lib is linked for MSVC builds (e.g., target_link_libraries(NVAPI PRIVATE delayimp.lib) under MSVC); keep regular static linking for other toolchains if needed and verify NvVrsController can still build without forcing nvapi64.dll at load time.
🧹 Nitpick comments (1)
src/Features/VRS/NvVrsController.h (1)
80-98: Make the NVAPI resource owner non-copyable.
NvVrsControllerownsshadingRateResourceViewas a raw resource alongsideComPtrs and has a destructor, but the compiler can still generate copy operations. A copied controller could double-release or alias the NVAPI resource.🔒 Suggested fix
NvVrsController() = default; ~NvVrsController(); + + NvVrsController(const NvVrsController&) = delete; + NvVrsController& operator=(const NvVrsController&) = delete; + NvVrsController(NvVrsController&&) = delete; + NvVrsController& operator=(NvVrsController&&) = delete; void Update(const Settings& settings, const FrameInfo& frameInfo, ID3D11Device* device, ID3D11DeviceContext* context);As per coding guidelines,
src/**/*.{h,hpp,cpp}should include proper resource management and graceful degradation for DirectX 11 resources.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRS/NvVrsController.h` around lines 80 - 98, NvVrsController currently owns a raw NVAPI resource (shadingRateResourceView) but allows compiler-generated copies, risking double-release; make the class non-copyable by deleting the copy constructor and copy assignment operator on NvVrsController, and optionally provide move constructor and move assignment that transfer ownership of shadingRateResourceView (setting the source's shadingRateResourceView to nullptr) so moves are safe; ensure the existing destructor still releases the NVAPI resource when non-null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakeLists.txt`:
- Around line 97-102: The CMakeLists hardcodes a vcpkg triplet include path for
${PROJECT_NAME}, which conflicts with custom vcpkg setups; remove the literal
"${CMAKE_BINARY_DIR}/vcpkg_installed/..." include and either drop those lines
entirely and rely on linking Microsoft::DirectX-Headers, or retrieve the
imported target's include dirs and apply them with
get_target_property(_directx_headers_includes Microsoft::DirectX-Headers
INTERFACE_INCLUDE_DIRECTORIES) followed by
target_include_directories(${PROJECT_NAME} BEFORE PRIVATE
${_directx_headers_includes}) to preserve include-order priority.
In `@src/Features/VRS.cpp`:
- Around line 285-289: DisableVRShadingRateState currently unconditionally
stamps the last disable reason as UIPass which hides real causes; change
DisableVRShadingRateState so it only calls
nvVrs.SetLastDisableReason(NvVrsController::DisableReason::UIPass) when VRS was
actually active right before the UI disable—e.g. check the controller state with
a predicate like nvVrs.IsActive() or nvVrs.IsEnabled() (or the appropriate
method on NvVrsController if named differently) and only set the UIPass reason
inside that conditional, then call nvVrs.Disable(globals::d3d::context) as
before.
- Around line 71-83: Implement GetActiveConstraints() in the VRS feature to
register the VRS vs Terrain Blending incompatibility: detect when
settings.vrEnableVRS is enabled and return a FeatureConstraint (using the
pattern from FeatureConstraints.h and existing GetActiveConstraints()
implementations referenced in Utils/UI.cpp) that disables or degrades the
Terrain Blending setting when both are active; ensure the constraint ID, message
and affected setting match the established convention so the constraint-aware UI
will prevent persisting the incompatible configuration.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Line 153: The NVAPI target currently forces a load-time dependency by linking
nvapi64.lib; modify the cmake/NVAPI/CMakeLists.txt to apply MSVC delay-load so
the plugin can load on non-NVIDIA systems: when configuring the NVAPI target
(the NVAPI target and its linkage to nvapi64.lib), add the linker option
/DELAYLOAD:nvapi64.dll (via target_link_options or set_target_properties
LINK_FLAGS) and ensure delayimp.lib is linked for MSVC builds (e.g.,
target_link_libraries(NVAPI PRIVATE delayimp.lib) under MSVC); keep regular
static linking for other toolchains if needed and verify NvVrsController can
still build without forcing nvapi64.dll at load time.
---
Nitpick comments:
In `@src/Features/VRS/NvVrsController.h`:
- Around line 80-98: NvVrsController currently owns a raw NVAPI resource
(shadingRateResourceView) but allows compiler-generated copies, risking
double-release; make the class non-copyable by deleting the copy constructor and
copy assignment operator on NvVrsController, and optionally provide move
constructor and move assignment that transfer ownership of
shadingRateResourceView (setting the source's shadingRateResourceView to
nullptr) so moves are safe; ensure the existing destructor still releases the
NVAPI resource when non-null.
🪄 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: ea8f72e6-e92c-4e40-92b5-f5504fe2b682
📒 Files selected for processing (19)
.gitmodulesCMakeLists.txtcmake/NVAPI/CMakeLists.txtextern/nvapifeatures/VRS/Shaders/Features/VRS.inisrc/Feature.cppsrc/Features/VRS.cppsrc/Features/VRS.hsrc/Features/VRS/NvVrsController.cppsrc/Features/VRS/NvVrsController.hsrc/Features/VRS/VrsLutManager.cppsrc/Features/VRS/VrsLutManager.hsrc/Features/VRS/VrsSrsBuilder.cppsrc/Features/VRS/VrsSrsBuilder.hsrc/Globals.cppsrc/Globals.hsrc/Utils/Subrect/Subrect.cppsrc/Utils/Subrect/Subrect.hvcpkg.json
✅ Files skipped from review due to trivial changes (6)
- vcpkg.json
- features/VRS/Shaders/Features/VRS.ini
- .gitmodules
- extern/nvapi
- src/Globals.cpp
- src/Features/VRS/VrsLutManager.h
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Feature.cpp
- cmake/NVAPI/CMakeLists.txt
- src/Features/VRS/VrsLutManager.cpp
- src/Utils/Subrect/Subrect.h
- src/Features/VRS/VrsSrsBuilder.cpp
- src/Utils/Subrect/Subrect.cpp
- src/Features/VRS/NvVrsController.cpp
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: 3
♻️ Duplicate comments (1)
CMakeLists.txt (1)
144-144:⚠️ Potential issue | 🟠 MajorNVAPI is still hard-linked; consider delay-load so the plugin loads on non‑NVIDIA systems.
The NVAPI linkage moved into
cmake/NVAPI/CMakeLists.txt, butnvapi64.libis still linked as a normal import library. On a system withoutnvapi64.dll(AMD/Intel GPUs, or NVIDIA machines with a stripped driver), the whole SKSE plugin will fail to load at import resolution time andNvVrsController::EnsureInitialized()’s runtime guards will never run. Adding/DELAYLOAD:nvapi64.dllplusdelayimp.libon the main target lets the plugin gracefully degrade when NVAPI isn’t present.Suggested fix (applied to the main target in this file)
if(MSVC) target_link_options( ${PROJECT_NAME} PRIVATE "$<$<CONFIG:RelWithDebInfo>:/LTCG>" "$<$<CONFIG:RelWithDebInfo>:/INCREMENTAL:NO>" + "/DELAYLOAD:nvapi64.dll" ) + target_link_libraries(${PROJECT_NAME} PRIVATE delayimp.lib) endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CMakeLists.txt` at line 144, The project currently hard-links nvapi64.lib which prevents the SKSE plugin from loading on systems without nvapi64.dll and blocks runtime guards like NvVrsController::EnsureInitialized(); update the main target's linker settings so nvapi is delay-loaded: keep linking to nvapi64.lib but add the MSVC linker flag /DELAYLOAD:nvapi64.dll (via target_link_options or add_link_options) and ensure delayimp.lib is linked (e.g., add delayimp.lib to target_link_libraries for the same target) so the plugin can start on non‑NVIDIA systems and let NvVrsController::EnsureInitialized() run to detect absence of NVAPI.
🧹 Nitpick comments (4)
src/Utils/Subrect/Subrect.cpp (2)
183-189: Double‑check the delete flow — the "built‑in default" assumption is implicit.Delete is only offered when
selectedPresetIndex > 0, under the implicit assumption that index 0 is always the auto-created "Full Left Eye" preset. That invariant is only maintained byEnsureDefaultPresetbeing called at the top ofApplyPreset/LoadSettings; nothing enforces that user-created presets can't end up at index 0 (e.g. after reordering, or if a future refactor removesEnsureDefaultPresetbefore apush_back). Consider marking the default explicitly (e.g., abool isBuiltinonPreset) and guarding deletion against that, instead of on the positional index.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/Subrect/Subrect.cpp` around lines 183 - 189, The delete guard currently relies on positional invariant (selectedPresetIndex > 0) which can break; instead add a persistent flag to the Preset type (e.g., bool isBuiltin) and set it for the default in EnsureDefaultPreset and when loading/creating builtin presets; update the delete button logic to check presets[selectedPresetIndex].isBuiltin before enabling/removing and prevent deletion if true; ensure ApplyPreset and LoadSettings preserve/initialize isBuiltin for all presets so the built‑in preset cannot be removed regardless of index or reorder.
194-195:SliderFloat2relies onUVRegionhaving contiguousfloatmembers.
ImGui::SliderFloat2("...", ¤tLeftEyeUV.x, ...)and the follow-up¤tLeftEyeUV.wassumeUVRegion { x, y, w, h }is laid out as 4 contiguous floats with no padding. That's true today, but it's a silent footgun if anyone ever reorders or adds a member. Astatic_assert(sizeof(UVRegion) == 4 * sizeof(float))(and offset checks) next to the struct, or using a temporaryfloat[2]round-trip, would make this safe against future changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/Subrect/Subrect.cpp` around lines 194 - 195, The ImGui calls assume UVRegion is tightly-packed as four contiguous floats (used via ¤tLeftEyeUV.x and ¤tLeftEyeUV.w), which is fragile; either add compile-time layout checks near the UVRegion definition (e.g., static_assert on sizeof(UVRegion) == 4 * sizeof(float) and optional offsetof checks for x,y,w,h) or avoid taking interior member addresses by using a temporary float[2] buffer and round-tripping values into currentLeftEyeUV around the ImGui::SliderFloat2 calls (use temp arrays for position and size, call SliderFloat2 with them, then copy back into currentLeftEyeUV.x/y and .w/.h). Ensure references to UVRegion, currentLeftEyeUV, and ImGui::SliderFloat2 are updated accordingly.src/Utils/Subrect/Subrect.h (1)
1-5: Header relies on transitive includes forjsonand D3D11 types.
json(lines 39-40) andID3D11ShaderResourceView*/ID3D11Texture2D*(line 41) are referenced in the public API without any include or forward declaration. It currently works because consumers (VRS.hviaFeature.h) pull these in first, but any reuse ofSubrect.houtside that context will break. Consider adding the minimal forward declarations / includes (e.g.,<d3d11.h>and the project’s json header, or forward-declare the D3D11 interfaces).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Utils/Subrect/Subrect.h` around lines 1 - 5, The header Subrect.h exposes json and D3D11 types in its public API but lacks includes/forward-declarations; add either a minimal include for your project's JSON header (or forward-declare nlohmann::json if that's used) and forward-declare the D3D11 interfaces (ID3D11ShaderResourceView and ID3D11Texture2D) or include <d3d11.h>, then update any references (e.g., where json is used around lines 39-40 and the ID3D11ShaderResourceView*/ID3D11Texture2D* on line 41) so the header is self-contained and no longer depends on transitive includes.src/Features/VRS.h (1)
47-70: Useboolfor binary toggles, and preferuint32_toveruintfor the preset indices.
vrEnableVRSis documented as0 = Disabled, 1 = Enabledand is forwarded toNvVrsController::Settings::enable(abool) — it should just beboolto match. Similarly,uintis not a standard C/C++ type; useuint32_tfor portability and consistency with theNvVrsController::Settingsstruct, which already usesuint32_tforsrsPreset/lutPreset.Suggested fix
- /// 0 = Disabled, 1 = Enabled. When off, controller unbinds surface and sends 1×1 LUT. - uint vrEnableVRS = 1; + /// When false, controller unbinds surface and sends 1×1 LUT. + bool vrEnableVRS = true; - /// SRS ring rate preset: 0=Default (6-step), 1=Faster (4-step), 2=Extreme (3-step). - uint vrVRSSrsPreset = 0; + /// SRS ring rate preset: 0=Default (6-step), 1=Faster (4-step), 2=Extreme (3-step). + uint32_t vrVRSSrsPreset = 0; - /// LUT debug override: 0=Default (1:1 mapping), 1=Full 1×1, 2=Full 4×4. - uint vrVRSLutPreset = 0; + /// LUT debug override: 0=Default (1:1 mapping), 1=Full 1×1, 2=Full 4×4. + uint32_t vrVRSLutPreset = 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRS.h` around lines 47 - 70, Change the binary toggle and preset index types in the Settings struct to match NvVrsController::Settings: make vrEnableVRS a bool (since it forwards to NvVrsController::Settings::enable) and change vrVRSSrsPreset and vrVRSLutPreset from uint to uint32_t (to match srsPreset/lutPreset and use a standard fixed-width type); keep the existing default values and update any includes or headers if needed to ensure uint32_t is defined.
🤖 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/VRS.h`:
- Around line 85-87: The field hardwareAvailable_ is set in PostPostLoad()
without verifying NVAPI, so hook thunks
(UpdateVRShadingRateState/DisableVRShadingRateState) can call into
NvVrsController::EnsureInitialized() every frame on unsupported GPUs; either add
a guard in the thunks to check hardwareAvailable_ before invoking
NvVrsController::Update()/DisableVRShadingRateState(), or move NVAPI
init/capability verification into PostPostLoad() and set hardwareAvailable_
(rename to nvrsSupported or nvrsSupported without trailing underscore) only
after successful init; update references to hardwareAvailable_ and the hook
thunk call-sites accordingly (look for PostPostLoad, UpdateVRShadingRateState,
DisableVRShadingRateState, and NvVrsController::Update/EnsureInitialized) so
unsupported systems never dispatch those update calls.
In `@src/Utils/Subrect/Subrect.cpp`:
- Around line 52-62: The function UVToPixelRegion can underflow when eyeWidth or
eyeHeight is 0 because expressions like eyeWidth - 1 wrap as unsigned; fix by
guarding that case up-front in UVToPixelRegion: if eyeWidth == 0 || eyeHeight ==
0 return a safe zero-sized Subrect::PixelRegion (set result.x = result.y =
result.w = result.h = 0). Keep the rest of the logic unchanged for non-zero
sizes so calculations for result.x, result.y, result.w and result.h use the
existing clamps.
- Line 220: The ImGui::Image calls cast GPU SRV pointers with
reinterpret_cast<ImTextureID>(previewSrv) which truncates pointers on 64-bit
ImGui builds; replace those casts so ImGui receives a void* (or use an intptr_t
intermediary) — update the call using the same safe pattern as other files (pass
(void*)previewSrv or cast via (ImTextureID)(intptr_t)previewSrv) for the
ImGui::Image invocation in Subrect.cpp (symbol: previewSrv) and make the
analogous change where debugSRV is passed in Features/VRS.cpp so no pointer
truncation occurs.
---
Duplicate comments:
In `@CMakeLists.txt`:
- Line 144: The project currently hard-links nvapi64.lib which prevents the SKSE
plugin from loading on systems without nvapi64.dll and blocks runtime guards
like NvVrsController::EnsureInitialized(); update the main target's linker
settings so nvapi is delay-loaded: keep linking to nvapi64.lib but add the MSVC
linker flag /DELAYLOAD:nvapi64.dll (via target_link_options or add_link_options)
and ensure delayimp.lib is linked (e.g., add delayimp.lib to
target_link_libraries for the same target) so the plugin can start on non‑NVIDIA
systems and let NvVrsController::EnsureInitialized() run to detect absence of
NVAPI.
---
Nitpick comments:
In `@src/Features/VRS.h`:
- Around line 47-70: Change the binary toggle and preset index types in the
Settings struct to match NvVrsController::Settings: make vrEnableVRS a bool
(since it forwards to NvVrsController::Settings::enable) and change
vrVRSSrsPreset and vrVRSLutPreset from uint to uint32_t (to match
srsPreset/lutPreset and use a standard fixed-width type); keep the existing
default values and update any includes or headers if needed to ensure uint32_t
is defined.
In `@src/Utils/Subrect/Subrect.cpp`:
- Around line 183-189: The delete guard currently relies on positional invariant
(selectedPresetIndex > 0) which can break; instead add a persistent flag to the
Preset type (e.g., bool isBuiltin) and set it for the default in
EnsureDefaultPreset and when loading/creating builtin presets; update the delete
button logic to check presets[selectedPresetIndex].isBuiltin before
enabling/removing and prevent deletion if true; ensure ApplyPreset and
LoadSettings preserve/initialize isBuiltin for all presets so the built‑in
preset cannot be removed regardless of index or reorder.
- Around line 194-195: The ImGui calls assume UVRegion is tightly-packed as four
contiguous floats (used via ¤tLeftEyeUV.x and ¤tLeftEyeUV.w), which
is fragile; either add compile-time layout checks near the UVRegion definition
(e.g., static_assert on sizeof(UVRegion) == 4 * sizeof(float) and optional
offsetof checks for x,y,w,h) or avoid taking interior member addresses by using
a temporary float[2] buffer and round-tripping values into currentLeftEyeUV
around the ImGui::SliderFloat2 calls (use temp arrays for position and size,
call SliderFloat2 with them, then copy back into currentLeftEyeUV.x/y and
.w/.h). Ensure references to UVRegion, currentLeftEyeUV, and ImGui::SliderFloat2
are updated accordingly.
In `@src/Utils/Subrect/Subrect.h`:
- Around line 1-5: The header Subrect.h exposes json and D3D11 types in its
public API but lacks includes/forward-declarations; add either a minimal include
for your project's JSON header (or forward-declare nlohmann::json if that's
used) and forward-declare the D3D11 interfaces (ID3D11ShaderResourceView and
ID3D11Texture2D) or include <d3d11.h>, then update any references (e.g., where
json is used around lines 39-40 and the
ID3D11ShaderResourceView*/ID3D11Texture2D* on line 41) so the header is
self-contained and no longer depends on transitive includes.
🪄 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: 92733d09-567b-430f-8be2-d725fd4f1e62
📒 Files selected for processing (4)
CMakeLists.txtsrc/Features/VRS.hsrc/Utils/Subrect/Subrect.cppsrc/Utils/Subrect/Subrect.h
|
Thanks for fixing the submodule and compilation — cherry-picked both commits. After rebasing I've tested with the default CS Upscaling pipeline and everything works correctly — SRS pattern and final output both look as expected (see screenshots). Regarding foliage shimmering — that issue doesn't appear here. |
Yes, been wanting to do this! |
…sses Terrain Blending's deferred re-render uses SV_Position to sample a full-res depth texture. Under VRS the centroid position is broadcast to the entire coarse pixel block, producing incorrect blend factors and white terrain artifacts. Add NvVrsController::Suspend/Resume (unbind without dirtying state) and wrap TerrainBlending::RenderTerrainBlendingPasses() in Deferred.cpp so terrain always shades at 1x1. Cost: 2 NVAPI state switches/frame.
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Awesome. I have ported the deferredcompositecs to pixel shader. This should give you a head start with some shared utility code. |
doodlum
changed the title
alandtse
added
deferred
NVIDIA VRS via NVAPI for VR.
Applies foveated shading rates based on a configurable center region, reducing pixel shader overhead at high resolutions.
Includes Subrect utility for VR viewport definition and LUT-based shading rate assignment.
Known: enabling VRS together with Terrain Blending causes distant terrain artifacts.
VRS does not affect compute shader cost. further adaptation needed.
Summary by CodeRabbit
Release Notes
New Features
Chores