TGeoTessellated volumes read from GDML impossible to be closed (6.40.00 regression)

[wdconinc]

Check duplicate issues.

• Checked for duplicates

Description

In #21409, the functionality of TGeoTessellated::AddFacet was modified to remove the automatic CloseShape(check=true) when the last facet is added.

In #21409 also, an early return was added to CloseShape (if fIsClosed && fBVH) that prevents it from being run multiple times (since fIsClosed is set and BuildBVH called unconditionally).

Finally (and pre-existing), TGDMLParse::Tessellated always ends with CloseShape(false).

These three aspects conspire to prevent the closure of tessellated volumes when entered via TGDMLParse. The CloseShape(false) in TGDMLParse essentially locks the default non-closed value since CheckClosure is not called. Later CloseShape(true) calls have no effect (and worse they just silently return without action).

Reproducer

Macro (see also zipped/attached below):

#include "TGeoManager.h"
#include "TGeoTessellated.h"

void tessellated_closeshape()
{
   // Load a minimal GDML containing a closed tetrahedron (4 triangular faces).
   // TGDMLParse::Tessellated() calls CloseShape(false) after parsing, leaving
   // fClosedBody=false regardless of ROOT version.
   TGeoManager::Import("tetrahedron.gdml");

   auto *tess = dynamic_cast<TGeoTessellated *>(
      gGeoManager->GetListOfShapes()->FindObject("Tetrahedron"));

   if (!tess) {
      printf("[ERROR] Tetrahedron shape not found in tetrahedron.gdml\n");
      return;
   }

   // Call CloseShape(true) to trigger CheckClosure() on the loaded shape.
   // REGRESSION: ROOT v6.40 added `if (fIsClosed && fBVH) return` which blocks
   // this call after CloseShape(false) was already invoked by the GDML parser.
   tess->CloseShape(true);

   bool ok = tess->IsClosedBody();
   printf("[%s] CloseShape(true) after GDML load -> IsClosedBody = %d (expected 1)\n",
          ok ? "PASS" : "FAIL", ok);
}

Minimal input gdml file (see also zipped/attached below):

<?xml version="1.0"?>
<gdml xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
      xsi:noNamespaceSchemaLocation="http://service-spi.web.cern.ch/service-spi/app/releases/GDML/schema/gdml.xsd">
  <define>
    <position name="v0" x="0" y="0" z="0"  unit="cm"/>
    <position name="v1" x="10" y="0" z="0" unit="cm"/>
    <position name="v2" x="5" y="10" z="0" unit="cm"/>
    <position name="v3" x="5" y="5" z="10" unit="cm"/>
  </define>
  <materials>
    <material name="Vacuum" Z="1">
      <D unit="g/cm3" value="1e-25"/>
      <atom unit="g/mole" value="1.008"/>
    </material>
  </materials>
  <solids>
    <tessellated name="Tetrahedron">
      <triangular vertex1="v0" vertex2="v2" vertex3="v1" type="ABSOLUTE"/>
      <triangular vertex1="v0" vertex2="v1" vertex3="v3" type="ABSOLUTE"/>
      <triangular vertex1="v1" vertex2="v2" vertex3="v3" type="ABSOLUTE"/>
      <triangular vertex1="v0" vertex2="v3" vertex3="v2" type="ABSOLUTE"/>
    </tessellated>
  </solids>
  <structure>
    <volume name="World">
      <materialref ref="Vacuum"/>
      <solidref ref="Tetrahedron"/>
    </volume>
  </structure>
  <setup name="Default" version="1.0">
    <world ref="World"/>
  </setup>
</gdml>

On ROOT 6.40.00 this produces

$ root -l -q tessellated_closeshape.C
Processing tessellated_closeshape.C...
Info in <TGeoManager::Import>: Reading geometry from file: tetrahedron.gdml
Info in <TGeoManager::TGeoManager>: Geometry GDMLImport, Geometry imported from GDML created
Info in <TGeoManager::SetTopVolume>: Top volume is World. Master volume is World
Info in <TGeoNavigator::BuildCache>: --- Maximum geometry depth set to 100
Info in <TGeoManager::CheckGeometry>: Fixing runtime shapes...
Info in <TGeoManager::CheckGeometry>: ...Nothing to fix
Info in <TGeoManager::CloseGeometry>: Counting nodes...
Info in <TGeoManager::Voxelize>: Voxelizing...
Info in <TGeoManager::CloseGeometry>: Building cache...
Info in <TGeoManager::CountLevels>: max level = 1, max placements = 0
Info in <TGeoManager::CloseGeometry>: 1 nodes/ 1 volume UID's in Geometry imported from GDML
Info in <TGeoManager::CloseGeometry>: ----------------modeler ready----------------
[FAIL] CloseShape(true) after GDML load -> IsClosedBody = 0 (expected 1)

