separated Stickman geometry PR from PR #1794

[YongyiBWu]

Sub-PR with only Stickman geometry update, separated from PR #1794
Suggested changes from PR#1794 were applied. Response details see PR #1794

===========================================
This pull request introduces support for a new production target geometry called "Stickman_v_1_0" to the Mu2e simulation framework. The changes include the implementation of the new target's construction logic, integration into the geometry selection mechanism, and the addition of a configuration file describing the Stickman geometry. The update ensures the new target can be selected and constructed in the same way as existing targets, such as "Hayman_v_2_0".

Support for new "Stickman_v_1_0" production target:

• Added a new geometry configuration file ProductionTarget_Stickman_v1_0.txt describing all parameters for the Stickman target, including plates, rods, spacers, support rings, and support wheel features.
• Implemented the makeStickman_v_1_0 method in ProductionTargetMaker to build the new target from configuration, including all relevant geometry and support features.
• Registered the new target type in ProductionTargetMaker.hh and exposed a corresponding static method for construction.

Integration into geometry selection and construction:

• Updated the factory logic in ProductionTargetMaker.cc and GeometryService.cc to recognize and construct the Stickman target when requested in the configuration, with appropriate error handling for unknown target types. [1] [2]
• Modified the geometry construction code in constructPS.cc to handle the new Stickman target in the same way as other supported targets.

Default geometry configuration update:

• Changed the default geometry run configuration (geom_run1_a.txt) to use the new Stickman target instead of the Hayman target.

[FNALbuild]

Hi @YongyiBWu,
You have proposed changes to files in these packages:

• GeometryService
• ProductionTargetGeom
• Mu2eG4

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for 850b78e: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[Copilot]

Pull request overview

Adds a new production target geometry option, Stickman_v_1_0, to the Mu2e Offline simulation stack, integrating it into both geometry configuration and Geant4 construction so it can be selected like existing targets (e.g., Hayman).

Changes:

• Added Stickman target parameters to ProductionTarget and implemented Stickman construction via ProductionTargetMaker.
• Implemented Geant4 geometry construction for Stickman plates/rods/end-rings (and reused support wheel logic) in constructTargetPS.cc, and wired selection in constructPS.cc.
• Added a new Stickman geometry config file and updated the default run geometry include to use it.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
ProductionTargetGeom/src/ProductionTarget.cc	Adds a Stickman-specific ProductionTarget constructor and derived parameters (e.g., rod length).
ProductionTargetGeom/inc/ProductionTarget.hh	Adds Stickman parameters/accessors and selection helpers for position/half-length by target type.
GeometryService/inc/ProductionTargetMaker.hh	Registers Stickman version constant and factory entry point.
GeometryService/src/ProductionTargetMaker.cc	Adds makeStickman_v_1_0() to build ProductionTarget from SimpleConfig.
GeometryService/src/GeometryService.cc	Extends PS shield placement logic to support Stickman target positioning.
Mu2eG4/src/constructPS.cc	Allows selecting Stickman model name and routes to constructTargetPS().
Mu2eG4/src/constructTargetPS.cc	Implements full Stickman Geant4 solid/placement construction and reuses support structures logic.
Mu2eG4/geom/ProductionTarget_Stickman_v1_0.txt	Introduces the Stickman geometry configuration parameters.
Mu2eG4/geom/geom_run1_a.txt	Switches default include from Hayman to Stickman geometry.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[FNALbuild]

☀️ The build tests passed at 850b78e.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 850b78e at 2cc428c
build (prof)	✅	Log file. Build time: 04 min 06 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	✅	Log file.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (0) FIXME (3) in 7 files
clang-tidy	➡️	6 errors 326 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 850b78e after being merged into the base branch at 2cc428c.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[YongyiBWu]

Separated run_1_a and run_1_a_stickman geometry as suggested in comments in PR #1794

[oksuzian]

I'll review this pull request by examining its details and changes.Now let me look at the existing review comments and check the existing geom_run1_a.txt file to understand the default geometry change impact:Here's my review of PR #1797. I've identified several issues across the changed files, organized by severity:

