feat(vr): foveated SSR raymarching#82
Conversation
|
No actionable suggestions for changed features. |
|
PR changed again? Review this PR in Change Stack to compare snapshots and stay oriented. Warning Review limit reached
More reviews will be available in 4 minutes and 31 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughImplements VR foveated SSR: shader superellipse mask and detail modes, per-pixel SSR iteration scaling and alpha modulation, CPU helpers and foveation profile API, VR settings/UI, SharedData cbuffer fields, State wiring, and optional Tracy GPU timing hooks. ChangesVR Foveated Screen-Space Reflections
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
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 |
There was a problem hiding this comment.
Pull request overview
Adds VR-only foveated SSR raymarching by deriving a per-eye foveation profile from the active Foveated DLSS subrect and using it to (a) scale SSR raymarch/binary-search iterations and (b) attenuate/early-out SSR in the periphery. This fits into the existing VR + Upscaling (DLSS) pipeline by extending the shared per-frame cbuffer and adding small reusable CPU/GPU foveation helpers.
Changes:
- Extend the shared
PerFrame/SharedDataconstant buffer with VR foveation parameters and populate them inState::UpdateSharedData. - Add a
FoveatedRender::GetFoveationProfile()adapter (rect subrect → centered-superellipse params) plus shared CPU clamping/mode helpers. - Update SSR shader to consume the foveation mask (VR only) and add a VR settings UI toggle + serialization.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/State.h | Adds VR foveation float4s to SharedDataCB and alignment assertions. |
| src/State.cpp | Populates VR foveation constants per-frame when VR + foveated DLSS + SSR are active. |
| src/Features/VR/SettingsUI.cpp | Adds VR UI toggles for foveated SSR and hard-cutoff mode, gated on DLSS foveation + SSR enabled. |
| src/Features/VR.h | Adds new VR settings fields for SSR foveation controls. |
| src/Features/VR.cpp | Persists the new VR settings via nlohmann JSON serialization. |
| src/Features/Upscaling/FoveatedRender.h | Introduces FoveationProfile and GetFoveationProfile() API. |
| src/Features/Upscaling/FoveatedRender.cpp | Implements profile synthesis from the DLSS subrect(s). |
| src/Features/FoveatedCommon.h | New CPU-side shared constants, clamping, and mode encoding helpers. |
| package/Shaders/ISReflectionsRayTracing.hlsl | VR-only SSR iteration scaling + alpha weighting + early-out using the foveation mask. |
| package/Shaders/Common/SharedData.hlsli | Mirrors the new shared cbuffer fields for VR foveation parameters. |
| package/Shaders/Common/FoveatedShaderDetail.hlsli | New GPU helper to evaluate per-pixel foveated “detail weight” (0..1). |
| package/Shaders/Common/FoveatedMask.hlsli | New GPU superellipse mask implementation and feathering math. |
3b04436 to
6544164
Compare
6544164 to
1916141
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Features/Upscaling/FoveatedRender.cpp (1)
127-127: 💤 Low valueUse the named threshold constant instead of a hardcoded
0.999f.
FoveatedCommon::kFullCoverageThreshold(0.999f) already encodes the "full coverage" cutoff and is used byIsActiveCoverage. Referencing it here keeps the early-out and the activation check moving together.♻️ Proposed change
- if (leftUV.w >= 0.999f && leftUV.h >= 0.999f) + if (leftUV.w >= FoveatedCommon::kFullCoverageThreshold && leftUV.h >= FoveatedCommon::kFullCoverageThreshold) return profile;🤖 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/Features/Upscaling/FoveatedRender.cpp` at line 127, Replace the hardcoded 0.999f comparison in FoveatedRender (the early-out that checks leftUV.w and leftUV.h) with the named constant FoveatedCommon::kFullCoverageThreshold so the early-exit uses the same threshold as IsActiveCoverage; update the conditional that currently checks leftUV.w >= 0.999f && leftUV.h >= 0.999f to compare against FoveatedCommon::kFullCoverageThreshold for both leftUV.w and leftUV.h.
🤖 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/Features/Upscaling/FoveatedRender.cpp`:
- Around line 137-139: GetFoveationProfile currently computes
profile.coverageArea from leftUV.h only, causing full-height but reduced-width
subrects to appear as full coverage and skip activation; change it to derive
coverage from both axes (e.g., compute float coverage = std::max(leftUV.w,
leftUV.h) or another combined metric and pass that to
FoveatedCommon::ClampCenterArea) so IsActiveCoverage(profile.coverageArea)
reflects narrow-width cases; update the assignment of profile.coverageArea and
ensure profile.centerHorizontalScale still uses the existing leftUV.w / leftUV.h
guard as before in GetFoveationProfile.
---
Nitpick comments:
In `@src/Features/Upscaling/FoveatedRender.cpp`:
- Line 127: Replace the hardcoded 0.999f comparison in FoveatedRender (the
early-out that checks leftUV.w and leftUV.h) with the named constant
FoveatedCommon::kFullCoverageThreshold so the early-exit uses the same threshold
as IsActiveCoverage; update the conditional that currently checks leftUV.w >=
0.999f && leftUV.h >= 0.999f to compare against
FoveatedCommon::kFullCoverageThreshold for both leftUV.w and leftUV.h.
🪄 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: 2c73b40b-2324-4ba9-a8b2-5fd8aaba7de9
📒 Files selected for processing (12)
package/Shaders/Common/FoveatedMask.hlslipackage/Shaders/Common/FoveatedShaderDetail.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/ISReflectionsRayTracing.hlslsrc/Features/FoveatedCommon.hsrc/Features/Upscaling/FoveatedRender.cppsrc/Features/Upscaling/FoveatedRender.hsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/SettingsUI.cppsrc/State.cppsrc/State.h
|
✅ A pre-release build is available for this PR: |
5e1a997 to
102d52b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Features/Upscaling/FoveatedRender.cpp (1)
137-140:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCoverage activation ignores reduced-width subrects.
profile.coverageAreais derived solely fromleftUV.h, so a full-height but reduced-width region (e.g.w=0.5, h=1.0) passes the&&early-out at Line 127, then yieldscoverageArea ≈ 1.0and is treated as full coverage by downstreamIsActiveCoverage(), disabling foveation even though width was reduced. Derive coverage from both axes.🤖 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/Features/Upscaling/FoveatedRender.cpp` around lines 137 - 140, profile.coverageArea currently only uses leftUV.h which ignores reduced-width subrects; update the assignment of profile.coverageArea to derive coverage from both leftUV.w and leftUV.h (for example compute an area factor like leftUV.w * leftUV.h and pass that into FoveatedCommon::ClampCenterArea) so downstream IsActiveCoverage() correctly treats reduced-width regions as partial coverage; keep the existing centerHorizontalScale calculation (profile.centerHorizontalScale) but ensure it still guards against zero height as it does now.
🤖 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.
Duplicate comments:
In `@src/Features/Upscaling/FoveatedRender.cpp`:
- Around line 137-140: profile.coverageArea currently only uses leftUV.h which
ignores reduced-width subrects; update the assignment of profile.coverageArea to
derive coverage from both leftUV.w and leftUV.h (for example compute an area
factor like leftUV.w * leftUV.h and pass that into
FoveatedCommon::ClampCenterArea) so downstream IsActiveCoverage() correctly
treats reduced-width regions as partial coverage; keep the existing
centerHorizontalScale calculation (profile.centerHorizontalScale) but ensure it
still guards against zero height as it does now.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8d7ee34-6c4d-4670-a011-2e821c108dd4
📒 Files selected for processing (12)
package/Shaders/Common/FoveatedMask.hlslipackage/Shaders/Common/FoveatedShaderDetail.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/ISReflectionsRayTracing.hlslsrc/Features/FoveatedCommon.hsrc/Features/Upscaling/FoveatedRender.cppsrc/Features/Upscaling/FoveatedRender.hsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/SettingsUI.cppsrc/State.cppsrc/State.h
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Features/VR.cpp
- src/Features/Upscaling/FoveatedRender.h
- package/Shaders/Common/SharedData.hlsli
- src/Features/VR/SettingsUI.cpp
- src/Features/VR.h
- src/Features/FoveatedCommon.h
- src/State.cpp
- package/Shaders/Common/FoveatedMask.hlsli
…ge comment Address Copilot review on #82: - GetFoveationProfile now disables only when BOTH eyes are full-eye and derives coverageArea/horizontalScale from the less-foveated (max) extent of the two eyes, so an asymmetric per-eye crop can't wrongly disable the profile and the single shared scale never foveates inside either eye's full-quality region. No-op for the symmetric seeded presets. - Clarify the VRFoveationData0.x comment as 'center coverage scale' (set from FoveationProfile::coverageArea) to disambiguate from horizontal scale. The disk-shader-cache stale-offset concern on SharedData layout changes is a pre-existing, general ShaderCache behavior (skip-unchanged-without-file-watcher ignores include mtimes) — tracked separately, not specific to this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ge comment Address Copilot review on #82: - GetFoveationProfile now disables only when BOTH eyes are full-eye and derives coverageArea/horizontalScale from the less-foveated (max) extent of the two eyes, so an asymmetric per-eye crop can't wrongly disable the profile and the single shared scale never foveates inside either eye's full-quality region. No-op for the symmetric seeded presets. - Clarify the VRFoveationData0.x comment as 'center coverage scale' (set from FoveationProfile::coverageArea) to disambiguate from horizontal scale. The disk-shader-cache stale-offset concern on SharedData layout changes is a pre-existing, general ShaderCache behavior (skip-unchanged-without-file-watcher ignores include mtimes) — tracked separately, not specific to this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cb5d115 to
02a3b04
Compare
…ge comment Address Copilot review on #82: - GetFoveationProfile now disables only when BOTH eyes are full-eye and derives coverageArea/horizontalScale from the less-foveated (max) extent of the two eyes, so an asymmetric per-eye crop can't wrongly disable the profile and the single shared scale never foveates inside either eye's full-quality region. No-op for the symmetric seeded presets. - Clarify the VRFoveationData0.x comment as 'center coverage scale' (set from FoveationProfile::coverageArea) to disambiguate from horizontal scale. The disk-shader-cache stale-offset concern on SharedData layout changes is a pre-existing, general ShaderCache behavior (skip-unchanged-without-file-watcher ignores include mtimes) — tracked separately, not specific to this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
02a3b04 to
73d7d0c
Compare
…ge comment Address Copilot review on #82: - GetFoveationProfile now disables only when BOTH eyes are full-eye and derives coverageArea/horizontalScale from the less-foveated (max) extent of the two eyes, so an asymmetric per-eye crop can't wrongly disable the profile and the single shared scale never foveates inside either eye's full-quality region. No-op for the symmetric seeded presets. - Clarify the VRFoveationData0.x comment as 'center coverage scale' (set from FoveationProfile::coverageArea) to disambiguate from horizontal scale. The disk-shader-cache stale-offset concern on SharedData layout changes is a pre-existing, general ShaderCache behavior (skip-unchanged-without-file-watcher ignores include mtimes) — tracked separately, not specific to this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
73d7d0c to
415ccf6
Compare
Address review feedback on the foveation profile adapter: replace the hard-coded 0.999f full-eye threshold with FoveatedCommon::kFullCoverageThreshold (same value, shared constant), and document that the mask carries a single scale for both eyes — size comes from the left eye since seeded presets are symmetric in size and only the per-eye center offset differs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ge comment Address Copilot review on #82: - GetFoveationProfile now disables only when BOTH eyes are full-eye and derives coverageArea/horizontalScale from the less-foveated (max) extent of the two eyes, so an asymmetric per-eye crop can't wrongly disable the profile and the single shared scale never foveates inside either eye's full-quality region. No-op for the symmetric seeded presets. - Clarify the VRFoveationData0.x comment as 'center coverage scale' (set from FoveationProfile::coverageArea) to disambiguate from horizontal scale. The disk-shader-cache stale-offset concern on SharedData layout changes is a pre-existing, general ShaderCache behavior (skip-unchanged-without-file-watcher ignores include mtimes) — tracked separately, not specific to this PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The value is a linear center coverage scale (vertical UV extent, used as radiusY = scale/2), not an area ratio. Rename the misleading 'Area' cluster to 'Scale' across C++ (FoveationProfile field, FoveatedCommon constants/helpers) and the HLSL mask include, matching the centerScale vocabulary the shader entry points already use. Pure identifier rename; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
e7c3a82 to
1ce96da
Compare
available was set true whenever either eye was non-full, but coverageScale takes the max of both eyes' extent, so a single full eye clamps the scale to full coverage while available stayed true — contradicting the header contract and forcing State.cpp to re-check IsActiveCoverage(). Derive available from the final clamped coverageScale and early-return when inactive; drop the now-redundant both-eyes full check and the caller-side coverage check. Consumer behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The PR added VRFoveationData0 / VRFoveationCenterOffsets to the SharedData cbuffer but only asserted the overall struct alignment. Add per-field offsetof % 16 == 0 guards (incl. the shifted HDRData) so a stray scalar inserted above can't silently misalign the float4s and corrupt shader reads. Addresses the Copilot review note on State.h. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cut incidental detail from the GetFoveationProfile mapping comment and the UpdateSharedData default-state comment; keep the load-bearing invariants (rect->superellipse mapping, max-of-two-eyes rationale, full-eye gate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6d35f08 to
de4f54b
Compare
|
Split the Tracy GPU profiling hook out into #106 ( |
FoveatedComputeMaskDistance ran three transcendental pow() calls per pixel; the default squircle (shape power 4) now uses (t*t)^2 and sqrt(sqrt()) instead, with the generic pow() path kept under #else for other powers. Also reword the GetFoveationProfile comment: the max-of-both-eyes center is a superset enclosing each eye's sharp region (the old wording said inside, the opposite of max). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Apply the tightened comment-length standard to the foveation comments this branch introduced: trim the file/namespace headers and the incidental block comments to 1-2 lines, pushing the long-form rationale to commit messages. Leaves the GetFoveationProfile max-of-both-eyes invariant intact (load-bearing). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Conform the namespace contract header to the tightened 1-2 line comment rule; keep the GPU-mirror + encoding contract and the compute-helpers- omitted note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds a diagnostic-only Tracy GPU zone around the engine SSR raymarch pass (`ISReflectionsRayTracing`), so the pass is visible on the Tracy GPU timeline. Split out of #82 (foveated SSR) since it's independent profiling infrastructure, not part of the feature. ## What it does Wraps the SSR raymarch DRAW with a `tracy::D3D11ZoneScope`, opened on the shader's `PreRender` (vfunc `0x0A`) and closed on `PostRender` (`0x0B`). ## Why these hook points RenderDoc callstacks proved the effect renders as a pixel-shader DRAW issued by the deferred batch renderer through the **non-virtual** `BSImagespaceShader::Render` — not the `ImageSpaceManager` vtable Render slot and not `DispatchComputeShader` (both of which were tried first and never fired). `Render` brackets the draw with the per-shader `PreRender`/`PostRender` vfuncs, which CommonLib keeps at stable indices across SE/AE/VR (the VR-only `FakeDispatchComputeShader` inserts at `0x0D`, after them), so hooking `0x0A`/`0x0B` on the cross-versioned `ReflectionsRayTracing` vtable wraps exactly the SSR draw on all three runtimes with no per-version addresses. ## Safety - `TRACY_ENABLE`-gated — zero impact on shipping builds. - Behaviour-preserving: each thunk only chains the original vfunc. - Single raw zone holder is safe: render-thread only, non-nested for this shader; the `PreRender` thunk defensively closes any prior zone if `PostRender` were ever skipped. Verified live (ALL-TRACY build): zone fires ~525/530 frames, ~88us baseline. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Adds foveated SSR raymarching for VR: screen-space reflections raymarch at full cost in the center of view and progressively cheaper toward the periphery, using the active Foveated DLSS region as the mask. Central reflections are unchanged; peripheral pixels fall back to the cubemap / water reflection. A net VR GPU-time saving on reflective scenes, at the cost of peripheral SSR detail.
Ported from the foveation framework in ParticleTroned/skyrim-community-shaders, scoped to SSR only with no dead code (compute-dispatch helpers omitted).
How it works
FoveatedRender::GetFoveationProfile()synthesizes the centered-superellipse params (coverage / horizontal scale / per-eye center offset) the mask consumes. We deliberately do not adopt the upstream "oval-as-source" foundation, which would collide with our PR feat(VR): add DLSS Enhancer with DLSSperf and subrect blend community-shaders/skyrim-community-shaders#2096 foveated-DLSS divergence.FoveatedShaderDetail.hlsli+FoveatedMask.hlsli(superellipse, feathered or hard-cutoff) → a per-pixel 0..1 weight.minFoveatedIterations = 16vs64), the SSR alpha is multiplied by the weight, and pixels outside the mask early-out. Non-VR is behaviorally identical: the non-VR branch setsrayCount = iterations/binCount = binaryIterations/fovWeight = 1.0, collapsing the (now VR-shared) loop body to the original constants and weight 1.0. The loop body is shared with the VR path rather than duplicated, so the source/bytecode is not byte-identical — equivalence is by construction + runtime A/B, not a bytecode diff.float4s added to the sharedPerFramecbuffer (VRFoveationData0+VRFoveationCenterOffsets), with matching C++ layout under the existingSTATIC_ASSERT_ALIGNAS_16size check.Foveate SSR Raymarching+Hard Cutofftoggles, gated (with hints) on Foveated DLSS being active and SSR enabled.Scope decisions (per design discussion)
Validation
BuildRelease.bat ALL(universal SE/AE/VR) — clean, exit 0, no warnings on the changed TUs.hlslkitcompile ofISReflectionsRayTracing.hlsl— 0 errors / 0 warnings on both the VR and Flatrim configs.ISReflectionsRayTracingGPU zone on anALL-TRACYbuild, same scene and camera, Center 50% feathered foveation: the pass drops from ~88 µs to ~44 µs — a saving of ~44 µs, about 0.4% of the 90 fps frame budget (11.1 ms). Small in absolute terms, but reclaimed entirely in the periphery where reflections aren't scrutinized, and it scales with how much SSR the scene actually draws (reflective interiors / water gain more). The rig was present-throttled with the HMD off, so per-pass GPU time is the signal here, not frame rate.Attribution
Foveation framework by ParticleTroned (ParticleTroned/skyrim-community-shaders).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
UI
Settings