Skip to content

[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
NVIDIA-RTX:mainfrom
congyue1977:main
Open

[ReSTIR PT] Fix no-RcVertex neighbor replay causing surface corruption and biased UCW in temporal/spatial resampling#42
congyue1977 wants to merge 1 commit into
NVIDIA-RTX:mainfrom
congyue1977:main

Conversation

@congyue1977
Copy link
Copy Markdown

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.

…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.
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.

1 participant