Skip to content

fix(#905): carry the BRepOffsetAPI_ThruSections capping-guard kernel patch - #909

Merged
gsdali merged 3 commits into
mainfrom
fix/905-thrusections-capping-guard
Aug 14, 2026
Merged

fix(#905): carry the BRepOffsetAPI_ThruSections capping-guard kernel patch#909
gsdali merged 3 commits into
mainfrom
fix/905-thrusections-capping-guard

Conversation

@gsdali

@gsdali gsdali commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

What & why

ThruSectionsBuilder(isSolid: true) silently omits both end-cap faces for a closed section wire
with two or more periods of out-of-plane variation around the loop (a genuinely non-planar closed
curve) — build() returns true, but checkResult.isValid is false with no localized error,
and it's the root cause behind #702's demoted-solid symptom. Root-caused in
BRepOffsetAPI_ThruSections.cxx's MakeSolid(): it already tracks whether capping succeeded in a
local flag and discards it. This PR carries the fix as Scripts/patches/0026-*.patch, files it
upstream as OCCT#1462 (CI green), and
corrects a process error along the way — see "Notes for the reviewer".

Closes #905

CHANGELOG entry

ThruSectionsBuilder(isSolid: true) no longer silently reports success on an uncapped loft (#905)

ThruSectionsBuilder(isSolid: true) used to silently omit both end-cap faces for a closed section
wire with two or more periods of out-of-plane variation around the loop — a genuinely non-planar
closed curve, not just one with nonzero Z spread. build() returned true, but
shape.checkResult.isValid was false with errorCount == 0 and no localized error, and a
subsequent .healed() call silently demoted the result from a solid to a shell rather than
repairing it (root cause of #702). MakeSolid()'s capping helper (PerformPlan(), which only fits
a plane or a surface already attached to a wire's edges) already tracked whether capping actually
succeeded in a local flag and discarded it, unconditionally marking the result Closed(true)
regardless. Fixed in Scripts/patches/0026-*: throw instead, matching the pattern this same
function already uses for a null shell. Filed upstream as
OCCT#1462, validated via override-link.
Not yet shipped: the fix is carried, not baked into this release's xcframework —
ThruSectionsBuilder(isSolid: true).build() for this input still reports the old, wrong true
until the kernel is next rebuilt and re-pinned.

SemVer impact

NONE for this PR. It carries a kernel patch and documentation only; Package.swift's pin is
unchanged, so no consumer sees any behavior difference from this diff by itself. The fix takes
effect only once the kernel is rebuilt and re-pinned (a separate release-engineering step, same as
patches 0022-0025), at which point it will be a PATCH-level fix: no API signature change,
ThruSectionsBuilder(isSolid: true).build() correctly reports failure instead of a wrong success
for a non-planar closed section wire.

Checklist

  • New or changed behavior is covered by a unit test in the same PR — N/A, see notes below:
    this diff has no OCCTSwift-observable behavior change (the kernel isn't rebuilt), so there is
    nothing for a Swift test to assert yet. The fix's test coverage is the new upstream GTests
    (NonPlanarClosedWireCappingFails, DegenerateVertexEndStillSucceeds) in
    OCCT#1462. A Swift-level regression test
    is deferred to whichever PR next rebuilds/re-pins the kernel, matching how 0022-0025 were
    each handled.
  • Every new test and every new --self-test case was run once with its subject broken — done
    for the upstream GTest, not an OCCTSwift one: override-linked the unpatched .cxx and
    confirmed NonPlanarClosedWireCappingFails fails exactly as the issue describes (IsDone()
    reads true); override-linked the patched .cxx and confirmed it passes, along with
    DegenerateVertexEndStillSucceeds and all pre-existing tests in the file. Also compiled and
    ran the actual BOPAlgo_PaveFillerTest.FuseConeLoftWithBox_DegeneratedEdge, unmodified,
    against both: fails against the first (null-face) attempt's guard, passes against this one.
  • The CHANGELOG entry above is complete, and docs/CHANGELOG.md is not in this diff.
  • The SemVer impact above is stated, and docs/SEMVER.md is not in this diff.

Notes for the reviewer

Process correction, separate from the fix itself. Issue #905's comment 1 (an earlier session)
proposed the guard against a same-repo staging PR on the gsdali/OCCT fork —
gsdali/OCCT#1, that fork's first PR ever (verified via
gh pr list --repo gsdali/OCCT --state all, one result). That isn't how any other patch in
Scripts/patches/README.md was validated: every prior one went straight from local override-link
testing to the real PR against Open-Cascade-SAS/OCCT. Comment 2 on #905 is CI on that fork PR
catching a real regression in the first attempt's null-face guard
(BOPAlgo_PaveFillerTest.FuseConeLoftWithBox_DegeneratedEdge). This PR's fix is a different,
correct approach (see the CHANGELOG entry and the Scripts/patches/README.md write-up), and
gsdali/OCCT#1 has been closed as erroneous — its own last comment explains why and points here.

Why the fix isn't a null-face check. PerformPlan() leaves the cap face null in two different
cases: capping genuinely failed, or the wire is degenerate (a point section from AddVertex(),
e.g. a cone's apex) and needs no cap at all. A null-face check can't tell these apart; the local
bool B that MakeSolid() already computes can, since PerformPlan()'s own degenerate-wire
shortcut leaves it true. Full mechanism in Scripts/patches/README.md's 0026 entry.

Deliberately not done here, matching the precedent for 0022-0025: no OCCT.xcframework
rebuild, no Package.swift re-pin, no Swift-level regression test (would fail against the
currently-pinned kernel). All three are the release-engineering step comment 1 on #905 already
flagged as separate.

gsdali added a commit that referenced this pull request Aug 14, 2026
Real findings from /code-review, addressed:

- MakeSolid()'s new throw could leave face1/face2 (aliased to myFirst/
  myLast, exposed via FirstShape()/LastShape()) holding a real,
  never-added-to-the-shell face when one wire's PerformPlan() succeeded
  before the other's failed. Nullify both before throwing. Re-verified via
  override-link: same 5/5 GTest pass, same FuseConeLoftWithBox_DegeneratedEdge
  pass. Pushed the corrected commit to the same gsdali/OCCT fork branch
  backing Open-Cascade-SAS/OCCT#1462 (force-push, not a new PR) -- CI
  re-running there.

- Scripts/patches/README.md's "Pin consequence" paragraph was self-
  contradictory: said 0026 was outside the pin "same as 0022-0025" while
  also listing the pin as including 0014-0025 (which includes them).
  Package.swift's own census says all fifteen, through 0025, are baked into
  the v2.0.0 release asset. Corrected.

- Documented, not fixed: why GetStatus() stays _Done on this failure path
  (Build()'s generic catch doesn't touch myStatus for any exception source;
  fixing it needs a call-site change the whole point of this fix was to
  avoid, for a value nothing in this tree reads), and why this patch throws
  rather than following 0025's (#597) surface-a-diagnostic-number pattern
  (different problem shape: wrong pass/fail vs. wrong diagnostic).

- Added Scripts/repro/905-thrusections-capping-guard/, matching every other
  carried patch's convention (a standalone ground-truth probe + real
  before/after transcripts) -- missing from the first commit.

- CLAUDE.md: added a pointer to the existing FillingSurface/BRepFill_Filling
  composable workaround for a caller who hits this on a real non-planar
  loft, since ThruSections' own capping stays plane-only.

Filed separately rather than folded in here: a pre-existing, unrelated
bridge gap the reviewer found (ThruSectionsBuilder.generatedFace(from:)
doesn't check IsDone(), can return stale data after a failed reused
build() -- not introduced or worsened by this fix, same shape already
existed for every other exception this file can throw). #910.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@gsdali

gsdali commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed a response to /code-review's findings (commit bd69e40):

  • Fixed, and re-verified via override-link + re-pushed to the fork branch backing
    OCCT#1462 (CI re-running there): the new
    throw could leave face1/face2 (aliased to myFirst/myLast, exposed via
    FirstShape()/LastShape()) holding a real, never-added face. Now nullified before the throw.
  • Fixed: Scripts/patches/README.md's "Pin consequence" paragraph was self-contradictory
    (said 0026 was outside the pin "same as 0022-0025" while also saying the pin includes
    0014-0025, which includes them — Package.swift's own census confirms all fifteen through
    0025 are baked in).
  • Documented, not fixed: why GetStatus() stays _Done on this path, and why this patch
    throws rather than following sibling 0025's (GeomFill_Sweep.cxx overwrites a measured approximation error with the requested tolerance #597) surface-a-diagnostic-number pattern — both
    addressed directly in Scripts/patches/README.md's 0026 entry.
  • Added: Scripts/repro/905-thrusections-capping-guard/, matching every other carried patch's
    convention (missing from the first commit).
  • Filed separately, not folded in: ThruSectionsBuilder.generatedFace(from:) not checking
    IsDone() — a real, pre-existing bridge gap, but not introduced or worsened by this fix (the
    same partial-mutation-before-failure shape already existed for every other exception this OCCT
    class can throw). #910.

gsdali and others added 3 commits August 14, 2026 23:42
…patch

ThruSectionsBuilder(isSolid: true) silently omits both end-cap faces for a
closed section wire with k >= 2 periods of out-of-plane variation around the
loop. MakeSolid()'s local `B` already tracks whether capping succeeded and
discards it, unconditionally marking the result Closed(true) regardless.

Carry Scripts/patches/0026-BRepOffsetAPI_ThruSections-capping-guard-905.patch:
throw StdFail_NotDone when B is still false after the capping block, matching
the exception this same function already throws for a null shell. No
signature or call-site change. Distinguishes a genuine capping failure from a
degenerate (point) end needing no cap, which an earlier null-face-based
attempt could not (it regressed
BOPAlgo_PaveFillerTest.FuseConeLoftWithBox_DegeneratedEdge).

Validated via override-link (compile the changed source standalone, link
ahead of the prebuilt OCCT archive, no full rebuild): the new upstream GTest
fails against the unpatched source and passes against the patched one, and
the actual FuseConeLoftWithBox_DegeneratedEdge GTest passes unmodified
against the patched source.

Filed upstream as Open-Cascade-SAS/OCCT#1462, CI green.

Documents the process correction: an earlier session had opened a same-repo
staging PR on the gsdali/OCCT fork (gsdali/OCCT#1, that fork's first PR) to
CI-validate a first attempt before submitting upstream. That isn't how any
other carried patch here was validated, and the first attempt's null-face
guard is what regressed the GTest above. gsdali/OCCT#1 is closed as
erroneous; this commit's Scripts/patches/README.md entry says so.

Not yet in a rebuilt xcframework -- Package.swift's pin is unchanged. That is
a separate release-engineering step, tracked the same way patches 0022-0025
already are.

Closes #905

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Real findings from /code-review, addressed:

- MakeSolid()'s new throw could leave face1/face2 (aliased to myFirst/
  myLast, exposed via FirstShape()/LastShape()) holding a real,
  never-added-to-the-shell face when one wire's PerformPlan() succeeded
  before the other's failed. Nullify both before throwing. Re-verified via
  override-link: same 5/5 GTest pass, same FuseConeLoftWithBox_DegeneratedEdge
  pass. Pushed the corrected commit to the same gsdali/OCCT fork branch
  backing Open-Cascade-SAS/OCCT#1462 (force-push, not a new PR) -- CI
  re-running there.

- Scripts/patches/README.md's "Pin consequence" paragraph was self-
  contradictory: said 0026 was outside the pin "same as 0022-0025" while
  also listing the pin as including 0014-0025 (which includes them).
  Package.swift's own census says all fifteen, through 0025, are baked into
  the v2.0.0 release asset. Corrected.

- Documented, not fixed: why GetStatus() stays _Done on this failure path
  (Build()'s generic catch doesn't touch myStatus for any exception source;
  fixing it needs a call-site change the whole point of this fix was to
  avoid, for a value nothing in this tree reads), and why this patch throws
  rather than following 0025's (#597) surface-a-diagnostic-number pattern
  (different problem shape: wrong pass/fail vs. wrong diagnostic).

- Added Scripts/repro/905-thrusections-capping-guard/, matching every other
  carried patch's convention (a standalone ground-truth probe + real
  before/after transcripts) -- missing from the first commit.

- CLAUDE.md: added a pointer to the existing FillingSurface/BRepFill_Filling
  composable workaround for a caller who hits this on a real non-planar
  loft, since ThruSections' own capping stays plane-only.

Filed separately rather than folded in here: a pre-existing, unrelated
bridge gap the reviewer found (ThruSectionsBuilder.generatedFace(from:)
doesn't check IsDone(), can return stale data after a failed reused
build() -- not introduced or worsened by this fix, same shape already
existed for every other exception this file can throw). #910.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Per okf/policies/changelog-on-merge.md: added as the last commit on the
branch before merging.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@gsdali
gsdali force-pushed the fix/905-thrusections-capping-guard branch from bd69e40 to 9ca09ab Compare August 14, 2026 13:43
@gsdali
gsdali merged commit 54e5c5d into main Aug 14, 2026
7 checks passed
@gsdali
gsdali deleted the fix/905-thrusections-capping-guard branch August 14, 2026 13:53
gsdali added a commit that referenced this pull request Aug 15, 2026
12 findings, all addressed or explicitly documented.

Finding 1 (critical, confirmed live via this PR's own first CI run,
which crashed swift build + test (macOS) with exactly the predicted
SIGSEGV): mismatchedSectionEdgeCountWithoutCheckFailsCleanly needs
patch 0027, which isn't in Package.swift's pinned kernel asset. SwiftPM
runs every test target in one process, so an unguarded crash aborts the
whole suite for every future PR until the pin moves -- the #585 failure
shape. Gated on @test(.enabled(if:
ProcessInfo.processInfo.environment["OCCTSWIFT_LOCAL"] == "1")),
matching #905/PR #909's precedent of adding no Swift test at all for
the identical reason. kernel-integration.yml sets OCCTSWIFT_LOCAL=1
when it builds Scripts/patches/ from source, so the test still runs
(and is verified) there.

Finding 3 (real, verified empirically before documenting): the guard's
inequality test already rejected fewer-edge sections, not just more --
previously silent, invalid-but-"successful" results, not just crashes.
Documented accurately everywhere (patch header, README, CLAUDE.md, this
PR's own SemVer note, which previously undersold this direction).

Finding 4: the punctual-section exemption now also verifies a section
has at least one edge before exempting it (w1Point/w2Point are
vacuously true for a genuinely empty wire). Attempted to reproduce a
live crash for this via both a fresh and reused builder and could not
-- documented as a hardening, not a proven fix; the GTest written for
it could not be made to fail without the check and was removed rather
than kept as unproven coverage.

Finding 5: added
punctualApexWithMatchingSectionsStillSucceedsUnderCreateSmoothed --
nothing previously pinned a legitimate cone-apex loft succeeding once
#913's guard reaches CreateSmoothed (3+ sections). Doesn't depend on
patch 0027 (the exemption itself is unmodified pre-existing behavior),
so it isn't gated; verified passing against both the pinned and the
patched kernel.

Finding 6: added a CLAUDE.md Known OCCT Bugs entry, matching every
other carried patch.

Finding 7: committed a reproducer at
Scripts/repro/913-thrusections-createsmoothed-section-edge-count-guard/,
matching #905's own convention (README + standalone .mm +
stock.txt/patched.txt).

Finding 8: converted guard-let-else-return setup to try #require(...).

Finding 9: hoisted the punctual-section predicate into a lambda shared
with the pre-existing fill loop, in both the local patch and the
already-submitted upstream OCCT#1466 PR (new commit pushed there).

Finding 11: fixed the factually wrong "(zero) edge count" comment --
AddVertex() creates a wire with exactly one degenerate edge, not zero.

Finding 12: SIGSEGV and SIGBUS were both genuinely observed, in
different binaries (a custom-handler standalone reproducer vs. a GTest
binary with OS default signal handling) -- not a contradiction to
resolve to one signal, exactly what heap corruption looks like.
Documented both, with which binary showed which.

Findings 2, 10: no action needed / documented rationale for leaving
the check's placement as-is (see the patch's own updated writeup).

Verified against a full local kernel rebuild with all 17 patches
(patch 0027 v2 correctly recognized as "already applied", confirming
the regenerated patch file matches): full swift test 5534/5534 passing,
all 6 static gates clean.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ThruSectionsBuilder(isSolid: true) silently omits both end caps for a non-planar closed section wire (k >= 2 out-of-plane periods)

1 participant