fix(VR): remove pixeloffset overload#2106
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a dedicated single-channel POM offset texture and sentinel, threads a Changes
Sequence DiagramsequenceDiagram
participant Deferred as Deferred
participant Lighting as LightingPS
participant PomUAV as PomOffsetTex_UAV
participant D3D as D3D11_Device
participant StereoBlend as StereoBlendCS
participant PomSRV as PomOffsetTexture_SRV
Deferred->>Deferred: ClearPomOffsetTexture() (write -1.0 sentinel)
Deferred->>Lighting: Run lighting pass (per-pixel)
Lighting->>Lighting: GetParallaxCoords(..., out hasPOM)
alt hasPOM == true
Lighting->>PomUAV: Write pixelOffset (>= 0.0)
else hasPOM == false
Lighting->>PomUAV: Write POM_NO_DATA (-1.0)
end
Lighting->>D3D: OMSetRenderTargets(...)
D3D->>D3D: Hook may rebind POM UAV at UAV slot 7
StereoBlend->>PomSRV: Sample PomOffsetTexture
alt pomOffset >= 0.0
StereoBlend->>StereoBlend: Apply POM reprojection correction
else
StereoBlend->>StereoBlend: Skip POM adjustment
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Features/VRStereoOptimizations.h (1)
66-67: Consider adding an auto-closing issue keyword in the PR description.If this fully resolves the dependency bug/feature, add
Closes#2097(or `Fixes `#2097) so tracking is automatic.As per coding guidelines: "When reviewing PRs, please provide suggestions for ... Issue References ... 'Fixes
#123' or 'Closes#123' for bug fixes."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Features/VRStereoOptimizations.h` around lines 66 - 67, Add an auto-closing issue keyword to the PR description so the change that touches StereoMode and disocclusionDepthThreshold automatically closes the tracked issue; update the PR description to include a line like "Closes `#2097`" (or "Fixes `#2097`") referencing the relevant issue number and ensure it appears in the top-level PR body so merging will auto-close the issue.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Features/VR.cpp`:
- Around line 123-127: The readiness check currently sets stereoOpt.loaded based
only on stereoOpt.GetModeTextureSRV(), but overwrite mode also needs the POM
texture (texPomOffset); update the logic in the branch that calls
stereoOpt.SetupResources() so that stereoOpt.loaded is true only if
GetModeTextureSRV() != nullptr AND, when stereoOpt.settings.stereoMode indicates
the overwrite/PO M path, the texPomOffset resource was successfully created
(non-null). Ensure this change references stereoOpt.settings.stereoMode,
VRStereoOptimizations::StereoMode::Off, stereoOpt.SetupResources(),
stereoOpt.GetModeTextureSRV(), and texPomOffset so failure to create
texPomOffset will set stereoOpt.loaded = false and allow graceful degradation.
In `@src/Features/VRStereoOptimizations.cpp`:
- Around line 244-249: The UAV clear in
VRStereoOptimizations::ClearPomOffsetTexture uses {0,0,0,0} which conflicts with
Lighting.hlsl's no-POM sentinel (-1.0) and StereoBlendCS.hlsl's logic that
treats >=0.0 as valid; change the clear value to use the no-POM sentinel (e.g.,
-1.0f in the relevant component(s)) so untouched pixels are marked as no-POM
before calling
globals::d3d::context->ClearUnorderedAccessViewFloat(texPomOffset->uav.get(),
...); ensure the same component layout expected by
StereoBlendCS.hlsl/Lighting.hlsl is used.
In `@src/Globals.cpp`:
- Around line 388-393: The detour installation currently gates
stl::detour_vfunc<33, ID3D11DeviceContext_OMSetRenderTargets>,
stl::detour_vfunc<36, ID3D11DeviceContext_OMSetDepthStencilState>, and
stl::detour_vfunc<53, ID3D11DeviceContext_ClearDepthStencilView> behind
globals::features::vr.stereoOpt.settings.stereoMode, preventing enabling stereo
later without restart; remove the stereoMode check so the hooks are installed
regardless of the current stereoMode (e.g., keep the check only on
globals::game::isVR or install unconditionally) so the detours are registered
once and will take effect when the user changes stereoMode at runtime.
---
Nitpick comments:
In `@src/Features/VRStereoOptimizations.h`:
- Around line 66-67: Add an auto-closing issue keyword to the PR description so
the change that touches StereoMode and disocclusionDepthThreshold automatically
closes the tracked issue; update the PR description to include a line like
"Closes `#2097`" (or "Fixes `#2097`") referencing the relevant issue number and
ensure it appears in the top-level PR body so merging will auto-close the issue.
🪄 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: 681ec89a-b9f6-4011-bfc1-244a8a1f5944
📒 Files selected for processing (10)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/VR/StereoBlendCS.hlslsrc/Deferred.cppsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/StereoBlend.cppsrc/Features/VRStereoOptimizations.cppsrc/Features/VRStereoOptimizations.hsrc/Globals.cpp
|
✅ A pre-release build is available for this PR: |
Contributor
|
TB works again as it should.
|
alandtse
force-pushed
the
stereopt_no_overload
branch
from
e2364a6 to
629a84c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Deferred.cpp (1)
263-265: Gate this clear on the stereo path being active.
REL::Module::IsVR()is broader than the feature state here. IfVRStereoOptimizationsis initialized whileStereoMode::Off, this still pays a full-frame UAV clear every deferred frame for a disabled path. Please key this off an active-state/helper instead of module type alone. As per coding guidelines, "Consider GPU workload and performance impact when implementing graphics features, with special attention to shader compilation and runtime performance."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Deferred.cpp` around lines 263 - 265, Replace the broad module check REL::Module::IsVR() with a stereo-path active check so the POM offset UAV clear only runs when the VR stereo path is enabled; specifically, in the block that calls globals::features::vr.stereoOpt.ClearPomOffsetTexture(), gate it on the stereo feature's active state (for example use globals::features::vr.stereoOpt.IsActive() or check globals::features::vr.stereoOpt.GetStereoMode() != StereoMode::Off) instead of REL::Module::IsVR() so the clear is skipped when the stereo path is initialized but inactive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 360-365: The comment on the PomOffsetTex RWTexture2D is wrong:
change the sentinel description to indicate that -1.0 means "no POM" and values
>= 0 mean "POM ran" (this matches the clear-to -1.0f in the VRStereo
optimization code and the StereoBlendCS check for >= 0); update the comment next
to PomOffsetTex in Lighting.hlsl and fix the corresponding explanatory comments
in Deferred.cpp and VRStereoOptimizations.h so they consistently state "-1.0 =
no POM; >= 0 = POM ran" and mention that StereoBlendCS treats >= 0 as POM having
executed.
---
Nitpick comments:
In `@src/Deferred.cpp`:
- Around line 263-265: Replace the broad module check REL::Module::IsVR() with a
stereo-path active check so the POM offset UAV clear only runs when the VR
stereo path is enabled; specifically, in the block that calls
globals::features::vr.stereoOpt.ClearPomOffsetTexture(), gate it on the stereo
feature's active state (for example use
globals::features::vr.stereoOpt.IsActive() or check
globals::features::vr.stereoOpt.GetStereoMode() != StereoMode::Off) instead of
REL::Module::IsVR() so the clear is skipped when the stereo path is initialized
but inactive.
🪄 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: 4561a89c-d8e8-4426-ae64-4332fbfee19e
📒 Files selected for processing (10)
features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/VR/StereoBlendCS.hlslsrc/Deferred.cppsrc/Features/VR.cppsrc/Features/VR.hsrc/Features/VR/StereoBlend.cppsrc/Features/VRStereoOptimizations.cppsrc/Features/VRStereoOptimizations.hsrc/Globals.cpp
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Features/VR/StereoBlend.cpp
- src/Features/VR.h
- src/Features/VR.cpp
- src/Features/VRStereoOptimizations.cpp
- src/Globals.cpp
- package/Shaders/VR/StereoBlendCS.hlsl
- features/Extended Materials/Shaders/ExtendedMaterials/ExtendedMaterials.hlsli
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Deferred.cpp`:
- Around line 263-265: The POM clear is always executed on VR frames even when
stereo reprojection is inactive; update the conditional around the
ClearPomOffsetTexture() call to also check stereoOpt state (e.g.,
globals::features::vr.stereoOpt.loaded and that
globals::features::vr.stereoOpt.mode is not StereoMode::Off) so
ClearPomOffsetTexture() is only invoked when stereo reprojection is active.
In `@src/Globals.cpp`:
- Around line 294-307: The hook in thunk currently re-binds the POM offset UAV
unconditionally during globals::deferred->deferredPass which leaves the UAV
bound when the MRTs are being cleared; modify the condition to also check
NumViews != 0 before calling stereoOpt.GetPomOffsetUAV() and
This->OMSetRenderTargetsAndUnorderedAccessViews so the UAV is only rebound
during actual render passes (NumViews > 0) and will be unbound during
OMSetRenderTargets(0, nullptr, nullptr) teardown (preventing conflicts with
DrawStereoBlend's CS SRV binding).
🪄 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: 7a9a4a6e-487e-4ed9-8314-c2058730a348
📒 Files selected for processing (7)
package/Shaders/Common/VR.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/VR/StereoBlendCS.hlslsrc/Deferred.cppsrc/Features/VRStereoOptimizations.cppsrc/Features/VRStereoOptimizations.hsrc/Globals.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Features/VRStereoOptimizations.h
doodlum
approved these changes
Apr 13, 2026
YtzyFvra
pushed a commit
to YtzyFvra/skyrim-community-shaders
that referenced
this pull request
Apr 19, 2026
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
3 participants
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.
Depends on #2097
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Performance