Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ if(BUILD_TESTING)
FetchContent_Declare(
Catch2
GIT_REPOSITORY https://github.com/catchorg/Catch2.git
GIT_TAG v3.15.3
GIT_TAG v3.8.0
)
FetchContent_MakeAvailable(Catch2)
list(APPEND CMAKE_MODULE_PATH ${catch2_SOURCE_DIR}/extras)
Expand Down
57 changes: 49 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,22 @@ ctest --test-dir build
# Build the complete SHiP geometry
./build/apps/build_geometry [output_file.db]

# Build a single subsystem on its own
./build/apps/build_geometry <Name> [output_file.db]

# List the available subsystem names
./build/apps/build_geometry --list

# View in gmex
gmex output_file.db
```

With no subsystem named, the complete detector is built. Naming a subsystem
(as spelled by `--list`, e.g. `Calorimeter`) builds just that subsystem in its
own local frame. A token ending in `.db` is taken as the output file; the
default is `ship_geometry.db` for the full build, or `<Name>.db` for a single
subsystem.

### Installing

```bash
Expand All @@ -96,20 +108,39 @@ GeoModelTools automatically.

### Factory Pattern

Each subsystem is implemented as a factory class:
Each subsystem is a self-contained factory class that builds its geometry,
describes where it belongs, and registers itself:

```cpp
class FooFactory {
public:
explicit FooFactory(SHiPMaterials& materials);
GeoPhysVol* build();

// Self-description consumed by the assembler:
// name, tree node, id, placement (x/y/z mm), and whether it is the world.
static SubsystemDescriptor descriptor() {
return {"Foo", "/SHiP/foo", 42, 0.0, 0.0, 12345.0, /*isWorld=*/false};
}
private:
SHiPMaterials& m_materials;
};
```

`SHiPGeometryBuilder::build()` orchestrates all factories, creating the world
volume (Cavern) and placing each subsystem at its global z-position.
with a single registration line in the factory's `.cpp` (inside
`namespace SHiPGeometry`):

```cpp
REGISTER_SUBSYSTEM(FooFactory)
```

The assembler names no subsystem. `assembleGeometry()` — also reachable via
the unchanged `SHiPGeometryBuilder::build()` — iterates the registry, builds
the world (the subsystem whose descriptor sets `isWorld`, i.e. the Cavern),
and places every other registered subsystem at its declared position, ordered
by `(z, id)`. A single subsystem can be built on its own, in its local frame,
with `buildSubsystem("Foo")`. The registry, descriptor type, and macro live in
`include/SHiPGeometry/SubsystemRegistry.h`.

### Materials

Expand All @@ -128,26 +159,36 @@ To add a new material, edit `src/SHiPMaterials.cpp`:
## Adding or Modifying a Subsystem

1. **Header**: `subsystems/<Name>/include/<Name>/<Name>Factory.h` — declare
the factory class with dimension constants as `static constexpr` members
the factory class with dimension constants as `static constexpr` members,
plus `static SubsystemDescriptor descriptor()` returning its name, tree
node, id, and placement
2. **Implementation**: `subsystems/<Name>/src/<Name>Factory.cpp` — implement
`build()` using GeoModel primitives (`GeoBox`, `GeoTubs`, `GeoLogVol`,
`GeoPhysVol`, `GeoTransform`, etc.)
3. **Registration**: add a `build()` + placement call in
`src/SHiPGeometry.cpp` (`SHiPGeometryBuilder::build()`)
4. **CMake**: add sources/headers to `subsystems/<Name>/CMakeLists.txt`
3. **Registration**: add one line — `REGISTER_SUBSYSTEM(<Name>Factory)` — at
the end of the factory `.cpp`, inside `namespace SHiPGeometry`. The
subsystem registers itself; **do not** edit `src/SHiPGeometry.cpp`, which
names no subsystem.
4. **CMake**: add sources/headers to `subsystems/<Name>/CMakeLists.txt`, and
add the new library to the subsystem list linked by `SHiPGeometry` in
`src/CMakeLists.txt`. That target applies `-Wl,--no-as-needed` (as an
INTERFACE link option) so the library is kept in each consumer's
`DT_NEEDED` and its `REGISTER_SUBSYSTEM` initializer is not dropped by the
linker's default `--as-needed`.
5. **Docs**: update the subsystem `README.md` with geometry tree, materials,
and status

