feat(csg): support partial-phi spheres and tubes - #426
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a194a3b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (has_deltaPhi) | ||
| return sn::ZSphere(radius, zmin, zmax, startPhi, deltaPhi); |
There was a problem hiding this comment.
When a partial-phi sphere is subtracted from another sphere with the same phi interval—as in sphere_phicut_quarter_shell.gdml, or for a G4Sphere with a nonzero inner radius—both leaves now report the same radial-wall intersection inside the inner radius. After positivization the outer hit is Enter and the complemented inner hit is Exit at equal t; intersect_tree treats the lhs as closer and returns the outer hit, producing a phantom cut face that seals the hollow region. The phi restriction needs to be represented once around the radial shell rather than baked into both operands.
Useful? React with 👍 / 👎.
| sn* inner = has_deltaPhi | ||
| ? sn::Cylinder(rmin, -(hz + dz), hz + dz, startPhi, deltaPhi) | ||
| : sn::Cylinder(rmin, -(hz + dz), hz + dz); |
There was a problem hiding this comment.
For every partial annular G4Tubs, giving the inner cylinder the same phi wedge creates radial faces exactly coincident with the outer cylinder for radii below rmin; the existing z nudge does not separate these faces. A ray crossing a cut plane through the nominal hole receives equal outer-Enter and complemented-inner-Exit hits, and the CSG intersection lookup returns the outer hit, so the annular hole appears capped by a false phi face. Subtracting a full inner cylinder from the wedged outer cylinder preserves the intended volume without introducing these coincident walls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds analytic phi-wedge (partial-phi) support for G4Sphere and G4Tubs during Geant4→CSG conversion by baking startPhi/deltaPhi into centered ZSphere and Cylinder leaves, and updating intersection + signed-distance logic to respect radial phi cut faces.
Changes:
- Preserve
startPhi/deltaPhiwhen converting partial-phiG4SphereandG4Tubsinto phi-awaresn::ZSphere/sn::Cylinderleaves. - Introduce shared host/device phi-wedge helpers and extend leaf intersection/SDF for
CylinderandZSphereto account for phi walls. - Add focused regression and primitive-level tests plus a GDML fixture for quarter-phi tubes.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| u4/U4Solid.h | Converts partial-phi spheres/tubes into phi-aware ZSphere/Cylinder leaves. |
| u4/tests/TubePhiCutQuarterShellTest.cc | New regression test validating tube phi metadata + canonical annulus structure. |
| u4/tests/SpherePhiCutQuarterShellTest.cc | Updates regression to require successful partial-phi sphere conversion with baked metadata. |
| u4/tests/CMakeLists.txt | Registers the new tube phi-cut regression test. |
| tests/geom/tube_phicut_quarter_shell.gdml | Adds a minimal GDML quarter-phi annular tube fixture. |
| sysrap/sn.h | Adds Cylinder/ZSphere overloads carrying phi metadata; updates AABB logic for new parameter layout. |
| CSG/tests/intersect_leaf_phi_wedge_test.cc | New low-level test exercising wedge intersections and SDF behavior. |
| CSG/tests/CMakeLists.txt | Registers the new CSG phi-wedge test. |
| CSG/CSGNode.cc | Updates cylinder AABB computation to match new parameter layout (phi metadata in q0). |
| CSG/csg_intersect_leaf_zsphere.h | Adds phi-wedge filtering, phi-wall intersections, and phi-aware SDF for ZSphere. |
| CSG/csg_intersect_leaf_phi_wedge.h | New shared phi-wedge helpers for inclusion tests, wall intersections, and SDF terms. |
| CSG/csg_intersect_leaf_cylinder.h | Adds phi-wedge filtering, phi-wall intersections, and phi-aware SDF for Cylinder. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
u4/U4Solid.h:983
- Same issue as in
init_Sphere_:has_deltaPhishould be based on the sweep (deltaPhi) rather thanstartPhi != 0. For a full sweep with non-zerostartPhi, this currently storesdeltaPhi == 2*piinto the leaf params instead of using the legacy full-cylinder encoding (deltaPhi == 0), and can be misclassified as a wedge on the GPU due to float rounding.
double startPhi = tubs->GetStartPhiAngle() / CLHEP::radian;
double deltaPhi = tubs->GetDeltaPhiAngle() / CLHEP::radian;
bool has_deltaPhi = startPhi != 0. || deltaPhi != 2. * CLHEP::pi;
sn* outer = has_deltaPhi
? sn::Cylinder(rmax, -hz, hz, startPhi, deltaPhi)
: sn::Cylinder(rmax, -hz, hz);
u4/U4Solid.h:588
has_deltaPhitreatsstartPhi != 0as evidence of a phi wedge. That can cause full-sweep solids (deltaPhi == 2*pi) to be encoded withdeltaPhistored as2*piinstead of using the legacy full-phi encoding (deltaPhi == 0). Besides breaking the stated compatibility invariant, storing2*piin the float param quad risks the GPU-sidedeltaPhi < 2*pitest classifying it as a wedge due to float rounding, introducing spurious radial walls.
This issue also appears on line 977 of the same file.
double startPhi = sphere->GetStartPhiAngle()/CLHEP::radian ;
double deltaPhi = sphere->GetDeltaPhiAngle()/CLHEP::radian ;
bool has_deltaPhi = startPhi != 0. || deltaPhi != 2.*CLHEP::pi ;
if (has_deltaPhi && layer == 'O')
return sn::ZSphere(radius, zmin, zmax, startPhi, deltaPhi);
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eefa6308f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -1,11 +1,13 @@ | |||
| #pragma once | |||
|
|
|||
| #include "csg_intersect_leaf_phi_wedge.h" | |||
There was a problem hiding this comment.
When consumers build against an installed CSG package, this include cannot be resolved: CSG/CMakeLists.txt installs only files listed in HEADERS, but the new csg_intersect_leaf_phi_wedge.h was not added to INTERSECT_HEADERS. Because the installed csg_intersect_leaf.h includes this cylinder header, downstream compilation fails with a missing-header error; add the helper to the installed header list.
Useful? React with 👍 / 👎.
Preserve Geant4 startPhi and deltaPhi on centered ZSphere and Cylinder leaves. Partial G4Sphere conversion previously aborted in Debug and terminated before macro execution in Release, while partial G4Tubs conversion silently produced full cylinders. Add shared host/device phi-interval helpers for curved-surface and cap filtering, radial cut-face intersections and normals, and signed-distance classification. Keep conservative full-radius bounds, preserve the legacy OldCylinder center encoding, and support wrapped intervals and sweeps larger than pi. Extend the existing quarter-shell sphere regression and add separate primitive and annular G4Tubs GDML tests. Validation covered eight focused and compatibility tests in Debug, three feature tests in Release, and successful simg4ox runs for both fixtures in both build types.
Apply partial-phi metadata only to the outer leaf of G4Sphere and G4Tubs shells. Leaving the complemented inner leaf full-angle prevents equal-t radial-wall intersections from producing phantom faces across the hollow region. For aligned Boolean subtractions whose primitive leaves carry matching phi intervals, clear the redundant RHS interval before canonicalization. Accept identity-displaced GDML operands while preserving phi metadata on genuinely transformed subtractions. Update the sphere regression to cover both the existing GDML subtraction and a native G4Sphere with nonzero inner radius, and update the tube regression to require a wedged outer and full-angle inner cylinder.
3eefa63 to
3531505
Compare
3531505 to
cf29586
Compare
The partial-phi sphere and tube implementation from #426 has focused geometry and intersection tests, but it lacks an end-to-end regression using a realistic detector that converts those solids and compares optical-photon detection across Geant4 and Opticks. The dRICH fixture provides that coverage with one partial sphere, three partial tubes, and 210 annotated photon-sensor volumes. Add a deterministic 10,000-photon, 420 nm torch configuration aimed at a dRICH SiPM patch so the CPU and GPU receive identical optical input and produce stable nonzero hit samples. Register Integration.simg4ox.drich through the common CMake helper, allow hit-validation cases to select a custom macro, and use run_validate.mac so Geant4 invokes the sensor SDs. The complete geometry is converted before transport, so the test exercises partial-phi construction even though the controlled beam targets the sensors directly. GDML import strips pointer-like suffixes from logical-volume names, leaving distinct dRICH sensor volumes with duplicate names. Append a numeric index to each PhotonSD name so G4SDManager collection keys remain unique and all Geant4 sensor hits are retained for the existing three-sigma count and distribution comparison.
Summary
Add analytic phi-wedge support for incomplete
G4SphereandG4Tubsgeometries during Geant4-to-CSG conversion.The conversion now preserves
startPhianddeltaPhion centeredZSphereandCylinderleaves. Their intersection and signed-distance implementations account for both radial cut faces as well as the existing curved surfaces and end caps.Add shared host/device phi-interval helpers for curved-surface and cap filtering, radial cut-face intersections and normals, and signed-distance classification. Keep conservative full-radius bounds, preserve the legacy
OldCylindercenter encoding, and support wrapped intervals and sweeps larger than pi.Extend the existing quarter-shell sphere regression and add separate primitive and annular
G4TubsGDML tests.Motivation
Partial-phi geometries were previously unsupported in different ways (see #334):
tests/geom/sphere_phicut_quarter_shell.gdmlaborted in Debug builds.G4Tubsgeometries silently lost their phi limits and were represented as complete cylinders.Consequently, a Release build could conceal the sphere assertion without producing a usable partial sphere, while tube conversion could succeed with incorrect geometry.
Implementation
Add phi metadata to the existing
ZSphereand modernCylinderleaf encodings.q0.f.xstoresstartPhi.q0.f.ystoresdeltaPhi.Update
U4Solidconversion:G4Spheresolids to phi-awareZSphereleaves.G4Tubssolids to phi-aware outer and innerCylinderleaves.Add shared host/device phi-wedge helpers for:
Extend sphere and cylinder intersections to:
Preserve compatibility:
OldCylindercenter encoding remains unchanged.Test coverage
Add a focused primitive-level phi-wedge test covering:
Extend the existing sphere regression to require successful conversion of:
tests/geom/sphere_phicut_quarter_shell.gdmlThe test verifies the two expected quarter-phi
ZSphereleaves.Add a separate annular tube fixture and conversion regression:
tests/geom/tube_phicut_quarter_shell.gdmlThe tube test verifies:
startPhianddeltaPhi;Scope
This change adds partial-phi support specifically for
G4SphereandG4Tubs. It does not extend other angularly clipped Geant4 solids such as cones, cut tubes, or polycones. Bounds remain deliberately conservative rather than being tightened to the phi wedge.This PR supersedes #345, which presented an earlier implementation of the same phi-aware
ZSphere/Cylinderapproach. Thanks to Gabor for the original investigation and groundwork.