From e5d51fe5bf201419a6c8338978061710d23f9bb4 Mon Sep 17 00:00:00 2001
From: Markus Frank <Markus.Frank@cern.ch>
Date: Tue, 13 Feb 2024 17:14:08 +0100
Subject: [PATCH] Fix DD4hep_DetectorDump plugin to avoid crash. Add test to
 check if DetElements have proper placement set

---
 DDCore/src/plugins/StandardPlugins.cpp        |  70 +++++------
 examples/ClientTests/CMakeLists.txt           |   9 ++
 .../ClientTests/compact/MiniTel_err_place.xml | 110 ++++++++++++++++++
 examples/ClientTests/src/MiniTel.cpp          |  11 +-
 4 files changed, 165 insertions(+), 35 deletions(-)
 create mode 100644 examples/ClientTests/compact/MiniTel_err_place.xml

diff --git a/DDCore/src/plugins/StandardPlugins.cpp b/DDCore/src/plugins/StandardPlugins.cpp
index 78fff1386..920e7365f 100644
--- a/DDCore/src/plugins/StandardPlugins.cpp
+++ b/DDCore/src/plugins/StandardPlugins.cpp
@@ -1480,15 +1480,17 @@ template <int flag> long dump_detelement_tree(Detector& description, int argc, c
       }
     }
     IDDescriptor get_id_descriptor(PlacedVolume pv)   {
-      Volume v = pv.volume();
-      SensitiveDetector sd = v.sensitiveDetector();
-      if ( sd.isValid() )   {
-        IDDescriptor dsc = sd.readout().idSpec();
-        if ( dsc.isValid() ) return dsc;
-      }
-      for (Int_t idau = 0, ndau = v->GetNdaughters(); idau < ndau; ++idau)  {
-        IDDescriptor dsc = get_id_descriptor(v->GetNode(idau));
-        if ( dsc.isValid() ) return dsc;
+      if ( pv.isValid() )  {
+        Volume v = pv.volume();
+        SensitiveDetector sd = v.sensitiveDetector();
+        if ( sd.isValid() )   {
+          IDDescriptor dsc = sd.readout().idSpec();
+          if ( dsc.isValid() ) return dsc;
+        }
+        for (Int_t idau = 0, ndau = v->GetNdaughters(); idau < ndau; ++idau)  {
+          IDDescriptor dsc = get_id_descriptor(v->GetNode(idau));
+          if ( dsc.isValid() ) return dsc;
+        }
       }
       return IDDescriptor();
     }
@@ -1498,51 +1500,53 @@ template <int flag> long dump_detelement_tree(Detector& description, int argc, c
     }
 
     long dump(DetElement de, int level, IDDescriptor& id_desc, VolIDs chain)   {
+      char fmt[256];
       PlacedVolume place = de.placement();
       const DetElement::Children& children = de.children();
-      bool  use_elt = path.empty() || de.path().find(path) != std::string::npos;
+      bool use_elt = path.empty() || de.path().find(path) != std::string::npos;
 
       if ( have_match < 0 && use_elt )  {
         have_match = level;
       }
-      use_elt = use_elt && ((level-have_match) <= analysis_level);
-      if ( de != det_world )    {
+
+      use_elt &= ((level-have_match) <= analysis_level);
+      if ( !place.isValid() )    {
+        std::snprintf(fmt,sizeof(fmt),"%03d %%-%ds %%s DetElement with INVALID PLACEMENT!", level+1, 2*level+1);
+        printout(ERROR,"DetectorDump", fmt, "", de.path().c_str());
+        use_elt = false;
+      }
+
+      if ( place.isValid() && de != det_world )    {
         chain.insert(chain.end(), place.volIDs().begin(), place.volIDs().end());
       }
       if ( use_elt )   {
         if ( !sensitive_only || 0 != de.volumeID() )  {
           char sens = place.isValid() && place.volume().isSensitive() ? 'S' : ' ';
-          char fmt[128];
-          switch(flag)  {
+          switch( flag )  {
           case 0:
             ++count;
             if ( de.placement() == de.idealPlacement() )  {
-              std::snprintf(fmt,sizeof(fmt),"%03d %%-%ds %%s NumDau:%%d VolID:%%08X Place:%%p  %%c",level+1,2*level+1);
-              printout(INFO,"DetectorDump",fmt,"",de.path().c_str(),int(children.size()),
+              std::snprintf(fmt, sizeof(fmt), "%03d %%-%ds %%s NumDau:%%d VolID:%%08X Place:%%p  %%c", level+1, 2*level+1);
+              printout(INFO, "DetectorDump", fmt, "", de.path().c_str(), int(children.size()),
                        (unsigned long)de.volumeID(), (void*)place.ptr(), sens);
               break;
             }
-            std::snprintf(fmt,sizeof(fmt),"%03d %%-%ds %%s NumDau:%%d VolID:%%08X Place:%%p [ideal:%%p aligned:%%p]  %%c",
-                          level+1,2*level+1);
-            printout(INFO,"DetectorDump",fmt,"",de.path().c_str(),int(children.size()),
+            std::snprintf(fmt, sizeof(fmt), "%03d %%-%ds %%s NumDau:%%d VolID:%%08X Place:%%p [ideal:%%p aligned:%%p]  %%c",
+                          level+1, 2*level+1);
+            printout(INFO, "DetectorDump", fmt, "", de.path().c_str(), int(children.size()),
                      (unsigned long)de.volumeID(), (void*)de.idealPlacement().ptr(),
                      (void*)place.ptr(), sens);
             break;
           case 1:
             ++count;
-            std::snprintf(fmt,sizeof(fmt),
-                          "%03d %%-%ds Detector: %%s NumDau:%%d VolID:%%p",
-                          level+1,2*level+1);
-            printout(INFO,"DetectorDump", fmt, "", de.path().c_str(), int(children.size()), (void*)de.volumeID());
+            std::snprintf(fmt, sizeof(fmt), "%03d %%-%ds Detector: %%s NumDau:%%d VolID:%%p", level+1, 2*level+1);
+            printout(INFO, "DetectorDump", fmt, "", de.path().c_str(), int(children.size()), (void*)de.volumeID());
             if ( de.placement() == de.idealPlacement() )  {
-              std::snprintf(fmt,sizeof(fmt),
-                            "%03d %%-%ds Placement: %%s  %%c",
-                            level+1,2*level+3);
+              std::snprintf(fmt, sizeof(fmt), "%03d %%-%ds Placement: %%s  %%c", level+1, 2*level+3);
               printout(INFO,"DetectorDump",fmt,"", de.placementPath().c_str(), sens);
               break;
             }
-            std::snprintf(fmt,sizeof(fmt),
-                          "%03d %%-%ds Placement: %%s  [ideal:%%p aligned:%%p] %%c",
+            std::snprintf(fmt,sizeof(fmt), "%03d %%-%ds Placement: %%s  [ideal:%%p aligned:%%p] %%c",
                           level+1,2*level+3);
             printout(INFO,"DetectorDump",fmt,"", de.placementPath().c_str(),
                      (void*)de.idealPlacement().ptr(), (void*)place.ptr(), sens);          
@@ -1591,11 +1595,11 @@ template <int flag> long dump_detelement_tree(Detector& description, int argc, c
       return 1;
     }
   };
-  VolIDs chain;
-  IDDescriptor id_desc;
-  Actor  a(description);
-  a.parse_args(argc, argv);
-  return a.dump(description.world(), 0, id_desc, chain);
+  VolIDs       chain;
+  IDDescriptor id_descriptor;
+  Actor        actor(description);
+  actor.parse_args(argc, argv);
+  return actor.dump(description.world(), 0, id_descriptor, chain);
 }
 DECLARE_APPLY(DD4hep_DetectorDump,dump_detelement_tree<0>)
 DECLARE_APPLY(DD4hep_DetectorVolumeDump,dump_detelement_tree<1>)
diff --git a/examples/ClientTests/CMakeLists.txt b/examples/ClientTests/CMakeLists.txt
index f60fe548a..3db078a03 100644
--- a/examples/ClientTests/CMakeLists.txt
+++ b/examples/ClientTests/CMakeLists.txt
@@ -374,6 +374,15 @@ foreach (test BoxTrafos CaloEndcapReflection IronCylinder MiniTel SiliconBlock N
 endforeach()
 #
 #
+# Test Minitel for missing DetElement placements
+dd4hep_add_test_reg( MiniTel_check_missing_placements
+  COMMAND    "${CMAKE_INSTALL_PREFIX}/bin/run_test_ClientTests.sh"
+  EXEC_ARGS  geoPluginRun -input ${ClientTestsEx_INSTALL}/compact/MiniTel_err_place.xml
+              -plugin DD4hep_DetectorDump --volids --shapes --materials --positions
+  REGEX_PASS "ERROR 004         /MyLHCBdetector1/side_0/module_9 DetElement with INVALID PLACEMENT"
+  REGEX_FAIL "Exception;EXCEPTION"
+)
+#
 # Checksum test of the Minitel3 sub-detector
 dd4hep_add_test_reg( MiniTel_check_checksum_Minitel3
   COMMAND    "${CMAKE_INSTALL_PREFIX}/bin/run_test_CLICSiD.sh"
diff --git a/examples/ClientTests/compact/MiniTel_err_place.xml b/examples/ClientTests/compact/MiniTel_err_place.xml
new file mode 100644
index 000000000..be9b6e862
--- /dev/null
+++ b/examples/ClientTests/compact/MiniTel_err_place.xml
@@ -0,0 +1,110 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<lccdd>
+<!-- #==========================================================================
+     #  AIDA Detector description implementation 
+     #==========================================================================
+     # Copyright (C) Organisation europeenne pour la Recherche nucleaire (CERN)
+     # All rights reserved.
+     #
+     # For the licensing terms see $DD4hepINSTALL/LICENSE.
+     # For the list of contributors see $DD4hepINSTALL/doc/CREDITS.
+     #
+     #==========================================================================
+-->
+
+  <info name="Sensor"
+	title="Sensor for New experiment"
+        author="Anastasia Karachaliou"
+        status="development"
+        url="/afs/cern.ch/user/a/akaracha/workspace/MyExperiment/DetDesc/xmlDDescr/geometry_myexper.xml"
+        version= "v0r1">
+    <comment>simple Detector as a small box</comment>
+  </info>
+
+  <includes>
+    <gdmlFile  ref="${DD4hepINSTALL}/DDDetectors/compact/elements.xml"/>
+    <gdmlFile  ref="${DD4hepINSTALL}/DDDetectors/compact/materials.xml"/>
+  </includes>
+
+  <define>
+    <constant name="world_side"             value="2*m"/>
+    <constant name="world_x"                value="world_side/2"/>
+    <constant name="world_y"                value="world_side/2"/>
+    <constant name="world_z"                value="world_side/2"/>
+    <constant name="CrossingAngle"          value="0.020"/>
+  </define>
+
+  <materials>
+  </materials>
+
+
+  <display>
+    <vis name="DetVis"  alpha="1.0"   r="0"   g="1.0" b="0.0"  showDaughters="true"  visible="true"/>
+    <vis name="ModVis"  alpha="1.0"   r="1"   g="0.0" b="0.0"  showDaughters="true"  visible="false"/>
+  </display>
+
+  <detectors>
+    <detector name="MyLHCBdetector1" type="MiniTelPixel" material="Silicon" vis="DetVis" id ="1" sensitive="true" readout="MyLHCBdetector1Hits" limits="minitel_limits_1" region="minitel_region_1">
+
+      <dimensions z="1*mm" y="10*cm" x="10*cm" />
+      <module_position z="30*mm" y="0*cm" x="0*cm" />
+      <module_position z="40*mm" y="0*cm" x="0*cm" />
+      <module_position z="50*mm" y="0*cm" x="0*cm" />
+      <module_position z="60*mm" y="0*cm" x="0*cm" />
+      <module_position z="70*mm" y="0*cm" x="0*cm" />
+      <module_position z="80*mm" y="0*cm" x="0*cm" />
+      <module_position z="90*mm" y="0*cm" x="0*cm" />
+      <module_position z="100*mm" y="0*cm" x="0*cm" />
+      <module_position z="110*mm" y="0*cm" x="0*cm" />
+      <module_position z="120*mm" y="0*cm" x="0*cm" />
+      <module name="pixel" type="MiniTelPixel" material="Silicon" x="6*mm" y="6*mm" z="1*mm" vis="ModVis" alpha="-2.*radian" beta="-2.*radian" gamma="-0.*radian" />
+      <missing_module_placements number="2"/>
+    </detector>
+  </detectors>
+
+  <limits>
+    <limitset name="minitel_limits_1">
+      <limit name="step_length_max" particles="e[+-]" value="1.0" unit="mm" />
+      <limit name="step_length_max" particles="mu[+-]" value="3.0" unit="mm" />
+      <limit name="step_length_max" particles="*" value="5.0" unit="mm" />
+      <limit name="track_length_max" particles="*" value="5.0" unit="mm" />
+      <limit name="time_max"         particles="*" value="5.0" unit="ns" />
+      <limit name="ekin_min"         particles="*" value="0.01" unit="MeV" />
+      <limit name="range_min"        particles="*" value="5.0" unit="mm" />
+      <cut   particles="e+"          value="2.0"   unit="mm" />
+      <cut   particles="e-"          value="2.0"   unit="mm" />
+      <cut   particles="gamma"       value="5.0"   unit="mm" />
+    </limitset>
+    <limitset name="minitel_limits">
+      <limit name="step_length_max" particles="e[+-]" value="1.0" unit="mm" />
+      <limit name="step_length_max" particles="mu[+-]" value="3.0" unit="mm" />
+      <limit name="step_length_max" particles="*" value="5.0" unit="mm" />
+    </limitset>
+  </limits>
+
+  <regions>
+    <region name="minitel_region_1" eunit="MeV" lunit="mm" cut="0.001" threshold="0.001">
+      <limitsetref name="minitel_limits_1"/>
+    </region>
+    <region name="minitel_region" eunit="MeV" lunit="mm" cut="0.001" threshold="0.001">
+      <limitsetref name="minitel_limits"/>
+    </region>
+  </regions>
+
+  <readouts>
+    <readout name="MyLHCBdetector1Hits">
+      <segmentation type="CartesianGridXY" grid_size_x="6*mm" grid_size_y="6*mm" />
+      <id>system:6,side:2,module:8,x:28:-12,y:52:-12</id>
+    </readout>
+  </readouts>
+
+  <fields>
+    <field name="GlobalSolenoid" type="solenoid" 
+	   inner_field="0.0*tesla"
+	   outer_field="0.0*tesla" 
+	   zmax="2*m"
+	   outer_radius="2*m">
+    </field>
+  </fields>
+
+</lccdd>
diff --git a/examples/ClientTests/src/MiniTel.cpp b/examples/ClientTests/src/MiniTel.cpp
index d1231c5df..ea5ae7c49 100644
--- a/examples/ClientTests/src/MiniTel.cpp
+++ b/examples/ClientTests/src/MiniTel.cpp
@@ -77,10 +77,9 @@ static Ref_t create_detector(Detector &description, xml_h e, SensitiveDetector s
   if ( x_det.isSensitive() ) {
     sens.setType("tracker");
   }
-  int count = 0;
   PlacedVolume pv;
   DetElement   side_det;
-  double       epsilon = 1e-10; 
+  double       epsilon = 1e-10;
   Position     side_pos(0,0,30*dd4hep::mm);
   Position     env_dim_min(sensor_box.x()+epsilon, sensor_box.y()+epsilon, +100000.0);
   Position     env_dim_max(sensor_box.x()+epsilon, sensor_box.y()+epsilon, -100000.0);
@@ -114,14 +113,22 @@ static Ref_t create_detector(Detector &description, xml_h e, SensitiveDetector s
     printout(ALWAYS,"","side_pos = %f",side_pos.z());
   }
 
+  Unicode tag("missing_module_placements");
+  xml_dim_t miss = x_det.child(tag, false);
+  int missing_placement = miss ? miss.number() : 0;
   pv = assembly.placeVolume(side_vol, side_pos);
   pv.addPhysVolID("side",0);
   side_det.setPlacement(pv);
+  int count = 0;
   for( xml_coll_t mpos(x_det, _U(module_position)); mpos; mpos++ )    {
     xml_comp_t x_pos = mpos;
     DetElement module(side_det, _toString(count, "module_%d"), count);
     pv = side_vol.placeVolume(sensor_vol,Transform3D(Position(x_pos.x(),x_pos.y(),x_pos.z())));
     pv.addPhysVolID("module", ++count);
+    if ( missing_placement > 0 && count%missing_placement == 0 )   {
+      printout(WARNING,"","Drop placement of DetElement: %s", module.path().c_str());
+      continue;
+    }
     module.setPlacement(pv);
   }
 
-- 
GitLab