PR feedback

Core/include/Acts/Geometry/Blueprint.hpp

@@ -46,9 +46,10 @@ class GeometryContext;
 ///
 /// The construction phases are documented in @c BlueprintNode, which is the
 /// base class for all nodes in the tree.
-/// @note This class inherits from @c BlueprintNode privately, because it is
-///       only ever intended to be the top-level node, and not anywhere else in
-///       the tree.
+/// @note This class inherits from @c BlueprintNode, but hides the main
+///       blueprint construction phase overloads. The @c Blueprint class is
+///       only ever intended to be the top-level node, and not anywhere else
+///       in the tree.
 class Blueprint : public BlueprintNode {
  public:
   struct Config {

Core/include/Acts/Geometry/BlueprintNode.hpp

@@ -48,9 +48,6 @@ class LayerBlueprintNode;
 /// carried out.
 class BlueprintNode {
  public:
-  /// Can be default constructed
-  BlueprintNode() = default;
-
   /// Virtual destructor to ensure correct cleanup
   virtual ~BlueprintNode() = default;
 
@@ -179,11 +176,11 @@ class BlueprintNode {
   /// overload will invoke the constructor of @ref Acts::TrackingVolume and use
   /// that volume to create the node.
   /// @param transform The transform of the volume
-  /// @param volbounds The bounds of the volume
+  /// @param volumeBounds The bounds of the volume
   /// @param volumeName The name of the volume
   /// @param callback An optional callback that receives the node as an argument
   StaticBlueprintNode& addStaticVolume(
-      const Transform3& transform, std::shared_ptr<VolumeBounds> volbounds,
+      const Transform3& transform, std::shared_ptr<VolumeBounds> volumeBounds,
       const std::string& volumeName = "undefined",
       const std::function<void(StaticBlueprintNode& cylinder)>& callback = {});
 
@@ -256,11 +253,11 @@ class BlueprintNode {
 
   /// Print the node tree starting from this node to graphviz format
   /// @param os The stream to print to
-  void graphViz(std::ostream& os) const;
+  void graphviz(std::ostream& os) const;
 
   /// Method that writes a representatiohn of **this node only** to graphviz.
   /// This should generally not be called on its own, but through the @ref
-  /// BlueprintNode::graphViz method.
+  /// BlueprintNode::graphviz method.
   /// @param os The stream to print to
   virtual void addToGraphviz(std::ostream& os) const;
 
@@ -294,4 +291,5 @@ class BlueprintNode {
   std::size_t m_depth{0};
   std::vector<std::shared_ptr<BlueprintNode>> m_children{};
 };
-};  // namespace Acts
+
+}  // namespace Acts

Core/include/Acts/Geometry/BlueprintOptions.hpp

@@ -20,8 +20,6 @@ struct BlueprintOptions {
 
   void validate() const;
 
-  ~BlueprintOptions();
-
  private:
   static std::unique_ptr<NavigationPolicyFactory>
   makeDefaultNavigationPolicyFactory();

Core/include/Acts/Geometry/CylinderContainerBlueprintNode.hpp

@@ -28,7 +28,7 @@ namespace Acts {
 ///       if this is not the case.
 class CylinderContainerBlueprintNode final : public BlueprintNode {
  public:
-  /// Main constructor for t he cylinder container node.
+  /// Main constructor for the cylinder container node.
   /// @param name The name of the node (for debug only)
   /// @param direction The stacking direction
   /// @param attachmentStrategy The attachment strategy for the stack
@@ -136,11 +136,13 @@ class CylinderContainerBlueprintNode final : public BlueprintNode {
 
   std::string m_name;
 
-  BinningValue m_direction;
+  BinningValue m_direction = BinningValue::binZ;
 
-  CylinderVolumeStack::AttachmentStrategy m_attachmentStrategy;
+  CylinderVolumeStack::AttachmentStrategy m_attachmentStrategy{
+      CylinderVolumeStack::AttachmentStrategy::Midpoint};
 
-  CylinderVolumeStack::ResizeStrategy m_resizeStrategy;
+  CylinderVolumeStack::ResizeStrategy m_resizeStrategy{
+      CylinderVolumeStack::ResizeStrategy::Expand};
 
   // Is only initialized during `build`
   std::vector<Volume*> m_childVolumes;

Core/include/Acts/Geometry/LayerBlueprintNode.hpp

@@ -10,7 +10,7 @@
 
 #include "Acts/Geometry/StaticBlueprintNode.hpp"
 
-#include <iosfwd>
+#include <ostream>
 
 namespace Acts {
 
@@ -100,6 +100,26 @@ class LayerBlueprintNode : public StaticBlueprintNode {
   /// @return The layer type
   const LayerType& layerType() const;
 
+  /// Output operator for the layer type enum.
+  /// @param os The output stream
+  /// @param type The layer type
+  friend std::ostream& operator<<(std::ostream& os,
+                                  LayerBlueprintNode::LayerType type) {
+    switch (type) {
+      using enum LayerBlueprintNode::LayerType;
+      case Cylinder:
+        os << "Cylinder";
+        break;
+      case Disc:
+        os << "Disc";
+        break;
+      case Plane:
+        os << "Plane";
+        break;
+    }
+    return os;
+  }
+
  private:
   /// @copydoc Acts::BlueprintNode::addToGraphviz
   void addToGraphviz(std::ostream& os) const override;
@@ -118,9 +138,4 @@ class LayerBlueprintNode : public StaticBlueprintNode {
   LayerType m_layerType = LayerType::Cylinder;
 };
 
-/// Output operator for the layer type enum.
-/// @param os The output stream
-/// @param type The layer type
-std::ostream& operator<<(std::ostream& os, LayerBlueprintNode::LayerType type);
-
 }  // namespace Acts

Core/include/Acts/Geometry/StaticBlueprintNode.hpp

@@ -37,7 +37,6 @@ class StaticBlueprintNode : public BlueprintNode {
   /// Build-phase of the blueprint construction. Returns the wrapped volume for
   /// sizing.
   Volume& build(const BlueprintOptions& options, const GeometryContext& gctx,
-
                 const Logger& logger = Acts::getDummyLogger()) override;
 
   /// @copydoc BlueprintNode::connect

Core/src/Geometry/BlueprintNode.cpp

@@ -16,6 +16,7 @@
 #include "Acts/Navigation/INavigationPolicy.hpp"
 #include "Acts/Navigation/TryAllNavigationPolicy.hpp"
 
+#include <concepts>
 #include <ostream>
 
 namespace Acts {
@@ -27,13 +28,10 @@ bool hasDescendent(const BlueprintNode& descendent,
     return true;
   }
 
-  for (const auto& child : ancestor.children()) {
-    if (hasDescendent(descendent, child)) {
-      return true;
-    }
-  }
-
-  return false;
+  return std::ranges::any_of(ancestor.children(),
+                             [&](const auto& child) -> bool {
+                               return hasDescendent(descendent, child);
+                             });
 }
 }  // namespace
 
@@ -107,11 +105,11 @@ StaticBlueprintNode& BlueprintNode::addStaticVolume(
 }
 
 StaticBlueprintNode& BlueprintNode::addStaticVolume(
-    const Transform3& transform, std::shared_ptr<VolumeBounds> volbounds,
+    const Transform3& transform, std::shared_ptr<VolumeBounds> volumeBounds,
     const std::string& volumeName,
     const std::function<void(StaticBlueprintNode& cylinder)>& callback) {
   return addStaticVolume(std::make_unique<TrackingVolume>(
-                             transform, std::move(volbounds), volumeName),
+                             transform, std::move(volumeBounds), volumeName),
                          callback);
 }
 
@@ -158,7 +156,7 @@ void BlueprintNode::clearChildren() {
   m_children.clear();
 }
 
-void BlueprintNode::graphViz(std::ostream& os) const {
+void BlueprintNode::graphviz(std::ostream& os) const {
   os << "digraph BlueprintNode {" << std::endl;
   addToGraphviz(os);
   os << "}" << std::endl;

Core/src/Geometry/BlueprintOptions.cpp

@@ -26,6 +26,4 @@ BlueprintOptions::makeDefaultNavigationPolicyFactory() {
       .asUniquePtr();
 }
 
-BlueprintOptions::~BlueprintOptions() = default;
-
 }  // namespace Acts

Core/src/Geometry/LayerBlueprintNode.cpp

@@ -146,20 +146,4 @@ void LayerBlueprintNode::addToGraphviz(std::ostream& os) const {
   BlueprintNode::addToGraphviz(os);
 }
 
-std::ostream& operator<<(std::ostream& os, LayerBlueprintNode::LayerType type) {
-  switch (type) {
-    using enum LayerBlueprintNode::LayerType;
-    case Cylinder:
-      os << "Cylinder";
-      break;
-    case Disc:
-      os << "Disc";
-      break;
-    case Plane:
-      os << "Plane";
-      break;
-  }
-  return os;
-}
-
 }  // namespace Acts

Examples/Python/src/Blueprint.cpp

@@ -271,9 +271,9 @@ void addBlueprint(Context& ctx) {
       .def("clearChildren", &BlueprintNode::clearChildren)
       .def_property_readonly("name", &BlueprintNode::name)
       .def_property_readonly("depth", &BlueprintNode::depth)
-      .def("graphViz", [](BlueprintNode& self, const py::object& fh) {
+      .def("graphviz", [](BlueprintNode& self, const py::object& fh) {
         std::stringstream ss;
-        self.graphViz(ss);
+        self.graphviz(ss);
         fh.attr("write")(ss.str());
       });
 

Examples/Python/tests/test_blueprint.py

@@ -18,7 +18,7 @@ def write(root: acts.BlueprintNode, stage: int):
         gz = tmp_path / f"blueprint_{stage}.dot"
         print(gz)
         with gz.open("w") as fh:
-            root.graphViz(fh)
+            root.graphviz(fh)
 
     base = acts.Transform3.Identity()
 

Examples/Scripts/Python/blueprint.py

@@ -84,7 +84,7 @@
 
 
 with open("blueprint.dot", "w") as fh:
-    root.graphViz(fh)
+    root.graphviz(fh)
 
 
 gctx = acts.GeometryContext()

Tests/UnitTests/Core/Geometry/BlueprintApiTests.cpp

@@ -357,7 +357,7 @@ BOOST_AUTO_TEST_CASE(NodeApiTestContainers) {
   });
 
   std::ofstream dot{"api_test_container.dot"};
-  root->graphViz(dot);
+  root->graphviz(dot);
 
   auto trackingGeometry = root->construct({}, gctx, *logger);