fix: clear helium frustum from the SBT side sensors in X

[olantwin]

Fixes a verified high-severity geometry overlap from the Fable review: the helium decay volume interpenetrated the ±X SBT side sensors by 120 mm along the full length of the decay volume.

Root cause

The helium frustum was inset from the container by the same container_thickness + helium_clearance (235 mm) in both X and Y. But the ±X side-sensor containers are additionally shifted inward by half a flange width (SBTSensorBuilder.cpp: x_half - 0.5*hbeam_flange_width - container_thickness). So the sensor inner face sits further in than the helium's inset accounted for.

Fix

Split the inset:

• insetY = container_thickness + helium_clearance (unchanged — top/bottom faces have no flange shift)
• insetX = insetY + 0.5*hbeam_flange_width

Numeric check (entrance face, x_half = 1000)

	value
sensor inner face	1000 − 130 − 225 = 645 mm
old helium edge	1000 − 235 = 765 mm → overlap 120 mm
new helium edge	1000 − 365 = 635 mm → clears by 10 mm (= helium_clearance)

Exit face (x_half = 2000): 1635 vs 1645 → same 10 mm clearance.

Verification

pixi run build clean; pixi run test → 33/33 passed. (Note: the automated OverlapCheck is a subsystem-level bounding-box check and does not probe intra-decay_volume sibling overlaps, so this fix is verified algebraically rather than by that test.)

Summary by CodeRabbit

• Bug Fixes
  • Corrected the decay volume geometry so the inner clearance is handled differently for the side and top/bottom faces, improving the fit around surrounding sensor structures.
  • Clarified the geometry notes to better describe the spacing and inset behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

The helium decay region frustum geometry calculation in DecayVolumeFactory.cpp was updated to use distinct inset values for X and Y directions instead of a single shared inset, with insetX accounting for an additional H-beam flange width offset. The sbt.toml comment was expanded to document this logic.

Changes

Helium Frustum Inset Calculation

Layer / File(s)	Summary
Split X/Y inset logic subsystems/DecayVolume/src/DecayVolumeFactory.cpp, subsystems/DecayVolume/sbt.toml	Computes separate insetY (thickness + helium clearance) and insetX (adds half H-beam flange width) values, applies them to the dx1/dy1/dx2/dy2 GeoTrap dimensions, and expands the sbt.toml comment describing the X/Y inset logic.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• ShipSoft/Geometry#33: Reworked DecayVolumeFactory to size the central helium frustum from SBTConfig/sbt.toml, directly overlapping with this inset logic update.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main fix: clearing the helium frustum from the SBT side sensors in X.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/helium-sbt-inset

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[olantwin]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[olantwin]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

subsystems/DecayVolume/src/DecayVolumeFactory.cpp (1)

124-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a regression test asserting the numeric X clearance fix.

The existing test only checks that the helium GeoTrap exists, not that insetX/insetY actually produce the 10 mm clearance from the ±X sensor faces this PR fixes. A targeted assertion (e.g., computing expected dx1/dx2 from SBTConfig and comparing against the built shape's dimensions, or checking the gap between the helium X face and side sensor container) would lock in the fix and prevent regressions.

🤖 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/DecayVolume/src/DecayVolumeFactory.cpp` around lines 124 - 130,
Add a regression test around DecayVolumeFactory that verifies the numeric X
clearance fix, not just that the helium GeoTrap is created. Use SBTConfig and
the DecayVolumeFactory-built shape to assert the expected X-side gap derived
from insetX/insetY, for example by checking the computed dx1/dx2 or the
resulting helium container dimensions against the intended 10 mm clearance from
the ±X sensor faces. Keep the test targeted to the geometry-building path so the
fix in DecayVolumeFactory is locked in against future changes.

🤖 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.

Nitpick comments:
In `@subsystems/DecayVolume/src/DecayVolumeFactory.cpp`:
- Around line 124-130: Add a regression test around DecayVolumeFactory that
verifies the numeric X clearance fix, not just that the helium GeoTrap is
created. Use SBTConfig and the DecayVolumeFactory-built shape to assert the
expected X-side gap derived from insetX/insetY, for example by checking the
computed dx1/dx2 or the resulting helium container dimensions against the
intended 10 mm clearance from the ±X sensor faces. Keep the test targeted to the
geometry-building path so the fix in DecayVolumeFactory is locked in against
future changes.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 93cb1e5f-8241-4060-baec-71c29381c50d

📥 Commits

Reviewing files that changed from the base of the PR and between 805969f and fe4dd00.

📒 Files selected for processing (2)

• subsystems/DecayVolume/sbt.toml
• subsystems/DecayVolume/src/DecayVolumeFactory.cpp

[jasminwss]

what is unclear to me here: from Santiagos STEP files I know: the distance in x between the inner walls of the cells In the beginning of the SBT is 1m in total. Here the helium should be placed (at the beginning of the SBT in +-dx = 0.635 cm = 100 -22.5-1-13 cm = x_half_entrance_mm - container_thickness_mm - helium_clearance_mm - 0.5 beam_flange_width_mm). Why are the containers additionally shifted inwards in X but not in Y?

[matclim]

I had a lool and I think the inset arithmetic can't be made right, because the problem isn't the value of the inset: it's that it's computed here at all.

I mean this fix will work but it isn't robust to future changes in the SBT geometry.

I suggest we tackle things a bit differently: the root cause is that DecayVolumeFactory holds a second, unsynchronised model of where the SBT is. Any expression written there will drift the next time either builder changes, and OverlapCheck won't notice. It's a subsystem-level bounding-box test and doesn't probe siblings inside decay_volume. That's why the original 120 mm overlap survived. I think CodeRabbit's suggested test (assert dx1/dx2 numerically) would make this worse: it pins the current formula, so a legitimate SBT change fails the test and the natural response is to update the expected number: accepting whatever overlap the change introduced.

I would suggest to instead move the placement rules (sideContainerCentreX, topBottomContainerCentreY, longBeamCentreY, zSplitOffset, xHalfAt/yHalfAt) onto SBTConfig as the single source of truth. Both builders place with them; a new SBTEnvelope reads them to derive innerFreeHalfX/Y(z). The arithmetic then exists in only one place. We then build the helium as one GeoTrap per envelope segment (2 per sub-frustum, 20 total) so it follows the shape exactly. helium_clearance_mm becomes a real gap and we can set it to 0. The algebraic verification can be replaced with a SAT test over the built tree. We would then assert that no helium slab intersects any SBT volume, and that the closest approach equals helium_clearance_mm (so an over-conservative envelope fails too). This is robust against re-parameterisations we may introduce in the future.

With that, the helium touches the SBT everywhere and protrudes nowhere (min separation 0.000 mm), and the fiducial volume goes to 485.85 m³ vs 482.41 m³ for this PR's (overlapping) shape. The SBT geometry will anyways undergo some fixes which should fit in nicely with this category of change.

I have an alternative build using this idea and can open a PR with it if this is seen as acceptable.

[olantwin]

I had a lool and I think the inset arithmetic can't be made right, because the problem isn't the value of the inset: it's that it's computed here at all.

I mean this fix will work but it isn't robust to future changes in the SBT geometry.

I suggest we tackle things a bit differently: the root cause is that DecayVolumeFactory holds a second, unsynchronised model of where the SBT is. Any expression written there will drift the next time either builder changes, and OverlapCheck won't notice. It's a subsystem-level bounding-box test and doesn't probe siblings inside decay_volume. That's why the original 120 mm overlap survived. I think CodeRabbit's suggested test (assert dx1/dx2 numerically) would make this worse: it pins the current formula, so a legitimate SBT change fails the test and the natural response is to update the expected number: accepting whatever overlap the change introduced.

I would suggest to instead move the placement rules (sideContainerCentreX, topBottomContainerCentreY, longBeamCentreY, zSplitOffset, xHalfAt/yHalfAt) onto SBTConfig as the single source of truth. Both builders place with them; a new SBTEnvelope reads them to derive innerFreeHalfX/Y(z). The arithmetic then exists in only one place. We then build the helium as one GeoTrap per envelope segment (2 per sub-frustum, 20 total) so it follows the shape exactly. helium_clearance_mm becomes a real gap and we can set it to 0. The algebraic verification can be replaced with a SAT test over the built tree. We would then assert that no helium slab intersects any SBT volume, and that the closest approach equals helium_clearance_mm (so an over-conservative envelope fails too). This is robust against re-parameterisations we may introduce in the future.

With that, the helium touches the SBT everywhere and protrudes nowhere (min separation 0.000 mm), and the fiducial volume goes to 485.85 m³ vs 482.41 m³ for this PR's (overlapping) shape. The SBT geometry will anyways undergo some fixes which should fit in nicely with this category of change.

I have an alternative build using this idea and can open a PR with it if this is seen as acceptable.

Thanks a lot @matclim, this indeed seems like the correct way forward. Calculating the same thing in multiple places will always be extremely fragile and needs to be avoided. I'll create an Issue from your comment and would greatly appreciate the PR.

Shall we merge this fix of the symptom here in the meantime or just wait for the fix of the underlying problem in your PR?

[matclim]

I suggest we keep this PR up but not merge it, I'll try to bring my PR up this evening (can't earlier as I have a lot of meetings in the meantime).

[olantwin]

Ok, sounds good!

[matclim]

PR #68 is up and ready to be reviewed, it should (in my opinion) supersede this one.

[olantwin]

Superseded by #68