diff --git a/Detray/core/include/detray/builders/homogeneous_material_factory.hpp b/Detray/core/include/detray/builders/homogeneous_material_factory.hpp index 8cfacb66bee..96630cbefa4 100644 --- a/Detray/core/include/detray/builders/homogeneous_material_factory.hpp +++ b/Detray/core/include/detray/builders/homogeneous_material_factory.hpp @@ -328,54 +328,55 @@ 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(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 &mat = m_materials.at(sf_idx); - scalar_type t = m_thickness.at(sf_idx); + const material &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) { - 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_slab 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) { - 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_rod 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(i)); DETRAY_DEBUG_HOST("-> Existing link: " << sf_desc.material()); DETRAY_DEBUG_HOST("-> Setting new link: " << new_link); @@ -383,7 +384,7 @@ class homogeneous_material_factory final // 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); } } diff --git a/Detray/tests/integration_tests/cpu/builders/homogeneous_material_builder.cpp b/Detray/tests/integration_tests/cpu/builders/homogeneous_material_builder.cpp index 6b210f0f397..89933d6b5e4 100644 --- a/Detray/tests/integration_tests/cpu/builders/homogeneous_material_builder.cpp +++ b/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; + using mat_factory_t = homogeneous_material_factory; + + 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_id::e_cylinder); + auto mat_builder = + homogeneous_material_builder{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(); + 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(i)}), 0u, + std::vector{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->add_material(material_id::e_material_slab, + {1.f * unit::mm, silicon(), 1u}); + mat_factory->add_material(material_id::e_material_slab, + {2.f * unit::mm, tungsten(), 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(), + 2u); + + const auto &slabs = + d.material_store().template get(); + + 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()); + EXPECT_NEAR(slab.thickness(), 1.f * unit::mm, tol); + } else { + EXPECT_EQ(slab.get_material(), tungsten()); + EXPECT_NEAR(slab.thickness(), 2.f * unit::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; diff --git a/Plugins/Detray/include/ActsPlugins/Detray/DetrayGeometryConverter.hpp b/Plugins/Detray/include/ActsPlugins/Detray/DetrayGeometryConverter.hpp index f9224d4c055..6298831943f 100644 --- a/Plugins/Detray/include/ActsPlugins/Detray/DetrayGeometryConverter.hpp +++ b/Plugins/Detray/include/ActsPlugins/Detray/DetrayGeometryConverter.hpp @@ -14,10 +14,12 @@ #include "ActsPlugins/Detray/DetrayConversionUtils.hpp" #include "ActsPlugins/Detray/DetrayPayloadConverter.hpp" +#include #include #include #include +#include #include #include #include @@ -119,8 +121,44 @@ class DetrayGeometryConverter { *payloads.detector); if (m_cfg.convertMaterial) { - detray::io::homogeneous_material_reader::from_payload( - 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) { + 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) { + 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 || + detray::concepts::has_material_rods) { + detray::io::homogeneous_material_reader::from_payload( + detectorBuilder, *payloads.homogeneousMaterial); + } + } detray::io::material_map_reader>:: from_payload(detectorBuilder, diff --git a/Plugins/Detray/src/DetrayPayloadConverter.cpp b/Plugins/Detray/src/DetrayPayloadConverter.cpp index 9edef1dfe65..a1824d69783 100644 --- a/Plugins/Detray/src/DetrayPayloadConverter.cpp +++ b/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::max(), - .thickness = 42, - .mat = {42, 42, 42, 42, 42, 42, 42}, -}; - -} // namespace - std::pair>, detray::io::material_volume_payload> @@ -523,18 +512,6 @@ 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 srfIdxToMaterial; auto assignMaterial = [&](const ISurfaceMaterial* material, @@ -542,27 +519,32 @@ DetrayPayloadConverter::convertMaterial( 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& detPayload = *payloads.detector; - payloads.homogeneousMaterial = - std::make_unique(); - detray::io::detector_homogeneous_material_payload& dthmPayload = - *payloads.homogeneousMaterial; - payloads.materialGrids = std::make_unique>(); @@ -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"); } { diff --git a/Tests/UnitTests/Plugins/Detray/DetrayPayloadConverterTests.cpp b/Tests/UnitTests/Plugins/Detray/DetrayPayloadConverterTests.cpp index b6f1d935a1c..1cb80a09de0 100644 --- a/Tests/UnitTests/Plugins/Detray/DetrayPayloadConverterTests.cpp +++ b/Tests/UnitTests/Plugins/Detray/DetrayPayloadConverterTests.cpp @@ -37,6 +37,7 @@ #include #include +#include #include #include @@ -603,6 +604,7 @@ BOOST_AUTO_TEST_CASE(DetrayTrackingGeometryConversionTests) { auto payloads = converter.convertTrackingGeometry(gctx, *tGeometry); const auto& detector = *payloads.detector; + BOOST_REQUIRE_NE(payloads.homogeneousMaterial, nullptr); const auto& homogeneousMaterial = *payloads.homogeneousMaterial; auto& materialGrids = *payloads.materialGrids; const auto& surfaceGrids = *payloads.surfaceGrids; @@ -618,26 +620,36 @@ BOOST_AUTO_TEST_CASE(DetrayTrackingGeometryConversionTests) { BOOST_CHECK_EQUAL(detector.volumes.at(4).name, "L2"); BOOST_CHECK_EQUAL(detector.volumes.at(5).name, "L3"); - // @HACK: At this time, the conversion introduces a number of dummy material slabs which - // should ultimately not be there. + // Homogeneous material is sparse: only volumes that actually carry it are + // registered, and within them only the surfaces that have it. + BOOST_CHECK(!homogeneousMaterial.volumes.empty()); + BOOST_CHECK_LE(homogeneousMaterial.volumes.size(), detector.volumes.size()); - BOOST_CHECK_EQUAL(homogeneousMaterial.volumes.size(), 6); - - for (const auto& volume : detector.volumes) { - auto it = std::ranges::find_if( - homogeneousMaterial.volumes, [&](const auto& hMat) { - return hMat.volume_link.link == volume.index.link; + for (const auto& hMat : homogeneousMaterial.volumes) { + auto volIt = + std::ranges::find_if(detector.volumes, [&](const auto& volume) { + return volume.index.link == hMat.volume_link.link; }); - if (it == homogeneousMaterial.volumes.end()) { - BOOST_FAIL("No material slab found for volume: " + volume.name); - continue; - } + BOOST_REQUIRE(volIt != detector.volumes.end()); + const auto& volume = *volIt; - auto& hMat = *it; + // A volume is only registered when it has material to contribute + BOOST_CHECK(!hMat.surface_mat.empty()); - // Currently, we expect exactly one slab per surface! - BOOST_CHECK_EQUAL(volume.surfaces.size(), hMat.surface_mat.size()); + // At most one material entry per surface, never more + BOOST_CHECK_LE(hMat.surface_mat.size(), volume.surfaces.size()); + + std::set seenSurfaces; + for (const auto& slab : hMat.surface_mat) { + // Every entry points at a surface that exists in this volume... + BOOST_CHECK_LT(slab.surface.link, volume.surfaces.size()); + // ...and does so exactly once + BOOST_CHECK(seenSurfaces.insert(slab.surface.link).second); + // The position in the detector material collection is left to the + // reader, so that it packs the collection densely + BOOST_CHECK(!slab.index_in_coll.has_value()); + } } BOOST_CHECK_EQUAL(materialGrids.grids.size(), 2);