perf: RT hot-path optimizations — TLAS refits, opaque geometry, NRD churn, shadow-ray early-outs - #38
Open
ShugokiFable wants to merge 13 commits into
Open
perf: RT hot-path optimizations — TLAS refits, opaque geometry, NRD churn, shadow-ray early-outs#38ShugokiFable wants to merge 13 commits into
ShugokiFable wants to merge 13 commits into
Conversation
The validation wrapper adds per-call CPU overhead to every NVRHI command (command list recording, state tracking, binding validation) in all builds. Keep the member and runtime branch in Renderer::Initialize so it can be re-enabled for debugging.
The scene TLAS was fully rebuilt every frame with PreferFastTrace. Create the TLAS with AllowUpdate and, per ring slot, hash the instance descriptor set (instance count + BLAS pointers). When the set matches the previous build on that slot, issue the build with PerformUpdate (refit): instance transforms may change freely during an update, while count/BLAS membership may not (enforced by the hash and asserted by NVRHI). A capacity resize or any membership change falls back to a full rebuild.
BaseMesh::Update marks DirtyFlags::Transform whenever the mesh world transform changes, and BLASCluster treated that as a BLAS refit (PerformUpdate) trigger. But geometry descs store cluster-relative local transforms (rewritten every frame by UpdateLocalTransform), and the world motion is carried by the TLAS instance transform, which is written unconditionally every frame for every valid cluster (BLASCluster::Update writes InstanceData; TopLevelAS::Update rebuilds/refits the TLAS with fresh InstanceDescs). The BLAS content is identical after a rigid move, so the refit was pure waste - one refit per moving cluster per frame. The Transform flag is kept for prev-transform/motion-vector bookkeeping and SubIndexSegmentMesh sync; it just no longer triggers BLAS work.
Geometry descs were built with no flags, so every triangle generated a non-opaque candidate and ran the any-hit transparency evaluation (ConsiderTransparentMaterial/Shadow) even for plain opaque lighting materials. BaseMesh::Update now recomputes an opacity predicate each frame from the freshly-synced Properties and material data: lighting-family material (Lighting/landscape/TruePBR), no alpha test/blend/additive/transmission, and no refraction/kAssumeShadowmask shader flags. That exactly matches the conditions under which both transparency functions unconditionally commit the hit, so skipping the callback is behavior-preserving. When the computed state changes (e.g. runtime alpha property toggle), the cluster is marked for a BLAS rebuild since the flag is baked into the AS at build time. SubIndexSegmentMesh syncs properties via SyncFrom instead of Update, so it reapplies the flags there.
NRDIntegration created a resource binding set for every Reblur dispatch every frame (~30 sets/frame), churning the descriptor heap. Cache one binding set per dispatch index and only recreate it when the bound texture pointers actually change (resolution change, resource recreation). NVRHI binding sets hold references to bound resources, so cached entries keep their textures alive and a pointer mismatch always means the texture identity changed. Also drop the per-frame full-resolution copy of the motion vector texture into a scratch texture: the scratch was created from the source desc with an identical format (RG16_FLOAT game MVs for GI, RGBA16_FLOAT MotionVectors3D for PT), and the copy performed no conversion, so the source can be bound directly as IN_MV. Behavior change: if the source MV texture is missing, the dispatch now aborts with the existing missing-resource warning instead of denoising with stale scratch contents.
…0/50->20/32) The stabilized-history pass at 63 frames and the oversized diffuse/specular prepass blur radii cost significant denoiser time at 4K for marginal quality gain over the NRD-recommended ranges. Remains user-configurable.
The far-field BCSDF hair model is considerably more expensive per hair hit than Chiang. Switch the default; remains user-configurable.
The loop skipped every non-null child (inverted guard), so passes were never notified of resolution changes from the root node - e.g. NRD kept stale resource sizes after a resize. Matches the guards in SettingsChanged/Execute.
- XSEPlugin: flush_on(info) -> flush_on(warn); every per-frame info log was forcing a file flush. - Scene::SetLogLevel: clamp flush_on to >= warn in release builds (debug builds keep flushing at the configured level). - BLASCluster::BuildUpdate: demote the two per-frame 'already built this frame' / 'no dirty flags set' messages from info to trace. Other logger::info call sites were checked and are init/resize/error paths only.
Shadow rays with translucent candidates (water, glass, emissive windows) accumulate Beer-Lambert transmission and keep traversing. Transmission only decreases along the ray, so once all channels are <= 1e-3 the result is zero shadow regardless of what remains: - Ray query path (TraceRayShadowFinite): break out of the Proceed loop. Uncommitted + transmission~0 returns transmission*missed(1) ~ 0, matching the result of running the loop to completion. - DXR pipeline path (ShadowAnyHit): AcceptHitAndEndSearch() once the payload is ~zero, which ends the search with a committed hit (missed stays 0).
ConsiderTransparentMaterialShadow fetched the diffuse texture alpha at mip 0 for the cutout/blend coverage test. Shadow rays don't need exact coverage - use a fixed mip 3 (SHADOW_ALPHA_MIP) instead: better texture cache behavior and no fine-mip residency dependency. The primary/bounce path (ConsiderTransparentMaterial) keeps the exact mip-0 fetch, and the colored transmission/specular tint fetches in the shadow path are unchanged. Caveat: alpha-tested foliage/hair shadows get slightly softer/fuller edges since mip-3 alpha is locally averaged.
…data race LandLODMesh::UpdateOcclusion ran for every intersecting mesh every frame: it wrote an occluder update entry and marked DirtyFlags::Vertex, forcing a BLAS refit per LandLOD mesh per frame. The occluder is stateless given (original vertices, transform, loadedRange), and loadedRange only moves in discrete cell-sized steps, so cache the last submitted transform+loadedRange and skip the resubmit (and refit) while both are within epsilon. The live vertex buffer and BLAS remain valid in between. Also fix a data race: UpdateOcclusion wrote SceneGraph::m_LandLODMeshUpdates (eastl::unordered_map) directly from parallel worker threads in SceneGraph::Update phase B. Route writes through SceneGraph::SetLandLODMeshUpdate under a dedicated mutex. The reader (LandLODOccluder pass) runs later on the render thread after the workers have joined, so it needs no lock. Also demotes the per-frame 'No geometry' log to trace.
📝 WalkthroughWalkthroughThis change updates ray-traced shadow handling, mesh and acceleration-structure updates, Land LOD occlusion submission, NRD resource binding, renderer defaults, logging, scene traversal, and resolution propagation. ChangesRay-tracing and acceleration structures
Render resource and occlusion flow
Runtime defaults and diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Renderer
participant NRDIntegration
participant BindingCache
participant NRDDispatch
Renderer->>NRDIntegration: provide current motion-vector texture
NRDIntegration->>BindingCache: compare SRV/UAV texture vectors
BindingCache-->>NRDIntegration: reuse or create binding set
NRDIntegration->>NRDDispatch: issue compute dispatch with cached bindings
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Pass/NRD/NRDIntegration.cpp (1)
625-626: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid vector copies by moving the local vectors.
Since
srvTexturesanduavTexturesare local vectors that are no longer needed after building the binding set, you caneastl::movethem into the cache. This skips a heap allocation and pointer copy for the vector contents.♻️ Proposed refactor to move vectors
- bindingCache.srvTextures = srvTextures; - bindingCache.uavTextures = uavTextures; + bindingCache.srvTextures = eastl::move(srvTextures); + bindingCache.uavTextures = eastl::move(uavTextures);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Pass/NRD/NRDIntegration.cpp` around lines 625 - 626, Update the assignments in the binding-cache construction to move the local srvTextures and uavTextures vectors into bindingCache.srvTextures and bindingCache.uavTextures using eastl::move, preserving the existing values while avoiding copies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Pass/NRD/NRDIntegration.cpp`:
- Around line 540-542: Update the cache sizing logic around
m_DispatchBindingCache to call resize(dispatchCount) unconditionally, rather
than only when dispatchCount grows. This must remove trailing binding sets when
the dispatch count shrinks so their texture references are released, while
preserving existing entries when the count is unchanged or increases.
In `@src/Types/TopLevelAS.h`:
- Around line 122-125: Update the buildFlags initialization in the TLAS
construction path to include AllowUpdate for full rebuilds when refit is false,
while preserving PerformUpdate for refits and PreferFastTrace in both cases. Use
the existing nvrhi::rt::AccelStructBuildFlags symbols.
---
Nitpick comments:
In `@src/Pass/NRD/NRDIntegration.cpp`:
- Around line 625-626: Update the assignments in the binding-cache construction
to move the local srvTextures and uavTextures vectors into
bindingCache.srvTextures and bindingCache.uavTextures using eastl::move,
preserving the existing values while avoiding copies.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3caf419b-529b-4b78-857e-ba3389c1ce90
⛔ Files ignored due to path filters (1)
shaders/raytracing/Common/ShadowAnyHit.hlslis excluded by!**/*.hlsl
📒 Files selected for processing (17)
shaders/raytracing/include/Rays.hlslishaders/raytracing/include/Transparency.hlslisrc/Core/BLASCluster.cppsrc/Core/BaseMesh.cppsrc/Core/BaseMesh.hsrc/Core/LandLODMesh.cppsrc/Core/LandLODMesh.hsrc/Core/SubIndexSegmentMesh.cppsrc/Pass/NRD/NRDIntegration.cppsrc/Pass/NRD/NRDIntegration.hsrc/Renderer.hsrc/Renderer/RootRenderNode.cppsrc/Scene.cppsrc/SceneGraph.hsrc/Types/Settings.hsrc/Types/TopLevelAS.hsrc/XSEPlugin.cpp
Comment on lines
+540
to
+542
| if (m_DispatchBindingCache.size() < dispatchCount) | ||
| m_DispatchBindingCache.resize(dispatchCount); | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent GPU memory leaks by unconditionally resizing the cache.
If dispatchCount shrinks (e.g., due to a settings change like toggling an NRD feature), the trailing elements in m_DispatchBindingCache remain untouched because resize is only called when growing. These stale cached binding sets will continue to hold strong references to their bound textures. If a resolution change later recreates the texture pools, the orphaned textures from the old resolution will be kept alive indefinitely by these untouched trailing binding sets, causing a GPU memory leak.
Unconditionally resizing the cache ensures that any unused trailing elements are destroyed, safely releasing their texture references.
🐛 Proposed fix to unconditionally resize
- if (m_DispatchBindingCache.size() < dispatchCount)
- m_DispatchBindingCache.resize(dispatchCount);
+ m_DispatchBindingCache.resize(dispatchCount);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (m_DispatchBindingCache.size() < dispatchCount) | |
| m_DispatchBindingCache.resize(dispatchCount); | |
| m_DispatchBindingCache.resize(dispatchCount); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Pass/NRD/NRDIntegration.cpp` around lines 540 - 542, Update the cache
sizing logic around m_DispatchBindingCache to call resize(dispatchCount)
unconditionally, rather than only when dispatchCount grows. This must remove
trailing binding sets when the dispatch count shrinks so their texture
references are released, while preserving existing entries when the count is
unchanged or increases.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Core/BLASCluster.cpp (1)
64-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate fallback logic to eliminate duplication.
The new fallback logic for when a reference loses its 3D (
!object) is functionally identical to the existing fallback logic for an orphan cluster (!m_Owner). You can merge both cases into a singleelsebranch, eliminating over 20 lines of duplicate code and simplifying the control flow.♻️ Proposed refactor
- if (m_Owner) { - const RE::NiAVObject* object = m_Owner->Get3D(); - if (!object) { - // A reference can lose its 3D during unload before every mesh-destroy - // notification is consumed. Fall back to a member transform rather - // than dereferencing a null scene object on a render worker. - if (m_Members.empty()) { - m_Transform = Constants::kIdentityTransform; - m_PrevTransform = Constants::kIdentityTransform; - m_NeedsPrevInit = false; - } - else { - const auto* mesh = m_Members.front(); - m_Transform = mesh->GetTransform(); - m_PrevTransform = m_NeedsPrevInit ? m_Transform : mesh->GetPrevTransform(); - m_NeedsPrevInit = false; - } - - m_ClusterPosition = float3(m_Transform._14, m_Transform._24, m_Transform._34); - return; - } - - float3x4 transform; - XMStoreFloat3x4(&transform, Util::Math::GetXMFromNiTransform(object->world)); - - if (m_NeedsPrevInit) { - m_PrevTransform = transform; - m_NeedsPrevInit = false; - } else { - m_PrevTransform = m_Transform; - } - - m_Transform = transform; - } - else { - if (m_Members.empty()) { - m_Transform = Constants::kIdentityTransform; - m_PrevTransform = Constants::kIdentityTransform; - m_NeedsPrevInit = false; - } - else { - const auto& mesh = m_Members.front(); - m_Transform = mesh->GetTransform(); - - if (m_NeedsPrevInit) { - m_PrevTransform = m_Transform; - m_NeedsPrevInit = false; - } else { - m_PrevTransform = mesh->GetPrevTransform(); - } - } - } + const RE::NiAVObject* object = m_Owner ? m_Owner->Get3D() : nullptr; + + if (object) { + float3x4 transform; + XMStoreFloat3x4(&transform, Util::Math::GetXMFromNiTransform(object->world)); + + if (m_NeedsPrevInit) { + m_PrevTransform = transform; + m_NeedsPrevInit = false; + } else { + m_PrevTransform = m_Transform; + } + + m_Transform = transform; + } + else { + // A reference can lose its 3D during unload before every mesh-destroy + // notification is consumed. Fall back to a member transform rather + // than dereferencing a null scene object on a render worker. + // This also correctly handles orphan (no-owner) clusters. + if (m_Members.empty()) { + m_Transform = Constants::kIdentityTransform; + m_PrevTransform = Constants::kIdentityTransform; + m_NeedsPrevInit = false; + } + else { + const auto* mesh = m_Members.front(); + m_Transform = mesh->GetTransform(); + m_PrevTransform = m_NeedsPrevInit ? m_Transform : mesh->GetPrevTransform(); + m_NeedsPrevInit = false; + } + } m_ClusterPosition = float3(m_Transform._14, m_Transform._24, m_Transform._34);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Core/BLASCluster.cpp` around lines 64 - 117, Consolidate the duplicated fallback handling in the transform-update logic: when m_Owner is absent or m_Owner->Get3D() returns null, route both cases through the same member/identity transform path. Preserve the existing owned-object transform and previous-transform initialization behavior, then keep the shared m_ClusterPosition update after the unified branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Core/BaseMesh.cpp`:
- Around line 60-64: Update the transform dirty-check logic around BaseMesh’s
m_LocalTransform assignment so MatrixNearEqual compares against a separate
last-submitted or last-dirtied transform rather than the continuously updated
logical transform. Preserve m_LocalTransform as the current transform for other
consumers, and update the comparison baseline only when the transform is marked
dirty or submitted, allowing sub-threshold motion to accumulate until
DirtyFlags::Transform is raised.
---
Nitpick comments:
In `@src/Core/BLASCluster.cpp`:
- Around line 64-117: Consolidate the duplicated fallback handling in the
transform-update logic: when m_Owner is absent or m_Owner->Get3D() returns null,
route both cases through the same member/identity transform path. Preserve the
existing owned-object transform and previous-transform initialization behavior,
then keep the shared m_ClusterPosition update after the unified branches.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fe0ffafd-34a4-4002-bd96-bfd33ea3b776
📒 Files selected for processing (7)
src/Core/BLASCluster.cppsrc/Core/BaseMesh.cppsrc/Core/BaseMesh.hsrc/Pass/NRD/NRDIntegration.cppsrc/Types/Settings.hsrc/Types/TopLevelAS.hsrc/Utils/Traversal.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Types/TopLevelAS.h
- src/Core/BaseMesh.h
- src/Pass/NRD/NRDIntegration.cpp
Comment on lines
+60
to
+64
| if (!Util::Math::MatrixNearEqual(localTransform, m_LocalTransform)) | ||
| MarkDirty(DirtyFlags::Transform); | ||
|
|
||
| m_LocalTransform = localTransform; | ||
| m_PrevLocalTransform = prevLocalTransform; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not advance the dirty-check baseline when suppressing a transform update.
MatrixNearEqual can treat each frame’s small motion as unchanged. Since m_LocalTransform is nevertheless overwritten at Line 63, continuous sub-threshold motion never accumulates and DirtyFlags::Transform may never be raised, leaving the BLAS transform stale. Compare against a separate last-submitted/last-dirtied transform, or accumulate deltas until the tolerance is exceeded.
Suggested direction
- if (!Util::Math::MatrixNearEqual(localTransform, m_LocalTransform))
+ if (!Util::Math::MatrixNearEqual(localTransform, m_LastSubmittedLocalTransform))
+ {
MarkDirty(DirtyFlags::Transform);
+ m_LastSubmittedLocalTransform = localTransform;
+ }Keep m_LocalTransform as the logical current transform if other consumers require it.
🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 60-60: Found an exit path from function with non-void return type that has missing return statement
(missingReturn)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Core/BaseMesh.cpp` around lines 60 - 64, Update the transform dirty-check
logic around BaseMesh’s m_LocalTransform assignment so MatrixNearEqual compares
against a separate last-submitted or last-dirtied transform rather than the
continuously updated logical transform. Preserve m_LocalTransform as the current
transform for other consumers, and update the comparison baseline only when the
transform is marked dirty or submitted, allowing sub-threshold motion to
accumulate until DirtyFlags::Transform is raised.
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
1 participant
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.
Summary
A pass over the per-frame hot paths in GI/PT modes, motivated by poor FPS on high-end hardware (RTX 4080 Super, 9800X3D, 4K). Each commit is self-contained and fixes one specific, measured-by-inspection inefficiency. No architectural changes; no behavior changes outside the noted caveats.
Changes
RendererSettings::ValidationLayerdefaulted totrue, wrapping every NVRHI call (binding-set validation per dispatch, duplicated state tracking per barrier) in release builds.PreferFastTrace. Now created withAllowUpdate; per-ring-slot FNV hash of instance count + BLAS pointers selectsPerformUpdaterefits when the instance set is unchanged (transforms may still change freely).DirtyFlags::Transformremoved from the BLAS-update trigger (kept for prev-transform bookkeeping).GeometryFlags::Opaque— no geometry ever set the flag, so every ray ran the any-hit/candidate path (3 vertex loads + material load + texture sample) even against fully opaque walls/terrain. Predicate mirrors exactly whatConsiderTransparentMaterial[Shadow]treats as always-commit; alpha-state flips rebuild the BLAS viaMarkDirty(Mesh).createBindingSetwas called ~10×/frame inside the dispatch loop; the MV scratch copy duplicated a texture of identical format. Cache invalidates on texture-pointer identity (safe: binding sets refcount resources).maxStabilizedFrameNum63→32, prepass blur radii 30/50→20/32.if (child)inRootRenderNode::ResolutionChanged— the check skipped every existing child, so resolution changes never propagated to any pass (NRD resource dirty flags included).flush_on(info)plus per-framelogger::infoin BLAS build paths did file I/O on the render thread.Proceed()-ing through all remaining candidates after Beer-Lambert/alpha drove transmission to ~0; both the ray-query loops andShadowAnyHitnow terminate (early-out ordered beforeIgnoreHit).GetLandLODMeshUpdates()was written from worker threads without a lock.Deliberately not done
SharcResolveEntryis not stateless per entry (cross-entry probe window, adjacent-level blends, per-entry frame-cadence counters,frameIndex % 32fade bits). Slicing would change cache behavior, not just cost.Testing
/W4 /WX(presetSkyrimAll, VS 2026, Release).Target: GI and PT modes at 4K on Ada-class GPUs; no intended visual changes except the shadow-alpha mip note above.
Summary by CodeRabbit
Bug Fixes
Performance
Configuration