feat(apps): build individual subsystems from build_geometry#52
Open
matclim wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds registry-based subsystem assembly, self-registering factories, single-subsystem CLI builds, linker retention, and updated README guidance. ChangesSubsystem Registry Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
include/SHiPGeometry/SubsystemRegistry.h (1)
54-57: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFail fast on duplicate subsystem names.
emplace()silently ignores a second registration with the samedesc.name, but still returnstrue; a duplicated descriptor would make one subsystem disappear from--list/assembly without any diagnostic.Proposed fix
+#include <stdexcept> + inline bool registerSubsystem(const SubsystemDescriptor& desc, std::function<GeoVPhysVol*(SHiPMaterials&)> build) { - registry().emplace(desc.name, SubsystemInfo{desc, std::move(build)}); + auto [_, inserted] = + registry().emplace(desc.name, SubsystemInfo{desc, std::move(build)}); + if (!inserted) { + throw std::runtime_error("Duplicate subsystem registration: '" + + std::string(desc.name) + "'"); + } return true; }🤖 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 `@include/SHiPGeometry/SubsystemRegistry.h` around lines 54 - 57, The registerSubsystem function currently returns success even when a duplicate desc.name is registered, because registry().emplace() silently ignores the second entry. Update registerSubsystem in SubsystemRegistry.h to detect an existing name before or after emplace, and fail fast by rejecting the duplicate instead of returning true. Use the registerSubsystem and registry() symbols to locate the check, and make sure duplicate subsystem descriptors cannot be added without a clear diagnostic.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@apps/build_geometry.cpp`:
- Around line 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.
- Around line 55-76: The default full-build path in build_geometry.cpp lacks the
same exception handling used for SHiPGeometry::buildSubsystem, so
builder.build() can terminate on a thrown std::runtime_error. Wrap the
names.empty() branch in the same try/catch pattern used for the single-subsystem
path, catch std::exception, print a clear error plus available subsystems if
helpful, and return 1 so buildGeometry fails gracefully instead of aborting.
In `@include/SHiPGeometry/SubsystemRegistry.h`:
- Around line 8-11: The header is not self-contained because
`SubsystemRegistry.h` uses `std::move` without including the declaration it
comes from. Add the missing `<utility>` include alongside the existing includes
in `SubsystemRegistry.h` so any translation unit using `SubsystemRegistry`
compiles independently.
In `@README.md`:
- 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.
In `@src/SHiPGeometry.cpp`:
- Around line 50-59: The world assembly path currently assumes
entry.second.build(materials) always returns a valid geometry, but a factory may
return null and lead to a bad cast or later GeoModel failure. In the world
selection logic in SHiPGeometry::buildSubsystem, check the build result before
dynamic_cast<GeoPhysVol*> and throw a clear error if it is null; apply the same
validation anywhere build results are passed into place() so null subsystem
geometries are rejected at the assembly boundary instead of propagating deeper.
In `@subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp`:
- Line 5: The UpstreamTagger registration path is losing the SHiPUBTManager
pointer, so build() never receives a valid manager and setContainerVolume() is
skipped. Update the REGISTER_SUBSYSTEM wiring in UpstreamTaggerFactory so the
registry callback preserves and forwards the SHiPUBTManager* (or otherwise
retrieves it) into build(), and ensure both buildSubsystem() and
assembleGeometry() use the same manager-aware path.
---
Nitpick comments:
In `@include/SHiPGeometry/SubsystemRegistry.h`:
- Around line 54-57: The registerSubsystem function currently returns success
even when a duplicate desc.name is registered, because registry().emplace()
silently ignores the second entry. Update registerSubsystem in
SubsystemRegistry.h to detect an existing name before or after emplace, and fail
fast by rejecting the duplicate instead of returning true. Use the
registerSubsystem and registry() symbols to locate the check, and make sure
duplicate subsystem descriptors cannot be added without a clear diagnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f28d3d8-52fb-4061-8ca5-100ab3561b6c
📒 Files selected for processing (25)
README.mdapps/build_geometry.cppinclude/SHiPGeometry/SubsystemRegistry.hsrc/CMakeLists.txtsrc/SHiPGeometry.cppsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Cavern/include/Cavern/CavernFactory.hsubsystems/Cavern/src/CavernFactory.cppsubsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.hsubsystems/DecayVolume/src/DecayVolumeFactory.cppsubsystems/Magnet/include/Magnet/MagnetFactory.hsubsystems/Magnet/src/MagnetFactory.cppsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.hsubsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cppsubsystems/Target/include/Target/TargetFactory.hsubsystems/Target/src/TargetFactory.cppsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.hsubsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
Comment on lines
+55
to
+76
| 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). | ||
| try { | ||
| geometry = SHiPGeometry::buildSubsystem(names[0]); | ||
| } 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; | ||
| } | ||
| label = names[0]; | ||
| if (outputFile.empty()) outputFile = names[0] + ".db"; | ||
| } else { | ||
| std::cerr << "Error: build at most one subsystem at a time (got " << names.size() | ||
| << ").\n"; | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Full-build path is missing the exception handling the single-subsystem path has.
SHiPGeometry::buildSubsystem() is wrapped in try/catch, but builder.build() (→ assembleGeometry({})) is not, even though it can throw std::runtime_error (e.g. no isWorld subsystem registered). Given this PR's own whole-archive linking is fragile across toolchains, this is a realistic way for the default build path to hit an uncaught exception and terminate ungracefully instead of printing a clean error.
🐛 Proposed fix: wrap both build paths in the same try/catch
GeoVPhysVol* geometry = nullptr;
std::string label;
- 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).
- try {
- geometry = SHiPGeometry::buildSubsystem(names[0]);
- } 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;
- }
- label = names[0];
- if (outputFile.empty()) outputFile = names[0] + ".db";
- } else {
+ if (names.size() > 1) {
std::cerr << "Error: build at most one subsystem at a time (got " << names.size()
<< ").\n";
return 1;
}
+
+ 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 {
+ // A single subsystem, on its own (local frame).
+ geometry = SHiPGeometry::buildSubsystem(names[0]);
+ label = names[0];
+ if (outputFile.empty()) outputFile = names[0] + ".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;
+ }📝 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 (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). | |
| try { | |
| geometry = SHiPGeometry::buildSubsystem(names[0]); | |
| } 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; | |
| } | |
| label = names[0]; | |
| if (outputFile.empty()) outputFile = names[0] + ".db"; | |
| } else { | |
| std::cerr << "Error: build at most one subsystem at a time (got " << names.size() | |
| << ").\n"; | |
| return 1; | |
| } | |
| if (names.size() > 1) { | |
| std::cerr << "Error: build at most one subsystem at a time (got " << names.size() | |
| << ").\n"; | |
| return 1; | |
| } | |
| 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 { | |
| // A single subsystem, on its own (local frame). | |
| geometry = SHiPGeometry::buildSubsystem(names[0]); | |
| label = names[0]; | |
| if (outputFile.empty()) outputFile = names[0] + ".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; | |
| } |
🤖 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 55 - 76, The default full-build path in
build_geometry.cpp lacks the same exception handling used for
SHiPGeometry::buildSubsystem, so builder.build() can terminate on a thrown
std::runtime_error. Wrap the names.empty() branch in the same try/catch pattern
used for the single-subsystem path, catch std::exception, print a clear error
plus available subsystems if helpful, and return 1 so buildGeometry fails
gracefully instead of aborting.
Comment on lines
83
to
85
| if (std::filesystem::exists(outputFile)) { | ||
| std::filesystem::remove(outputFile); | ||
| } |
There was a problem hiding this comment.
🩺 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.
Comment on lines
+50
to
+59
| GeoPhysVol* world = nullptr; | ||
| for (auto& entry : reg) { | ||
| if (entry.second.desc.isWorld) { | ||
| world = dynamic_cast<GeoPhysVol*>(entry.second.build(materials)); | ||
| break; | ||
| } | ||
| } | ||
| if (!world) { | ||
| throw std::runtime_error("No world (isWorld) subsystem is registered"); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate factory build results before casting or placing.
buildSubsystem()/the CLI already treat a null geometry as possible, but full assembly passes placed subsystem results directly into place(). If a registered factory returns nullptr, this can add a null child or fail later with an unclear GeoModel error; validate at the assembly boundary instead.
Proposed fix
GeoPhysVol* world = nullptr;
for (auto& entry : reg) {
if (entry.second.desc.isWorld) {
- world = dynamic_cast<GeoPhysVol*>(entry.second.build(materials));
+ auto* builtWorld = entry.second.build(materials);
+ world = dynamic_cast<GeoPhysVol*>(builtWorld);
+ if (!world) {
+ throw std::runtime_error("World subsystem '" + entry.first +
+ "' did not return a GeoPhysVol");
+ }
break;
}
}
@@
for (const SubsystemInfo* info : placed) {
- place(world, info->build(materials), info->desc);
+ auto* volume = info->build(materials);
+ if (!volume) {
+ throw std::runtime_error("Subsystem '" + std::string(info->desc.name) +
+ "' returned null geometry");
+ }
+ place(world, volume, info->desc);
}Also applies to: 78-80
🤖 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 `@src/SHiPGeometry.cpp` around lines 50 - 59, The world assembly path currently
assumes entry.second.build(materials) always returns a valid geometry, but a
factory may return null and lead to a bad cast or later GeoModel failure. In the
world selection logic in SHiPGeometry::buildSubsystem, check the build result
before dynamic_cast<GeoPhysVol*> and throw a clear error if it is null; apply
the same validation anywhere build results are passed into place() so null
subsystem geometries are rejected at the assembly boundary instead of
propagating deeper.
| // Copyright (C) CERN for the benefit of the SHiP Collaboration | ||
|
|
||
| #include "UpstreamTagger/UpstreamTaggerFactory.h" | ||
| #include "SHiPGeometry/SubsystemRegistry.h" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how SHiPUBTManager is wired today, and whether anything besides this
# build() overload sets it, to confirm the registry path truly loses it.
rg -n "SHiPUBTManager" --type=cpp -C3
rg -n "buildSubsystem|assembleGeometry" --type=cpp -C3Repository: ShipSoft/Geometry
Length of output: 155
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the factory/registry wiring.
git ls-files | rg 'UpstreamTaggerFactory\.cpp|UpstreamTagger.*\.cpp|SHiPGeometry/SubsystemRegistry\.h|SubsystemRegistry\.h|REGISTER_SUBSYSTEM'
echo '--- UpstreamTaggerFactory.cpp ---'
cat -n subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp | sed -n '1,220p'
echo '--- SubsystemRegistry.h (if present) ---'
for f in $(git ls-files | rg 'SubsystemRegistry\.h$'); do
echo "### $f"
cat -n "$f" | sed -n '1,260p'
done
echo '--- any REGISTER_SUBSYSTEM uses ---'
rg -n "REGISTER_SUBSYSTEM|build\(SHiPMaterials|SHiPUBTManager|setContainerVolume|containerPhys|GeoFullPhysVol" subsystems -C 3Repository: ShipSoft/Geometry
Length of output: 45346
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect registry consumers to see whether they ever pass a manager or provide
# a post-build hook.
for f in $(git ls-files | rg 'SubsystemRegistry\.(cpp|cc|cxx|h)$|.*\.cpp$|.*\.h$'); do
if rg -n "buildSubsystem\(|assembleGeometry\(|registerSubsystem\(|setContainerVolume\(" "$f" >/dev/null 2>&1; then
echo "### $f"
rg -n "buildSubsystem\(|assembleGeometry\(|registerSubsystem\(|setContainerVolume\(" "$f" -C 4
fi
doneRepository: ShipSoft/Geometry
Length of output: 3955
REGISTER_SUBSYSTEM drops the SHiPUBTManager*, so the UpstreamTagger tree-top never gets registered through the registry path. The registry callback only forwards SHiPMaterials&, so build() always sees manager == nullptr and setContainerVolume() is skipped in both buildSubsystem() and assembleGeometry().
🤖 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 `@subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp` at line 5, The
UpstreamTagger registration path is losing the SHiPUBTManager pointer, so
build() never receives a valid manager and setContainerVolume() is skipped.
Update the REGISTER_SUBSYSTEM wiring in UpstreamTaggerFactory so the registry
callback preserves and forwards the SHiPUBTManager* (or otherwise retrieves it)
into build(), and ensure both buildSubsystem() and assembleGeometry() use the
same manager-aware path.
Contributor
Author
|
This PR is in preparation for another PR which will include visualisation, both for all subsystems and for individual subsystems. |
Contributor
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/SHiPGeometry.cpp (1)
49-59: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnvalidated factory build results still propagate into cast/placement.
The world lookup at Line 53 casts the factory's return value without checking for
nullptr/cast failure, and the placement loop at Line 79 passesinfo->build(materials)straight intoplace()without a null check. A misbehaving or mis-descriptored factory (e.g.isWorld=trueon a non-GeoPhysVol, or a build path that legitimately returnsnullptr) will either silently produce a null world or crash deep inside GeoModel with an unclear error.Proposed fix
GeoPhysVol* world = nullptr; for (auto& entry : reg) { if (entry.second.desc.isWorld) { - world = dynamic_cast<GeoPhysVol*>(entry.second.build(materials)); + auto* builtWorld = entry.second.build(materials); + world = dynamic_cast<GeoPhysVol*>(builtWorld); + if (!world) { + throw std::runtime_error("World subsystem '" + entry.first + + "' did not return a GeoPhysVol"); + } break; } } @@ for (const SubsystemInfo* info : placed) { - place(world, info->build(materials), info->desc); + auto* volume = info->build(materials); + if (!volume) { + throw std::runtime_error("Subsystem '" + std::string(info->desc.name) + + "' returned null geometry"); + } + place(world, volume, info->desc); }Also applies to: 78-80
🤖 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 `@src/SHiPGeometry.cpp` around lines 49 - 59, The world selection in SHiPGeometry currently trusts factory build results without validation, so a bad isWorld subsystem or a build that returns null can propagate into the GeoPhysVol cast and later placement. In the world lookup logic, check the result of entry.second.build(materials) before dynamic_cast and verify the cast succeeded before assigning world; if either fails, throw a clear runtime error mentioning the subsystem. In the placement loop, validate info->build(materials) before calling place() and skip/throw on null so GeoModel never receives an invalid object from SHiPGeometry.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@README.md`:
- Around line 162-164: The CMake guidance in README.md is misleading because
src/CMakeLists.txt does not use a --whole-archive block for subsystem libraries;
update the instructions to reference the actual --no-as-needed / --as-needed
linker block and the existing inline comment about whole-archive being a no-op.
Make sure the step tells contributors to add the new subsystem target to that
real link section so REGISTER_SUBSYSTEM initialization is preserved.
---
Duplicate comments:
In `@src/SHiPGeometry.cpp`:
- Around line 49-59: The world selection in SHiPGeometry currently trusts
factory build results without validation, so a bad isWorld subsystem or a build
that returns null can propagate into the GeoPhysVol cast and later placement. In
the world lookup logic, check the result of entry.second.build(materials) before
dynamic_cast and verify the cast succeeded before assigning world; if either
fails, throw a clear runtime error mentioning the subsystem. In the placement
loop, validate info->build(materials) before calling place() and skip/throw on
null so GeoModel never receives an invalid object from SHiPGeometry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3f6c31d-2095-4c0a-a342-3d9cb4907ffe
📒 Files selected for processing (25)
README.mdapps/build_geometry.cppinclude/SHiPGeometry/SubsystemRegistry.hsrc/CMakeLists.txtsrc/SHiPGeometry.cppsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Cavern/include/Cavern/CavernFactory.hsubsystems/Cavern/src/CavernFactory.cppsubsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.hsubsystems/DecayVolume/src/DecayVolumeFactory.cppsubsystems/Magnet/include/Magnet/MagnetFactory.hsubsystems/Magnet/src/MagnetFactory.cppsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.hsubsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cppsubsystems/Target/include/Target/TargetFactory.hsubsystems/Target/src/TargetFactory.cppsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.hsubsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
matclim
force-pushed
the
self-registration
branch
from
July 11, 2026 20:23
b7d33d4 to
99a36d4
Compare
added 3 commits
July 12, 2026 00:24
docs(readme): document self-registration and single-subsystem builds
matclim
force-pushed
the
self-registration
branch
from
July 11, 2026 23:07
99a36d4 to
6bddf29
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
182-193: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
NeutrinoDetectorin the structure tree.It is listed as an implemented subsystem above and participates in the registry, but is missing from this repository layout. This makes the documented subsystem structure incomplete.
🤖 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` around lines 182 - 193, Update the README repository structure tree under subsystems to include the implemented NeutrinoDetector directory, keeping it alongside the other detector subsystem entries and preserving the documented layout.
♻️ Duplicate comments (1)
include/SHiPGeometry/SubsystemRegistry.h (1)
8-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing
<utility>include forstd::move(duplicate of prior review).
std::moveis used on line 56 but<utility>is not included. The header is not self-contained. This was previously flagged and remains unresolved.Proposed fix
`#include` <functional> `#include` <map> `#include` <string> +#include <utility> `#include` <vector>🤖 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 `@include/SHiPGeometry/SubsystemRegistry.h` around lines 8 - 11, Update the includes in SubsystemRegistry.h to add the standard <utility> header, ensuring the std::move usage in the registry implementation is declared and the header remains self-contained.
🧹 Nitpick comments (1)
src/CMakeLists.txt (1)
28-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider guarding
--no-as-neededfor non-GNU linkers.
-Wl,--no-as-neededis specific to GNU ld and gold. LLD supports it with GNU-compatible defaults, but Apple'slddoes not. If the project only targets Linux with GNU ld this is fine; otherwise consider a linker check:# Only apply on GNU-compatible linkers if(CMAKE_LINKER_ID STREQUAL "GNU" OR CMAKE_LINKER_ID STREQUAL "Gold") target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed") endif()This is a minor portability note — skip if Linux/GNU-only is an established constraint.
🤖 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 `@src/CMakeLists.txt` around lines 28 - 34, Guard the --no-as-needed option on target_link_options(SHiPGeometry) using a CMAKE_LINKER_ID check for GNU-compatible linkers such as GNU or Gold, leaving the option unset for unsupported linkers like Apple ld.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@include/SHiPGeometry/SubsystemRegistry.h`:
- Around line 54-58: Update registerSubsystem to inspect the insertion result
from registry().emplace. When desc.name is already registered, emit a diagnostic
or throw instead of silently discarding the new factory; preserve the successful
registration and true return behavior for unique names.
---
Outside diff comments:
In `@README.md`:
- Around line 182-193: Update the README repository structure tree under
subsystems to include the implemented NeutrinoDetector directory, keeping it
alongside the other detector subsystem entries and preserving the documented
layout.
---
Duplicate comments:
In `@include/SHiPGeometry/SubsystemRegistry.h`:
- Around line 8-11: Update the includes in SubsystemRegistry.h to add the
standard <utility> header, ensuring the std::move usage in the registry
implementation is declared and the header remains self-contained.
---
Nitpick comments:
In `@src/CMakeLists.txt`:
- Around line 28-34: Guard the --no-as-needed option on
target_link_options(SHiPGeometry) using a CMAKE_LINKER_ID check for
GNU-compatible linkers such as GNU or Gold, leaving the option unset for
unsupported linkers like Apple ld.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f8c5f1e5-fedc-4a7e-9bc3-bf827e1168d9
📒 Files selected for processing (26)
Geometry]:README.mdapps/build_geometry.cppinclude/SHiPGeometry/SubsystemRegistry.hsrc/CMakeLists.txtsrc/SHiPGeometry.cppsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Cavern/include/Cavern/CavernFactory.hsubsystems/Cavern/src/CavernFactory.cppsubsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.hsubsystems/DecayVolume/src/DecayVolumeFactory.cppsubsystems/Magnet/include/Magnet/MagnetFactory.hsubsystems/Magnet/src/MagnetFactory.cppsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.hsubsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cppsubsystems/Target/include/Target/TargetFactory.hsubsystems/Target/src/TargetFactory.cppsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.hsubsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
✅ Files skipped from review due to trivial changes (2)
- subsystems/Trackers/src/TrackersFactory.cpp
- subsystems/Cavern/src/CavernFactory.cpp
🚧 Files skipped from review as they are similar to previous changes (20)
- subsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.h
- subsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cpp
- subsystems/Magnet/include/Magnet/MagnetFactory.h
- subsystems/Calorimeter/src/CalorimeterFactory.cpp
- subsystems/MuonShield/include/MuonShield/MuonShieldFactory.h
- subsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.h
- subsystems/Cavern/include/Cavern/CavernFactory.h
- subsystems/Trackers/include/Trackers/TrackersFactory.h
- subsystems/DecayVolume/src/DecayVolumeFactory.cpp
- subsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.h
- subsystems/Target/include/Target/TargetFactory.h
- subsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.h
- subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
- subsystems/Magnet/src/MagnetFactory.cpp
- subsystems/Target/src/TargetFactory.cpp
- subsystems/MuonShield/src/MuonShieldFactory.cpp
- subsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.h
- apps/build_geometry.cpp
- subsystems/TimingDetector/src/TimingDetectorFactory.cpp
- src/SHiPGeometry.cpp
…tion registerSubsystem inspected nothing from emplace(), so a second subsystem declaring an already-registered descriptor name was silently discarded and registration still reported success. Check the insertion result and abort with a diagnostic naming the offending subsystem; registration runs at static-init, so a throw would reach no handler. Add <cstdio>, <cstdlib> and <utility> so the header is self-contained, and correct the REGISTER_SUBSYSTEM note, which still described the subsystems as static archives needing --whole-archive. Restrict -Wl,--no-as-needed to non-Apple UNIX, and add NeutrinoDetector to the README structure tree.
Contributor
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 36 minutes. |
Contributor
Author
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/CMakeLists.txt`:
- Around line 28-38: Remove the unconditional target_link_options call for
SHiPGeometry, leaving only the --no-as-needed option inside the existing
“if(UNIX AND NOT APPLE)” guard. Preserve the guarded platform-specific linker
behavior.
In `@subsystems/Magnet/include/Magnet/MagnetFactory.h`:
- Around line 31-34: Update MagnetFactory::descriptor so its placement envelope
no longer overlaps the TrackersFactory envelope when both are assembled as world
siblings; preserve the intentional shared z-center while adjusting the Magnet
envelope’s z extent or placement to fit outside the Trackers span. Verify the
resulting descriptor bounds are non-intersecting.
In `@subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp`:
- Around line 145-146: Update the registry integration for UpstreamTaggerFactory
so the SHiPUBTManager* reaches build() when constructed through
REGISTER_SUBSYSTEM, allowing setContainerVolume() to execute. Modify the
relevant factory registration/build path rather than the unrelated
container-volume logic, and preserve direct construction behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 26b0db48-46bf-4ac2-8c71-09f8ce040c1f
📒 Files selected for processing (26)
Geometry]:README.mdapps/build_geometry.cppinclude/SHiPGeometry/SubsystemRegistry.hsrc/CMakeLists.txtsrc/SHiPGeometry.cppsubsystems/Calorimeter/include/Calorimeter/CalorimeterFactory.hsubsystems/Calorimeter/src/CalorimeterFactory.cppsubsystems/Cavern/include/Cavern/CavernFactory.hsubsystems/Cavern/src/CavernFactory.cppsubsystems/DecayVolume/include/DecayVolume/DecayVolumeFactory.hsubsystems/DecayVolume/src/DecayVolumeFactory.cppsubsystems/Magnet/include/Magnet/MagnetFactory.hsubsystems/Magnet/src/MagnetFactory.cppsubsystems/MuonShield/include/MuonShield/MuonShieldFactory.hsubsystems/MuonShield/src/MuonShieldFactory.cppsubsystems/NeutrinoDetector/include/NeutrinoDetector/NeutrinoDetectorFactory.hsubsystems/NeutrinoDetector/src/NeutrinoDetectorFactory.cppsubsystems/Target/include/Target/TargetFactory.hsubsystems/Target/src/TargetFactory.cppsubsystems/TimingDetector/include/TimingDetector/TimingDetectorFactory.hsubsystems/TimingDetector/src/TimingDetectorFactory.cppsubsystems/Trackers/include/Trackers/TrackersFactory.hsubsystems/Trackers/src/TrackersFactory.cppsubsystems/UpstreamTagger/include/UpstreamTagger/UpstreamTaggerFactory.hsubsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp
Comment on lines
+28
to
+38
|
|
||
| # The subsystems are referenced only through their REGISTER_SUBSYSTEM static | ||
| # initialisers, so the toolchain's default --as-needed drops them from the | ||
| # DT_NEEDED of every executable that links SHiPGeometry, and their | ||
| # registrations never run. INTERFACE puts --no-as-needed on the *consumer's* | ||
| # link line, which is where the entries were being dropped. | ||
| target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed") | ||
|
|
||
| if(UNIX AND NOT APPLE) | ||
| target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed") | ||
| endif() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Remove the unconditional --no-as-needed on line 34; keep only the platform-guarded block.
Line 34 unconditionally adds -Wl,--no-as-needed to INTERFACE_LINK_OPTIONS. This flag is GNU ld-specific — the Apple linker on macOS and MSVC on Windows do not recognize it, causing link failures on those platforms. The if(UNIX AND NOT APPLE) guard on lines 36–38 is the correct place for this option, but line 34 makes the guard ineffective. On Linux, the option is also duplicated (once unconditionally, once conditionally), which is harmless but redundant.
🔧 Proposed fix: remove unconditional call, keep platform guard
# link line, which is where the entries were being dropped.
-target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed")
if(UNIX AND NOT APPLE)
target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed")
endif()📝 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
| # The subsystems are referenced only through their REGISTER_SUBSYSTEM static | |
| # initialisers, so the toolchain's default --as-needed drops them from the | |
| # DT_NEEDED of every executable that links SHiPGeometry, and their | |
| # registrations never run. INTERFACE puts --no-as-needed on the *consumer's* | |
| # link line, which is where the entries were being dropped. | |
| target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed") | |
| if(UNIX AND NOT APPLE) | |
| target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed") | |
| endif() | |
| # The subsystems are referenced only through their REGISTER_SUBSYSTEM static | |
| # initialisers, so the toolchain's default --as-needed drops them from the | |
| # DT_NEEDED of every executable that links SHiPGeometry, and their | |
| # registrations never run. INTERFACE puts --no-as-needed on the *consumer's* | |
| # link line, which is where the entries were being dropped. | |
| if(UNIX AND NOT APPLE) | |
| target_link_options(SHiPGeometry INTERFACE "-Wl,--no-as-needed") | |
| endif() |
🤖 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 `@src/CMakeLists.txt` around lines 28 - 38, Remove the unconditional
target_link_options call for SHiPGeometry, leaving only the --no-as-needed
option inside the existing “if(UNIX AND NOT APPLE)” guard. Preserve the guarded
platform-specific linker behavior.
Comment on lines
+31
to
+34
| /// This subsystem's self-description (name, node, id, placement). | ||
| static SubsystemDescriptor descriptor() { | ||
| return {"Magnet", "/SHiP/magnet", 6, 0.0, 0.0, 89570.0, false}; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check TrackersFactory container dimensions and station z-positions
rg -n 's_containerHalfZ|s_containerCentreZ|s_station[1-4]Z|s_halfX|s_halfY|s_halfZ' \
subsystems/Trackers/include/Trackers/TrackersFactory.hRepository: ShipSoft/Geometry
Length of output: 842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== MagnetFactory.h =="
nl -ba subsystems/Magnet/include/Magnet/MagnetFactory.h | sed -n '1,220p'
echo
echo "== Placement / assembleGeometry references =="
rg -n 'assembleGeometry|descriptor\(\)|s_containerHalfZ|s_containerCentreZ|halfZ|inner cutout|yoke|MagnetFactory|TrackersFactory' subsystems -g '!**/build/**'Repository: ShipSoft/Geometry
Length of output: 217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== MagnetFactory.h =="
sed -n '1,220p' subsystems/Magnet/include/Magnet/MagnetFactory.h | cat -n
echo
echo "== TrackersFactory.h =="
sed -n '1,220p' subsystems/Trackers/include/Trackers/TrackersFactory.h | cat -n
echo
echo "== Placement references =="
rg -n 'assembleGeometry|descriptor\(\)|s_containerHalfZ|s_containerCentreZ|halfZ|inner cutout|yoke|MagnetFactory|TrackersFactory' subsystems -g '!**/build/**'Repository: ShipSoft/Geometry
Length of output: 27957
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SubsystemRegistry / assembleGeometry =="
rg -n 'assembleGeometry|SubsystemDescriptor|descriptor\(\)|register|place|GeoPhysVol|GeoLogVol|add\(new GeoNameTag|add\(.*GeoPhysVol' \
subsystems -g '!**/build/**' | sed -n '1,240p'
echo
echo "== Assembly code files =="
fd -a '.*(Assembly|assembleGeometry|Geometry).*' . || trueRepository: ShipSoft/Geometry
Length of output: 26726
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/SHiPGeometry.cpp =="
sed -n '1,220p' src/SHiPGeometry.cpp | cat -n
echo
echo "== SubsystemRegistry header =="
sed -n '1,220p' include/SHiPGeometry/SubsystemRegistry.h | cat -n
echo
echo "== Magnet / Trackers build placement in source =="
sed -n '1,180p' subsystems/Magnet/src/MagnetFactory.cpp | cat -n
echo
sed -n '1,180p' subsystems/Trackers/src/TrackersFactory.cpp | cat -nRepository: ShipSoft/Geometry
Length of output: 24881
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '292,330p' subsystems/Trackers/src/TrackersFactory.cpp | cat -nRepository: ShipSoft/Geometry
Length of output: 1452
Top-level Tracker/Magnet envelopes still intersect. The shared z-center is intentional, but TrackersFactory spans 83.57–95.57 m while MagnetFactory spans 87.07–92.07 m. Since assembleGeometry() places both as world siblings, this is an actual overlap unless the Magnet is meant to be nested under the Trackers envelope instead.
🤖 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 `@subsystems/Magnet/include/Magnet/MagnetFactory.h` around lines 31 - 34,
Update MagnetFactory::descriptor so its placement envelope no longer overlaps
the TrackersFactory envelope when both are assembled as world siblings; preserve
the intentional shared z-center while adjusting the Magnet envelope’s z extent
or placement to fit outside the Trackers span. Verify the resulting descriptor
bounds are non-intersecting.
Comment on lines
+145
to
+146
| REGISTER_SUBSYSTEM(UpstreamTaggerFactory) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
REGISTER_SUBSYSTEM(UpstreamTaggerFactory) silently drops the SHiPUBTManager*, so setContainerVolume() is never called through the registry path.
The REGISTER_SUBSYSTEM macro generates a lambda [](SHiPMaterials& materials) { return UpstreamTaggerFactory(materials).build(); } which calls build() with no arguments, defaulting manager to nullptr. As a result, the if (manager) guard at line 138 always fails and manager->setContainerVolume(containerPhys) is never executed when the subsystem is built via the registry.
This was flagged in a previous review and remains unresolved in the current code.
🤖 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 `@subsystems/UpstreamTagger/src/UpstreamTaggerFactory.cpp` around lines 145 -
146, Update the registry integration for UpstreamTaggerFactory so the
SHiPUBTManager* reaches build() when constructed through REGISTER_SUBSYSTEM,
allowing setContainerVolume() to execute. Modify the relevant factory
registration/build path rather than the unrelated container-volume logic, and
preserve direct construction behavior.
Contributor
|
One general comment: let's use spdlog for logging. I didn't catch it in the original commits. |
Contributor
|
Full review coming... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
feat(apps): build individual subsystems from build_geometry
build_geometry now accepts a subsystem name and builds that subsystem on
its own (local frame), alongside the default full-detector build:
build_geometry # full detector -> ship_geometry.db
build_geometry Calorimeter # single subsystem -> Calorimeter.db
build_geometry --list # list subsystem names
A token ending in .db is treated as the output file; any other token names
a subsystem, as spelled by --list. The standalone build_subsystem
executable is removed and its behaviour folded into build_geometry.
Update the README build, factory-pattern, structure, and "adding a
subsystem" sections to match.
Summary by CodeRabbit
.dboutput (<name>.db/ship_geometry.db).gmex.