## Structure

```

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Add a language identifier to the fenced code block.

markdownlint flags this block (MD040) since it has no language specified.

📝 Proposed fix
-```
+```text
 geometry/
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 170-170: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@README.md` at line 170, The fenced code block in the README is missing a
language identifier, which triggers markdownlint MD040. Update that markdown
fence to include a language specifier, using the fenced block in the README
around the geometry/ snippet as the target so the block is properly annotated.

Source: Linters/SAST tools

✅ Addressed in commits 40b4a66 to b7d33d4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this is not actually resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- README structure section ---'
nl -ba README.md | sed -n '160,195p'

printf '%s\n' '--- Markdown lint configuration files ---'
fd -HI '(^|/)(\.markdownlint.*|markdownlint.*)$' . -t f -x sh -c 'printf "\n--- %s ---\n" "$1"; cat "$1"' sh {} \; || true

printf '%s\n' '--- All unlabeled Markdown fences in README.md ---'
awk '
  /^```[[:space:]]*$/ { print "Line " NR ": " $0 }
' README.md

Length of output: 816


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- README structure section (numbered) ---'
awk 'NR >= 160 && NR <= 195 { printf "%d:%s\n", NR, $0 }' README.md

printf '%s\n' '--- Markdown lint configuration files ---'
find . -type f \( -name '.markdownlint*' -o -name 'markdownlint*' \) \
  -not -path './.git/*' \
  -print \
  -exec sh -c 'printf "\n--- %s ---\n" "$1"; cat "$1"' sh {} \;

printf '%s\n' '--- Unlabeled Markdown fences in README.md ---'
awk '/^```[[:space:]]*$/ { printf "Line %d: %s\n", NR, $0 }' README.md

Length of output: 2577


@olantwin, you’re right — it is not resolved. README.md still has an unlabeled opening fence at Line 180 for the geometry/ structure tree, so it still triggers MD040.

The minimal fix remains:

```text
geometry/
...

I’ll keep this finding unresolved.

geometry/
├── include/SHiPGeometry/ # Public headers (SHiPGeometry, SHiPMaterials)
├── include/SHiPGeometry/ # Public headers (SHiPGeometry, SHiPMaterials, SubsystemRegistry)
├── src/ # Core implementation
├── subsystems/ # Detector subsystem factories
│ ├── Cavern/
│ ├── Target/
│ ├── MuonShield/
│ ├── NeutrinoDetector/
│ ├── Magnet/
│ ├── DecayVolume/
│ ├── Trackers/
Expand Down
82 changes: 65 additions & 17 deletions apps/build_geometry.cpp
Original file line number Diff line number Diff line change
@@ -1,47 +1,95 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// Copyright (C) CERN for the benefit of the SHiP Collaboration
//
// Build the SHiP GeoModel geometry and serialise it to a GeoModel SQLite (.db).
//
// Usage:
// build_geometry # full detector -> ship_geometry.db
// build_geometry out.db # full detector -> out.db
// build_geometry Calorimeter # just that subsystem -> Calorimeter.db
// build_geometry Calorimeter c.db # just that subsystem -> c.db
// build_geometry Target Magnet # assemble a selection -> ship_selection.db
// build_geometry --list # list available subsystems
//
// A token ending in ".db" is the output file; any other token names a
// subsystem. With no subsystem named, the complete detector is built.

#include "SHiPGeometry/SHiPGeometry.h"
#include "SHiPGeometry/SubsystemRegistry.h"

#include <GeoModelDBManager/GMDBManager.h>
#include <GeoModelKernel/GeoPhysVol.h>
#include <GeoModelWrite/WriteGeoModel.h>

#include <filesystem>
#include <iostream>
#include <string>
#include <vector>

int main(int argc, char* argv[]) {
std::string outputFile = "ship_geometry.db";
if (argc > 1) {
outputFile = argv[1];
std::string outputFile;
std::vector<std::string> names; // requested subsystem(s)

for (int i = 1; i < argc; ++i) {
const std::string arg = argv[i];
if (arg == "--list") {
for (const auto& n : SHiPGeometry::subsystemNames())
std::cout << n << "\n";
return 0;
}
if (arg.ends_with(".db")) {
outputFile = arg;
} else {
names.push_back(arg);
}
}

std::cout << "Building SHiP geometry..." << std::endl;
GeoVPhysVol* geometry = nullptr;
std::string label;

SHiPGeometry::SHiPGeometryBuilder builder;
GeoPhysVol* world = builder.build();
try {
if (names.empty()) {
// Default: the complete detector (unchanged behaviour).
SHiPGeometry::SHiPGeometryBuilder builder;
geometry = builder.build();
label = "full SHiP geometry";
if (outputFile.empty())
outputFile = "ship_geometry.db";
} else if (names.size() == 1) {
// A single subsystem, on its own (local frame).
geometry = SHiPGeometry::buildSubsystem(names[0]);
label = names[0];
if (outputFile.empty())
outputFile = names[0] + ".db";
} else {
// Several subsystems: assemble them into the world at their declared
// placements (the world/cavern is always included).
geometry = SHiPGeometry::assembleGeometry(names);
label = "selection (" + std::to_string(names.size()) + " subsystems)";
if (outputFile.empty())
outputFile = "ship_selection.db";
}
} catch (const std::exception& e) {
std::cerr << "Error: " << e.what() << "\nAvailable subsystems:\n";
for (const auto& n : SHiPGeometry::subsystemNames())
std::cerr << " " << n << "\n";
return 1;
}

if (!world) {
std::cerr << "Error: Geometry not yet implemented" << std::endl;
if (!geometry) {
std::cerr << "Error: geometry is null (not yet implemented?)." << std::endl;
return 1;
}

// Remove existing output file
if (std::filesystem::exists(outputFile)) {
std::filesystem::remove(outputFile);
}
Comment on lines 84 to 86

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

std::filesystem::remove can throw uncaught on permission/lock errors.

The throwing overload is used; a locked or permission-denied output file would crash the tool instead of producing a clean error. The preceding exists() check is also redundant since remove() returns false harmlessly when the path doesn't exist.

🛡️ Proposed fix
-    if (std::filesystem::exists(outputFile)) {
-        std::filesystem::remove(outputFile);
-    }
+    std::error_code ec;
+    std::filesystem::remove(outputFile, ec);
+    if (ec) {
+        std::cerr << "Warning: could not remove existing " << outputFile << ": " << ec.message()
+                   << std::endl;
+    }
📝 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
if (std::filesystem::exists(outputFile)) {
std::filesystem::remove(outputFile);
}
std::error_code ec;
std::filesystem::remove(outputFile, ec);
if (ec) {
std::cerr << "Warning: could not remove existing " << outputFile << ": " << ec.message()
<< std::endl;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/build_geometry.cpp` around lines 83 - 85, The output-file cleanup in
build_geometry.cpp uses the throwing std::filesystem::remove overload after a
redundant exists() check, which can crash on permission or lock failures. Update
the logic around the outputFile deletion to call the non-throwing remove variant
and handle its result (and any error_code) in build_geometry so missing files
are treated as a no-op while real filesystem errors are reported cleanly.


std::cout << "Writing geometry to " << outputFile << std::endl;

std::cout << "Writing " << label << " to " << outputFile << std::endl;
GMDBManager db(outputFile);
GeoModelIO::WriteGeoModel writer(db);

// Traverse the geometry tree
world->exec(&writer);

// Save to database
geometry->exec(&writer);
writer.saveToDB();

std::cout << "Done." << std::endl;
return 0;
}
30 changes: 30 additions & 0 deletions include/SHiPGeometry/SubsystemDescriptor.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// Copyright (C) CERN for the benefit of the SHiP Collaboration

#pragma once

namespace SHiPGeometry {

/**
* @brief A subsystem's self-description: everything the assembler needs.
*
* This is the "required input to the geometry builder" that each subsystem
* yields about itself. It is a plain data type with no GeoModel or standard-
* library dependency, so it is cheap to include from the factory headers
* (which only need it as the return type of `descriptor()`). The registry
* machinery that consumes it lives in SubsystemRegistry.h, included by the
* factory implementations.
*
* Translations are in millimetres from the world origin.
*/
struct SubsystemDescriptor {
const char* name; ///< registry key / CLI name, e.g. "Calorimeter"
const char* node; ///< GeoNameTag, e.g. "/SHiP/calorimeter"
int id; ///< GeoIdentifierTag
double x_mm; ///< placement translation X (mm)
double y_mm; ///< placement translation Y (mm)
double z_mm; ///< placement translation Z, beam direction (mm)
bool isWorld = false; ///< true for the mother/world volume (the cavern)
};

} // namespace SHiPGeometry
94 changes: 94 additions & 0 deletions include/SHiPGeometry/SubsystemRegistry.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// SPDX-License-Identifier: LGPL-3.0-or-later
// Copyright (C) CERN for the benefit of the SHiP Collaboration

#pragma once

#include "SHiPGeometry/SubsystemDescriptor.h"

#include <GeoModelKernel/GeoPhysVol.h> // complete GeoPhysVol/GeoVPhysVol for the macro's upcast

#include <cstdio>
#include <cstdlib>
#include <functional>
#include <map>
#include <string>
#include <utility>
#include <vector>
Comment thread
coderabbitai[bot] marked this conversation as resolved.

namespace SHiPGeometry {

class SHiPMaterials;

/// A registered subsystem: its descriptor plus how to build it (local frame).
struct SubsystemInfo {
SubsystemDescriptor desc;
std::function<GeoVPhysVol*(SHiPMaterials&)> build;
};

/**
* @brief The global subsystem registry.
*
* A Meyers singleton (function-local static in an inline function) so it is
* a single shared instance across all translation units and is guaranteed to
* exist before any static registration runs. No file names any subsystem;
* subsystems add themselves via REGISTER_SUBSYSTEM.
*/
inline std::map<std::string, SubsystemInfo>& registry() {
static std::map<std::string, SubsystemInfo> instance;
return instance;
}

/// Add a subsystem to the registry. Returns true (usable as a static
/// initialiser). A duplicate name is a programming error (two subsystems
/// declaring the same descriptor name): it is reported and aborts, rather
/// than being silently dropped by emplace(). Runs at static-init, so this
/// diagnoses to stderr and aborts instead of throwing.
inline bool registerSubsystem(const SubsystemDescriptor& desc,
std::function<GeoVPhysVol*(SHiPMaterials&)> build) {
const auto result = registry().emplace(desc.name, SubsystemInfo{desc, std::move(build)});
if (!result.second) {
std::fprintf(stderr,
"SHiPGeometry: duplicate subsystem name '%s' registered; "
"each subsystem's descriptor() must return a unique name.\n",
desc.name);
std::abort();
}
return true;
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// ── Generic consumers — these name no subsystem ─────────────────────────────

/// Assemble the world plus a selection of subsystems (empty selection = all),
/// each placed at its own declared position. Unknown names throw
/// std::runtime_error. Returns the world volume.
GeoPhysVol* assembleGeometry(const std::vector<std::string>& only = {});

/// Build a single subsystem on its own, in its local frame (no world, no
/// placement). Throws std::runtime_error if the name is not registered.
GeoVPhysVol* buildSubsystem(const std::string& name);

/// The names of all registered subsystems (including the world), sorted.
std::vector<std::string> subsystemNames();

} // namespace SHiPGeometry

/**
* @brief Register a subsystem factory with the global registry.
*
* Placed once in each subsystem's own .cpp (inside namespace SHiPGeometry).
* The factory must expose `static SubsystemDescriptor descriptor()` and be
* constructible from `SHiPMaterials&` with a `build()` returning a volume.
*
* NOTE: nothing references this registration, so the subsystem library would
* otherwise be dropped from a consumer's DT_NEEDED by the toolchain's default
* --as-needed and the initialiser would never run. src/CMakeLists.txt applies
* -Wl,--no-as-needed as an INTERFACE link option on SHiPGeometry so the flag
* lands on each executable's link line. Do not remove it.
*/
#define REGISTER_SUBSYSTEM(FACTORY) \
namespace { \
const bool FACTORY##_registered = ::SHiPGeometry::registerSubsystem( \
FACTORY::descriptor(), [](::SHiPGeometry::SHiPMaterials& materials) -> ::GeoVPhysVol* { \
return FACTORY(materials).build(); \
}); \
}
Loading
Loading