feat(ui): DLSS preset selection#1645
Conversation
WalkthroughThe change removes a VR condition from the DLSS Preset UI slider in Upscaling.cpp, making the slider always visible when DLSS is active rather than only in VR mode. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Features/Upscaling.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/Upscaling.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.cpp
**/*
⚙️ CodeRabbit configuration file
**/*: When reviewing PRs, please provide suggestions for:
Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:
- feat(vr): add cross-eye sampling
- fix(water): resolve flowmap bug
- docs: update shader documentation
Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:Otherwise, use your standard review approach focusing on code quality.
Files:
src/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). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (1)
src/Features/Upscaling.cpp (1)
219-220: LGTM: UI change successfully enables DLSS preset selection for all users.The removal of the VR-only restriction makes DLSS preset tuning accessible to all users, addressing the ghosting and particle trail issues described in the PR objectives. The implementation correctly places the preset control within the DLSS-specific UI block.
Per the coding guidelines, consider adding an issue reference to the PR description since this addresses issue #1627:
Addresses #1627This will automatically link the PR to the issue and help with tracking.
| const char* presets[] = { "F (Fast)", "J (Quality)", "K (Ultra)" }; | ||
| ImGui::SliderInt("DLSS Preset", (int*)&settings.DLSSPreset, 0, 2, presets[settings.DLSSPreset]); |
There was a problem hiding this comment.
Critical: Add bounds validation for DLSSPreset to prevent out-of-bounds array access.
The settings.DLSSPreset value is used as an array index without validation after being loaded from disk. If the saved settings file contains an invalid value (due to corruption, manual editing, or loading settings from a future version), this will cause undefined behavior.
Apply this diff to add validation in LoadSettings, following the pattern used for other enum fields:
if (settings.upscaleMethodNoDLSS >= static_cast<uint>(enumCount)) {
logger::warn("[Upscaling] Loaded upscaleMethodNoDLSS {} out of range, clamping to {}", settings.upscaleMethodNoDLSS, enumCount ? enumCount - 1 : 0);
settings.upscaleMethodNoDLSS = enumCount ? enumCount - 1 : 0;
}
+ constexpr auto dlssPresetCount = 3; // Number of DLSS presets: F, J, K
+ if (settings.DLSSPreset >= static_cast<uint>(dlssPresetCount)) {
+ logger::warn("[Upscaling] Loaded DLSSPreset {} out of range, clamping to {}", settings.DLSSPreset, dlssPresetCount ? dlssPresetCount - 1 : 0);
+ settings.DLSSPreset = dlssPresetCount ? dlssPresetCount - 1 : 0;
+ }
auto iniSettingCollection = globals::game::iniPrefSettingCollection;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/Features/Upscaling.cpp around lines 219-220, the code uses
settings.DLSSPreset as an array index without validation; update LoadSettings to
validate/clamp the loaded settings.DLSSPreset to the valid range 0..2 (or set a
safe default) before it is used (follow the same pattern as other enum fields:
check value after reading from disk, if out of range set to default/nearest
valid value and optionally log a warning), ensuring no out-of-bounds access when
indexing the presets array.
|
✅ A pre-release build is available for this PR: |
|
The plan is to remove the masks for DLSS for performance as they do nothing now. Preset F is terrible, Preset E is better but NVIDIA removed it. No plans to use anything that's not the latest technology. The fix is to add depth or motion vectors to the rain, doesn't need to be correct, just enough to trick the AI model. Running DLSS in LDR after tonemapping may also fix the described issues. |
2 participants
Description
I enabled DLSS preset selection in the UI (previously VR-only). Using preset F (“Fast”) reduces ghosting and, in my setup, fixes the dark particle trails from this issue: #1627.
Details
ReactiveMaskandTransparencyCompositionMaskfromEncodeTexturesCS.hlsl. This likely affects particles in particular, which don’t have reliable physics or motion vectors.ReactiveMask,TransparencyCompositionMask), helping reduce ghosting.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.