feat(utils): add stereo support to Util::Subrect::Controller#2356
Closed
alandtse wants to merge 2 commits into
Closed
feat(utils): add stereo support to Util::Subrect::Controller#2356alandtse wants to merge 2 commits into
alandtse wants to merge 2 commits into
Conversation
Adds an opt-in stereo extension to the screenshot's subrect controller
so stereo-aware consumers (the DLSS Enhancer subrect framework) can
track a mirrored right-eye UV alongside the existing primary UV without
forking the util. Mono callers (ScreenshotFeature) are unaffected:
SaveSettings emits bit-identical JSON, LoadSettings round-trips the
legacy schema, and the DrawEditor signature is unchanged.
New API:
- SetStereoEnabled(bool) / IsStereoEnabled()
- GetRightEyeUV() — folds onto primary UV in mono mode
- GetStereoPixelRegions(fullW, fullH) — resolves both eyes against
per-eye half-width for SBS textures
- Preset gains an optional rightUV{} field
In stereo mode, drag/slider edits auto-mirror the primary UV to the
right eye around x=0.5 (HMD nose-side overlap symmetry). LoadSettings
mirrors left → right only for the legacy upgrade case (CropX/Y/W/H
present, no CropRight*), so a seeded preset's explicit right-eye UV
survives a fresh load.
The header is now self-contained (forward-declares D3D11 types,
includes nlohmann/json), so a test target can compile it without the
plugin PCH.
Adds a C++ unit test target (tests/cpp) covering mono back-compat,
stereo round-trip, legacy upgrade mirror, and per-eye pixel-region
math. 9 test cases, 33 assertions. Reuses Catch2 via FetchContent
dedup with tests/shaders. New CMake target run_cpp_tests.
Decomposed from community-shaders#2096 (PR-1 of 4); see DLSS-PR-PLAN.md.
Co-Authored-By: YtzyFvra <59631290+YtzyFvra@users.noreply.github.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds stereo-aware right-eye UVs to Util::Subrect::Controller with mirroring, persistence, editor sync, SBS per-eye pixel-region resolution, and a Catch2 C++ test suite plus optional CMake wiring and a run target. ChangesStereo Sub-Rectangle Cropping Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 OpenGrep (1.20.0)OpenGrep fatal error (exit code 2): [00.12][ERROR]: Error: exception Unix_error: No such file or directory stat tests/cpp/test_subrect.cpp 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Utils/Subrect.cpp (1)
211-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreset save omits right-eye UV in stereo mode.
When saving a new preset while stereo is enabled, only
currentUVis stored. ThecurrentRightUVis not captured, so the saved preset will have a default-initializedrightUV. When re-applied, the preset loses the user's right-eye crop.Proposed fix
if (ImGui::Button("Save Preset")) { std::string presetName = newPresetName; if (!presetName.empty()) { - presets.push_back(Preset{ .name = presetName, .uv = currentUV }); + presets.push_back(Preset{ .name = presetName, .uv = currentUV, .rightUV = currentRightUV }); selectedPresetIndex = static_cast<int>(presets.size()) - 1; newPresetName[0] = '\0'; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Utils/Subrect.cpp` around lines 211 - 218, The Save Preset button handler saves only currentUV and omits the right-eye UV, so update the push to presets in the ImGui::Button("Save Preset") block to construct the Preset with both .uv = currentUV and .rightUV = currentRightUV (use the Preset aggregate initialized where presets.push_back(Preset{ .name = presetName, .uv = currentUV }) is currently used), leaving selectedPresetIndex and newPresetName handling unchanged; this ensures a saved preset captures both eyes' UVs and will restore the right-eye crop when re-applied.
🧹 Nitpick comments (2)
CMakeLists.txt (2)
1052-1061: ⚡ Quick winHook
run_cpp_testsinto package/deploy flows to prevent untested artifactsRight now C++ tests are runnable but not enforced before packaging/deployment, so regressions can slip through even when shader tests are gated.
Proposed CMake wiring
if(EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/tests/cpp/CMakeLists.txt") enable_testing() add_subdirectory(tests/cpp) if(TARGET cpp_tests) add_custom_target( run_cpp_tests COMMAND $<TARGET_FILE:cpp_tests> --reporter compact DEPENDS cpp_tests WORKING_DIRECTORY $<TARGET_FILE_DIR:cpp_tests> COMMENT "Running C++ unit tests..." VERBATIM ) + # Keep packaging/deploy parity with shader test gating. + if(TARGET "Package-Core") + add_dependencies("Package-Core" run_cpp_tests) + endif() + if(TARGET "Package-AIO-Manual") + add_dependencies("Package-AIO-Manual" run_cpp_tests) + endif() + if(TARGET DEPLOY_ALL) + add_dependencies(DEPLOY_ALL run_cpp_tests) + endif() endif() endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 1052 - 1061, The C++ test target run_cpp_tests is defined but not run as part of packaging/deploy; make the package/deploy targets depend on it so artifacts are only produced after tests pass. Locate the add_custom_target for run_cpp_tests and add add_dependencies(<package_target> run_cpp_tests) and add_dependencies(<deploy_target> run_cpp_tests) (or include run_cpp_tests in the DEPENDS list of your existing package/deploy add_custom_target/add_library calls) so that the package and deploy targets (replace <package_target>/<deploy_target> with your actual package/deploy target names) will fail if cpp_tests fails; ensure WORKING_DIRECTORY and VERBATIM remain unchanged for run_cpp_tests.
1047-1049: 💤 Low valuePR metadata polish: shorten title and add issue keyword
Suggested title (≤50 chars):
feat(utils): add stereo subrect support
Suggested footer/body:Implements#2096``As per coding guidelines, "Conventional Commit Titles ... Length: 50 characters limit for title" and "Issue References ... 'Implements
#123' for features."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CMakeLists.txt` around lines 1047 - 1049, Update the PR metadata: shorten the pull request title to "feat(utils): add stereo subrect support" (≤50 chars) and add "Implements `#2096`" to the PR body or footer to reference the issue; apply this change in the repository hosting UI or the commit/PR description so the title and footer match the conventional commit and issue reference guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Utils/Subrect.cpp`:
- Around line 211-218: The Save Preset button handler saves only currentUV and
omits the right-eye UV, so update the push to presets in the ImGui::Button("Save
Preset") block to construct the Preset with both .uv = currentUV and .rightUV =
currentRightUV (use the Preset aggregate initialized where
presets.push_back(Preset{ .name = presetName, .uv = currentUV }) is currently
used), leaving selectedPresetIndex and newPresetName handling unchanged; this
ensures a saved preset captures both eyes' UVs and will restore the right-eye
crop when re-applied.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 1052-1061: The C++ test target run_cpp_tests is defined but not
run as part of packaging/deploy; make the package/deploy targets depend on it so
artifacts are only produced after tests pass. Locate the add_custom_target for
run_cpp_tests and add add_dependencies(<package_target> run_cpp_tests) and
add_dependencies(<deploy_target> run_cpp_tests) (or include run_cpp_tests in the
DEPENDS list of your existing package/deploy add_custom_target/add_library
calls) so that the package and deploy targets (replace
<package_target>/<deploy_target> with your actual package/deploy target names)
will fail if cpp_tests fails; ensure WORKING_DIRECTORY and VERBATIM remain
unchanged for run_cpp_tests.
- Around line 1047-1049: Update the PR metadata: shorten the pull request title
to "feat(utils): add stereo subrect support" (≤50 chars) and add "Implements
`#2096`" to the PR body or footer to reference the issue; apply this change in the
repository hosting UI or the commit/PR description so the title and footer match
the conventional commit and issue reference guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c77719ee-a87d-4734-9545-48915b36eda5
📒 Files selected for processing (6)
CMakeLists.txtsrc/Utils/Subrect.cppsrc/Utils/Subrect.htests/cpp/CMakeLists.txttests/cpp/test_main.cpptests/cpp/test_subrect.cpp
CodeRabbit Major @ scs#2356: the Save Preset button only stored currentUV, so a saved preset's rightUV was default-initialized. Re-applying that preset would zero out the user's right-eye crop in stereo mode. Fix by extending the Preset constructor at the push site to capture both eyes. In mono mode currentRightUV stays at default; in stereo mode the user's actual right-eye UV is now persisted. Adds a regression test guarding that SaveSettings emits right_uv keys in CropPresets when stereo is enabled. Bumps the suite to 35 assertions / 10 test cases. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
✅ A pre-release build is available for this PR: |
Collaborator
Author
|
Addressed CodeRabbit's Major in 499ce35:
🤖 Reviewed by Claude Code via review-comment audit. |
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
Opt-in stereo extension to the screenshot subrect controller so future stereo-aware consumers (the DLSS Enhancer foveated subrect framework decomposed from #2096) can track a mirrored right-eye UV without forking the util that ScreenshotFeature already ships.
Mono callers (ScreenshotFeature) are unaffected:
SaveSettingsemits bit-identical JSON,LoadSettingsround-trips the legacy schema, and theDrawEditorsignature is unchanged.What changes
New API on
Util::Subrect::Controller:SetStereoEnabled(bool)/IsStereoEnabled()— opt-in toggle (default off)GetRightEyeUV()— folds onto the primary UV in mono mode so callers can stay agnostic of stereo stateGetStereoPixelRegions(fullW, fullH)— resolves both eyes against per-eye half-width for SBS texturesPresetgains an optionalrightUV{}field (default-init when designated initializers omit it)Behavior:
DrawEditorauto-mirror the primary UV to the right eye aroundx=0.5(HMD nose-side overlap symmetry)SaveSettingsonly emits the newCropRight*and presetright_uvkeys when stereo is enabled — preserves existing on-disk shapeLoadSettingsmirrors left → right only for the legacy upgrade case (CropX/Y/W/Hpresent, noCropRight*), so a seeded preset's explicit right-eye UV survives a fresh loadThe header is now self-contained (forward-declares D3D11 types, includes
nlohmann/jsondirectly, declares its ownusing json = nlohmann::json;). This lets a unit-test target compile it without the plugin PCH.Tests
Adds a new
tests/cpp/target with a Catch2 unit test executable (cpp_tests). Reuses Catch2 viaFetchContentdedup withtests/shaders. New CMake targetrun_cpp_tests.9 test cases, 33 assertions, covering:
LoadSettingsround-trips legacy JSON unchanged;SaveSettingsemits no right-eye keysCropX/Y/W/Hto right eye when noCropRight*keys presentGetStereoPixelRegionsSBS half-width split + mono-mode fallback (identical eyes)MirrorUVHorizontalmath viaSetStereoEnabledTests caught a real bug in my initial
LoadSettingslogic — an unconditional post-loadSyncRightUVwas stomping on a preset's explicit right-eye value when JSON had no top-levelCrop*keys. Now correctly gated onhasExplicitLeft && !hasExplicitRight.Why this PR exists
Decomposed from @YtzyFvra's #2096 — the original PR forked this util into its own
Subrect::namespace, which conflicted with #2092's later landing ofUtil::Subrect::Controllerfor screenshot. This PR establishes the shared API so the DLSS Enhancer subrect work (PR-3 of the decomposition) can consume it cleanly without a second fork.See DLSS-PR-PLAN.md for the full decomposition strategy. PR-2 (DLSSperf) and PR-3 (DlssEnhancer foveated subrect) are on my fork awaiting separate review.
Test plan
Credits
Decomposed from @YtzyFvra's #2096 (PR-1 of 4). Stereo API design is theirs; this PR adapts it to extend dev's existing
Util::Subrectrather than fork it. Co-authored on commit.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests