CSG: phi-wedge support for ZSphere/Cylinder (bake startPhi/deltaPhi) - #345
Open
ggalgoczi wants to merge 3 commits into
Open
CSG: phi-wedge support for ZSphere/Cylinder (bake startPhi/deltaPhi)#345ggalgoczi wants to merge 3 commits into
ggalgoczi wants to merge 3 commits into
Conversation
G4Sphere/G4Tubs with a phi wedge previously aborted in U4Solid. Bake the wedge into the primitive instead of composing CSG_INTERSECTION(ZSphere/Cylinder, PhiCut), which leaks rays whose origin is outside the wedge. sn::ZSphere/Cylinder gain 5-arg overloads parking startPhi/deltaPhi in the cx/cy slots; the csg_intersect_leaf_{zsphere,cylinder}.h leaves clip candidates to the wedge; U4Solid::init_Sphere_/init_Tubs emit the phi-baked primitives; CSGNode cylinder AABB no longer treats cx/cy as the centre. deltaPhi==0 (or >=2pi) => no clip, so non-phi geometry is unchanged.
…der)
Self-checking ctest for the per-primitive phi-wedge clip in
csg_intersect_leaf_{zsphere,cylinder}.h: asserts a ray whose near surface
candidate is outside the wedge still recovers the in-wedge hit (the original
CSG_INTERSECTION(prim,PhiCut) leak), and a ray crossing entirely outside the
wedge misses. Exit code is meaningful so regressions fail under ctest.
ggalgoczi
marked this pull request as ready for review
May 30, 2026 14:50
Contributor
Author
ggalgoczi
force-pushed
the
csg-phi-wedge-primitives
branch
from
May 30, 2026 15:00
5c6c76b to
8c2414b
Compare
This was referenced May 30, 2026
Member
Member
Could you remind me where was this happening? |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds phi-wedge support for G4Sphere/G4Tubs by baking startPhi/deltaPhi directly into the CSG_ZSPHERE / CSG_CYLINDER leaf primitives (instead of composing an intersection with a separate PhiCut), addressing previously observed ray “leaks” for wedge geometries.
Changes:
- Add 5-arg
sn::ZSphere/sn::Cylinderoverloads that parkstartPhi/deltaPhiinto the param slots formerly used forcx/cy. - Update
U4Solidto emit phi-baked primitives for spheres and tubs. - Extend leaf intersection + distance logic for
CSG_ZSPHEREandCSG_CYLINDER, adjust cylinder AABB handling, and add a regression test.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| u4/U4Solid.h | Emits phi-baked ZSphere/Cylinder primitives instead of aborting/avoiding phi wedges. |
| sysrap/sn.h | Adds 5-arg Cylinder/ZSphere constructors encoding startPhi/deltaPhi into params. |
| CSG/csg_intersect_leaf_zsphere.h | Implements optional phi-wedge clipping for ZSphere leaf distance/intersection. |
| CSG/csg_intersect_leaf_cylinder.h | Implements optional phi-wedge clipping (including wedge walls) for Cylinder leaf distance/intersection. |
| CSG/CSGNode.cc | Updates cylinder AABB computation to ignore cx/cy now repurposed for phi params. |
| CSG/tests/intersect_leaf_phi_wedge_test.cc | Adds CPU regression tests for baked-in phi-wedge behavior on ZSphere/Cylinder. |
| CSG/tests/CMakeLists.txt | Registers the new regression test with CMake/ctest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+174
to
178
| if (t1sph > t_min && t1sph_ok) | ||
| t_cand = t1sph; // t1sph qualified and t1cap disabled or disqualified -> t1sph | ||
| else if (t1cap > t_min) | ||
| t_cand = t1cap; // t1cap qualifies -> t1cap | ||
| else if( t2cap > t_min ) t_cand = t2cap ; // t2cap qualifies -> t2cap |
Comment on lines
+32
to
+33
| float r_xy = sqrtf(p.x * p.x + p.y * p.y); | ||
| float d_to_edge = r_xy * fminf(dphi - deltaPhi, twoPi - dphi); |
Comment on lines
+249
to
+250
| float r_xy = sqrtf(pos.x * pos.x + pos.y * pos.y); | ||
| float d_to_edge = r_xy * fminf(dphi - deltaPhi, twoPi - dphi); |
Comment on lines
+152
to
+162
| auto phi_in_wedge = [&](float t) -> bool { | ||
| if (!has_phi_clip) | ||
| return true; | ||
| float x = O.x + t * D.x; // local-frame hit coords (O = origin − center) | ||
| float y = O.y + t * D.y; | ||
| float phi = atan2f(y, x); | ||
| const float twoPi = 2.f * M_PIf; | ||
| while (phi < startPhi) phi += twoPi; | ||
| float dphi = phi - startPhi; | ||
| return dphi <= deltaPhi; | ||
| }; |
Comment on lines
+72
to
+79
| auto phi_in_wedge = [&](float xh, float yh) -> bool { | ||
| if (!has_phi_clip) | ||
| return true; | ||
| float phi = atan2f(yh, xh); | ||
| const float twoPi = 2.f * M_PIf; | ||
| while (phi < startPhi) phi += twoPi; | ||
| return (phi - startPhi) <= deltaPhi; | ||
| }; |
Contributor
Author
Contributor
Author
Two places in bool has_deltaPhi_expect = has_deltaPhi == false ; Any G4Sphere with startPhi != 0 or deltaPhi != 2π triggers assert + SIGINT. G4Tubs (init_Tubs) — silent bug. init_Tubs() on main never reads startPhi/deltaPhi from the G4Tubs at all — it passes only (rmax, -hz, hz) to sn::Cylinder, producing a full cylinder regardless. Photons leak through where the phi boundary should be. |
Member
What should I run to reproduce the abort? |
Member
|
Superseded by #426 |
plexoos
added a commit
that referenced
this pull request
Jul 29, 2026
## Summary Add analytic phi-wedge support for incomplete `G4Sphere` and `G4Tubs` geometries during Geant4-to-CSG conversion. The conversion now preserves `startPhi` and `deltaPhi` on centered `ZSphere` and `Cylinder` leaves. 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 `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. ## Motivation Partial-phi geometries were previously unsupported in different ways (see #334): - Converting `tests/geom/sphere_phicut_quarter_shell.gdml` aborted in Debug builds. - Release builds terminated before macro execution through the corresponding unsupported-geometry path. - Partial `G4Tubs` geometries 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 `ZSphere` and modern `Cylinder` leaf encodings. - `q0.f.x` stores `startPhi`. - `q0.f.y` stores `deltaPhi`. - A zero delta retains the legacy full-solid behavior. - Update `U4Solid` conversion: - Convert partial `G4Sphere` solids to phi-aware `ZSphere` leaves. - Convert partial `G4Tubs` solids to phi-aware outer and inner `Cylinder` leaves. - Preserve the existing canonical intersection with a complemented inner cylinder for annular tubes. - Add shared host/device phi-wedge helpers for: - normalized relative azimuth tests; - wrapped angular intervals; - sweeps both smaller and larger than π; - radial cut-face intersections; - outward cut-face normals; - signed-distance classification. - Extend sphere and cylinder intersections to: - reject curved-surface and end-cap candidates outside the phi interval; - intersect the two radial cut faces; - select the nearest valid candidate across all surfaces; - return the appropriate outward normal. - Preserve compatibility: - Full spheres and cylinders retain their existing representation and behavior. - Legacy `OldCylinder` center encoding remains unchanged. - Phi-wedged primitives retain conservative full-radius AABBs. ## Test coverage Add a focused primitive-level phi-wedge test covering: - quarter-wedge curved-surface intersections; - radial start and end cut faces; - rays outside the wedge; - signed-distance classification; - z-cap filtering; - wrapped intervals crossing phi zero; - sweeps larger than π. Extend the existing sphere regression to require successful conversion of: `tests/geom/sphere_phicut_quarter_shell.gdml` The test verifies the two expected quarter-phi `ZSphere` leaves. Add a separate annular tube fixture and conversion regression: `tests/geom/tube_phicut_quarter_shell.gdml` The tube test verifies: - preserved `startPhi` and `deltaPhi`; - outer and inner cylinder radii; - canonical intersection structure; - exactly one complemented inner cylinder; - conservative bounds. ## Scope This change adds partial-phi support specifically for `G4Sphere` and `G4Tubs`. 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](#345), which presented an earlier implementation of the same phi-aware `ZSphere`/`Cylinder` approach. Thanks to Gabor for the original investigation and groundwork.
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
Phi-wedged
G4SphereandG4Tubscurrently either abort inU4Solidor rely on a composedCSG_INTERSECTION(primitive, PhiCut)representation. That composition can lose valid hits when the first primitive candidate lies outside the wedge but a later in-wedge candidate should still be returned.This PR bakes
startPhi/deltaPhidirectly into the origin-centeredZSphereandCylinderprimitives so phi clipping happens inside the primitive intersector, where candidate ordering is known.Motivation
CSG_INTERSECTION(primitive, PhiCut), where a valid later hit can be dropped if the near candidate falls outside the wedge.G4Sphere/G4Tubsto convert throughU4Solidinstead of hitting the previous abort path.What Changed
sn::ZSphere(radius, z1, z2, startPhi, deltaPhi)sn::Cylinder(radius, z1, z2, startPhi, deltaPhi)startPhi/deltaPhiin radians.csg_intersect_leaf_zsphere.hto reject sphere-surface candidates outside the phi wedge.csg_intersect_leaf_cylinder.hto:U4Solid::init_Sphere_andU4Solid::init_Tubs()to emit phi-baked primitives instead of aborting or depending on a separatePhiCutnode.cx/cyare no longer interpreted as a geometric center when those slots carry phi parameters.intersect_leaf_phi_wedge_test.ccregression coverage for recovered far hits, outside-wedge misses, and no-clip controls.Behavior / Compatibility
deltaPhi == 0ordeltaPhi >= 2pipreserves the previous full-primitive behavior.Sphere/ZSphere/Cylinder/Tubspaths are intended to be unchanged.U4Solidnow emits a phi-bakedZSphereover the full[-radius, +radius]z-range.CSG_INTERSECTION(primitive, PhiCut)and keeps wedge logic local to the primitive intersector.Test Coverage
CSG/tests/intersect_leaf_phi_wedge_test.ccThis test covers:
Scope Note
This PR mainly covers the primitive/intersector path and the corresponding
U4Solidemitters. Additional end-to-end and sliced-sphere edge-case coverage would still be useful follow-up work.