[ReSTIR PT] Fix no-RcVertex neighbor replay causing surface corruption and biased UCW in temporal/spatial resampling#42
Open
congyue1977 wants to merge 1 commit into
Open
Conversation
…n and biased UCW in temporal/spatial resampling
Problem
During temporal and spatial resampling, neighbor reservoirs whose path has no reconnection vertex (RcVertexLength > PathLength) are passed to ComputeHybridShift without being rejected. This causes two distinct bugs.
Bug 1 — Biased Unbiased Contribution Weight (UCW)
CombineReservoirs unconditionally accumulates M regardless of whether the candidate's TargetFunction is zero:
```
targetReservoir.M += SampleM; // always runs
targetReservoir.WeightSum += RisWeight; // += 0 when TargetFunction == 0
```
For a no-RcVertex neighbor, ComputeHybridShift returns TargetFunction = 0 (the hybrid-shift context forces termination with Radiance = 0 in case 2 of RecordPathIntersection), so WeightSum is unaffected while M is inflated. The inflated M feeds into the MIS normalization denominator (PiSum += TemporalP * PrevSample.M), producing a UCW that is systematically too small — a darkening bias.
Bug 2 — Surface corruption crashing the entire MIS normalization
RandomReplay ends with:
`surfaceForResampling = ctx.GetRcPrevSurface();`
RcPrevSurface is initialized to RAB_EmptySurface() inside RTXDI_InitializePathTracerContext and is only written when RcVertexLength <= SelectedPathLength (case 1). For a no-RcVertex path (case 2), the write never happens, so surfaceForResampling — which is the caller's inout RAB_Surface surface — is overwritten with RAB_EmptySurface() (world position (0,0,0)).
After ResampleTemporalNeighbor returns, the Surface variable in RTXDI_PTTemporalResampling is now zeroed. BiasCorrection then calls:
```
float Jacobian = RTXDI_CalculateJacobian(
RAB_GetSurfaceWorldPos(PrevSurface), // valid
RAB_GetSurfaceWorldPos(surface), // (0,0,0) — corrupted
targetReservoir.TranslatedWorldPosition,
targetReservoir.WorldNormal
);
```
The resulting Jacobian is completely wrong, making every value derived from it (Pi, PiSum, final UCW) meaningless. Furthermore, BiasCorrection then calls ComputeHybridShift on targetReservoir using the corrupted PrevSurface context, potentially propagating the corruption further.
This is not merely a bias — it is a full data corruption that invalidates all resampling results for any pixel that encounters a no-RcVertex temporal neighbor.
Root Cause
A no-RcVertex path stores RcVertexLength = maxRcVertexLength (set by SetMaxRcVertexLengthIfUnset), which satisfies RcVertexLength > 2, triggering NeedToRunRandomReplayPathTracer = true. The hybrid-shift path tracer context correctly identifies the shift as non-invertible and zeroes the radiance, but the caller does not guard against the inout surface being clobbered by the subsequent ctx.GetRcPrevSurface() call, nor does it prevent the spurious M accumulation.
Fix
Reject no-RcVertex neighbors (RcVertexLength > PathLength) before any resampling or replay takes place, in both temporal and spatial resampling paths.
In TemporalResampling.hlsli, inside ValidateTemporalNeighbor:
```
// Reject no-Rc history paths (RcVertexLength > PathLength) before any mutations.
// These paths corrupt Surface via RAB_EmptySurface() in RandomReplay and produce
// a mismatched TargetFunction, causing biased WeightSum and BiasCorrection errors.
if (PrevSample.RcVertexLength > PrevSample.PathLength)
{
return false;
}
```
In SpatialResampling.hlsli, inside ResampleNeighbors:
```
// Reject no-Rc neighbor paths (RcVertexLength > PathLength).
// RandomReplay produces an incorrect TargetFunction for these paths, which can
// cause single-frame fireflies. CachedResult will not be set, so BiasCorrection
// automatically skips the same neighbors via the CachedResult mask.
if (NeighborReservoir.RcVertexLength > NeighborReservoir.PathLength)
{
continue;
}
```
The CachedResult bit for a skipped spatial neighbor is never set, so BiasCorrection naturally skips it as well via (CachedResult & (1u << i)) == 0, keeping forward and backward MIS coverage sets identical.
Impact
No-RcVertex paths are not portable across pixels under hybrid shift (the shift is by definition non-invertible for such paths), so excluding them from cross-pixel reuse is theoretically correct. The fix eliminates the darkening bias and the surface corruption without affecting the estimator's unbiasedness for valid reconnectable paths.
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
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.
Problem
During temporal and spatial resampling, neighbor reservoirs whose path has no reconnection vertex (RcVertexLength > PathLength) are passed to ComputeHybridShift without being rejected. This causes two distinct bugs.
Bug 1 — Biased Unbiased Contribution Weight (UCW)
CombineReservoirs unconditionally accumulates M regardless of whether the candidate's TargetFunction is zero:
For a no-RcVertex neighbor, ComputeHybridShift returns TargetFunction = 0 (the hybrid-shift context forces termination with Radiance = 0 in case 2 of RecordPathIntersection), so WeightSum is unaffected while M is inflated. The inflated M feeds into the MIS normalization denominator (PiSum += TemporalP * PrevSample.M), producing a UCW that is systematically too small — a darkening bias.
Bug 2 — Surface corruption crashing the entire MIS normalization
RandomReplay ends with:
surfaceForResampling = ctx.GetRcPrevSurface();RcPrevSurface is initialized to RAB_EmptySurface() inside RTXDI_InitializePathTracerContext and is only written when RcVertexLength <= SelectedPathLength (case 1). For a no-RcVertex path (case 2), the write never happens, so surfaceForResampling — which is the caller's inout RAB_Surface surface — is overwritten with RAB_EmptySurface() (world position (0,0,0)).
After ResampleTemporalNeighbor returns, the Surface variable in RTXDI_PTTemporalResampling is now zeroed. BiasCorrection then calls:
The resulting Jacobian is completely wrong, making every value derived from it (Pi, PiSum, final UCW) meaningless. Furthermore, BiasCorrection then calls ComputeHybridShift on targetReservoir using the corrupted PrevSurface context, potentially propagating the corruption further.
This is not merely a bias — it is a full data corruption that invalidates all resampling results for any pixel that encounters a no-RcVertex temporal neighbor.
Root Cause
A no-RcVertex path stores RcVertexLength = maxRcVertexLength (set by SetMaxRcVertexLengthIfUnset), which satisfies RcVertexLength > 2, triggering NeedToRunRandomReplayPathTracer = true. The hybrid-shift path tracer context correctly identifies the shift as non-invertible and zeroes the radiance, but the caller does not guard against the inout surface being clobbered by the subsequent ctx.GetRcPrevSurface() call, nor does it prevent the spurious M accumulation.
Fix
Reject no-RcVertex neighbors (RcVertexLength > PathLength) before any resampling or replay takes place, in both temporal and spatial resampling paths.
In TemporalResampling.hlsli, inside ValidateTemporalNeighbor:
In SpatialResampling.hlsli, inside ResampleNeighbors:
The CachedResult bit for a skipped spatial neighbor is never set, so BiasCorrection naturally skips it as well via (CachedResult & (1u << i)) == 0, keeping forward and backward MIS coverage sets identical.
Impact
No-RcVertex paths are not portable across pixels under hybrid shift (the shift is by definition non-invertible for such paths), so excluding them from cross-pixel reuse is theoretically correct. The fix eliminates the darkening bias and the surface corruption without affecting the estimator's unbiasedness for valid reconnectable paths.