fix: fix normals precision#2233
Closed
doodlum wants to merge 3 commits into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors GBuffer normal encoding from sqrt-based to octahedral, adds an Changes
Sequence Diagram(s)sequenceDiagram
participant Engine
participant Hook as RT Hook
participant GPU as Render Targets
participant Shader as DeferredCompositePS
Engine->>Hook: Request create render target (normals)
Hook->>Engine: Modify properties (format = R10G10B10A2) and forward
Engine->>GPU: Allocate RT with R10G10B10A2
Shader->>GPU: Encode normals via GBuffer::EncodeNormalVanilla and write to RT1
Shader->>Shader: Later composite reads RT1 for lighting passes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package/Shaders/DeferredCompositePS.hlsl (1)
121-128:⚠️ Potential issue | 🔴 CriticalDeclare
NormalRoughnessTexturefor the base permutation.
output.Normalnow always depends onnormalVS, butnormalVSstill comes fromNormalRoughnessTexture, which is only declared behindSSGI || DYNAMIC_CUBEMAPS || DEBUG.Deferred::GetCompositePS()still builds a permutation with none of those defines, so the base-game variant will fail to compile. Sincet2is already bound unconditionally on the C++ side, this texture declaration should be unconditional too.Suggested fix
-#if defined(SSGI) || defined(DYNAMIC_CUBEMAPS) || defined(DEBUG) Texture2D<float4> NormalRoughnessTexture : register(t2); -#endifAlso applies to: 339-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package/Shaders/DeferredCompositePS.hlsl` around lines 121 - 128, The shader fails to compile for the base permutation because NormalRoughnessTexture is only declared under SSGI||DYNAMIC_CUBEMAPS||DEBUG while normalVS and output.Normal always use it; make the NormalRoughnessTexture declaration unconditional (remove the preprocessor guards) so it is available for the base permutation that Deferred::GetCompositePS() builds (t2 is already bound on the C++ side), and apply the same unconditional declaration fix to the other occurrence around lines 339-342.
🤖 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/Hooks.cpp`:
- Around line 591-596: The thunk copies a_properties into a local properties,
mutates properties.format, then calls func(...) but fails to run the shared
mutation in globals::state->ModifyRenderTarget, so normals targets lose common
flags (e.g., UAV). Fix: after setting properties.format and before calling
func(This, a_target, &properties), call
globals::state->ModifyRenderTarget(a_target, properties) (or the correct
overload that mutates the struct in-place) and then pass the resulting/modified
properties pointer to func; apply the same change to the other similar thunk
implementations that set the normal buffer formats (the other thunk blocks that
set kR10G10B10A2_UNORM and the other normal-format thunk around the other
reported ranges).
---
Outside diff comments:
In `@package/Shaders/DeferredCompositePS.hlsl`:
- Around line 121-128: The shader fails to compile for the base permutation
because NormalRoughnessTexture is only declared under
SSGI||DYNAMIC_CUBEMAPS||DEBUG while normalVS and output.Normal always use it;
make the NormalRoughnessTexture declaration unconditional (remove the
preprocessor guards) so it is available for the base permutation that
Deferred::GetCompositePS() builds (t2 is already bound on the C++ side), and
apply the same unconditional declaration fix to the other occurrence around
lines 339-342.
🪄 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: f6a567db-15e4-432e-9b80-160c0daf7e41
📒 Files selected for processing (4)
package/Shaders/Common/GBuffer.hlslipackage/Shaders/DeferredCompositePS.hlslsrc/Deferred.cppsrc/Hooks.cpp
Comment on lines
591
to
+596
| static void thunk(RE::BSGraphics::Renderer* This, RE::RENDER_TARGETS::RENDER_TARGET a_target, RE::BSGraphics::RenderTargetProperties* a_properties) | ||
| { | ||
| globals::state->ModifyRenderTarget(a_target, a_properties); | ||
| func(This, a_target, a_properties); | ||
| RE::BSGraphics::RenderTargetProperties properties = *a_properties; | ||
| properties.format.set(RE::BSGraphics::Format::kR10G10B10A2_UNORM); | ||
| func(This, a_target, &properties); | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve the shared render-target mutation for the normals targets.
These two hooks now override the format and call func(...) directly, but unlike CreateRenderTarget_Main they never pass the copied properties through globals::state->ModifyRenderTarget(...). That drops the common render-target adjustments from src/State.cpp for both normal buffers, so they no longer inherit flags like UAV support. Please apply the global mutation to the copied struct before calling the original function.
Suggested fix
struct CreateRenderTarget_Normals
{
static void thunk(RE::BSGraphics::Renderer* This, RE::RENDER_TARGETS::RENDER_TARGET a_target, RE::BSGraphics::RenderTargetProperties* a_properties)
{
RE::BSGraphics::RenderTargetProperties properties = *a_properties;
properties.format.set(RE::BSGraphics::Format::kR10G10B10A2_UNORM);
+ globals::state->ModifyRenderTarget(a_target, &properties);
func(This, a_target, &properties);
}
static inline REL::Relocation<decltype(thunk)> func;
};
struct CreateRenderTarget_NormalsSwap
{
static void thunk(RE::BSGraphics::Renderer* This, RE::RENDER_TARGETS::RENDER_TARGET a_target, RE::BSGraphics::RenderTargetProperties* a_properties)
{
RE::BSGraphics::RenderTargetProperties properties = *a_properties;
properties.format.set(RE::BSGraphics::Format::kR10G10B10A2_UNORM);
+ globals::state->ModifyRenderTarget(a_target, &properties);
func(This, a_target, &properties);
}
static inline REL::Relocation<decltype(thunk)> func;
};Also applies to: 602-606, 967-968
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Hooks.cpp` around lines 591 - 596, The thunk copies a_properties into a
local properties, mutates properties.format, then calls func(...) but fails to
run the shared mutation in globals::state->ModifyRenderTarget, so normals
targets lose common flags (e.g., UAV). Fix: after setting properties.format and
before calling func(This, a_target, &properties), call
globals::state->ModifyRenderTarget(a_target, properties) (or the correct
overload that mutates the struct in-place) and then pass the resulting/modified
properties pointer to func; apply the same change to the other similar thunk
implementations that set the normal buffer formats (the other thunk blocks that
set kR10G10B10A2_UNORM and the other normal-format thunk around the other
reported ranges).
|
✅ A pre-release build is available for this PR: |
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/Hooks.cpp (1)
599-617:⚠️ Potential issue | 🟠 MajorPreserve the shared render-target mutation.
These two hooks still bypass
globals::state->ModifyRenderTarget(...), so the new normals targets will miss the common render-target adjustments applied elsewhere in the pipeline. Please run the shared mutation on the copiedpropertiesbefore callingfunc(...).Suggested fix
static void thunk(RE::BSGraphics::Renderer* This, RE::RENDER_TARGETS::RENDER_TARGET a_target, RE::BSGraphics::RenderTargetProperties* a_properties) { RE::BSGraphics::RenderTargetProperties properties = *a_properties; properties.format.set(RE::BSGraphics::Format::kR10G10B10A2_UNORM); + globals::state->ModifyRenderTarget(a_target, &properties); func(This, a_target, &properties); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Hooks.cpp` around lines 599 - 617, Both CreateRenderTarget_Normals::thunk and CreateRenderTarget_NormalsSwap::thunk copy a_properties and change properties.format but never call globals::state->ModifyRenderTarget, so the shared render-target mutations are skipped; after creating the local properties copy (the properties variable) and before calling func(This, a_target, &properties), call globals::state->ModifyRenderTarget(a_target, properties) (or the correct signature used elsewhere) to apply the common adjustments to properties, then pass the mutated properties to func; update both thunk functions accordingly so they run the shared mutation on the copied properties prior to invoking func.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/Hooks.cpp`:
- Around line 599-617: Both CreateRenderTarget_Normals::thunk and
CreateRenderTarget_NormalsSwap::thunk copy a_properties and change
properties.format but never call globals::state->ModifyRenderTarget, so the
shared render-target mutations are skipped; after creating the local properties
copy (the properties variable) and before calling func(This, a_target,
&properties), call globals::state->ModifyRenderTarget(a_target, properties) (or
the correct signature used elsewhere) to apply the common adjustments to
properties, then pass the mutated properties to func; update both thunk
functions accordingly so they run the shared mutation on the copied properties
prior to invoking func.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3febf2c-e911-4cf9-a4fc-e5e06904447e
📒 Files selected for processing (1)
src/Hooks.cpp
Collaborator
|
This does not appear to handle the regression reported for reflection jittering unfortunately. |
Collaborator
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
2 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.
Normal precision improvements now apply to the base game as well (partially).
Summary by CodeRabbit