---

🔴 Critical Issues

1. Default geometry silently changed for everyone

-#include "Offline/Mu2eG4/geom/geom_run1_a.txt"
+#include "Offline/Mu2eG4/geom/geom_run1_a_stickman.txt"

geom_common.txt is the global default geometry redirect. Changing it from the existing geom_run1_a.txt to geom_run1_a_stickman.txt switches the default production target for all users of the framework to the new, not-yet-validated Stickman target. This is almost certainly unintentional for a geometry-addition PR. The Stickman config file should be added alongside the existing one, and users should opt in explicitly. The original geom_run1_a.txt should remain the default.

2. No validation of per-plate/per-fin vector sizes in ProductionTargetMaker::makeStickman_v_1_0

c.getVectorString("targetPS_plateMaterial", plateMaterial);
c.getVectorDouble("targetPS_rOut", plateROut);
c.getVectorDouble("targetPS_plateFinAngles", plateFinAngles);
c.getVectorDouble("targetPS_plateThickness", plateThickness);
c.getVectorDouble("targetPS_plateLugThickness", plateLugThickness);

These vectors are read from config but never checked against targetPS_numberOfPlates or targetPS_nStickmanFins. If a config file has mismatched array sizes, the downstream code will silently access out-of-bounds memory. Add explicit size checks with cet::exception after reading each vector.

---

🟠 Moderate Issues

3. Unchecked vector indexing in accessors (operator[] instead of .at())

In ProductionTarget.hh, the original code used operator[] for several indexed accessors (already partially fixed in the latest commit per Copilot review suggestions). Verify all indexed accessors now use .at():

• plateMaterial(int i), plateROut(int i), plateFinAngle(int i), plateThickness(int i), plateLugThickness(int i), supportRingCutoutAngle(int i)

4. Misleading comment in geometry config file

double targetPS_productionTargetMotherHalfLength = 120.; //mm  full length 232.2 mm

The comment says "full length 232.2 mm" but 2 × 120 = 240 mm. Either the value or the comment is wrong. This is in a geometry definition file so correctness matters.

5. _halfStickmanLength comment is confusing

double targetPS_halfStickmanLength = 116.1; // mm. 35 * 6 mm / 2 + 8.1 mm + 3 mm. We do not need this except for debugging.

The comment says "We do not need this except for debugging" but this value is actually used in the construction logic (e.g., _currentZ calculation and support ring placement in constructTargetPS.cc). The comment is misleading and should be corrected.

6. Inconsistent error message wording in GeometryService.cc

throw cet::exception("GEOM") << " " << static_cast<char const*>(__func__) << " illegal production target version specified in GeometryService_service = " << _config->getString("targetPS_model")  << std::endl;

The message says "version" but the selection is by targetPS_model. Should say "model" for clarity.

7. Unused variables in constructTargetPS.cc Stickman branch

• prodTargetVisible, prodTargetSolid, forceAuxEdgeVisible, placePV, doSurfaceCheck — these are declared from geomOptions but several are never used in the Stickman plate placement path (plates use finishNesting with string-based lookup token instead). This will generate compiler warnings and may indicate missing visualization/placement logic.
• zeroTranslation is declared but never referenced.

---

🟡 Minor / Style Issues

8. Very large code duplication from Hayman support structures

The support wheel/spoke construction (~200 lines) in constructTargetPS.cc is nearly verbatim copy-pasted from the Hayman branch. The PR author acknowledges this in a comment. While understandable for isolation, this creates a long-term maintenance burden. Consider extracting a shared helper function.

9. _hallOriginInMu2e is assigned but only used in one debug print

G4ThreeVector _hallOriginInMu2e = parent.centerInMu2e();

This is only referenced in one verbosityLevel > 0 print. The leading underscore also suggests it's a member variable, which is misleading for a local.

10. geom_run1_a_stickman.txt header comment is stale

// Based off Mu2eG4/geom/geom_2021_PhaseI_v02.txt
// Updates made to include the detector stand geometry for the PTM

This header comment is inherited from another file and doesn't describe the Stickman geometry purpose.

11. return 0; removed from maker — good, but note error path change

The old code in ProductionTargetMaker::make() had a dead return 0; after the throw. The PR correctly removes this. However, the new error message in make() now says "model" while the GeometryService.cc still says "version" — these should be consistent.

---

Summary

Severity	Count	Key Items
🔴 Critical	2	Default geometry changed globally; no vector size validation
🟠 Moderate	5	Unchecked indexing, misleading comments, unused variables
🟡 Minor	4	Code duplication, stale comments, naming conventions

Recommendation: The most important fix before merging is reverting geom_common.txt to keep geom_run1_a.txt as the default, and adding size validation for the per-plate/per-fin config vectors in makeStickman_v_1_0.

[oksuzian]

@FNALbuild run build test

[FNALbuild]

⌛ The following tests have been triggered for 63edf06: build (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at 63edf06.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 63edf06 at 1d1b9f0
build (prof)	✅	Log file. Build time: 04 min 06 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❓	Log file. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (0) FIXME (3) in 7 files
clang-tidy	➡️	6 errors 326 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 63edf06 after being merged into the base branch at 1d1b9f0.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[YongyiBWu]

1. Stickman is the actual as-built target. This should become the default geometry. The change is intentional. As requested, a Offline/Mu2eG4/geom/geom_run1_a_stickman.txt file is added along-side Offline/Mu2eG4/geom/geom_run1_a.txt, which can be simply switched in geom_common.txt.
2. Changed. Additional checks were added in the new commit.
3. Changed as suggested.
4. Comment updated. As explained in the comment, the mother volume is made slightly larger than the actual target size.
5. Corrected.
6. Changed as suggested.
7. "zeroTranslation" references are removed. The other variables are used in placing the rods, so they are not orphaned.
8. Actually there are corrections in the wheel construction code, as the Hayman construction was flawed. I think it makes sense to leave the Hayman codes (legacy) as it is so we can easily go back to an old geometry without the need to change codes, and make corrections for the Stickman alone. If there are any future corrections they can be applied to the new geometry alone.
9. This appeared for the other geometries too. Necessary print out for understanding the geometry when debugging.
10. Updated.
11. Both lists errored model now.

[YongyiBWu]

@oksuzian I finished addressing the review comments.

[YongyiBWu]

The new commit is for a minor wheel spoke wire fix. It was noticed the wheel rods were not pinned exactly to the wheel center plane. Added variable and appropriate access to hold this variable. The actual rod offsets now becomes the offset between rod center to pinhole, plus the new offset divided by cos(rotY). Note with this change the unconstrained spoke wire lengths now agree to within 0.5 mm (rounding errors, previously 3mm), proving the correctness of this change.

[YongyiBWu]

@FNALbuild run build test

[FNALbuild]

⌛ The following tests have been triggered for 621df83: build (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at 621df83.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 621df83 at 1d1b9f0
build (prof)	✅	Log file. Build time: 08 min 31 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❓	Log file. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (0) FIXME (3) in 7 files
clang-tidy	➡️	6 errors 326 warnings
whitespace check	➡️	found whitespace errors

N.B. These results were obtained from a build of this Pull Request at 621df83 after being merged into the base branch at 1d1b9f0.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[YongyiBWu]

@FNALbuild run build test

[FNALbuild]

⌛ The following tests have been triggered for dc28811: build (Build queue - API unavailable)

[FNALbuild]

☀️ The build tests passed at dc28811.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged dc28811 at 1d1b9f0
build (prof)	✅	Log file. Build time: 04 min 07 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❓	Log file. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (0) FIXME (3) in 7 files
clang-tidy	➡️	6 errors 326 warnings
whitespace check	➡️	found whitespace errors

N.B. These results were obtained from a build of this Pull Request at dc28811 after being merged into the base branch at 1d1b9f0.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.