fix(CSGOptiX): handle coincident sibling faces - #346
Open
ggalgoczi wants to merge 9 commits into
Open
Conversation
ggalgoczi
marked this pull request as draft
May 29, 2026 14:09
ggalgoczi
marked this pull request as ready for review
May 30, 2026 14:57
ggalgoczi
force-pushed
the
qbnd-sibling-pair-matline
branch
from
May 30, 2026 15:00
15e0da7 to
6f9d9bd
Compare
ggalgoczi
force-pushed
the
qbnd-sibling-pair-matline
branch
from
May 30, 2026 15:57
a212e4c to
325e81f
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses two OptiX failure modes at CSG sibling primitives with coincident faces by (1) carrying the photon’s “authoritative current-medium” material-line across boundary steps, and (2) biasing the reported intersection distance on the exiting side so OptiX consistently selects the intended “entering sibling” hit in coplanar tie cases.
Changes:
- Add
sctx::current_matlineand propagate it across boundary interactions, using it to overridem1inqbnd::fill_statewhen valid. - Introduce per-boundary gating (
params.boundary_face_bias) and apply a +10 µmtbias on exit-side coplanar intersections in__intersection__is. - Compute the per-boundary face-bias gate from the
bndtable inCSGOptiX::initSimulateand plumb it throughParams.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sysrap/sctx.h | Adds per-photon carried current_matline state to sctx. |
| qudarap/qsim.h | Updates/propagates current_matline at boundaries and passes it into qbnd::fill_state. |
| qudarap/qbnd.h | Extends fill_state to optionally override m1_line using carried matline. |
| CSGOptiX/Params.h | Adds boundary_face_bias pointer to launch params. |
| CSGOptiX/Params.cc | Initializes boundary_face_bias to nullptr. |
| CSGOptiX/CSGOptiX7.cu | Seeds carried matline at birth and biases optixReportIntersection t for exit-side coplanar sibling faces. |
| CSGOptiX/CSGOptiX.cc | Computes per-boundary face-bias gate from bnd refractive-index contrast and uploads it to device memory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+442
to
+449
| // F7: seed carried matline from the genstep's source-medium bnd row | ||
| // (gs.q0.u.z == scerenkov/sscint matline). Updated through | ||
| // propagate_at_boundary. Guard the -1 sentinel (0xFFFFFFFF) that | ||
| // SEvt::setGenstep emits for bad_ck/unmapped mtindex. | ||
| { | ||
| const unsigned gm = gs.q0.u.z; | ||
| ctx.current_matline = (gm == 0xFFFFFFFFu) ? 0u : gm; | ||
| } |
Comment on lines
+213
to
+220
| // Guards: 0 = "no carry" (legacy callers); 0xFFFFFFFF = the lookup_mtline=-1 | ||
| // sentinel from SEvt::setGenstep, not a valid table row; and the carried | ||
| // slot must be a material row (OMAT/IMAT), not a surface row (OSUR/ISUR). | ||
| const unsigned matline_slot = carried_matline & 0x3u; | ||
| if (carried_matline != 0u && carried_matline != 0xFFFFFFFFu && (matline_slot == unsigned(OMAT) || matline_slot == unsigned(IMAT))) | ||
| { | ||
| m1_line = int(carried_matline); | ||
| } |
Comment on lines
+645
to
+656
| static unsigned char* s_face_bias = nullptr; | ||
| static unsigned s_face_bias_num = 0u; | ||
| if (s_face_bias == nullptr) | ||
| { | ||
| CUDA_CHECK(cudaMalloc((void**)&s_face_bias, num_bnd)); | ||
| s_face_bias_num = num_bnd; | ||
| } | ||
| if (s_face_bias_num == num_bnd) | ||
| { | ||
| CUDA_CHECK(cudaMemcpy(s_face_bias, bits.data(), num_bnd, cudaMemcpyHostToDevice)); | ||
| params->boundary_face_bias = s_face_bias; | ||
| } |
Comment on lines
+641
to
+644
| } | ||
| bits[b] = (dnmax > DN_THRESHOLD) ? 1u : 0u; | ||
| LOG(LEVEL) << "boundary_face_bias bnd " << b << " dnmax " << dnmax << " gate " << unsigned(bits[b]); | ||
| } |
plexoos
force-pushed
the
qbnd-sibling-pair-matline
branch
from
July 28, 2026 22:16
c243bd4 to
d82daf4
Compare
plexoos
changed the title
plexoos
changed the title
plexoos
pushed a commit
that referenced
this pull request
Jul 30, 2026
Seed the carried material line only for Cerenkov and scintillation gensteps, whose q0.z field has that contract. Treat UINT_MAX as the unavailable sentinel so material line zero remains valid, and validate carried rows against the uploaded boundary table before property or optical-buffer lookup. Replace the fixed 10 um, refractive-index-gated exit bias with a simulation-only one-ULP tie-break. This lets an entering sibling win exact coplanar ties without crossing a representable physical gap, while preserving the unbiased intersection for position advancement. Removing the old gate also eliminates its static GPU allocation and per-boundary debug logging. Add regression coverage for genstep material-line ownership; valid line-zero and inner-material carries; sentinel, surface-row, and out-of-range fallbacks; and the updated Params layout. Validated with a Release CUDA/OptiX build of CSGOptiX, OpticksGenstepTest, qbnd_boundary_lookup_test, and ParamsTest. All three focused CTest cases pass. Refs: #346 Refs: #346 (comment) Refs: #346 (comment) Refs: #346 (comment) Refs: #346 (comment)
plexoos
force-pushed
the
qbnd-sibling-pair-matline
branch
from
July 30, 2026 15:28
d82daf4 to
f5177fe
Compare
GBndLib does not enumerate boundary entries for CSG sibling-pair faces (two daughters of one parent sharing a face), so OptiX can pick a boundary whose m1/m2 do not reference the photon's current medium -> absorption/scattering sampled in the wrong material1. Carry the photon's matline (seeded from the genstep at birth in CSGOptiX7.cu, updated each propagate_at_boundary) in sctx::current_matline and use it as m1 in qbnd::fill_state when valid. No-op for correctly-enumerated boundaries (carried == the boundary's own m1_line).
…dent faces At a face shared by two sibling primitives (segmented radiator bars + glue, multi-layer CSG lens) OptiX sorts the two coplanar intersections inconsistently; the "exiting" hit (cosI>0) carries the parent medium as m2, wrong for a sibling crossing. Bias the t reported to OptiX's closest-hit sort by +10 um on the exit side so the entering sibling wins. The real unbiased distance is preserved in the PRD, so photon position advancement is unperturbed. Gate the bias per boundary by index contrast |n_m1 - n_m2| > 0.1 (max over wavelength), computed at translation in CSGOptiX::initSimulate from the bnd table and read in __intersection__is. High-contrast faces (e.g. quartz/air ~0.47) need the entering-sibling selection; low-contrast thin gaps (e.g. aerogel/air ~0.025) are harmed by it and gate off. No per-detector hardcode; nullptr => always-on fallback. Stacks on the matline-carry m1 fix (carry source-medium matline across CSG sibling-pair faces): that fixes *which material* m1 is sampled; this fixes *which face* OptiX selects. The two are complementary - on a TIR light-guide either alone collapses the GPU hit count. Verified ePIC hpDIRC mu- 3 GeV: GPU/CPU 0.987, bit-identical to the ungated bias (all glued-sibling faces clear the 0.1 threshold). Low-index-contrast detectors are unaffected (gate stays off at their thin gaps).
Reflection never changes the photon's medium, so recomputing the carry from this boundary's own m1 line on the reflect branch could clobber a sibling-pair override from qbnd::fill_state with the parent medium's line (e.g. after a Fresnel/TIR reflection at a sibling-coincident face), poisoning m1 lookups at every subsequent boundary. Leave the carry untouched on reflect; transmit keeps the entered-medium (m2) line as before.
The script referenced epic_drich_simple.xml / drich_1sector_simple.xml, which were never committed, and hardcoded /tmp/simphony as the script directory. Use the bundled single-sector compact via DRICH_1SECTOR_XML, matching benchmark_drich.py, and derive the script directory from __file__ so the example runs from any checkout location. Note: constructing the 1-sector geometry against epic >= 26.06 still fails until the bundled radiator XML gains the <coronas> element that newer epic DRICH plugins require; that applies equally to benchmark_drich.py and is left to a separate change.
Seed the carried material line only for Cerenkov and scintillation gensteps, whose q0.z field has that contract. Treat UINT_MAX as the unavailable sentinel so material line zero remains valid, and validate carried rows against the uploaded boundary table before property or optical-buffer lookup. Replace the fixed 10 um, refractive-index-gated exit bias with a simulation-only one-ULP tie-break. This lets an entering sibling win exact coplanar ties without crossing a representable physical gap, while preserving the unbiased intersection for position advancement. Removing the old gate also eliminates its static GPU allocation and per-boundary debug logging. Add regression coverage for genstep material-line ownership; valid line-zero and inner-material carries; sentinel, surface-row, and out-of-range fallbacks; and the updated Params layout. Validated with a Release CUDA/OptiX build of CSGOptiX, GenstepTest, qbnd_boundary_lookup_test, and ParamsTest. All three focused CTest cases pass. Refs: #346 Refs: #346 (comment) Refs: #346 (comment) Refs: #346 (comment) Refs: #346 (comment)
plexoos
force-pushed
the
qbnd-sibling-pair-matline
branch
from
July 30, 2026 16:41
f5177fe to
57340fc
Compare
Align OpticksGenstep_UsesMaterialLine with the current genstep protocol after #429 retired the five legacy suffixed identifiers. Only the generic Cerenkov, scintillation, and modified G4 Cerenkov producers retain the q0.z material-line contract. Cover numeric protocol slots 1 through 5 as retired non-material-line gensteps so a future reintroduction cannot silently restore the stale classification. Validated by rebuilding CSGOptiX7_ptx and OpticksGenstepTest and running SysRapTest.OpticksGenstepTest plus SysRapTest.GenstepTest. Refs: #346 Refs: #429
plexoos
force-pushed
the
qbnd-sibling-pair-matline
branch
from
July 30, 2026 17:39
effd842 to
a9fd8a3
Compare
plexoos
force-pushed
the
qbnd-sibling-pair-matline
branch
from
July 30, 2026 17:42
a9fd8a3 to
72ddeaa
Compare
Exercise the carried material-line lifecycle directly through qsim, checking that transmission records the entered material while reflection preserves the current one. Share the CUDA-local genstep classification and reported-distance rules with an on-device test covering TORCH/Cerenkov handling and exact entering-versus-exiting intersection ties. Add a compact touching-sibling GDML fixture and simg4ox CPU/GPU hit comparison so the production propagation path must cross the shared face and retain the correct medium properties to reach the sensor. Refs: #346
Contributor
Cpp-Linter Report
|
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
This PR fixes GPU optical-photon propagation where two sibling CSG volumes share the same face, as can happen in segmented radiator bars with glue layers and multi-layer lenses.
At a shared face, OptiX can select a boundary involving the common parent volume instead of the adjacent sibling. The photon may then use the wrong optical material or behave as if it crossed a thin layer of the parent medium, leading to incorrect refraction, total internal reflection, and lost photons.
What changes
0.1, using the existing boundary table without detector-specific configuration.~0.47) while leaving near-matched boundaries such as aerogel/air (~0.025) unchanged.