refactor(utils): add stereo support to Util::Subrect::Controller#23
Merged
Conversation
📝 WalkthroughWalkthroughAdds stereo (side-by-side) per-eye UV support to Subrect: header/API and internal state, load/save with backward compatibility, editor/runtime sync and clamping, per-eye pixel-region resolution, and comprehensive Catch2 tests plus optional CMake integration for running C++ tests. ChangesStereo UV Support
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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 docstrings
🧪 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.21.0)OpenGrep fatal error (exit code 2): [00.14][ERROR]: Error: exception Unix_error: No such file or directory stat src/Utils/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. |
alandtse
changed the title
alandtse
force-pushed
the
pr1-stereo-subrect
branch
from
499ce35 to
ff27e7a
Compare
|
✅ A pre-release build is available for this PR: |
alandtse
force-pushed
the
pr1-stereo-subrect
branch
from
f5f4712 to
7eeec46
Compare
alandtse
force-pushed
the
pr1-stereo-subrect
branch
from
7eeec46 to
348166e
Compare
There was a problem hiding this comment.
Pull request overview
Adds an opt-in stereo extension to Util::Subrect::Controller so future stereo-aware consumers can track a mirrored right-eye crop UV while preserving the existing mono JSON schema used by ScreenshotFeature. Also introduces a new Catch2-based C++ unit test target to validate serialization and stereo behaviors independently of the full plugin build.
Changes:
- Extend
Util::Subrect::Controllerwith stereo state, right-eye UV tracking/accessors, and SBS-aware pixel region resolution. - Update JSON load/save to conditionally persist right-eye crop keys/preset fields only when stereo is enabled, with a legacy upgrade path.
- Add
tests/cppCatch2 unit test executable + CMake plumbing and arun_cpp_teststarget.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/Utils/Subrect.h |
Adds stereo-facing types/APIs (StereoPixelRegions, right-eye UV accessors, stereo enable toggle) and makes the header more standalone. |
src/Utils/Subrect.cpp |
Implements stereo mirroring logic, conditional JSON persistence for right-eye fields, and SBS pixel-region helper. |
tests/cpp/test_subrect.cpp |
Adds unit tests for mono back-compat and stereo save/load/mirroring behaviors. |
tests/cpp/test_main.cpp |
Provides an explicit Catch2 main() for the new C++ test executable. |
tests/cpp/CMakeLists.txt |
Adds a new Catch2-based cpp_tests target that compiles Subrect.cpp directly. |
CMakeLists.txt |
Wires tests/cpp into the top-level build and adds a run_cpp_tests custom target. |
alandtse
added a commit
that referenced
this pull request
May 21, 2026
…y regions Three Copilot findings on PR #23: - SetStereoEnabled(true) unconditionally called SyncRightUV(), so a caller that LoadSettings first (with stereo off) then enabled stereo would silently overwrite an explicit JSON CropRight* value with the mirror of the left eye. Track whether the right UV was loaded explicitly and skip the auto-mirror in that case. Add a regression test for the call order. - The "Stereo preset save captures right-eye UV" test only validated that SaveSettings emits right_uv for existing presets — it didn't exercise the round-trip semantics it claimed to cover. Rewrite to stage an asymmetric right UV (one that doesn't equal MirrorUVHorizontal of left), save, reload into a fresh controller, and assert the right eye survived. That actually pins the regression. - GetStereoPixelRegions returns regions in per-eye coordinate space (right eye x in [0, fullWidth/2), NOT pre-offset). Document this in the header so callers don't double-offset or place the right eye on top of the left. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
CMakeLists.txt (2)
1317-1332: ⚡ Quick winTighten PR metadata for traceability.
Consider shortening the PR title to stay within the 50-char conventional-commit guidance (e.g.,
feat(subrect): add stereo uv support) and add an issue keyword likeImplements #<id>if this maps to a tracked issue.As per coding guidelines,
**/*: "When reviewing PRs, please provide suggestions for: 1. Conventional Commit Titles ... 2. Issue References ...".🤖 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 1317 - 1332, Shorten the PR title to follow the 50-char conventional-commit guideline (e.g., "feat(subrect): add stereo uv support") and add an issue reference like "Implements #<id>" in the PR description or body for traceability; update the PR title and description fields accordingly and include the issue keyword so reviewers can easily map this change to the tracked issue (no code changes needed in CMakeLists.txt, just update the pull request metadata).
1317-1332: ⚡ Quick winWire
run_cpp_testsinto release-critical targets.
run_cpp_testsis created, but unlike shader tests it does not gate package/deploy targets, so stereo regressions can slip through release flows.Suggested 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 ) + 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 1317 - 1332, The run_cpp_tests custom target is not currently required by release packaging/deploy flows, so add it as a dependency of the release-critical targets (the packaging and deploy targets used in this project) to ensure cpp_tests must pass before those targets run; locate the created custom target run_cpp_tests (and cpp_tests) in the CMakeLists snippet and call add_dependencies(<release_target> run_cpp_tests) for each relevant release target (e.g., package, deploy, or any targets analogous to shader-test gating) so the packaging/deploy targets depend on run_cpp_tests and will fail if the tests do.
🤖 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.
Inline comments:
In `@src/Utils/Subrect.cpp`:
- Around line 150-155: SaveSettings() currently only writes CropRightX/Y/W/H
when stereoEnabled is true, leaving stale right-eye keys in the caller-owned
JSON a_json when stereo is later turned off; update SaveSettings() to explicitly
remove/erase the "CropRightX", "CropRightY", "CropRightW", and "CropRightH"
entries from a_json whenever stereoEnabled is false (use a_json.erase(...) or
clearing API provided by your JSON type) and keep references to stereoEnabled
and currentRightUV to locate the block to modify.
- Around line 336-344: Controller::GetStereoPixelRegions must guard against
zero/one fullWidth that makes eyeWidth = fullWidth/2 == 0; detect when eyeWidth
== 0 or fullHeight == 0 and early-return a safe/empty StereoPixelRegions instead
of calling UVToPixelRegion with invalid dimensions. Update
Controller::GetStereoPixelRegions to validate fullWidth and fullHeight (e.g.,
require fullWidth >= 2 and fullHeight >= 1), and when invalid, set
regions.leftEye/rightEye to a zero-sized or clamped region (or return a
default-constructed StereoPixelRegions) rather than invoking UVToPixelRegion
with currentUV/currentRightUV; keep the stereoEnabled logic but only call
UVToPixelRegion when dimensions are valid.
- Around line 85-100: The bug is that rightUVLoadedFromJson is only ever set
true and never reset, causing stale state across LoadSettings() calls; in
Subrect.cpp inside the settings load routine (where hasExplicitRight is computed
and currentRightUV is assigned) reset rightUVLoadedFromJson to false at the
start of each load (e.g., at the top of LoadSettings() or immediately before
reading CropRight*), then keep the existing logic that sets it to true when
hasExplicitRight is detected so left→right auto-mirroring works correctly when
CropRight* keys are absent.
In `@src/Utils/Subrect.h`:
- Around line 43-48: The Preset struct's rightUV currently default-initializes
to a full-frame UV (rightUV{}) which hides "not provided" and causes ApplyPreset
to use a full-frame right-eye crop; change Preset::rightUV to
std::optional<UVRegion> (or add a bool hasRightUV) so callers like
SeedDefaultPresets({ .name, .uv }) remain treated as "no right UV", then update
loading/seed/apply code paths (e.g., ApplyPreset, SeedDefaultPresets, and any
loaders) to use preset.rightUV.value_or(MirrorUVHorizontal(preset.uv)) (or the
equivalent hasRightUV check) when resolving the right-eye UV.
In `@tests/cpp/test_subrect.cpp`:
- Around line 24-27: The test uses std::abs in the function UVApprox (which
compares UVRegion members) but the file is missing an explicit `#include` <cmath>;
add an explicit include of <cmath> at the top of tests/cpp/test_subrect.cpp so
std::abs is defined (or use std::fabs if preferred) to avoid relying on
transitive includes and ensure correct overload resolution for floating-point
comparisons in UVApprox.
---
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 1317-1332: Shorten the PR title to follow the 50-char
conventional-commit guideline (e.g., "feat(subrect): add stereo uv support") and
add an issue reference like "Implements #<id>" in the PR description or body for
traceability; update the PR title and description fields accordingly and include
the issue keyword so reviewers can easily map this change to the tracked issue
(no code changes needed in CMakeLists.txt, just update the pull request
metadata).
- Around line 1317-1332: The run_cpp_tests custom target is not currently
required by release packaging/deploy flows, so add it as a dependency of the
release-critical targets (the packaging and deploy targets used in this project)
to ensure cpp_tests must pass before those targets run; locate the created
custom target run_cpp_tests (and cpp_tests) in the CMakeLists snippet and call
add_dependencies(<release_target> run_cpp_tests) for each relevant release
target (e.g., package, deploy, or any targets analogous to shader-test gating)
so the packaging/deploy targets depend on run_cpp_tests and will fail if the
tests do.
🪄 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: d5015d26-fd02-4cd3-aa59-4aeb0fd5a1dc
📒 Files selected for processing (6)
CMakeLists.txtsrc/Utils/Subrect.cppsrc/Utils/Subrect.htests/cpp/CMakeLists.txttests/cpp/test_main.cpptests/cpp/test_subrect.cpp
alandtse
added a commit
that referenced
this pull request
May 21, 2026
Five CodeRabbit findings on PR #23: - rightUVLoadedFromJson was only ever set to true. A later LoadSettings without CropRight* keys would inherit the stale `true` and SetStereoEnabled would no longer auto-mirror as intended. Set the flag to hasExplicitRight unconditionally so each load owns the state. Regression test added. - SaveSettings updated a caller-owned JSON in place but skipped clearing CropRight* keys when stereo was disabled. If a host re-saved into the same object the keys lingered and the next load looked like explicit stereo data. Erase the four keys in the mono branch. Regression test added. - GetStereoPixelRegions divided fullWidth by 2 unconditionally; for widths 0 or 1 the per-eye width was 0 and UVToPixelRegion's `width - 1` underflowed. Guard `fullWidth < 2 || fullHeight == 0` with an empty region pair so callers can detect via .w == 0. Edge test added. - Preset::rightUV defaulted to UVRegion{} (full frame), which conflated "no explicit right eye supplied" with "user wants full frame". The SeedDefaultPresets caller in ScreenshotFeature only sets .name + .uv, so in a stereo-consumer host the right eye would silently land on full frame instead of mirroring left. Make rightUV a std::optional<UVRegion>: nullopt means "auto-mirror at apply time". ApplyPreset and EnsureDefaultPreset fall back to MirrorUVHorizontal when nullopt; SaveSettings only persists right_uv when both stereo is enabled and an explicit value exists. Existing in-fork callers (ScreenshotFeature, in-tree tests, Save Preset button via implicit T->optional<T>) work unchanged. Regression test added. - test_subrect.cpp used std::abs via transitive includes. Add an explicit #include <cmath>. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/cpp/test_subrect.cpp (2)
19-19: 💤 Low valueUnused import:
WithinAbsmatcher is never used.The
WithinAbsmatcher is imported but the tests use the customUVApproxhelper instead. Consider removing the unused import or refactoring the tests to use Catch2's built-in matcher for consistency.🧹 Suggested cleanup
-using Catch::Matchers::WithinAbs; using Util::Subrect::Controller;🤖 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 `@tests/cpp/test_subrect.cpp` at line 19, The import Using Catch::Matchers::WithinAbs in tests/cpp/test_subrect.cpp is unused because the test suite uses the custom UVApprox helper; remove the unused import or replace usages of UVApprox with Catch2's WithinAbs matcher. Locate the top-level using Catch::Matchers::WithinAbs statement and either delete that line, or refactor tests that currently call UVApprox to use Catch::Matchers::WithinAbs (and update any EXPECT/REQUIRE calls accordingly) so the import is actually used.
344-348: 💤 Low valueConsider extracting test data for clarity.
The inline initialization of test pairs within the range-based for-loop is functional but slightly dense. For improved readability, consider extracting the degenerate dimension test cases to a named constant.
💡 Optional refactor for clarity
+ const std::vector<std::pair<uint32_t, uint32_t>> degenerateDimensions = { + { 0, 100 }, { 1, 100 }, { 100, 0 } + }; - for (auto [w, h] : std::vector<std::pair<uint32_t, uint32_t>>{ { 0, 100 }, { 1, 100 }, { 100, 0 } }) { + for (auto [w, h] : degenerateDimensions) { const auto regions = c.GetStereoPixelRegions(w, h);🤖 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 `@tests/cpp/test_subrect.cpp` around lines 344 - 348, Extract the inline test data used in the range-based for loop into a named constant (e.g., const std::vector<std::pair<uint32_t,uint32_t>> degenerateCases = { {0,100}, {1,100}, {100,0} }) and then iterate over degenerateCases when calling c.GetStereoPixelRegions(w,h); keep the assertions on regions.leftEye.w and regions.rightEye.w unchanged so the test behavior is identical but the test data is clearer to readers.
🤖 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.
Inline comments:
In `@src/Utils/Subrect.cpp`:
- Around line 99-102: The code sets rightUVLoadedFromJson = hasExplicitRight
even when only a subset of the CropRight* fields were present, which lets
partial configs override stale values and disable left→right mirroring; change
this so rightUVLoadedFromJson is only set true when the full quartet of
CropRight* keys was provided. Update the LoadSettings logic around
hasExplicitRight/rightUVLoadedFromJson to require a complete set (e.g., require
hasCropRightX && hasCropRightY && hasCropRightW && hasCropRightH or track
per-field presence flags) before marking rightUVLoadedFromJson true; if any of
the four are missing, leave rightUVLoadedFromJson false so
SetStereoEnabled(true) can auto-mirror. Ensure the new check references the same
symbols (hasExplicitRight, rightUVLoadedFromJson, and the per-field presence
flags used when parsing CropRight*).
- Around line 112-116: The current code assigns preset.rightUV =
LoadUVArray(entry["right_uv"]) whenever entry.contains("right_uv"), but
LoadUVArray returns a default full-frame UV on malformed input which incorrectly
suppresses auto-mirroring and applies a full-frame crop; change the logic in
Subrect.cpp so that when entry.contains("right_uv") you validate the payload
before assigning: either use a LoadUVArray variant that returns std::optional
(or validate entry["right_uv"] shape/size/types) and only set preset.rightUV
when the parsed value is valid, otherwise leave preset.rightUV as nullopt so
ApplyPreset can still auto-mirror the left eye; reference LoadUVArray,
preset.rightUV, ApplyPreset, and entry["right_uv"] when making the change.
---
Nitpick comments:
In `@tests/cpp/test_subrect.cpp`:
- Line 19: The import Using Catch::Matchers::WithinAbs in
tests/cpp/test_subrect.cpp is unused because the test suite uses the custom
UVApprox helper; remove the unused import or replace usages of UVApprox with
Catch2's WithinAbs matcher. Locate the top-level using
Catch::Matchers::WithinAbs statement and either delete that line, or refactor
tests that currently call UVApprox to use Catch::Matchers::WithinAbs (and update
any EXPECT/REQUIRE calls accordingly) so the import is actually used.
- Around line 344-348: Extract the inline test data used in the range-based for
loop into a named constant (e.g., const
std::vector<std::pair<uint32_t,uint32_t>> degenerateCases = { {0,100}, {1,100},
{100,0} }) and then iterate over degenerateCases when calling
c.GetStereoPixelRegions(w,h); keep the assertions on regions.leftEye.w and
regions.rightEye.w unchanged so the test behavior is identical but the test data
is clearer to readers.
🪄 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: e0595b7d-33d7-4850-bbb2-0f9ff6d9b05a
📒 Files selected for processing (3)
src/Utils/Subrect.cppsrc/Utils/Subrect.htests/cpp/test_subrect.cpp
alandtse
added a commit
that referenced
this pull request
May 21, 2026
…y regions Three Copilot findings on PR #23: - SetStereoEnabled(true) unconditionally called SyncRightUV(), so a caller that LoadSettings first (with stereo off) then enabled stereo would silently overwrite an explicit JSON CropRight* value with the mirror of the left eye. Track whether the right UV was loaded explicitly and skip the auto-mirror in that case. Add a regression test for the call order. - The "Stereo preset save captures right-eye UV" test only validated that SaveSettings emits right_uv for existing presets — it didn't exercise the round-trip semantics it claimed to cover. Rewrite to stage an asymmetric right UV (one that doesn't equal MirrorUVHorizontal of left), save, reload into a fresh controller, and assert the right eye survived. That actually pins the regression. - GetStereoPixelRegions returns regions in per-eye coordinate space (right eye x in [0, fullWidth/2), NOT pre-offset). Document this in the header so callers don't double-offset or place the right eye on top of the left. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
alandtse
added a commit
that referenced
this pull request
May 21, 2026
Five CodeRabbit findings on PR #23: - rightUVLoadedFromJson was only ever set to true. A later LoadSettings without CropRight* keys would inherit the stale `true` and SetStereoEnabled would no longer auto-mirror as intended. Set the flag to hasExplicitRight unconditionally so each load owns the state. Regression test added. - SaveSettings updated a caller-owned JSON in place but skipped clearing CropRight* keys when stereo was disabled. If a host re-saved into the same object the keys lingered and the next load looked like explicit stereo data. Erase the four keys in the mono branch. Regression test added. - GetStereoPixelRegions divided fullWidth by 2 unconditionally; for widths 0 or 1 the per-eye width was 0 and UVToPixelRegion's `width - 1` underflowed. Guard `fullWidth < 2 || fullHeight == 0` with an empty region pair so callers can detect via .w == 0. Edge test added. - Preset::rightUV defaulted to UVRegion{} (full frame), which conflated "no explicit right eye supplied" with "user wants full frame". The SeedDefaultPresets caller in ScreenshotFeature only sets .name + .uv, so in a stereo-consumer host the right eye would silently land on full frame instead of mirroring left. Make rightUV a std::optional<UVRegion>: nullopt means "auto-mirror at apply time". ApplyPreset and EnsureDefaultPreset fall back to MirrorUVHorizontal when nullopt; SaveSettings only persists right_uv when both stereo is enabled and an explicit value exists. Existing in-fork callers (ScreenshotFeature, in-tree tests, Save Preset button via implicit T->optional<T>) work unchanged. Regression test added. - test_subrect.cpp used std::abs via transitive includes. Add an explicit #include <cmath>. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
alandtse
force-pushed
the
pr1-stereo-subrect
branch
from
5cc98f9 to
f6eb4b8
Compare
alandtse
added a commit
that referenced
this pull request
May 21, 2026
…y regions Three Copilot findings on PR #23: - SetStereoEnabled(true) unconditionally called SyncRightUV(), so a caller that LoadSettings first (with stereo off) then enabled stereo would silently overwrite an explicit JSON CropRight* value with the mirror of the left eye. Track whether the right UV was loaded explicitly and skip the auto-mirror in that case. Add a regression test for the call order. - The "Stereo preset save captures right-eye UV" test only validated that SaveSettings emits right_uv for existing presets — it didn't exercise the round-trip semantics it claimed to cover. Rewrite to stage an asymmetric right UV (one that doesn't equal MirrorUVHorizontal of left), save, reload into a fresh controller, and assert the right eye survived. That actually pins the regression. - GetStereoPixelRegions returns regions in per-eye coordinate space (right eye x in [0, fullWidth/2), NOT pre-offset). Document this in the header so callers don't double-offset or place the right eye on top of the left. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
alandtse
added a commit
that referenced
this pull request
May 21, 2026
Five CodeRabbit findings on PR #23: - rightUVLoadedFromJson was only ever set to true. A later LoadSettings without CropRight* keys would inherit the stale `true` and SetStereoEnabled would no longer auto-mirror as intended. Set the flag to hasExplicitRight unconditionally so each load owns the state. Regression test added. - SaveSettings updated a caller-owned JSON in place but skipped clearing CropRight* keys when stereo was disabled. If a host re-saved into the same object the keys lingered and the next load looked like explicit stereo data. Erase the four keys in the mono branch. Regression test added. - GetStereoPixelRegions divided fullWidth by 2 unconditionally; for widths 0 or 1 the per-eye width was 0 and UVToPixelRegion's `width - 1` underflowed. Guard `fullWidth < 2 || fullHeight == 0` with an empty region pair so callers can detect via .w == 0. Edge test added. - Preset::rightUV defaulted to UVRegion{} (full frame), which conflated "no explicit right eye supplied" with "user wants full frame". The SeedDefaultPresets caller in ScreenshotFeature only sets .name + .uv, so in a stereo-consumer host the right eye would silently land on full frame instead of mirroring left. Make rightUV a std::optional<UVRegion>: nullopt means "auto-mirror at apply time". ApplyPreset and EnsureDefaultPreset fall back to MirrorUVHorizontal when nullopt; SaveSettings only persists right_uv when both stereo is enabled and an explicit value exists. Existing in-fork callers (ScreenshotFeature, in-tree tests, Save Preset button via implicit T->optional<T>) work unchanged. Regression test added. - test_subrect.cpp used std::abs via transitive includes. Add an explicit #include <cmath>. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
alandtse
force-pushed
the
pr1-stereo-subrect
branch
from
f6eb4b8 to
f0c6520
Compare
alandtse
added a commit
that referenced
this pull request
May 22, 2026
Address remaining Copilot nits on PR #23 test file: - WithinAbs matcher and the floating-point matcher header were imported but never referenced; UVApprox uses std::abs directly. Drop both to keep the warnings-as-errors path clean. - The degenerate-dimensions test enumerates uint32 pairs via std::vector<std::pair<...>>; include <vector> and <utility> explicitly instead of relying on transitive headers.
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>
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>
Empty commit to trigger CodeQL on this PR. The default-setup CodeQL workflow only fires on synchronize events, not reopened, so a push is required to gate the PR on the new code scan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…y regions Three Copilot findings on PR #23: - SetStereoEnabled(true) unconditionally called SyncRightUV(), so a caller that LoadSettings first (with stereo off) then enabled stereo would silently overwrite an explicit JSON CropRight* value with the mirror of the left eye. Track whether the right UV was loaded explicitly and skip the auto-mirror in that case. Add a regression test for the call order. - The "Stereo preset save captures right-eye UV" test only validated that SaveSettings emits right_uv for existing presets — it didn't exercise the round-trip semantics it claimed to cover. Rewrite to stage an asymmetric right UV (one that doesn't equal MirrorUVHorizontal of left), save, reload into a fresh controller, and assert the right eye survived. That actually pins the regression. - GetStereoPixelRegions returns regions in per-eye coordinate space (right eye x in [0, fullWidth/2), NOT pre-offset). Document this in the header so callers don't double-offset or place the right eye on top of the left. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Five CodeRabbit findings on PR #23: - rightUVLoadedFromJson was only ever set to true. A later LoadSettings without CropRight* keys would inherit the stale `true` and SetStereoEnabled would no longer auto-mirror as intended. Set the flag to hasExplicitRight unconditionally so each load owns the state. Regression test added. - SaveSettings updated a caller-owned JSON in place but skipped clearing CropRight* keys when stereo was disabled. If a host re-saved into the same object the keys lingered and the next load looked like explicit stereo data. Erase the four keys in the mono branch. Regression test added. - GetStereoPixelRegions divided fullWidth by 2 unconditionally; for widths 0 or 1 the per-eye width was 0 and UVToPixelRegion's `width - 1` underflowed. Guard `fullWidth < 2 || fullHeight == 0` with an empty region pair so callers can detect via .w == 0. Edge test added. - Preset::rightUV defaulted to UVRegion{} (full frame), which conflated "no explicit right eye supplied" with "user wants full frame". The SeedDefaultPresets caller in ScreenshotFeature only sets .name + .uv, so in a stereo-consumer host the right eye would silently land on full frame instead of mirroring left. Make rightUV a std::optional<UVRegion>: nullopt means "auto-mirror at apply time". ApplyPreset and EnsureDefaultPreset fall back to MirrorUVHorizontal when nullopt; SaveSettings only persists right_uv when both stereo is enabled and an explicit value exists. Existing in-fork callers (ScreenshotFeature, in-tree tests, Save Preset button via implicit T->optional<T>) work unchanged. Regression test added. - test_subrect.cpp used std::abs via transitive includes. Add an explicit #include <cmath>. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- LoadSettings flagged hasExplicitRight if ANY single CropRight* key was present. A partial config (e.g. only CropRightW) then reused stale values for the missing components AND suppressed the mirror fallback. Require the full quartet via AND semantics so partial loads behave as "not explicit" and the auto-mirror runs. - The preset-loop also accepted `right_uv` of any type and silently full-framed it via LoadUVArray's default. Validate is_array() + size() == 4 before treating it as explicit, otherwise leave the preset's rightUV as nullopt so ApplyPreset mirrors at apply time. - "Save Preset" in mono mode snapshotted currentRightUV into the new preset's rightUV (now std::optional<UVRegion>), which falsely marked the preset as having an explicit right-eye crop. When stereo was later enabled and the preset re-applied, the auto-mirror was suppressed by the .has_value() check and the right eye landed on a stale UV. Only copy currentRightUV when stereoEnabled; otherwise leave rightUV as nullopt. Three regression tests added covering each branch.
Empty commit to trigger CodeQL on this PR. The default-setup CodeQL workflow only fires on synchronize events, not reopened, so a push is required to gate the PR on the new code scan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address remaining Copilot nits on PR #23 test file: - WithinAbs matcher and the floating-point matcher header were imported but never referenced; UVApprox uses std::abs directly. Drop both to keep the warnings-as-errors path clean. - The degenerate-dimensions test enumerates uint32 pairs via std::vector<std::pair<...>>; include <vector> and <utility> explicitly instead of relying on transitive headers.
alandtse
force-pushed
the
pr1-stereo-subrect
branch
from
1d34f35 to
84e9901
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CMakeLists.txt (1)
1319-1334: Consider tightening PR metadata for merge hygiene.Current PR title is descriptive and conventional, but likely over the 50-char title guideline. A shorter variant like
refactor(utils): add subrect stereo supportwould fit better.
If this maps to a tracking issue, add a keyword line such asImplements #<issue-id>in the PR description.As per coding guidelines, "Format: type(scope): description", "Length: 50 characters limit for title", and "Suggest adding appropriate GitHub keywords: 'Implements
#123' or 'Addresses#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 1319 - 1334, The PR metadata needs tightening: shorten the PR title to the recommended "type(scope): description" format under 50 chars (for example use "refactor(utils): add subrect stereo support" instead of the long title) and add a tracking keyword line in the PR description like "Implements #<issue-id>" or "Addresses #<issue-id>"; update the repository PR (the change affects the CMakeLists-related work such as targets cpp_tests and run_cpp_tests) so the title follows the guideline and include the appropriate issue reference in the description.src/Utils/Subrect.h (1)
15-18: 💤 Low valueConsider scoping the
jsonalias inside the namespace.Placing
using json = ...at global scope pollutes the namespace for all consumers of this header. Moving it insidenamespace Util::Subrectwould limit exposure while still allowing standalone compilation for unit tests.💡 Alternative placement
-// Mirrors the global `using json = nlohmann::json;` from the plugin PCH so -// the header builds standalone (e.g. in unit-test targets that don't -// precompile PCH). Identical aliases in the same scope are well-defined. -using json = nlohmann::json; - namespace Util::Subrect { + // Mirrors the global `using json = nlohmann::json;` from the plugin PCH so + // the header builds standalone (e.g. in unit-test targets that don't + // precompile PCH). + using json = nlohmann::json; + struct UVRegion🤖 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.h` around lines 15 - 18, The global using alias "using json = nlohmann::json;" in Subrect.h pollutes global scope; move the alias into the header's namespace block (e.g., inside "namespace Util::Subrect { ... }") so tests and header users still get the alias when including this header but it is not leaked to all consumers; locate the declaration of the alias in Subrect.h and wrap or relocate it into the "Util::Subrect" namespace so that other code outside that namespace is unaffected.
🤖 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.
Nitpick comments:
In `@CMakeLists.txt`:
- Around line 1319-1334: The PR metadata needs tightening: shorten the PR title
to the recommended "type(scope): description" format under 50 chars (for example
use "refactor(utils): add subrect stereo support" instead of the long title) and
add a tracking keyword line in the PR description like "Implements #<issue-id>"
or "Addresses #<issue-id>"; update the repository PR (the change affects the
CMakeLists-related work such as targets cpp_tests and run_cpp_tests) so the
title follows the guideline and include the appropriate issue reference in the
description.
In `@src/Utils/Subrect.h`:
- Around line 15-18: The global using alias "using json = nlohmann::json;" in
Subrect.h pollutes global scope; move the alias into the header's namespace
block (e.g., inside "namespace Util::Subrect { ... }") so tests and header users
still get the alias when including this header but it is not leaked to all
consumers; locate the declaration of the alias in Subrect.h and wrap or relocate
it into the "Util::Subrect" namespace so that other code outside that namespace
is unaffected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba7ff56e-7ba2-44da-bafd-5e2be58ba817
📒 Files selected for processing (6)
CMakeLists.txtsrc/Utils/Subrect.cppsrc/Utils/Subrect.htests/cpp/CMakeLists.txttests/cpp/test_main.cpptests/cpp/test_subrect.cpp
alandtse
added a commit
that referenced
this pull request
May 22, 2026
Mirrors the shader_tests pattern so cpp_tests is built and run only when relevant files change (and always on push / workflow_dispatch / release events). pr-checks.yaml: - New `cpp_tests` files_yaml category: `tests/cpp/**` - New `cpp-tests-should-build` output combining cpp_tests + cpp (since the target compiles src/Utils/Subrect.cpp directly per tests/cpp/CMakeLists.txt — conservative-but-correct) + cmake + build_ci. Falls through to true on changed-files failure. - New `run-cpp-tests` and `cpp-tests-should-build` inputs passed into _shared-build.yaml. _shared-build.yaml: - New `run-cpp-tests` (boolean, default true) and `cpp-tests-should-build` (string, default "true") inputs. - New `cpp-unit-tests` job parallel to `shader-unit-tests`. Builds the `cpp_tests` target and runs `ctest -R CppUtilTests`. Gated inline (no composite action — the single branch is too small for the indirection). Without this gate, the PR #23 stereo Subrect tests broke the cpp build but CI stayed green because `--target shader_tests` never touched the cpp_tests source files. Next time a cpp_tests test fails to compile, this gate catches it at PR-check time. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
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 community-shaders#2096 upstream) 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 loadcurrentRightUV(fixed during the upstream review cycle — CodeRabbit Major)The 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.10 test cases, 35 assertions, covering:
LoadSettingsround-trips legacy JSON unchanged;SaveSettingsemits no right-eye keysrightUV(regression test for the upstream CodeRabbit Major)CropX/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 upstream community-shaders#2096 — the original PR forked this util into its own
Subrect::namespace, which conflicted with community-shaders#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) is also open on this repo; PR-3 + PR-3b (DlssEnhancer foveated subrect) are on the fork awaiting VR validation.
Test plan
Credits
Decomposed from @YtzyFvra's community-shaders#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
Chores