On ROOT 6.38.00 this produces:

$ root -l -q tessellated_closeshape.C

Processing tessellated_closeshape.C...
Info in <TGeoManager::Import>: Reading geometry from file: tetrahedron.gdml
Info in <TGeoManager::TGeoManager>: Geometry GDMLImport, Geometry imported from GDML created
Info in <TGeoManager::SetTopVolume>: Top volume is World. Master volume is World
Info in <TGeoNavigator::BuildCache>: --- Maximum geometry depth set to 100
Info in <TGeoManager::CheckGeometry>: Fixing runtime shapes...
Info in <TGeoManager::CheckGeometry>: ...Nothing to fix
Info in <TGeoManager::CloseGeometry>: Counting nodes...
Info in <TGeoManager::Voxelize>: Voxelizing...
Info in <TGeoManager::CloseGeometry>: Building cache...
Info in <TGeoManager::CountLevels>: max level = 1, max placements = 0
Info in <TGeoManager::CloseGeometry>: 1 nodes/ 1 volume UID's in Geometry imported from GDML
Info in <TGeoManager::CloseGeometry>: ----------------modeler ready----------------
[PASS] CloseShape(true) after GDML load -> IsClosedBody = 1 (expected 1)

tessellated_closeshape.C.zip
tetrahedron.gdml.zip

ROOT version

   -------------------------------------------------------------------------------------------------------
  | Welcome to ROOT 6.40.00                                                             https://root.cern |
  | (c) 1995-2025, The ROOT Team; conception: R. Brun, F. Rademakers                                      |
  | Built for linuxx8664gcc on May 23 2026, 20:08:51                                                      |
  | From tags/6-40-00@6-40-00                                                                             |
  | With Ubuntu clang version 23.0.0 (++20260517084559+5f2bedca745d-1~exp1~20260517204734.778) std202002  |
  | Try '.help'/'.?', '.demo', '.license', '.credits', '.quit'/'.q'                                       |
   -------------------------------------------------------------------------------------------------------

Installation method

Spack (develop)

Operating system

Linux Ubuntu 26.04

Additional context

A minimal fix would likely be to turn TGDMLParse tessellated solids into closed solids by default, i.e. tsl->CloseShape(true, true, false) (aligning with the main use case), but in a sense that just switches to the different default and may affect people who are using gdml imports otherwise.

Introducing early returns without warning for functions like CloseShape should be avoided.

The more fundamental fix is probably to revisit the PR that introduced this regression and disentangle the early return to avoid closure checks from the attempted avoidance of BVH rebuilds.

[wdconinc]

FYI @sawenzel

[dpiparo]

Thanks @wdconinc . Adding @agheata in the loop.

[sawenzel]

I understand the problem. A minimal fix could be as easy as

if (fIsClosed && fBVH && !check) {
   return;  // Skip if already closed and user isn't requesting checks
}

(so giving special attention to the check=true mode which ultimately set fBodyClosed.

However, maybe a cleaner solution would be to separate concerns: Finalization of international data structures and the checking logic. So we could for instance add another fIsDefined or fIsFullyInitialized which could just prevent to redo BVH and allowing to perform the checks. Or take out the checking from closing and offer it as a dedicated function.

[wdconinc]

However, maybe a cleaner solution would be to separate concerns

I think adding additional fIsThis or fIsThat flags looks to me like a workaround for something that should be considered at a deeper level instead. The addition of BVH building may have brought TGeoTessellated from only needing a single closed flag to needing more than just more flags.

The starting point is this behavior pattern:

• TGeoTessellated is a structure that has (and needs to have) setters that modify it after construction,
• there are expensive initialization operations (facet flipping, BVH building) that we want to do lazily on (or closer to) first use (instead of after each modification),
• after the initialization operations, we want to make sure that a future setter call either invalidates the initialization so is gets recomputed (or, maybe even better, potentially opt-in or opt-out fail with error or exception since a lazy initialization means some caller used the outputs prematurely and they are invalid for the later configuration of the object).

So, while you can separate the BVH building from closure check, they are still both part of the initialization which you want to run and re-run at some point. Multiple flags may muddy the situation a bit, and maybe the issue is not with the presence/absence of dedicated flags but with when/where the BVH is built: if the goal is lazy initialization (or thinking of the BVH as the performance-enhancing cache structure it is), then it may be better to trigger the build not in what is essentially a configuration setter (CloseShape) but trigger the build on first use (e.g. in a call_once triggered from DistFromOutside etc).

Just my 2cts, and I clearly haven't spent as much time in this class as you have.