Skip to content

fix(upscaling): fix motion vectors underwater#2023

Merged
alandtse merged 2 commits into
community-shaders:devfrom
soda3000:dev-28-03-2026
Mar 29, 2026
Merged

fix(upscaling): fix motion vectors underwater#2023
alandtse merged 2 commits into
community-shaders:devfrom
soda3000:dev-28-03-2026

Conversation

@soda3000
Copy link
Copy Markdown
Contributor

@soda3000 soda3000 commented Mar 28, 2026

Summary by CodeRabbit

  • Refactor
    • Optimized shader rendering pipeline by removing an unnecessary output target from a specific rendering path, reducing computational overhead while maintaining visual quality.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 28, 2026

📝 Walkthrough

Walkthrough

A removal of the Normal render target output from the PS_OUTPUT struct in a non-DEFERRED and non-MOTIONVECTORS_NORMALS conditional compilation path, with corresponding deletion of the normal output assignment in the shader's main function.

Changes

Cohort / File(s) Summary
Shader Output Modification
package/Shaders/Effect.hlsl
Removed float4 Normal: SV_Target1; field from PS_OUTPUT struct and its assignment in the main function for the non-DEFERRED, non-MOTIONVECTORS_NORMALS path.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested reviewers

  • jiayev
  • doodlum

Poem

🐰 With a hop and a skip, away goes the glow,
No more normals flowing where shaders don't go,
One path grows cleaner, more streamlined and lean,
The finest refactor a rabbit has seen!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'fix(upscaling): fix motion vectors underwater' does not match the actual objective described in the PR, which addresses corrupted motion vectors in forward rendered effect shaders affecting frame generation, not specifically underwater motion vectors. Update the title to accurately reflect the main issue being fixed, such as 'fix(upscaling): fix motion vectors in forward rendered effect shaders' or similar to align with the stated objective.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ 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.

@soda3000
Copy link
Copy Markdown
Contributor Author

fixes issue #1766

@doodlum doodlum changed the title fix(upscaling): corrupted motion vectors in forward rendered effect shaders that broke frame gen fix(upscaling): fix motion vectors underwater Mar 28, 2026
@soda3000
Copy link
Copy Markdown
Contributor Author

@claude what is wrong with the tests

@soda3000
Copy link
Copy Markdown
Contributor Author

@coderabbitai what is wrong with the tests

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Mar 28, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get PR checks/status for PR `#2023`
gh pr checks 2023 --repo doodlum/skyrim-community-shaders

echo "---"
# Get PR status and details
gh pr view 2023 --repo doodlum/skyrim-community-shaders --json statusCheckRollup,reviews,state

Length of output: 6637


🏁 Script executed:

#!/bin/bash
# Fetch logs for the failing jobs
echo "=== Run Shader Unit Tests ==="
gh api /repos/doodlum/skyrim-community-shaders/actions/jobs/69030360847/logs 2>&1 | tail -100

echo ""
echo "=== Validate shader compilation (Flatrim) ==="
gh api /repos/doodlum/skyrim-community-shaders/actions/jobs/69030360864/logs 2>&1 | tail -80

echo ""
echo "=== Validate shader compilation (VR) ==="
gh api /repos/doodlum/skyrim-community-shaders/actions/jobs/69030360866/logs 2>&1 | tail -80

Length of output: 50343


🏁 Script executed:

#!/bin/bash
# Get more of the logs - specifically the error parts (not just the tail)
echo "=== Run Shader Unit Tests - searching for errors ==="
gh api /repos/doodlum/skyrim-community-shaders/actions/jobs/69030360847/logs 2>&1 | grep -i -E "error|fail|FAILED|assert|warning|Effect" | head -60

echo ""
echo "=== Validate shader compilation (Flatrim) - searching for errors ==="
gh api /repos/doodlum/skyrim-community-shaders/actions/jobs/69030360864/logs 2>&1 | grep -i -E "error|fail|FAILED|assert|Effect" | head -60

echo ""
echo "=== Validate shader compilation (VR) - searching for errors ==="
gh api /repos/doodlum/skyrim-community-shaders/actions/jobs/69030360866/logs 2>&1 | grep -i -E "error|fail|FAILED|assert|Effect" | head -60

Length of output: 10590


@soda3000 The test failures are not caused by your changes. All three failing jobs crash at the same place — the MSVC compiler hits an Internal Compiler Error (ICE) while building the Catch2 test framework itself, not your shader code:

catch_errno_guard.cpp : fatal error C1001: Internal compiler error.
LINK : fatal error LNK1000: Internal error during IMAGE::BuildImage

The failure is deep inside catch2-src/src/catch2/internal/catch_errno_guard.cpp — a Catch2 library internal file — not in Effect.hlsl or anything related to this PR.

A few things that confirm this is a CI infrastructure issue:

  • Both plugin builds (vs2022 and vs2026) passed — the actual compilation of the codebase works fine.
  • ❌ All 3 test/shader-compilation jobs fail at the exact same Catch2 build step with the same MSVC ICE.
  • C1001 is an MSVC internal compiler crash — it means MSVC itself crashed, not that your code is wrong.

This is likely a transient runner issue (e.g., resource pressure or an MSVC regression on the CI machine). You can try re-running the failed jobs from the Actions page to see if they pass on a fresh runner.


