Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: GeoShapes to DetectorVolumes and GeoMaterial Converters #3268

Closed
wants to merge 37 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
5c69706
Changes to fix finding geomodel on system and first round of converter
Jun 3, 2024
daaf5b8
format changes
Jun 7, 2024
0700fc7
Changes to fix finding geomodel on system and first round of converter
Jun 3, 2024
891280f
format changes
Jun 7, 2024
15612c9
fix
Jun 7, 2024
b8712b4
small changes
Jun 7, 2024
fa55d0e
Final changes for using system geomodel
Jun 7, 2024
656c771
oops
Jun 7, 2024
b0c1a09
Merge branch 'main' into geoShapeToDetVol
Matthewharri Jun 12, 2024
1660a53
Use forward declare and move around some includes
Jun 13, 2024
a7fb341
Change to GeoModel namespace
Jun 13, 2024
53334ab
fix format
Jun 13, 2024
6b016ca
change from dynamic_cast to static_cast
Jun 17, 2024
cd12571
Address Johannes comments
Jun 28, 2024
47547d9
bump GeoModel to 6.0.0 and add GeoMaterial converter
Jul 4, 2024
c418f5b
Changes to DetVol converter
Jul 4, 2024
e6c8ac0
format fix
Jul 4, 2024
2792e65
last format fix i hope
Jul 4, 2024
fe54079
fix
Jul 4, 2024
e892533
Apply suggestions from code review
Matthewharri Jul 4, 2024
140cd91
fix GeoModelTree.hpp
Jul 4, 2024
b021387
switch from class to namespace definition for mat conv
Jul 4, 2024
b7463da
parse shape by pointer
Jul 4, 2024
35fbd02
simplify transforms
Jul 4, 2024
7c7c3c3
format yet again
Jul 4, 2024
8761e4a
move to 6.3.0 and change to returning a volume instead of vector
Jul 11, 2024
6fdc26b
Merge branch 'main' into geoShapeToDetVol
Matthewharri Jul 11, 2024
47b7235
Merge branch 'main' into geoShapeToDetVol
Matthewharri Jul 19, 2024
a052cfc
Merge branch 'main' into geoShapeToDetVol
Matthewharri Jul 30, 2024
1a5afbb
fix build issue + renaming
Aug 2, 2024
68d51a0
fix format
Aug 2, 2024
247f90c
change the version to fetch from gitlab
Aug 2, 2024
787daf5
Merge branch 'geoShapeToDetVol' of github.com:Matthewharri/acts into …
Aug 2, 2024
4a140af
Merge branch 'main' into geoShapeToDetVol
Matthewharri Aug 19, 2024
f457684
Apply suggestions from code review
Matthewharri Aug 19, 2024
d5b763a
move converison constant into function
Matthewharri Aug 19, 2024
7608689
Merge branch 'main' into geoShapeToDetVol
Matthewharri Aug 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Plugins/GeoModel/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ add_library(
SHARED
src/GeoModelBlueprintCreater.cpp
src/GeoModelConversionError.cpp
src/GeoModelToDetectorVolume.cpp
src/GeoModelReader.cpp
src/GeoModelDetectorElement.cpp
src/GeoModelDetectorSurfaceFactory.cpp
src/GeoModelMaterialConverter.cpp
src/detail/GeoBoxConverter.cpp
src/detail/GeoTrdConverter.cpp
src/detail/GeoTubeConverter.cpp
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// This file is part of the Acts project.
//
// Copyright (C) 2024 CERN for the benefit of the Acts project
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.

#pragma once
Matthewharri marked this conversation as resolved.
Show resolved Hide resolved

#include "Acts/Material/Material.hpp"

class GeoMaterial;

namespace Acts::GeoModel {

/// @brief Convert GeoMaterial to Acts::Material
///
/// @param gm The GeoMaterial to be converted
/// @return the Acts::Material
Material geoMaterialConverter(const GeoMaterial* gm,
bool useMolarDensity = true);

} // namespace Acts::GeoModel
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// This file is part of the Acts project.
//
// Copyright (C) 2024 CERN for the benefit of the Acts project
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.

#pragma once
Matthewharri marked this conversation as resolved.
Show resolved Hide resolved

#include "Acts/Detector/DetectorVolume.hpp"

#include "GeoModelKernel/GeoDefinitions.h"

class GeoShape;
Comment on lines +11 to +15
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing includes, I sense.

Required for your journey, these includes are:

  • GeometryContext, used in function signature it is
  • Transform3D from GeoTrf namespace, needed it appears

Add these includes, you must:

#include "Acts/Detector/DetectorVolume.hpp"
+#include "Acts/Geometry/GeometryContext.hpp"

#include "GeoModelKernel/GeoDefinitions.h"
+#include "GeoModelKernel/GeoTrf.h"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
#include "Acts/Detector/DetectorVolume.hpp"
#include "GeoModelKernel/GeoDefinitions.h"
class GeoShape;
#include "Acts/Detector/DetectorVolume.hpp"
#include "Acts/Geometry/GeometryContext.hpp"
#include "GeoModelKernel/GeoDefinitions.h"
#include "GeoModelKernel/GeoTrf.h"
class GeoShape;


namespace Acts::GeoModel {

/// @brief Convert a GeoModel shape to a DetectorVolume
///
/// @param shape the GeoModel shape
/// @param transform the transform to be applied
/// @return the DetectorVolume
std::shared_ptr<Experimental::DetectorVolume> convertVolume(
const GeometryContext& context, const GeoShape* shape,
const std::string& name, const GeoTrf::Transform3D transform);
Comment on lines +24 to +26
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Limited shape handling, I sense in this code.

In GeoModelToDetectorVolume.cpp, handle only these shapes, the implementation does:

  • GeoShapeUnion
  • GeoShapeSubtraction
  • GeoPcon
  • GeoShapeShift

But many more shapes in the GeoModel universe, there are. A default error throwing for unsupported shapes, I see. Dangerous this path is.

  • Missing handling for basic shapes like boxes, tubes, and trapezoids, the implementation is
  • Default error message "FATAL: Unsupported GeoModel shape" provides little guidance to young padawans
  • Consider implementing converters for all fundamental GeoModel shapes, you must
🔗 Analysis chain

Verify shape handling completeness, we must.

Ensure that all GeoModel shapes, handled they are. Search the implementation, we shall.


The search continues, young padawan. More information about shape handling, we must find. The implementation of convertVolume and shape converters, reveal they will.


Hmm, elusive the implementation is. Through different paths, search we must. The Force guides us to look deeper into the GeoModel plugin's implementation files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all GeoShape derived classes and verify implementation handles them

# Find all GeoShape derived classes
echo "=== GeoShape derived classes ==="
ast-grep --pattern 'class $_ : public GeoShape'

# Find shape handling in implementation
echo -e "\n=== Shape handling in implementation ==="
rg "dynamic_cast.*GeoShape" -A 5

Length of output: 2378


Script:

#!/bin/bash
# Search for implementation of convertVolume
echo "=== Implementation of convertVolume ==="
rg "convertVolume.*GeoShape.*transform" -A 20

# Find all shape converters
echo -e "\n=== Shape converters ==="
fd "converter" --exec rg "class.*Converter" {}

Length of output: 443


Script:

#!/bin/bash
# Search for implementation files in GeoModel plugin
echo "=== GeoModel implementation files ==="
fd "GeoModelTo.*cpp$"

# Search for shape handling in implementation
echo -e "\n=== Shape handling patterns ==="
rg "GeoShape.*\{" -A 10 --type cpp

Length of output: 9474


} // namespace Acts::GeoModel
38 changes: 38 additions & 0 deletions Plugins/GeoModel/src/GeoModelMaterialConverter.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
// This file is part of the Acts project.
//
// Copyright (C) 2024 CERN for the benefit of the Acts project
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.

#include "Acts/Plugins/GeoModel/GeoModelMaterialConverter.hpp"

#include "Acts/Material/Material.hpp"

#include "GeoModelKernel/GeoMaterial.h"
#include "GeoModelKernel/Units.h"

Acts::Material Acts::GeoModel::geoMaterialConverter(const GeoMaterial* gm,
bool useMolarDensity) {
constexpr double densityCnvFactor = 1. / GeoModelKernelUnits::gram;
double x0 = gm->getRadLength();
double l0 = gm->getIntLength();
double density = gm->getDensity() * densityCnvFactor;
double A = 0.;
Comment on lines +16 to +22
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Validate the input pointer, we must!

Dangerous, null pointers are. Check them, we should, before proceeding.

 Acts::Material Acts::GeoModel::geoMaterialConverter(const GeoMaterial* gm,
                                                     bool useMolarDensity) {
+  if (gm == nullptr) {
+    throw std::invalid_argument("Null GeoMaterial pointer provided");
+  }
   constexpr double densityCnvFactor = 1. / GeoModelKernelUnits::gram;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Acts::Material Acts::GeoModel::geoMaterialConverter(const GeoMaterial* gm,
bool useMolarDensity) {
constexpr double densityCnvFactor = 1. / GeoModelKernelUnits::gram;
double x0 = gm->getRadLength();
double l0 = gm->getIntLength();
double density = gm->getDensity() * densityCnvFactor;
double A = 0.;
Acts::Material Acts::GeoModel::geoMaterialConverter(const GeoMaterial* gm,
bool useMolarDensity) {
if (gm == nullptr) {
throw std::invalid_argument("Null GeoMaterial pointer provided");
}
constexpr double densityCnvFactor = 1. / GeoModelKernelUnits::gram;
double x0 = gm->getRadLength();
double l0 = gm->getIntLength();
double density = gm->getDensity() * densityCnvFactor;
double A = 0.;

double Z = 0.;
// Get number elements
int numberOfElements = gm->getNumElements();
// Loop
for (int iEl = 0; iEl < numberOfElements; ++iEl) {
const GeoElement* geoEl = gm->getElement(iEl);
double fraction = gm->getFraction(iEl);
A += fraction * (geoEl->getA() / GeoModelKernelUnits::gram);
Z += fraction * geoEl->getZ();
}
if (useMolarDensity) {
return Material::fromMolarDensity(x0, l0, A, Z, density);
} else {
return Material::fromMassDensity(x0, l0, A, Z, density);
}
}
168 changes: 168 additions & 0 deletions Plugins/GeoModel/src/GeoModelToDetectorVolume.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
// This file is part of the Acts project.
//
// Copyright (C) 2024 CERN for the benefit of the Acts project
//
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this
// file, You can obtain one at http://mozilla.org/MPL/2.0/.

#include "Acts/Plugins/GeoModel/GeoModelToDetectorVolume.hpp"

#include "Acts/Detector/GeometryIdGenerator.hpp"
#include "Acts/Detector/PortalGenerators.hpp"
#include "Acts/Geometry/CuboidVolumeBounds.hpp"
#include "Acts/Geometry/CutoutCylinderVolumeBounds.hpp"
#include "Acts/Geometry/CylinderVolumeBounds.hpp"
#include "Acts/Geometry/GeometryIdentifier.hpp"
#include "Acts/Geometry/TrapezoidVolumeBounds.hpp"
#include "Acts/Navigation/InternalNavigation.hpp"

#include <GeoModelKernel/GeoBox.h>
#include <GeoModelKernel/GeoPcon.h>
#include <GeoModelKernel/GeoShapeShift.h>
#include <GeoModelKernel/GeoShapeSubtraction.h>
#include <GeoModelKernel/GeoShapeUnion.h>
#include <GeoModelKernel/GeoSimplePolygonBrep.h>
#include <GeoModelKernel/GeoTrd.h>
#include <GeoModelKernel/GeoTube.h>
#include <GeoModelKernel/GeoTubs.h>

namespace Acts::GeoModel {

std::shared_ptr<Experimental::DetectorVolume> convertVolume(
const GeometryContext& context, const GeoShape* shape,
const std::string& name, const GeoTrf::Transform3D transform) {
auto portalGenerator = Experimental::defaultPortalAndSubPortalGenerator();
if (shape->typeID() == GeoTube::getClassTypeID()) {
const GeoTube* tube = static_cast<const GeoTube*>(shape);
std::shared_ptr<CylinderVolumeBounds> bounds =
std::make_shared<CylinderVolumeBounds>(tube->getRMin(), tube->getRMax(),
tube->getZHalfLength());
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, transform, bounds,
Experimental::tryAllPortalsAndSurfaces());
} else if (shape->typeID() == GeoTubs::getClassTypeID()) {
const GeoTubs* tubs = static_cast<const GeoTubs*>(shape);
std::shared_ptr<CylinderVolumeBounds> bounds =
std::make_shared<CylinderVolumeBounds>(tubs->getRMin(), tubs->getRMax(),
tubs->getZHalfLength(),
tubs->getDPhi() / 2);
GeoTrf::Transform3D newTransform =
transform * GeoTrf::RotateZ3D(tubs->getSPhi() + 0.5 * tubs->getDPhi());
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, newTransform, bounds,
Experimental::tryAllPortalsAndSurfaces());
} else if (shape->typeID() == GeoBox::getClassTypeID()) {
const GeoBox* box = static_cast<const GeoBox*>(shape);
std::shared_ptr<CuboidVolumeBounds> bounds =
std::make_shared<CuboidVolumeBounds>(box->getXHalfLength(),
box->getYHalfLength(),
box->getZHalfLength());
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, transform, bounds,
Experimental::tryAllPortalsAndSurfaces());
} else if (shape->typeID() == GeoSimplePolygonBrep::getClassTypeID()) {
const GeoSimplePolygonBrep* brep =
static_cast<const GeoSimplePolygonBrep*>(shape);
double xmin{0}, xmax{0}, ymin{0}, ymax{0}, zmin{0}, zmax{0};
brep->extent(xmin, ymin, zmin, xmax, ymax, zmax);
std::shared_ptr<CuboidVolumeBounds> bounds =
std::make_shared<CuboidVolumeBounds>(
(xmax - xmin) / 2, (ymax - ymin) / 2, (zmax - zmin) / 2);
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, transform, bounds,
Experimental::tryAllPortalsAndSurfaces());
Comment on lines +67 to +74
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Refactor the duplicated code, you should.

Duplicate code for calculating CuboidVolumeBounds from shape extents, I see. A helper function, create, to improve maintainability and reduce repetition.

} else if (shape->typeID() == GeoTrd::getClassTypeID()) {
const GeoTrd* trd = static_cast<const GeoTrd*>(shape);
double x1 = trd->getXHalfLength1();
double x2 = trd->getXHalfLength2();
double y1 = trd->getYHalfLength1();
double y2 = trd->getYHalfLength2();
double z = trd->getZHalfLength();
if (y1 == y2) {
if (x1 <= x2) {
// y axis in ACTS is z axis in geomodel
std::shared_ptr<TrapezoidVolumeBounds> bounds =
std::make_shared<TrapezoidVolumeBounds>(x1, x2, z, y1);
constexpr double rotationAngle = M_PI / 2;
GeoTrf::Transform3D newTransform =
transform * GeoTrf::RotateX3D(rotationAngle);
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, newTransform, bounds,
Experimental::tryAllPortalsAndSurfaces());
} else {
std::shared_ptr<TrapezoidVolumeBounds> bounds =
std::make_shared<TrapezoidVolumeBounds>(x2, x1, z, y1);
constexpr double rotationAngle = M_PI;
GeoTrf::Transform3D newTransform = transform *
GeoTrf::RotateY3D(rotationAngle) *
GeoTrf::RotateZ3D(rotationAngle);
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, newTransform, bounds,
Experimental::tryAllPortalsAndSurfaces());
}
} else if (x1 == x2) {
if (y1 < y2) {
std::shared_ptr<TrapezoidVolumeBounds> bounds =
std::make_shared<TrapezoidVolumeBounds>(y1, y2, z, x1);
auto rotationAngle = M_PI / 2;
GeoTrf::Transform3D newTransform = transform *
GeoTrf::RotateZ3D(rotationAngle) *
GeoTrf::RotateX3D(rotationAngle);
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, newTransform, bounds,
Experimental::tryAllPortalsAndSurfaces());
} else {
std::shared_ptr<TrapezoidVolumeBounds> bounds =
std::make_shared<TrapezoidVolumeBounds>(y2, y1, z, x1);
auto rotationAngle = M_PI;
GeoTrf::Transform3D newTransform =
transform * GeoTrf::RotateX3D(rotationAngle) *
GeoTrf::RotateZ3D(rotationAngle / 2) *
GeoTrf::RotateX3D(rotationAngle / 2);
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, newTransform, bounds,
Experimental::tryAllPortalsAndSurfaces());
}
} else {
throw std::runtime_error("FATAL: Translating GeoTrd to ACTS failed");
}
} else if (shape->typeID() == GeoShapeUnion::getClassTypeID()) {
const GeoShapeUnion* unionShape = static_cast<const GeoShapeUnion*>(shape);
double xmin{0}, xmax{0}, ymin{0}, ymax{0}, zmin{0}, zmax{0};
unionShape->extent(xmin, ymin, zmin, xmax, ymax, zmax);
std::shared_ptr<CuboidVolumeBounds> bounds =
std::make_shared<CuboidVolumeBounds>(
(xmax - xmin) / 2, (ymax - ymin) / 2, (zmax - zmin) / 2);
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, transform, bounds,
Experimental::tryAllPortalsAndSurfaces());
} else if (shape->typeID() == GeoShapeSubtraction::getClassTypeID()) {
// Go down the left side (opA) of the subtraction until we reach a normal
// shape
const GeoShapeSubtraction* subtractionShape =
static_cast<const GeoShapeSubtraction*>(shape);
const GeoShape* shapeA = subtractionShape->getOpA();
return convertVolume(context, shapeA, name, transform);
} else if (shape->typeID() == GeoPcon::getClassTypeID()) {
// Will change in future, get bounding box for now
double xmin{0}, xmax{0}, ymin{0}, ymax{0}, zmin{0}, zmax{0};
const GeoPcon* pcon = static_cast<const GeoPcon*>(shape);
pcon->extent(xmin, ymin, zmin, xmax, ymax, zmax);
std::shared_ptr<CuboidVolumeBounds> bounds =
std::make_shared<CuboidVolumeBounds>(
(xmax - xmin) / 2, (ymax - ymin) / 2, (zmax - zmin) / 2);
return Experimental::DetectorVolumeFactory::construct(
portalGenerator, context, name, transform, bounds,
Experimental::tryAllPortalsAndSurfaces());
}
if (shape->typeID() == GeoShapeShift::getClassTypeID()) {
const GeoShapeShift* shiftShape = static_cast<const GeoShapeShift*>(shape);
const GeoShape* shapeOp = shiftShape->getOp();
GeoTrf::Transform3D newTransform = transform * shiftShape->getX();
return convertVolume(context, shapeOp, name, newTransform);
}
throw std::runtime_error("FATAL: Unsupported GeoModel shape");
}

} // namespace Acts::GeoModel
Loading