fix(detray): allow homogeneous material on a sparse set of surfaces

Detray/core/include/detray/builders/homogeneous_material_factory.hpp

@@ -328,62 +328,63 @@ class homogeneous_material_factory final
     }
     DETRAY_DEBUG_HOST("-> Indices: " << DETRAY_LOG_VECTOR(m_indices));
 
-    // Correctly index the data in this factory
-    std::size_t sf_offset{*std::ranges::min_element(m_indices)};
-
-    DETRAY_DEBUG_HOST("-> sf_offset=" << sf_offset);
+    // Add the material to the surfaces that the data links against.
+    //
+    // The n-th entries of @c m_indices, @c m_materials, @c m_thickness and
+    // @c m_links all describe the same material instance, so the factory data
+    // is indexed by the position in that sequence, while @c m_indices holds
+    // the index of the surface the material belongs to. The surfaces that
+    // carry material therefore do not have to be contiguous in the volume.
+    for (const auto [n, i] : detray::views::enumerate(m_indices)) {
+      auto &sf_desc = surfaces.at(static_cast<dindex>(i));
 
-    // Add the material to the surfaces that the data links against
-    for (auto [i, sf] : detray::views::pick(surfaces, m_indices)) {
-      if (sf.material().id() < detector_t::material::id::e_none) {
+      if (sf_desc.material().id() < detector_t::material::id::e_none) {
         DETRAY_WARN_HOST("Surface descriptor already has a material link:"
-                         << sf.material().id()
+                         << sf_desc.material().id()
                          << " skipping configured homogeneous assignment");
         continue;
       }
 
-      std::size_t sf_idx{i - sf_offset};
-      const material<scalar_type> &mat = m_materials.at(sf_idx);
-      scalar_type t = m_thickness.at(sf_idx);
+      const material<scalar_type> &mat = m_materials.at(n);
+      scalar_type t = m_thickness.at(n);
 
-      DETRAY_DEBUG_HOST("-> Adding: sf_idx=" << sf_idx << ", i=" << i
-                                             << ", sf=" << sf);
+      DETRAY_DEBUG_HOST("-> Adding: n=" << n << ", sf_index=" << i
+                                        << ", sf=" << sf_desc);
       DETRAY_DEBUG_HOST("           mat=" << mat << " thickness=" << t);
 
       dindex mat_idx{0u};
 
       if constexpr (concepts::has_material_slabs<detector_t>) {
-        if (m_links.at(sf_idx).first == material_id::e_material_slab) {
+        if (m_links.at(n).first == material_id::e_material_slab) {
           auto &mat_coll =
               materials.template get<material_id::e_material_slab>();
 
           material_slab<scalar_type> mat_slab{mat, t};
           mat_idx = this->insert_in_container(mat_coll, mat_slab,
-                                              m_links.at(sf_idx).second);
+                                              m_links.at(n).second);
         }
       }
       if constexpr (concepts::has_material_rods<detector_t>) {
-        if (m_links.at(sf_idx).first == material_id::e_material_rod) {
+        if (m_links.at(n).first == material_id::e_material_rod) {
           auto &mat_coll =
               materials.template get<material_id::e_material_rod>();
 
           material_rod<scalar_type> mat_rod{mat, t};
           mat_idx = this->insert_in_container(mat_coll, mat_rod,
-                                              m_links.at(sf_idx).second);
+                                              m_links.at(n).second);
         }
       }
       DETRAY_DEBUG_HOST("-> After insert: mat_idx=" << mat_idx);
-      link_t new_link{m_links.at(sf_idx).first, mat_idx};
+      link_t new_link{m_links.at(n).first, mat_idx};
 
-      auto &sf_desc = surfaces.at(static_cast<dindex>(i));
       DETRAY_DEBUG_HOST("-> Existing link: " << sf_desc.material());
       DETRAY_DEBUG_HOST("-> Setting new link: " << new_link);
 
       // Set the initial surface material link (will be updated when
       // added to the detector)
       sf_desc.material() = new_link;
 
-      DETRAY_DEBUG_HOST("-> Added material to surface: " << sf);
+      DETRAY_DEBUG_HOST("-> Added material to surface: " << sf_desc);
     }
   }
 

Detray/tests/integration_tests/cpu/builders/homogeneous_material_builder.cpp

@@ -179,6 +179,85 @@ GTEST_TEST(detray_builders, decorator_homogeneous_material_builder) {
   }
 }
 
+/// Integration test: homogeneous material on a sparse subset of surfaces
+///
+/// Regression test: the material factory used to index its own data by
+/// (surface index - smallest surface index), which silently assumed that the
+/// surfaces carrying material form a contiguous block. For material on a
+/// non-contiguous set of surfaces that index ran past the end of the factory's
+/// material vector, so only detectors where every surface carried material
+/// could be built.
+GTEST_TEST(detray_builders, homogeneous_material_on_sparse_surfaces) {
+  using transform3 = typename detector_t::transform3_type;
+  using material_id = typename detector_t::material::id;
+
+  using rectangle_factory = surface_factory<detector_t, rectangle2D>;
+  using mat_factory_t = homogeneous_material_factory<detector_t>;
+
+  vecmem::host_memory_resource host_mr;
+  detector_t d(host_mr);
+  auto geo_ctx = typename detector_t::geometry_context{};
+
+  auto vbuilder =
+      std::make_unique<volume_builder<detector_t>>(volume_id::e_cylinder);
+  auto mat_builder =
+      homogeneous_material_builder<detector_t>{std::move(vbuilder)};
+
+  // Five sensitive surfaces, none of which carry material yet
+  constexpr std::size_t n_surfaces{5u};
+
+  auto rect_factory = std::make_shared<rectangle_factory>();
+  typename rectangle_factory::sf_data_collection rect_sf_data;
+  for (std::size_t i = 0u; i < n_surfaces; ++i) {
+    rect_sf_data.emplace_back(
+        surface_id::e_sensitive,
+        transform3(point3{0.f, 0.f, -10.f * static_cast<scalar>(i)}), 0u,
+        std::vector<scalar>{10.f, 8.f});
+  }
+  rect_factory->push_back(std::move(rect_sf_data));
+  mat_builder.add_surfaces(rect_factory, geo_ctx);
+
+  // Attach material to surfaces 1 and 4 only: there is a gap in between, the
+  // block does not start at the first surface and the material is passed in
+  // with explicit surface indices
+  auto mat_factory = std::make_shared<mat_factory_t>();
+  mat_factory->add_material(material_id::e_material_slab,
+                            {1.f * unit<scalar>::mm, silicon<scalar>(), 1u});
+  mat_factory->add_material(material_id::e_material_slab,
+                            {2.f * unit<scalar>::mm, tungsten<scalar>(), 4u});
+  mat_builder.add_surfaces(mat_factory, geo_ctx);
+
+  mat_builder.build(d);
+
+  // One slab per material entry: the gaps must not be padded with filler
+  EXPECT_EQ(d.surfaces().size(), n_surfaces);
+  EXPECT_EQ(d.material_store().template size<material_id::e_material_slab>(),
+            2u);
+
+  const auto &slabs =
+      d.material_store().template get<material_id::e_material_slab>();
+
+  for (const auto [idx, sf_desc] : detray::views::enumerate(d.surfaces())) {
+    const auto &mat_link = sf_desc.material();
+
+    if (idx == 1u || idx == 4u) {
+      ASSERT_EQ(mat_link.id(), material_id::e_material_slab);
+
+      const auto &slab = slabs.at(mat_link.index());
+      if (idx == 1u) {
+        EXPECT_EQ(slab.get_material(), silicon<scalar>());
+        EXPECT_NEAR(slab.thickness(), 1.f * unit<scalar>::mm, tol);
+      } else {
+        EXPECT_EQ(slab.get_material(), tungsten<scalar>());
+        EXPECT_NEAR(slab.thickness(), 2.f * unit<scalar>::mm, tol);
+      }
+    } else {
+      // Surfaces that were not given material must not have any
+      EXPECT_NE(mat_link.id(), material_id::e_material_slab);
+    }
+  }
+}
+
 /// Integration test to build an empty cuboid volume with material
 GTEST_TEST(detray_builders, detector_builder_with_material) {
   using namespace detray;

Plugins/Detray/include/ActsPlugins/Detray/DetrayGeometryConverter.hpp

@@ -14,10 +14,12 @@
 #include "ActsPlugins/Detray/DetrayConversionUtils.hpp"
 #include "ActsPlugins/Detray/DetrayPayloadConverter.hpp"
 
+#include <algorithm>
 #include <memory>
 #include <string>
 
 #include <detray/builders/detector_builder.hpp>
+#include <detray/core/concepts.hpp>
 #include <detray/io/backend/geometry_reader.hpp>
 #include <detray/io/backend/homogeneous_material_reader.hpp>
 #include <detray/io/backend/material_map_reader.hpp>
@@ -119,8 +121,44 @@ class DetrayGeometryConverter {
                                                           *payloads.detector);
 
     if (m_cfg.convertMaterial) {
-      detray::io::homogeneous_material_reader::from_payload<detector_t>(
-          detectorBuilder, *payloads.homogeneousMaterial);
+      if (!payloads.homogeneousMaterial) {
+        ACTS_DEBUG("No homogeneous material payload found, skipping");
+      } else {
+        const auto& materialVolumes = payloads.homogeneousMaterial->volumes;
+        ACTS_DEBUG("Found homogeneous material payload with "
+                   << materialVolumes.size() << " volumes");
+
+        auto hasMaterialType =
+            [&materialVolumes](detray::io::material_id type) {
+              return std::ranges::any_of(
+                  materialVolumes, [type](const auto& volume) {
+                    return std::ranges::any_of(
+                        volume.surface_mat,
+                        [type](const auto& sm) { return sm.type == type; });
+                  });
+            };
+
+        if (hasMaterialType(detray::io::material_id::slab) &&
+            !detray::concepts::has_material_slabs<detector_t>) {
+          throw std::invalid_argument(
+              "DetrayGeometryConverter: the tracking geometry contains "
+              "homogeneous material slabs, but the target Detray metadata "
+              "type does not support material slabs");
+        }
+        if (hasMaterialType(detray::io::material_id::rod) &&
+            !detray::concepts::has_material_rods<detector_t>) {
+          throw std::invalid_argument(
+              "DetrayGeometryConverter: the tracking geometry contains "
+              "homogeneous material rods, but the target Detray metadata "
+              "type does not support material rods");
+        }
+
+        if constexpr (detray::concepts::has_material_slabs<detector_t> ||
+                      detray::concepts::has_material_rods<detector_t>) {
+          detray::io::homogeneous_material_reader::from_payload<detector_t>(
+              detectorBuilder, *payloads.homogeneousMaterial);
+        }
+      }
 
       detray::io::material_map_reader<std::integral_constant<std::size_t, 2>>::
           from_payload<detector_t>(detectorBuilder,

Plugins/Detray/src/DetrayPayloadConverter.cpp

@@ -498,17 +498,6 @@ void DetrayPayloadConverter::handlePortal(
   }
 }
 
-namespace {
-
-constexpr static detray::io::surface_material_payload s_dummyMaterialSlab{
-    .type = detray::io::material_id::slab,
-    .index_in_coll = std::numeric_limits<std::size_t>::max(),
-    .thickness = 42,
-    .mat = {42, 42, 42, 42, 42, 42, 42},
-};
-
-}  // namespace
-
 std::pair<std::vector<detray::io::grid_payload<
               detray::io::surface_material_payload, detray::io::material_id>>,
           detray::io::material_volume_payload>
@@ -523,46 +512,39 @@ DetrayPayloadConverter::convertMaterial(
   detray::io::material_volume_payload homogeneous;
   homogeneous.volume_link.link = volPayload.index.link;
 
-  // @HACK: Detray does not like homoegeneous material only on SOME surfaces.
-  ACTS_INFO("Adding dummy material slabs to homogeneous collection for "
-            << volPayload.surfaces.size()
-            << " surfaces (detray "
-               "hack)");
-  for (const auto& surface : volPayload.surfaces) {
-    auto& slabPayload =
-        homogeneous.surface_mat.emplace_back(s_dummyMaterialSlab);
-    slabPayload.index_in_coll = surface.index_in_coll.value();
-    slabPayload.surface.link = surface.index_in_coll.value();
-  }
-
   std::map<std::size_t, const ISurfaceMaterial*> srfIdxToMaterial;
 
   auto assignMaterial = [&](const ISurfaceMaterial* material,
                             DetraySurfaceMaterial& detrayMaterial,
                             std::size_t srfIdx, const Surface& surface) {
     auto handleHomogeneous =
         [&](const detray::io::surface_material_payload& slab) {
-          // Given the pseudo slabs from before, we need to either update an
-          // existing slab or create a new one.
-
+          // A surface can be visited more than once (e.g. through decomposed
+          // portal links), so update an existing entry instead of duplicating.
+          //
+          // `index_in_coll` is deliberately left unset: it is the position in
+          // the detector's material collection, and forcing it to the surface
+          // index would make detray size that collection to the largest
+          // surface index and default-fill the gaps with invalid material.
+          // Leaving it unset packs the collection in payload order instead.
           auto it = std::ranges::find_if(
               homogeneous.surface_mat, [srfIdx](const auto& matslab) {
                 return matslab.surface.link == srfIdx;
               });
 
           if (it != homogeneous.surface_mat.end()) {
-            ACTS_VERBOSE("Adding slab to homogeneous material for surface "
+            ACTS_VERBOSE("Updating slab in homogeneous material for surface "
                          << srfIdx);
             auto& targetSlab = *it;
             targetSlab = slab;
-            targetSlab.index_in_coll = srfIdx;
+            targetSlab.index_in_coll.reset();
             targetSlab.surface.link = srfIdx;
           } else {
-            ACTS_VERBOSE("Updating slab in homogeneous material for surface "
+            ACTS_VERBOSE("Adding slab to homogeneous material for surface "
                          << srfIdx);
-            homogeneous.surface_mat.emplace_back(slab);
-            homogeneous.surface_mat.back().index_in_coll = srfIdx;
-            homogeneous.surface_mat.back().surface.link = srfIdx;
+            auto& newSlab = homogeneous.surface_mat.emplace_back(slab);
+            newSlab.index_in_coll.reset();
+            newSlab.surface.link = srfIdx;
           }
 
           auto sit = srfIdxToMaterial.find(srfIdx);
@@ -758,11 +740,6 @@ DetrayPayloadConverter::convertTrackingGeometry(
   payloads.detector = std::make_unique<detray::io::detector_payload>();
   detray::io::detector_payload& detPayload = *payloads.detector;
 
-  payloads.homogeneousMaterial =
-      std::make_unique<detray::io::detector_homogeneous_material_payload>();
-  detray::io::detector_homogeneous_material_payload& dthmPayload =
-      *payloads.homogeneousMaterial;
-
   payloads.materialGrids = std::make_unique<detray::io::detector_grids_payload<
       detray::io::surface_material_payload, detray::io::material_id>>();
 
@@ -854,7 +831,12 @@ DetrayPayloadConverter::convertTrackingGeometry(
       // NOTE: Currently, it'll always be populated by at least the homogeneous
       // NOTE: Volume association is internal to
       // `detray::io::material_volume_payload`
-      dthmPayload.volumes.emplace_back(std::move(homogeneous));
+      if (!payloads.homogeneousMaterial) {
+        payloads.homogeneousMaterial = std::make_unique<
+            detray::io::detector_homogeneous_material_payload>();
+      }
+      payloads.homogeneousMaterial->volumes.emplace_back(
+          std::move(homogeneous));
     }
 
     ACTS_DEBUG("Volume " << volume.volumeName()
@@ -937,7 +919,10 @@ DetrayPayloadConverter::convertTrackingGeometry(
     }
   }
 
-  {
+  if (payloads.homogeneousMaterial) {
+    ACTS_DEBUG("Adjusting homogeneous material entries after swapping");
+    auto& dthmPayload = *payloads.homogeneousMaterial;
+
     // Possibly swap homogeneous material entries in vector if they both exist
     auto find = [](std::size_t id) {
       return [id](const auto& vol) { return vol.volume_link.link == id; };
@@ -966,6 +951,8 @@ DetrayPayloadConverter::convertTrackingGeometry(
         mat.volume_link.link = beampipeIdx;
       }
     }
+  } else {
+    ACTS_DEBUG("No homogeneous material payload to adjust after swapping");
   }
 
   {