Skip to content

fix(VR): broken desktop window when HDR enabled#2098

Merged
doodlum merged 2 commits into
community-shaders:devfrom
alandtse:hdr_vr_desktop_window
Apr 12, 2026
Merged

fix(VR): broken desktop window when HDR enabled#2098
doodlum merged 2 commits into
community-shaders:devfrom
alandtse:hdr_vr_desktop_window

Conversation

@alandtse
Copy link
Copy Markdown
Collaborator

@alandtse alandtse commented Apr 12, 2026

Move hook to account for VR rendering. HDR is still not fully functional as HMDs do not support HDR. Preview window does not allow HDR slider adjustments.

Summary by CodeRabbit

  • Bug Fixes

    • VR now displays the correct HDR/tonemapped scene in the framebuffer and avoids incorrect UI redirection.
    • ImGui/UI rendering in VR falls back to the appropriate framebuffer path for consistent visuals.
    • Temporal anti-aliasing shader handling improved for VR to prevent incorrect shader selection.
  • Improvements

    • Swap-chain format detection and logging enhanced for clearer diagnostics.

Move hook to account for VR rendering. HDR is still not fully functional
as HMDs do not support HDR. Preview window does not allow HDR slider
adjustments.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 12, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4548d87c-09e5-47f0-825c-4d39276089f6

📥 Commits

Reviewing files that changed from the base of the PR and between 006e66e and 6dea1a7.

📒 Files selected for processing (2)
  • src/Features/HDRDisplay.cpp
  • src/Hooks.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Features/HDRDisplay.cpp

📝 Walkthrough

Walkthrough

Adds VR-aware HDR framebuffer handling: copies tonemapped scene into the runtime kFRAMEBUFFER for VR, prevents UI buffer redirection under VR, prioritizes kFRAMEBUFFER.SRV when VR is active, logs swapchain format, and makes the ISTemporalAA imagespace shader resolution unconditional.

Changes

Cohort / File(s) Summary
HDR & VR framebuffer / presentation
src/Features/HDRDisplay.cpp, src/Hooks.cpp
Added VR-specific CopyResource of hdr->hdrTexture->resource → runtime kFRAMEBUFFER when HDR+VR; early return in SetUIBuffer() for VR to avoid redirecting kFRAMEBUFFER.RTV; ApplyHDR() selects kFRAMEBUFFER.SRV first in VR; ImGui routing in Present skips the non‑VR HDR UI path when VR. Also added swapchain format logging.
Image-space shader descriptor resolution
src/ShaderCache.cpp
Removed VR-gated special-case for BSImagespaceShaderISTemporalAA and added it to the shared descriptors map for unconditional lookup.

Sequence Diagram(s)

sequenceDiagram
    participant Renderer as Game Renderer
    participant HDR as HDR Module
    participant VR as VR Runtime (kFRAMEBUFFER)
    participant GPU as GPU/Resources
    participant ISC as Imagespace/ISCopy

    Renderer->>HDR: render scene -> hdrTexture
    HDR->>GPU: RestoreFramebuffer()
    HDR->>GPU: CopyResource(hdrTexture.resource -> kFRAMEBUFFER)
    GPU->>VR: present framebuffer content
    ISC->>GPU: read kFRAMEBUFFER.SRV for IS processing
    HDR->>Renderer: ApplyHDR(using kFRAMEBUFFER.SRV in VR)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Suggested reviewers

  • doodlum
  • jiayev

Poem

🐰 In pixels and frames my whiskers hop,
I copy the scene so nothing will stop,
VR and HDR now share the same view,
Shaders align, the colors ring true—woop! 🎨✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(VR): broken desktop window when HDR enabled' directly and specifically describes the main objective of the PR—fixing a broken desktop window preview when HDR is enabled in VR scenarios.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

No actionable suggestions for changed features.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/Features/HDRDisplay.cpp (1)

888-900: ⚠️ Potential issue | 🟠 Major

Don't bind a second UI layer in the VR path.

Line 889 switches VR to kFRAMEBUFFER, which already contains vanilla UI + ImGui. But Lines 897-900 still bind uiTexture->srv, even though SetUIBuffer() now exits early in VR. That leaves the VR companion pass sampling an uninitialized/stale UI texture on top of a scene that already has UI baked in.

Proposed fix
-		ID3D11ShaderResourceView* uiSRV = nullptr;
-		if (upscaling.d3d12SwapChainActive && upscaling.dx12SwapChain.uiBufferWrapped) {
-			uiSRV = upscaling.dx12SwapChain.uiBufferWrapped->srv;
-		} else if (uiTexture && uiTexture->srv) {
-			uiSRV = uiTexture->srv.get();
-		}
+		ID3D11ShaderResourceView* uiSRV = nullptr;
+		if (!globals::game::isVR) {
+			if (upscaling.d3d12SwapChainActive && upscaling.dx12SwapChain.uiBufferWrapped) {
+				uiSRV = upscaling.dx12SwapChain.uiBufferWrapped->srv;
+			} else if (uiTexture && uiTexture->srv) {
+				uiSRV = uiTexture->srv.get();
+			}
+		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/Features/HDRDisplay.cpp` around lines 888 - 900, The VR path sets
sceneSRV to the framebuffer (which already contains vanilla UI/ImGui), but uiSRV
is still bound from uiTexture leading to a stale/duplicated UI layer; update the
uiSRV selection (the block that sets uiSRV using upscaling.d3d12SwapChainActive,
upscaling.dx12SwapChain.uiBufferWrapped and uiTexture) to skip binding any
uiTexture when globals::game::isVR is true (or when SetUIBuffer early-returns
for VR), i.e., only assign uiSRV from uiBufferWrapped or uiTexture if not in VR,
otherwise leave uiSRV null so the VR companion pass does not sample a second UI
layer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/Features/HDRDisplay.cpp`:
- Around line 888-900: The VR path sets sceneSRV to the framebuffer (which
already contains vanilla UI/ImGui), but uiSRV is still bound from uiTexture
leading to a stale/duplicated UI layer; update the uiSRV selection (the block
that sets uiSRV using upscaling.d3d12SwapChainActive,
upscaling.dx12SwapChain.uiBufferWrapped and uiTexture) to skip binding any
uiTexture when globals::game::isVR is true (or when SetUIBuffer early-returns
for VR), i.e., only assign uiSRV from uiBufferWrapped or uiTexture if not in VR,
otherwise leave uiSRV null so the VR companion pass does not sample a second UI
layer.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7da143b1-365a-4310-bcb5-65ae7e5cac7b

📥 Commits

Reviewing files that changed from the base of the PR and between 479dc36 and 006e66e.

📒 Files selected for processing (3)
  • src/Features/HDRDisplay.cpp
  • src/Hooks.cpp
  • src/ShaderCache.cpp

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 12, 2026

✅ A pre-release build is available for this PR:
Download

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants