feat(upscaling): add nis sharpening#1457
Conversation
WalkthroughAdds NVIDIA Image Sharpening (NIS) into the upscaling pipeline: new Upscaling settings and UI, resource creation for a NIS sharpener texture, Streamline feature detection/binding and ApplyNISSharpening calls integrated into post-processing; disables FidelityFX internal sharpening. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Settings UI
participant Ups as Upscaling
participant SL as Streamline
participant GPU as D3D Device
UI->>Ups: set enableNISSharpening, nisSharpness
Ups->>Ups: perform upscaling pass
note over Ups: Post-upscale processing
Ups->>Ups: ApplyNISSharpening()
alt NIS enabled && SL.featureNIS
Ups->>SL: ApplyNISSharpening(mainRT, nisSharpness)
SL->>SL: slNISSetOptions(mode=Sharpen, strength)
SL->>GPU: bind input/output, slEvaluateFeature(NIS)
SL-->>Ups: writes sharpened result via nisSharpenerTexture -> mainRT
else NIS disabled or unsupported
Ups-->>Ups: skip NIS sharpening
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/Upscaling.cpp (1)
14-23: Settings (de)serialization missing new NIS fields — values won’t persist across runs.enableNISSharpening and nisSharpness are not included in the NLOHMANN macro, so SaveSettings/LoadSettings won’t persist them.
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT( Upscaling::Settings, upscaleMethod, upscaleMethodNoDLSS, qualityMode, frameLimitMode, frameGenerationMode, frameGenerationForceEnable, - streamlineLogLevel); + streamlineLogLevel, + enableNISSharpening, + nisSharpness);
🧹 Nitpick comments (8)
src/Features/Upscaling/FidelityFX.cpp (1)
290-292: Disable FSR sharpening to avoid double-sharpening — consider conditional fallback.Turning off FSR sharpening is correct given the new NIS pass. As an optional improvement, you could fall back to FSR sharpening when NIS is disabled or unavailable so users still get sharpening on non-NVIDIA systems.
- dispatchUpscale.enableSharpening = false; - dispatchUpscale.sharpness = 0.0f; + const bool nisActive = globals::features::upscaling.settings.enableNISSharpening != 0 + && globals::features::upscaling.streamline.featureNIS; + dispatchUpscale.enableSharpening = !nisActive; + dispatchUpscale.sharpness = dispatchUpscale.enableSharpening ? std::clamp(globals::features::upscaling.settings.nisSharpness, 0.0f, 1.0f) : 0.0f;src/Features/Upscaling/Streamline.h (1)
17-17: NIS integration looks good; align default sharpness for consistency.Header wiring is consistent with DLSS. Minor nit: default sharpness here (0.3f) differs from Upscaling settings default (0.15f). Align to avoid confusion.
- void ApplyNISSharpening(ID3D11Resource* a_texture, float sharpness = 0.3f); + void ApplyNISSharpening(ID3D11Resource* a_texture, float sharpness = 0.15f);Also applies to: 35-36, 65-68, 90-91
src/Features/Upscaling.h (1)
58-61: New NIS settings added — clamp on load to protect against bad INI/JSON values.Consider clamping nisSharpness (0..1) during LoadSettings to guard against out-of-range persisted values.
src/Features/Upscaling.cpp (3)
202-228: NIS UI section — solid; small UX tweak optional.Looks good. Optional: use ImGui::Checkbox for the on/off toggle for consistency with other boolean options.
1535-1537: Sharpen after upscaling — correct ordering; add a GPU marker for profiling.Add Begin/End perf marker like other stages for consistent GPU timing.
void Upscaling::PerformUpscaling() { Upscale(); - ApplyNISSharpening(); + globals::state->BeginPerfEvent("NIS Sharpening"); + ApplyNISSharpening(); + globals::state->EndPerfEvent();
1716-1727: ApplyNISSharpening() gate is fine; clamp sharpness and early-outs are OK.Consider also clamping settings.nisSharpness at load time (see earlier comment) and wrapping this with perf markers if not added in PerformUpscaling().
src/Features/Upscaling/Streamline.cpp (2)
105-110: numFeaturesToLoad should match the selected array.Currently hard-coded to _countof(featuresToLoad). If VR arrays diverge later, this will be wrong. Use the selected array’s count.
- pref.featuresToLoad = REL::Module::IsVR() ? featuresToLoadVR : featuresToLoad; - pref.numFeaturesToLoad = _countof(featuresToLoad); + const bool isVR = REL::Module::IsVR(); + pref.featuresToLoad = isVR ? featuresToLoadVR : featuresToLoad; + pref.numFeaturesToLoad = static_cast<uint32_t>(isVR ? _countof(featuresToLoadVR) : _countof(featuresToLoad));
418-454: ApplyNISSharpening implementation — add missing include and perf marker; improve error logs.
- std::clamp requires (unless transitively included).
- Optional: add GPU marker for profiling.
- Optional: log the SL error code for easier triage.
@@ -#include "../../Util.h" +#include "../../Util.h" +#include <algorithm> @@ - if (SL_FAILED(result, slNISSetOptions(viewport, nisOptions))) { - logger::error("[Streamline] Could not set NIS options"); + if (SL_FAILED(result, slNISSetOptions(viewport, nisOptions))) { + logger::error("[Streamline] Could not set NIS options (rc={})", (int)result); return; } @@ - const sl::BaseStructure* inputs[] = { &view }; - if (SL_FAILED(result, slEvaluateFeature(sl::kFeatureNIS, *frameToken, inputs, _countof(inputs), globals::d3d::context))) { - logger::error("[Streamline] Failed to evaluate NIS feature"); + const sl::BaseStructure* inputs[] = { &view }; + globals::state->BeginPerfEvent("NIS Sharpening"); + if (SL_FAILED(result, slEvaluateFeature(sl::kFeatureNIS, *frameToken, inputs, _countof(inputs), globals::d3d::context))) { + logger::error("[Streamline] Failed to evaluate NIS feature (rc={})", (int)result); } + globals::state->EndPerfEvent();
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
features/Upscaling/Shaders/Upscaling/Streamline/sl.nis.dllis excluded by!**/*.dll
📒 Files selected for processing (5)
src/Features/Upscaling.cpp(3 hunks)src/Features/Upscaling.h(2 hunks)src/Features/Upscaling/FidelityFX.cpp(1 hunks)src/Features/Upscaling/Streamline.cpp(4 hunks)src/Features/Upscaling/Streamline.h(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/Upscaling.hsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling/Streamline.hsrc/Features/Upscaling/Streamline.cppsrc/Features/Upscaling.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Upscaling.hsrc/Features/Upscaling/FidelityFX.cppsrc/Features/Upscaling/Streamline.hsrc/Features/Upscaling/Streamline.cppsrc/Features/Upscaling.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
🔇 Additional comments (3)
src/Features/Upscaling.h (1)
128-129: Public ApplyNISSharpening() declaration — OK.Matches the new pipeline order added in PerformUpscaling().
src/Features/Upscaling/Streamline.cpp (2)
184-199: NIS feature discovery and logging — looks correct.Loaded/supported checks mirror DLSS; helpful diagnostics.
211-215: Verify PostDevice() precedes first NIS call — slNISSetOptions and slNISGetState remain null until PostDevice() (or equivalent backend‐device setup) runs; confirm your app startup invokes this before any NIS feature use.
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Features/Upscaling.cpp (1)
14-22: Add NIS settings to JSON serializationWithout these, toggles won’t persist across sessions.
Apply this diff:
NLOHMANN_DEFINE_TYPE_NON_INTRUSIVE_WITH_DEFAULT( Upscaling::Settings, upscaleMethod, upscaleMethodNoDLSS, qualityMode, frameLimitMode, frameGenerationMode, frameGenerationForceEnable, - streamlineLogLevel); + streamlineLogLevel, + enableNISSharpening, + nisSharpness);
🧹 Nitpick comments (3)
src/Features/Upscaling.h (1)
135-135: Handle nisSharpenerTexture lifetime and resizingThis texture is created in SetupResources but never released or resized explicitly. Tie its (re)creation to resolution changes and free it during teardown/resource switches.
Apply outside-range changes:
- Delete and recreate nisSharpenerTexture when the backbuffer size/format changes (e.g., in CheckResources or UpdateSharedResources).
- Release it during shutdown or when switching to paths where NIS is unused.
src/Features/Upscaling.cpp (2)
202-229: UI is clear; minor UX polish optionalConsider ImGui::Checkbox for the toggle to simplify and reduce range checks; current SliderInt works though.
Example:
- ImGui::SliderInt("Enable NIS Sharpening", (int*)&settings.enableNISSharpening, 0, 1, nisToggleModes[settings.enableNISSharpening]); + bool nisEnabled = settings.enableNISSharpening != 0; + if (ImGui::Checkbox("Enable NIS Sharpening", &nisEnabled)) settings.enableNISSharpening = nisEnabled ? 1 : 0;
857-871: Avoid hardcoding the NIS intermediate texture formatFor robustness, match the current framebuffer/backbuffer format to prevent CopyResource mismatches on systems where swap-chain format differs.
Apply this diff to prefer the main render target’s format:
- D3D11_TEXTURE2D_DESC nisTexDesc = texDesc; - nisTexDesc.Format = DXGI_FORMAT_R10G10B10A2_UNORM; + D3D11_TEXTURE2D_DESC nisTexDesc = texDesc; // start from main RT desc + // Prefer the active framebuffer format; if different, query and set that here. + // nisTexDesc.Format = texDesc.Format; // or query kFRAMEBUFFER RTV resource desc and use its FormatIf you’d like, I can wire a small helper that queries RE::RENDER_TARGETS::kFRAMEBUFFER RTV->GetResource()->GetDesc() and uses that format.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Features/Upscaling.cpp(4 hunks)src/Features/Upscaling.h(2 hunks)src/Features/Upscaling/Streamline.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/Upscaling.hsrc/Features/Upscaling.cppsrc/Features/Upscaling/Streamline.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Upscaling.hsrc/Features/Upscaling.cppsrc/Features/Upscaling/Streamline.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build plugin and addons
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
🔇 Additional comments (5)
src/Features/Upscaling/Streamline.cpp (3)
184-199: NIS feature detection logic looks consistentLoad/support checks mirror DLSS and final availability log is clear. LGTM.
416-417: No action neededBrace/format-only churn.
418-453: Verify<algorithm>include andslSetTagerror handling inApplyNISSharpening
I couldn’t locate theApplyNISSharpeningdefinition—please confirm thatstd::clampis covered by including<algorithm>and that failures fromslSetTagare properly handled.src/Features/Upscaling.h (1)
128-128: Public ApplyNISSharpening API is fineClear, minimal surface area. LGTM.
src/Features/Upscaling.cpp (1)
1702-1705: Call placement looks correctRunning sharpening after post-processing is sensible. LGTM.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Features/Upscaling/Streamline.cpp (2)
105-110: NIS added and feature count now correctly tied to VR/non-VR. LGTM.Nice follow-through aligning numFeaturesToLoad with the selected array.
211-214: Guard NIS function pointers to prevent null-call crashes.slGetFeatureFunction may return nullptr; calls in ApplyNISSharpening would then crash.
Apply this minimal fix:
if (featureNIS) { slGetFeatureFunction(sl::kFeatureNIS, "slNISSetOptions", (void*&)slNISSetOptions); slGetFeatureFunction(sl::kFeatureNIS, "slNISGetState", (void*&)slNISGetState); + if (!slNISSetOptions || !slNISGetState) { + logger::warn("[Streamline] NIS functions are missing; disabling NIS feature"); + featureNIS = false; + } }
🧹 Nitpick comments (4)
src/Features/Upscaling/Streamline.cpp (1)
184-199: NIS feature discovery flow looks correct.Loaded/supported checks and logging are fine. Consider surfacing the unsupported reason in UI later if helpful, but not required here.
src/Features/Upscaling.cpp (3)
204-230: NIS UI: good UX; minor toggle polish.
- When NIS isn’t available, disable the Enable toggle too to avoid “Enabled (no-op)” confusion.
Apply:
-const char* nisToggleModes[] = { "Disabled", "Enabled" }; -ImGui::SliderInt("Enable NIS Sharpening", (int*)&settings.enableNISSharpening, 0, 1, nisToggleModes[settings.enableNISSharpening]); +const char* nisToggleModes[] = { "Disabled", "Enabled" }; +if (!streamline.featureNIS) ImGui::BeginDisabled(); +ImGui::SliderInt("Enable NIS Sharpening", (int*)&settings.enableNISSharpening, 0, 1, nisToggleModes[settings.enableNISSharpening]); +if (!streamline.featureNIS) ImGui::EndDisabled();
859-873: Create NIS scratch texture lazily and guard against re-init leaks.
- Only allocate when NIS is available and sharpening is enabled.
- If SetupResources can be re-entered (resizes/device reset), release the previous texture before recreating.
Suggested changes:
-// Create NIS sharpener texture with swapchain format and UAV access +// Create NIS sharpener texture with swapchain format and UAV access +if (nisSharpenerTexture) { + delete nisSharpenerTexture; + nisSharpenerTexture = nullptr; +} +D3D11_TEXTURE2D_DESC nisTexDesc = texDesc; +nisTexDesc.Format = DXGI_FORMAT_R10G10B10A2_UNORM; +nisTexDesc.BindFlags = D3D11_BIND_SHADER_RESOURCE | D3D11_BIND_UNORDERED_ACCESS; - -D3D11_TEXTURE2D_DESC nisTexDesc = texDesc; -nisTexDesc.Format = DXGI_FORMAT_R10G10B10A2_UNORM; -nisTexDesc.BindFlags = D3D11_BIND_SHADER_RESOURCE | D3D11_BIND_UNORDERED_ACCESS; +// Optionally: gate allocation until first use if you want to save memory.
1733-1754: ApplyNISSharpening(): early-out and minor hazards.
- Skip copies when sharpness is ~0 to save bandwidth.
- Optional: unbind PS SRVs that might reference the framebuffer to avoid hidden hazards on some drivers.
- Consider calling Streamline directly on the framebuffer resource (in-place) if NIS permits same input/output, to remove both copies.
Minimal changes:
void Upscaling::ApplyNISSharpening() { if (!settings.enableNISSharpening || !streamline.featureNIS) { return; } + if (settings.nisSharpness <= 0.0f) { + return; + } @@ context->OMSetRenderTargets(0, nullptr, nullptr); // Unbind all bound render targets + // Optional: also clear PS SRV slots to avoid hazards with CopyResource + // ID3D11ShaderResourceView* nullSRVs[D3D11_COMMONSHADER_INPUT_RESOURCE_SLOT_COUNT] = {}; + // context->PSSetShaderResources(0, D3D11_COMMONSHADER_INPUT_RESOURCE_SLOT_COUNT, nullSRVs);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/Upscaling.cpp(5 hunks)src/Features/Upscaling/Streamline.cpp(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/Upscaling.cppsrc/Features/Upscaling/Streamline.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Upscaling.cppsrc/Features/Upscaling/Streamline.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Validate shader compilation (Flatrim, .github/configs/shader-validation.yaml)
- GitHub Check: Validate shader compilation (VR, .github/configs/shader-validation-vr.yaml)
- GitHub Check: Build plugin and addons
🔇 Additional comments (3)
src/Features/Upscaling/Streamline.cpp (1)
416-417: No action.Closing brace churn only.
src/Features/Upscaling.cpp (2)
22-24: Settings schema update looks good.enableNISSharpening and nisSharpness are properly serialized.
1704-1705: Sharpen after vanilla post-processing: placement looks right.Calling ApplyNISSharpening after func() ensures it’s the last pass before UI/Reshade. Good.
2 participants
Summary by CodeRabbit
New Features
Changes