Support for spherical op.det. in geometry (issue #25003)

[PetrilloAtWork]

This should solve issue #25003.
I tested it on ICARUS code by running the geometry test (which is not expected to exercise the change) and checking the geometry dump.
I would like SBND to check this commit as well, since SBND has both bar- and hemisphere-shaped photodetectors.

[FNALbuild]

A new Pull Request was created by @PetrilloAtWork (Gianluca Petrillo) for develop.

It involves the following packages:

larcorealg

@LArSoft/level-1-managers, @LArSoft/level-2-managers can you please review it and eventually sign? Thanks.

cms-bot commands are listed here

[FNALbuild]

The code-checks are being triggered in jenkins.

[FNALbuild]

Pull request #11 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again.

[FNALbuild]

+code-checks
for details see
https://buildmaster.fnal.gov/buildmaster/job/GithubIntegration/job/run-pr-code-checks//715/console

[pgreen135]

@PetrilloAtWork I had a go using this for SBND with the opDets in OpFastScintillation.cxx (~line 251):
for (size_t const i : util::counter(fPVS->NOpChannels())) { geo::OpDetGeo const& opDet = geom.OpDetGeoFromOpDet(i);
The opDets are a mixture of bars (arapucas) and spheres (pmts), but for every opDet isBar(), isSphere() and isDisk() all return false. In the previous version isBar() was working when called like this, so something seems to be an issue in this implementation.

[PetrilloAtWork]

I should have fixed the bug. Even if I have tested this change with sbndcode, I would appreciate cross testing.

[pgreen135]

@PetrilloAtWork I've repeated my tests in SBND and this appears to be working properly now, at least with bars and spheres. Re Kyle's comment - in the case of spheres isBar() is indeed correctly returning false in my tests.

[FNALbuild]

Pull request #11 was updated. @LArSoft/level-1-managers, @LArSoft/level-2-managers can you please check and sign again.

[FNALbuild]

The code-checks are being triggered in jenkins.

[FNALbuild]

+code-checks
for details see
https://buildmaster.fnal.gov/buildmaster/job/GithubIntegration/job/run-pr-code-checks//718/console

[lgarren]

trigger build

[FNALbuild]

The tests are being triggered in jenkins.

[FNALbuild]

The code-checks are being triggered in jenkins.

[FNALbuild]

+code-checks
for details see
https://buildmaster.fnal.gov/buildmaster/job/GithubIntegration/job/run-pr-code-checks//719/console

[lgarren]

trigger build

[FNALbuild]

The tests are being triggered in jenkins.

[PetrilloAtWork]

Err... ok 🙄
I feel like Clang is telling me "Warning: typeid() is operating exactly as everybody should expect it to!".
I have pushed the "fix" (which AFAIK does exactly the same as before, while trying also to impress Clang).

I request a C++ expert review of this part of the code (@knoepfel?) with particular attention to typeid here, since usually Clang is smarter than me.
The context is that TGeoShape is base class of all shapes in ROOT geometry, Shape() returns a pointer to a polymorphically-aware class derived from TGeoShape, and my code was supposed to access and test the exact type of that derived class.
I think that *Shape() (and in the new code *shape) return the glvalue that typeid() needs, but I need cross checks.
At least it seems to work...

Thanks, Lynn, for reporting it to me.

---

P.S. For SBND folks: you may be already aware of that, anyway geo::OpDetGeo::ThetaZ() for the arapuca bars is broken (returns nan). I suspect the reason is that, as any reasonable person would, the bars are described in GDML with the longest side aligned with z. If that's the problem, changing the definition so that z is aligned to the thickness (i.e. toward the TPC after the final rotation) would fix ThetaZ(), and also break (or at least change) HalfL(), which would then report half the thickness. A real fix requires additional coding in the geometry builder to provide the object with contextual information ("where is the TPC") which OpDeTGeo normally does not have.

[FNALbuild]

+LArSoft tests OK on slf7 for c7:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/9672&builds=
+build

[FNALbuild]

+LArSoft tests OK on slf7 for e19:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/9671&builds=
+build

[FNALbuild]

-icarus tests failed on slf7 for e19:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/ns:icarus/view_builds/index?offset=0&builds=icarus_ci/1170&builds=
for details of the parent CI build see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/9671&builds=

[FNALbuild]

+sbnd tests OK on slf7 for e19:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/ns:sbnd/view_builds/index?offset=0&builds=sbnd_ci/5554&builds=
for details of the parent CI build see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/9671&builds=

[FNALbuild]

-uboone tests warning on slf7 for e19:prof
for details see
https://lar-ci-history.fnal.gov/LarCI/app/ns:uboone/view_builds/index?offset=0&builds=uboone_ci/5603&builds=
for details of the parent CI build see
https://lar-ci-history.fnal.gov/LarCI/app/view_builds/index?offset=0&builds=lar_ci/9671&builds=

[knoepfel]

@PetrilloAtWork, I agree the warning emitted by Clang is over-cautious. Regardless, your commit does address the issue.

On a side note: the fact that the user has to ask what shape is represented by an OpDetGeo object indicates a broken abstraction. The class should be well enough encapsulated that such questions are not necessary when using the object. Someday, when everyone has the time, we should reconsider the OpDetGeo design. But it is not this day...

[knoepfel]

approve

[FNALbuild]

This pull request is fully signed and it will be merged to develop and built in the next LArSoft release after it passes the integration tests.