Volume correctness#298
Merged
Merged
Conversation
Quality's volume shadow anyhit ran sampleDistanceVolume, took the first accepted event, multiplied attenuation by the albedo (TF colour, not transmittance), and ignored the intersection. Two bugs: albedo ≠ transmittance (white volumes cast no shadow); optixIgnoreIntersection resumes at the next BVH AABB, not the next Woodcock step (one event per segment regardless of optical depth). Replace with ratio-tracking: walk GridTraversal, multiply attenuation *= (1 - σ_t / σ_maj) per candidate, RR-terminate near zero.
Volume in-scatter NEE was calling the surface sampleLights, returning ambient with cosine-hemisphere pdf cos(θ)/π around the TF-gradient normal. Two bugs: phase is isotropic (1/(4π)), not Lambertian; gradient = 0 in uniform regions biases the basis toward world +Z. Split out sampleLightsVolume with uniform-sphere ambient pdf 1/(4π). Surface NEE keeps the cosine-hemisphere variant.
TF parameter commits (UI slider drags on TF colour/opacity) forced a full volume BLAS + TLAS rebuild via Volume::markFinalized, even though TF doesn't affect AABB. Bump lastVolumeBLASChange only on visible-flag flip or m_field pointer change. Frame accumulation reset still fires off commitBuffer, so TF edits re-converge as before — just without paying for the BVH rebuild.
Pure delta/ratio tracking wasted Woodcock candidates on near-uniform volume interiors. Lift per-cell σ_min into a control variate (Kutz et al. 2017 / Novák et al. 2014): race a closed-form Exp(σ_c) free-flight against a residual Woodcock walk on (σ_maj - σ_c). Also drop MAX_WOODCOCK_STEPS_PER_CELL — the 128-event cap silently truncated free-flights in dense regions, biasing distributions toward longer paths. Loop is finite without the cap; RR (added later in the stack) is the right mitigation if pathological cells appear.
Interactive/Fast's rayMarchVolume did a TF lookup + classify every dt step even in empty regions (>90% of steps on sparse data). Wrap in the existing GridTraversal; per-cell max-opacity == 0 ⇒ skip the cell. dt-lattice phase preserved across hops so stratification matches the old marcher on non-empty cells.
NEE samples HDRI at every non-Dirac bounce; miss path was adding HDRI radiance on the BSDF-sampled direction on top, double-counting. Background bled into diffuse path bounces at the wrong intensity. Gate miss-path HDRI on isFirstBounce. Caveat: under-counts BSDF-sampled HDRI for Dirac mirrors / specular transmission until MIS lands.
Stored object-space bounds all along (GridTraversal consumes object-space ray). Misleading name fed at least one false-positive review finding about a non-existent world/object t mismatch. Pure rename.
Two helper pairs named surface/volumeAttenuation existed with incompatible semantics — one returns opacity (float, init 0), the other transmittance (vec3, init 1). Readers read the same name as if it meant the same thing. Rename: *ShadowOpacity vs *ShadowTransmittance. No behavioural change.
ANARI 1.1 §5.12.1 says final TF α = color.w · opacity. discritizeTFData already does that per bin; commitParameters was *also* pre-folding m_uniformColor.w into m_uniformOpacity → A² for uniform color + uniform opacity. Bug visible only when m_uniformColor.w ≠ 1, which is why it survived.
Collaborator
|
LGTM, thanks! |
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.
Improve
Qualityvolume rendering:Improve
Interactive/Fastvolume rendering:Transfer function:
In severe-broken cases:
Subsequent PRs to work on performances.