feat: Adding option for CylinderBounds to have a bevelled edges

[noraemi]

PR to introduce a modification of the CylinderBounds to include an angle to tilt both edges of the cylinder.
WIP for now to receive initial feedback on the implementation, e.g. if this should rather be an extension

• Before updating all the boundary checks / isInside / etc that I haven't done yet, I wonder if its fine to update the VerticesHelper as I have done or if its preferred to do this tilt purely by a transform (which proved to be quite difficult)

Current implementation keeps the length in z at the mid-point of the cylinder and tilt the edges with an defined angle

[asalzburger]

The word is 'bevel' - had to look it up as well.

[paulgessinger]

Thanks @noraemi!

I left a few comments. One general one: I think we'll need the possibility to have only one of the sides of the cylinder have an inclined cutoff. I guess this would mean having two angle parameters?

[asalzburger]

Great to get this started, @noraemi !

I think we need two different angles, and -- what I totally forgot to mention -- of course we need to update the

• SurfaceBounds::inside()
• VolumeBounds::inside()

methods, there's a lot of code we can just take from the equivalent ATLAS classes.

Then, to complete the PR, this needs UnitTests for the added code to check consistency.

[asalzburger]

As for the format checks, we are using clang-format, and if the correct/right version is installed locally, one can run

./Ci/check_format .

on the repository and this should fix the format checker.

[codecov]

Codecov Report

Merging #1104 (e673e26) into main (a200144) will decrease coverage by 0.09%.
The diff coverage is 27.92%.

@@            Coverage Diff             @@
##             main    #1104      +/-   ##
==========================================
- Coverage   48.13%   48.03%   -0.10%     
==========================================
  Files         341      341              
  Lines       17660    17740      +80     
  Branches     8342     8385      +43     
==========================================
+ Hits         8500     8521      +21     
- Misses       3372     3405      +33     
- Partials     5788     5814      +26     

Impacted Files	Coverage Δ
Core/include/Acts/Surfaces/CylinderSurface.hpp	100.00% <ø> (ø)
...re/include/Acts/Surfaces/detail/VerticesHelper.hpp	51.61% <ø> (ø)
...ore/include/Acts/Geometry/CylinderVolumeBounds.hpp	46.42% <14.28%> (-4.56%)	⬇️
Core/include/Acts/Surfaces/CylinderBounds.hpp	65.51% <20.00%> (-10.49%)	⬇️
Core/src/Surfaces/CylinderSurface.cpp	36.57% <25.00%> (-0.53%)	⬇️
Core/src/Geometry/CylinderVolumeBounds.cpp	46.15% <25.80%> (-5.13%)	⬇️
Core/src/Surfaces/CylinderBounds.cpp	44.32% <31.25%> (-16.65%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a200144...e673e26. Read the comment docs.

[stale]

This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.

[noraemi]

I ran into a snag trying to finish up the last parts, and some feedback would be helpful. The issue is with the "inside" check for the cylinderbounds. This check does box, with the phi/Z, check as far as I can tell. For the bevel part this would obviously not work. Would the right approach here be to extend the functionality in BoundaryCheck to have say an "isInsideRhombus" function or similar? Where I could define the tilted box rather. And call this function instead of isInside for the cases where the bevel parameters are set.

bool Acts::CylinderBounds::inside(const Vector2& lposition,
                                  const BoundaryCheck& bcheck) const {
  return bcheck.transformed(jacobian())
      .isInside(shifted(lposition),
                Vector2(-get(eHalfPhiSector), -get(eHalfLengthZ)),
                Vector2(get(eHalfPhiSector), get(eHalfLengthZ)));
}

[paulgessinger]

[asalzburger]

Regarding the BoundaryCheck - as a start, it will be good enough with the fixed tolerance, although we should be able to use the generic code when unfolded.

[paulgessinger]

I think since in local coordinates the edges are straight, even the covariance check should be ok: convert to local with jacobian und use the vertex inside check in BoundaryCheck.

[noraemi]

Regarding the BoundaryCheck - as a start, it will be good enough with the fixed tolerance, although we should be able to use the generic code when unfolded.

Isn't this already handled by boundaryCheck.isTolerated(closestPoint - lposition); ? It performs the type check there whether to use the absolute or more

[asalzburger]

Hi @noraemi - one quick question: can the bevelling also be in the same direction on both sides?

The ATHENA-EIC guys need a tilted beam pipe inlay into a cylinder volume (we can specify that volume shape later), but that would need some sort of parallel cut off on both sides.

[noraemi]

Hi @noraemi - one quick question: can the bevelling also be in the same direction on both sides?

The ATHENA-EIC guys need a tilted beam pipe inlay into a cylinder volume (we can specify that volume shape later), but that would need some sort of parallel cut off on both sides.

Yes! the bevel can be done in both directions e.g: \___ or /___
the two sides are independent as well. Will add a comment about what the bevel is w.r.t somewhere in the CylinderBounds

[asalzburger]

This looks ok to me now, is there more to do here from your side, Nora?

[asalzburger]

Updating branch to check merge-ability,

[noraemi]

This looks ok to me now, is there more to do here from your side, Nora?

I'm just adding the unit-tests for the bevel, after that I think its done

[noraemi]

This PR should be ready now, if the CI doesn't fail.