Zero size bounding box for some TGeoVolumeAssembly nodes after transformation by translation or rotation

[PALoizeau]

• Checked for duplicates
  => Maybe linked/similar to https://sft.its.cern.ch/jira/browse/ROOT-7420 ? (Zero size BBox after using AddNode)

Describe the bug

After a translation/rotation is applied to a TGeoVolumeAssembly through the MakePhysicalNode > Align sequence, the BoundingBox of this TGeoVolumeAssembly may end up with 0 size (GetDX, GetDY and GetDZ returning 0).
This is not expected as it means there are other effects on the Volume aside from the translation/rotation, even when shifting something without leaving its parent volume.

Something similar happens if a translation/rotation is applied to a Node which is part of a TGeoVolumeAssembly, but there it probably happens only if the changes leads to part of the assembly exceeding the original boundaries.
We (me and @fuhlig1) suspect that this may partially be recursive (affecting the parent of the aligned volume).

This can be cured by forcing the recomputation of the BoundingBox of the affected Volume.

Expected behavior

After alignment of the nodes, all volumes/nodes are offset/rotated with their other properties valid.
Or if not possible due to performance, a warning should be printout with hint of the proper way to restore/recompute these values.

A temporary fix was introduced for now in FairRoot, which is to simply do the recompute for all TGeoVolumeAssembly in the geometry tree, which seems to be fast enough that we do not have to try to track affected Volumes to be more selective.
This may however not be the case later when applied to complete geometries for experiments like CBM.

To Reproduce

Steps to reproduce the behavior with the attached files (includes "ROOT-only" examples both for problem and for macro-level fix):

1. Compile a copy of ROOT 6.22.08 (done with FairSoft apr21p2 and FairRoot v18.6.7, should work with any older versions of each and with recent versions of FairRoot, as well as with a standalone version of ROOT)
2. Download BboxAlignPb_example_2023_02_07.zip and unzip the macro, the standard container dictionary header and the two root files in a folder
3. Go to this folder, load your ROOT environment
4. Run

   root -l -b
   root [0] .L alignment_map_def.h+
   root [1] .x BboxAlignBug.C("mcbm_beam_2022_05_23_nickel.geo.root", "AlignmentMatrices_mcbm_beam_2022_05_23_nickel.root")
   

Of the 3 transformations visible in the printout

• Two are applied to TGeoVolumeAssembly objects and lead to these assembly having zero size bounding boxes (/cave_1/sts_v22e_mcbm_0 and /cave_1/sts_v22e_mcbm_0/Station01_1)
• One is applied to a Node and leads to 2 TGeoVolumeAssembly higher in the hierarchy to have zero size bounding boxes (/cave_1/sts_v22e_mcbm_0/Station02_2/Ladder10_2/HalfLadder10d_2/HalfLadder10d_Module04_2)

Setup

1. ROOT 6.22.08, 6.26.10
2. OS: Debian 8, Debian 10, Ubuntu 20.04
3. Compiler: GCC > 8
4. ROOT provided (compilation/installation) from:
   • FairSoft apr21 (and patch releases) for 6.22, Legacy mode ("not Spack")
   • FairSoft nov22 (and patch releases) for 6.26, Legacy mode ("not Spack")

Additional context

• Fixed for now on the Fairroot side by PR 1244 (Issue 1243)
• Unsure if this is a not-well documented intended behavior or an unexpected bug
• Adding a call to gGeoManager->CloseGeometry(); instead of the fix used in FairRoot (as advised in ROOT-7420 for the AddNode case) does not cure the problem and results in a warning that the geometry is already closed
• I am not the author nor an expert of the section of code used to apply the alignment matrices in Fairsoft, which I copied in the example macro. For questions there I may have to forward to the corresponding developers.
• Investigation done by myself and @fuhlig1

[agheata]

Looking into this. There is a provision for recomputing the bounding boxes and voxel structures for hierarchies of assemblies affected by TGeoPhysicalNode::Align, I have to check what gets wrong there.

[agheata]

Ok, I've had a first quick look. This is rather a feature than a bug. During alignment, some bounding boxes and voxels need to be recomputed (see the logic in TGeoPhysicalNode::Align). Since a given volume may be changed by many physical node alignments, their bounding boxes and voxels are only marked to be recomputed, otherwise the operation takes too long for complex cases. This lazy recomputation occurs only when any navigation function is called for the given volume, but not when calling just the getter of the box dimensions. So the behavior runtime should be OK, even if you see that some bounding boxes appear to be zero just after alignment.

The recomputation of boxes/voxels can be however triggered manually using the following code:

   TIter nextv(gGeoManager->GetListOfVolumes());
   TGeoVolume *vol;
   while ((vol = (TGeoVolume*)nextv())) {
      if (vol->IsAssembly()) vol->GetShape()->ComputeBBox();
      auto finder = vol->GetVoxels();
      if (finder && finder->NeedRebuild()) {
         finder->Voxelize();
         vol->FindOverlaps();
      }
   }

I guess this code could be automatically called inside TGeoManager::RefreshPhysicalNodes() which needs to be called anyway by the user to recover pointer consistency after all alignments are applied. Let me know if this code does the job for you and I'll put it there.

[PALoizeau]

Thanks for the hint, I tested the following two cases and did not see a problem:

• Test example, identical outcome as our quick and dirty fix, as I expected
• Replacing the fix in the FairRoot class by this and re-running their test-suite, this ran through successfully (so it does not break any other tested behavior of them)

So I would think that your proposal would be perfect for us, as I had anyway introduced a RefreshPhysicalNodes just before adding the call to our fix.

@fuhlig1 Do you think I should also do the full test with the original error report? (= removing the fix in FairRoot + replacing the fix in Cbmroot by this block + re-running the mCBM macro)
Or would what I already checked be enough from our side?

[PALoizeau]

The patch is in fact working quite well and one of our users just found a case where it is needed because our original solution was too coarse.

We covered it on our side (cf pinged PR on FairRoot) as our adoption of new ROOT releases is typically quite slow, but I think it would indeed be a nice feature to have it done automatically on refresh (as far as I could test it will not hurt even if for some transition period in the future we have it executed once both in ROOT and FairRoot)

[github-actions]

Hi @agheata,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely,
🤖