RT world path tracer: SER/payload/RIS perf pass + push-constant cleanup - #27
Merged
Conversation
traceRadianceReordered read all ten payload members back out of the payload variable after HitObject::TraceRay, held them across ReorderThread, and reconstructed them for HitObject::Invoke. Traversal only runs world.rahit, which never writes a radiance payload: its terrain tint paths live in the translucent/water buckets, which carry no any-hit record for radiance rays, and its entity paths are gated on RAY_FLAG_SKIP_CLOSEST_HIT_SHADER. The values carried across the reorder were therefore always dead. Pass the trace state (show-celestial, ray cone) as explicit parameters and rebuild the payload from it on each side of the reorder instead. Only the two packed trace-state words now span ReorderThread. Nsight attributed the peak live state in this shader to the reorder point, which is what prompted the change; the measured win is instead from instruction count, ~60 redundant load/store ops per radiance trace across three trace sites. Payload storage bytes are unchanged, since those are a per-call-site reservation rather than SSA liveness. SPIR-V: payload access chains before each reorder 30 -> 0, total RayPayloadKHR access chains 67 -> 34, zero non-debug instructions between TraceRay and ReorderThread. Payload struct and cross-stage ABI are untouched. Measured ~0.8ms/frame. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
albedo, normal, motionPrev and f0 become half3, taking Payload from 72 to 48 bytes. Payload storage is reserved per trace call site, so every byte is paid twice per radiance trace and preserved across the SER reorder. hitT stays f32: it reaches 10000 blocks and the hit position is reconstructed from it, where half's ~4-block spacing at that magnitude would be visible. No device-feature change. The baseline already declared OpCapability Float16/Int16 via packHalf2, and VulkanBackendMixin's SDK_SHADER_FEATURES already enables shaderFloat16 at device creation. world.rmiss clamps sky radiance to HALF_MAX so an out-of-range value cannot reach raygen as +inf and propagate as NaN. This is a guard, not a correction: SUN_DISC_RADIANCE is 24.0, three orders under the ceiling. Slang rejects implicit float3 -> half3 under -warnings-as-errors, so producers cast explicitly; reads widen implicitly and are unchanged. Measured 0.3ms/frame. Nsight peak live state at the reorder 790 -> 614 bytes, and the HitObject-attributed values 110/110/78 -> 62/62/62, so hit-object live state includes payload state it references. Visuals verified by hand. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RIS fetches these at random indices, M times per shading vertex, and at
48 bytes about half of them straddled a 64-byte cache line and cost two
transactions. 32 divides the line, so a record now never straddles one:
~1.5 transactions per candidate becomes 1.0.
Layout is {pos.xyz, packedLe} {halfU.xy, halfU.z|halfV.x, halfV.yz,
section}, half axes packed two per lane. The centre stays f32 because
the RIS target divides by squared distance to it; the axes are
block-scale offsets well inside half's range.
The area lane is dropped, not approximated. It is exactly 4*|halfU x
halfV|, since the collector builds halfU = 0.5*(aHi-aLo)*e01 and
rectArea = |e01 x e03|*(aHi-aLo)*(bHi-bLo). world.rgen derives it from
the cross product lightGeometricNormal already computes, so it costs one
length() the compiler shares with the normalize. p-hat is unchanged.
Measured 0.5ms/frame, against ~1ms predicted from the transaction count
alone. The shortfall is informative: per-candidate cost is not only
bytes but the three dependent round trips (span, alias, record), which
this does not reduce.
RtLightHierarchyTest hardcoded the old stride and section lane; it now
derives both from GPU_FLOATS_PER_LIGHT so a future layout change fails
at the constant rather than drifting silently.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Probes added here bound what memory-side RIS work can be worth. At M=8 against an 18.0ms frame: pinning every candidate to one light costs 5.9ms, of which ~4.3ms sits at secondary vertices and ~1.1ms at the primary hit. Forcing coherent selection recovers at most 1.3ms, so the cost is chasing depth rather than lane divergence. Secondary vertices are therefore where the money is, and they are also the forgiving place to spend variance: that radiance is integrated over a diffuse lobe before the denoiser sees it. The primary hit keeps worldPush.risCandidates; bounces past it run M/4. Measured 18.0 -> 16.1ms, and indistinguishable from divisor 1 by eye. This is why a global M reduction was the wrong test: M=2 everywhere was clearly worse, because it also degraded the primary hit. M=2 is the natural floor rather than a tuning accident. The proposal stratification keeps at least one global candidate, so M=2 is the smallest count that still schedules one local and one global; M=1 degenerates to global-only and loses the light-grid proposal that makes nearby emitters sample well. The candidate loop tested worldPush.risCandidates directly while the stratification used candidateCount. Identical before, but with a divisor applied it would have run 8 iterations against a schedule built for 2 and corrupted the mixture pdf. It now uses candidateCount throughout. Also adds RIS_MAX_BOUNCE, defaulted off, to gate RIS entirely past a depth: unlike the divisor that also drops shadeReservoir's per-vertex shadow ray, which does not scale with M. gateEmitter reads the same gate so a skipped bounce gathers emitters on direct hits instead of losing their energy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RIS_MAX_BOUNCE goes from off to 2: 16.1 -> 15.7ms, visually hard to distinguish. Unlike SECONDARY_RIS_DIVISOR this also drops shadeReservoir's per-vertex shadow ray, which never scaled with M and was a large share of the remainder. 2 rather than lower, and the reason belongs in the source: `bounce` is not indirect depth. MATERIAL_GLASS and MATERIAL_WATER continue without being diffuse vertices, so a pane spends bounce 0 and the first diffuse vertex behind it lands at bounce 1. Gating below 2 starves surfaces seen through glass or water -- visually primary, but counted as depth -- and they go black, since only a path randomly striking an emitter can light them. Counting diffuse vertices instead of bounces would let this go lower and be more correct at once; noted in the source as the next cleanup. Removes RIS_PROBE and its three sites now that the numbers are banked. The measurements it produced are kept as comments where they justify the two constants: 5.9ms total for RIS's memory side against an 18.0ms frame at M=8, ~4.3ms of it at secondary vertices vs ~1.1ms at the primary hit, and at most 1.3ms attributable to coherence rather than to chasing depth. That last number is why the note on proposalSeed now says a future presampled pool should share the pool, not the seed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
RIS_MAX_BOUNCE and SECONDARY_RIS_DIVISOR both keyed on `bounce`, which also counts dielectric interfaces: MATERIAL_GLASS and MATERIAL_WATER continue without shading anything, so a wall behind a pane is bounce 1 but is the first surface this path has actually shaded. Keyed on bounce, that wall got the secondary candidate count and could fall off the RIS gate entirely — visually primary content penalised as if it were deep indirect light, which is what the earlier black-behind-glass report was. Adds an explicit diffuseDepth counter, incremented once per path at every point a surface is actually shaded (the particle billboard branch, and the main opaque/PBR path after all its NEE/RIS/SSS terms), and NOT in the glass/water continue paths. Both risInitial call sites and the RIS_MAX_DIFFUSE_DEPTH gate now key on it. Renamed RIS_MAX_BOUNCE to RIS_MAX_DIFFUSE_DEPTH and risInitial's bounceIndex to shadedDepth to keep the distinction visible at every call site. Default stays 2, matching the old RIS_MAX_BOUNCE, so this is a correctness fix isolated from any further budget change: frame time should be flat, while surfaces seen through glass or water should get full RIS again instead of the degraded/absent treatment they got before. 1 and 0 are now meaningful budgets to sweep, where they previously included an unpaid-for dielectric prefix. MAX_SSS_BOUNCE has the identical defect and is not touched here — left for its own change since it is a separate visual behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same defect as the RIS gate fixed in 2e0c42c, in the leaf/grass backlight term. MATERIAL_GLASS and MATERIAL_WATER continue without shading a surface, so foliage seen through a window was being counted a bounce deeper than it visually is and could fall outside the SSS budget -- a backlit leaf loses its glow if there happens to be a pane between it and the camera. Renamed MAX_SSS_BOUNCE to MAX_SSS_DIFFUSE_DEPTH and keyed both the RIS activeSss gate and the direct backlight term on diffuseDepth. Value unchanged at 1, so this is the same kind of isolated correctness fix as 2e0c42c: foliage behind glass/water should backlight correctly again, frame time should not move. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
worldPush.flags bit 1 gated GGX BRDF + material guides vs. a Lambertian fallback, but RtComposite.java packed it as a hardcoded `0b10` -- nothing ever cleared it. Removed the flag, the `pbr` parameter from evalSampleContrib/risInitial/shadeReservoir, and every `pbr ? x : y` site in tracePath and refractedGuideHit; each now takes the branch that was always live. RtComposite's flags int drops to 0 as its base value, with bit 1 left unused rather than reassigned so a stale reader elsewhere can't silently pick up the wrong meaning. One site needed more than deletion. The particle billboard path called risInitial/shadeReservoir with pbr=false, rough=1.0, F0=0 -- unlike the main path, which always passed the (always-true) global flag. That false was load-bearing: fresnelSchlick(cosT, f0) returns up to full white at grazing angles regardless of f0 (that's the Fresnel effect itself), so zeroing F0 does not zero the specular term the way it looks like it should. Mechanically deleting the pbr gate would have put a white grazing-angle rim on every particle billboard (smoke, rain, ...) that was never there before. twoSided and the old pbr=false correlate exactly in this codebase -- particles are the only twoSided caller and the only one that disabled specular -- so evalSampleContrib now gates the GGX term on !twoSided instead of on the removed pbr, preserving the exact prior behavior without carrying the parameter. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WorldPushConstants exists specifically so hit shaders and raygen control flow can read a 64-bit address without first dereferencing worldPushAddr to find it (see the struct's own doc comment). WorldPush had drifted away from that: two of its addresses were exact duplicates never read through worldPush at all, and five more were unique addresses that could have lived in either place. tableAddr and entityTableAddr are deleted outright, not moved. Every actual read in world.rahit/world.rchit already went through pc.tableAddr/pc.entityTableAddr -- grep confirms no `worldPush.tableAddr` or `worldPush.entityTableAddr` existed anywhere. The WorldPush copies were populated from the same Java values and never read back. lightBufAddr/lightAliasAddr/lightLocalAliasAddr/lightGridCellAddr/ lightGridSpanAddr move to WorldPushConstants. These are read only in world.rgen, which already loads WorldPush once at the top of main(), so the move buys no per-access saving there -- the point is architectural: one struct now holds every device address, so adding the next buffer means one decision instead of two, and nothing can quietly duplicate itself across both again. WorldPushConstantsData/WorldPushData are Slang-reflection-generated records (GenerateShaderRecords), so the Java-side fix was reordering the two constructor call sites in RtComposite to the new field layout -- nothing manual to keep in sync. RtMaterialLayoutTest hardcoded the old 40-byte size and byte offsets for a positional WorldPushConstantsData; updated to the new 80-byte, 11-arg shape it now reflects. WorldPushConstants grows from 40 to 80 bytes, still far under the 128-byte minimum Vulkan guarantees for push constants. Full build + test suite green (34/34). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Summary
Profiling-driven performance work on the world path tracer (
world.rgen.slang), plus two correctness fixes surfaced along the way and one cleanup requested afterward.Measured, cumulative: ~19.2ms → ~15.7ms/frame (~18%) on the profiled scene.
traceRadianceReorderedwas reading all 10 payload members back out afterTraceRay, holding them live acrossReorderThread, then rebuilding them forInvoke. Traversal never writes a radiance payload, so those values were always dead. Rebuilding from trace state instead of carrying the payload across the reorder: ~0.8ms.half3—albedo/normal/motionPrev/f0fromfloat3tohalf3(72 → 48 bytes). Payload storage is reserved per trace call site and paid twice per radiance trace, so every byte here is doubled up.hitTstaysfloat(reaches 10000 blocks). No new device feature —Float16/Int16were already required bypackHalf2: ~0.3ms.4*|halfU x halfV|) rather than stored — exact, not approximate, since the collector already builds the half-axes that way: ~0.5ms.M/4past the primary hit, M unchanged at the primary hit: ~1.9ms, visually indistinguishable from a full M/1 test at the same setting.shadeReservoir's per-vertex shadow ray, which doesn't scale with M and was a large share of what remained: ~0.4ms.Correctness fixes (found via the above)
bounce, not on shaded surfaces.MATERIAL_GLASS/MATERIAL_WATERcontinue without shading anything, so a wall seen through a pane was bounce 1 but visually the first shaded surface — it could fall off the RIS budget entirely and read as black. Introduced an explicitdiffuseDepthcounter (incremented only where a surface is actually shaded) and rekeyed the RIS gate and the secondary-vertex divisor on it instead ofbounce.MAX_SSS_BOUNCE→MAX_SSS_DIFFUSE_DEPTH) — leaves/grass seen through glass or water were losing their sun-backlit glow for the same reason. Rekeyed identically.Cleanup
worldPush.flagsbit 1 gated GGX BRDF vs. a Lambertian fallback, but the Java side packed it as a hardcoded0b10— nothing ever cleared it. Removed the flag and everypbr ? x : ysite; each now takes the branch that was always live. One site needed care rather than deletion: the particle billboard path passedpbr=falseexplicitly (independent of the global flag) to suppress a Fresnel rim highlight thatF0=0alone does not zero out at grazing angles. Preserved that behavior by gating on!twoSidedinstead, since particles are the onlytwoSidedcaller.WorldPushto the push-constant block (WorldPushConstants). That struct exists specifically so hit shaders/raygen can read a 64-bit address without dereferencingworldPushAddrfirst.tableAddr/entityTableAddrinWorldPushwere exact duplicates never actually read throughworldPush.*anywhere — deleted outright. The five RIS light-buffer addresses were unique and move there for consistency: one struct now holds every device address instead of two.WorldPushConstantsData/WorldPushDataare Slang-reflection-generated, so the Java-side change was reordering two constructor call sites to match the new field layout; grows from 40 to 80 bytes, still well under the 128-byte minimum Vulkan guarantee.Test plan
./gradlew build— full build green, including shader compile +spirv-valfor both SER (EXT/NV) variants./gradlew test— 34/34 passSECONDARY_RIS_DIVISOR=4/RIS_MAX_DIFFUSE_DEPTH=2(kept as committed), particle billboards (no unwanted specular rim), foliage seen through glass/water (SSS backlight restored)🤖 Generated with Claude Code