🧠 Learnings used
Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1271-1274
Timestamp: 2026-03-22T18:40:49.633Z
Learning: Repo: doodlum/skyrim-community-shaders — Maintainer preference (PR `#2000` on 2026-03-22): In src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI, keep `resetLayout = false;` at the end of the frame (after PaletteWindow::Draw). The reset is intentionally one-shot for windows rendered that frame; closed windows are allowed to reopen with their previous geometry. Do not move the clear later or broadcast resets to closed windows.

Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2003
File: features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl:55-61
Timestamp: 2026-03-24T04:30:32.809Z
Learning: In `features/Upscaling/Shaders/Upscaling/DepthUpscalePS.hlsl` (doodlum/skyrim-community-shaders PR `#2003`), the shader is dispatched per-eye using per-eye dimensions (DstWidth/DstHeight). The UV (`input.TexCoord`) is derived from `dispatchID / DstDim`, which is already eye-local — not full SBS/packed stereo coordinates. The per-eye UV clamping logic (isRight, halfScale) operates entirely in eye-local space, and there is no stereo seam issue. Do not flag SBS seam concerns for this shader.

Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1950
File: package/Shaders/Lighting.hlsl:2008-2012
Timestamp: 2026-03-07T00:53:21.267Z
Learning: In doodlum/skyrim-community-shaders PR `#1950`, the maintainer (Dlizzio) confirmed that per-axis normal reorientation for triplanar-projected normals/detail in package/Shaders/Lighting.hlsl is not required for their use case; the current approach intentionally mixes samples without axis-specific swizzle/sign correction.

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.

Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 2000
File: src/WeatherEditor/EditorWindow.cpp:1239-1244
Timestamp: 2026-03-22T18:32:22.461Z
Learning: In doodlum/skyrim-community-shaders (PR `#2000`, 2026-03-22), maintainer preference: keep the default browser width logic in src/WeatherEditor/EditorWindow.cpp within EditorWindow::RenderUI — browserWidth = min(availableWidth * 0.5f, 960.0f * Util::GetUIScale()) is applied unconditionally (before checking settings.showViewport) to preserve a consistent left-rail width on first-use/reset. Do not change this to fill full width when the viewport is hidden.

Learnt from: Dlizzio
Repo: doodlum/skyrim-community-shaders PR: 1966
File: package/Shaders/ISVolumetricLightingGenerateCS.hlsl:0-0
Timestamp: 2026-03-11T08:05:10.801Z
Learning: In doodlum/skyrim-community-shaders, the `sqrt()` applied to `CloudShadows::GetCloudShadowMult()` in `package/Shaders/ISVolumetricLightingGenerateCS.hlsl` is intentional. The maintainer (Dlizzio) confirmed it is used to increase cloud shadow strength at harsh angles for volumetric lighting, and should not be removed or replaced with a direct linear multiply.

Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 1947
File: package/Shaders/DeferredCompositeCS.hlsl:234-243
Timestamp: 2026-03-14T08:35:42.651Z
Learning: In `package/Shaders/DeferredCompositeCS.hlsl` (doodlum/skyrim-community-shaders), the fallback non-IBL SKYLIGHTING specular path intentionally double-weights with `skylightingSpecular`: `dalcScaled` is computed as `IrradianceToGamma(IrradianceToLinear(directionalAmbientColorSpecular) * skylightingSpecular)`, and the final `finalIrradiance` is then `lerp(specularIrradiance, specularIrradianceReflections, skylightingSpecular)`. This is original engine logic preserved from before PR `#1947` and should not be flagged as a double-attenuation bug.

Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 1982
File: src/Features/VR/StereoBlend.cpp:98-109
Timestamp: 2026-03-16T13:13:31.383Z
Learning: In `src/Features/VR/StereoBlend.cpp` (doodlum/skyrim-community-shaders), debug modes 4/5 ("Overwrite", "Overwrite Eye1") in the `StereoBlendDebugMode` selector are NOT separate shader variants. They use the same `stereoBlendOverwriteCS` shader with different cbuffer parameters. When `vrStereoOptActive` is true, `stereoBlendOverwriteCS` is always selected unconditionally and `StereoBlendDebugMode` values 4/5 do not map to any distinct code path.

Learnt from: jiayev
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.

Learnt from: vrnord
Repo: doodlum/skyrim-community-shaders PR: 2002
File: package/Shaders/VRStereoOptimizations/StencilCS.hlsl:97-143
Timestamp: 2026-03-24T05:34:21.730Z
Learning: In `package/Shaders/VRStereoOptimizations/StencilCS.hlsl` (doodlum/skyrim-community-shaders PR `#2002`), `EdgeWidth` has been fully removed as a setting. The edge detection search width is hardcoded to `kInnerWidth = 2` with `maxWidth = kInnerWidth`. Do not flag EdgeWidth as a dead/unused parameter — it no longer exists in `VRStereoOptParams` or `cbuffers.hlsli` in this PR.

@alandtse
Copy link
Copy Markdown
Collaborator

Wait. This may fix a single bad path, but this will impact any define where the motion vectors aren't defined. We need to confirm this pattern doesn't happen more.

@alandtse
Copy link
Copy Markdown
Collaborator

Unrelated CI failure, #2026

@alandtse alandtse merged commit 15df3df into community-shaders:dev Mar 29, 2026
20 of 26 checks passed
ParticleTroned pushed a commit to ParticleTroned/skyrim-community-shaders that referenced this pull request May 2, 2026
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