fix: fix bad camera data#1459
Conversation
WalkthroughReplaces camera/projection data access in LightLimitFix with frameBufferCached getters and transposed matrices. In Upscaling, forces TAA off after upscaling and removes prior re-toggle in post-processing. No signatures changed; logic flow largely maintained except for TAA state handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Upscaling::PerformUpscaling
participant R as RuntimeData
participant I as ImageSpaceManager (singleton)
participant T as BSImagespaceShaderISTemporalAA
U->>R: dynamicResolutionLock = 1
Note over U,R: After upscaling step completes
U->>I: Get singleton
I-->>U: manager*
U->>T: GET_INSTANCE_MEMBER(manager)
U->>T: taaEnabled = false
Note over T: TAA forcibly disabled post-upscale
rect rgba(235,235,255,0.4)
Note over U,T: Previous flow removed
U-x T: (Post-processing thunk no longer toggles TAA by upscaleMethod)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Poem
✨ Finishing Touches
🧪 Generate 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Features/Upscaling.cpp (1)
1560-1564: Redundant TAA toggle; clarify intent (“jitter” vs full TAA) and de-duplicateYou disable TAA here, and Main_PostProcessing later sets
taaEnabled = (upscaleMethod == kTAA)on Line 1705. If the goal is “no TAA when using FSR/XeSS/DLSS,” the later assignment already enforces that per-frame, making this toggle redundant. If the goal is to prevent any post-upscale jitter/temporal blend before camera constants refresh, please document it and guard it explicitly to avoid flip-flopping.Suggestion: centralize the TAA state change in one place (Main_PostProcessing), or at least guard this block to make the intent obvious.
- // Disable jitter after upscaling - auto imageSpaceManager = RE::ImageSpaceManager::GetSingleton(); - GET_INSTANCE_MEMBER(BSImagespaceShaderISTemporalAA, imageSpaceManager); - BSImagespaceShaderISTemporalAA->taaEnabled = false; + // Ensure TAA is disabled for non-TAA upscalers before camera constants update + // (kept only if disabling here is required to avoid a transient blend) + if (GetUpscaleMethod() != UpscaleMethod::kTAA) { + auto imageSpaceManager = RE::ImageSpaceManager::GetSingleton(); + GET_INSTANCE_MEMBER(BSImagespaceShaderISTemporalAA, imageSpaceManager); + BSImagespaceShaderISTemporalAA->taaEnabled = false; + }Note: The AI summary claims the post-processing reassertion was removed, but Line 1705 still reassigns
taaEnabled; please reconcile.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Features/LightLimitFix.cpp(2 hunks)src/Features/Upscaling.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not include TODO/FIXME placeholders; provide complete, working solutions
Files:
src/Features/Upscaling.cppsrc/Features/LightLimitFix.cpp
src/**/*.{cpp,cxx,cc,h,hpp,hxx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code
Files:
src/Features/Upscaling.cppsrc/Features/LightLimitFix.cpp
🧠 Learnings (1)
📓 Common learnings
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#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.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (2)
src/Features/LightLimitFix.cpp (2)
812-816: Ensure consistent matrix convention: Verify that bothframeBufferCached.GetCameraViewandUtil::GetCameraData(eyeIndex).viewMatuse the same row/column-major orientation—either drop the unnecessaryTranspose()on the cached path or apply a corresponding transpose in the non-cached path so thatSimpleMath::Vector3::Transformalways receives uniformly oriented matrices.
1024-1036: Verify file presence and FOV derivation
I couldn’t locateLightLimitFix.cppin the repo—please confirm its path and that the vertical FOV is derived frompm.m[1][1](notm[0][0]), and verify the transpose aligns with your HLSL row- vs column-major expectations.
|
✅ A pre-release build is available for this PR: |
2 participants
Summary by CodeRabbit
Bug Fixes
Refactor