Skip to content

fix(#1393): trim the infinite lines LocOpe_SplitDrafts hands GeomFill_Pipe - #1627

Closed
gsdali wants to merge 1 commit into
mainfrom
fix/1393-splitdrafts-infinite-pipe-curves
Closed

gsdali wants to merge 1 commit into
mainfrom
fix/1393-splitdrafts-infinite-pipe-curves

Conversation

@gsdali

@gsdali gsdali commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

What & why

Shape.splitDrafts has never once produced a result. Every call that gets past the planar-face
check throws Standard_DomainError("No such curve"), so a shipped, documented, public API returns
nil for every input. Scripts/patches/0034 fixes the kernel, and the regression test becomes a
behavioural one.

The brief this started from, and #1393's own writeup, had the fix in the wrong place, and the
correction is the substance of this PR.
Both concluded that GeomConvert::CurveToBSplineCurve
"has no Geom_Line case" and that adding one was the fix. It has two branches, not one chain:

  • The Geom_TrimmedCurve branch does handle Geom_Line, at GeomConvert.cxx:190, and always
    has. Verified directly: a Geom_TrimmedCurve(line, -50, 50) converts to degree 1 with poles at
    the two trim points.
  • Only the untrimmed branch lacks a line case, and that refusal is deliberate and documented.
    GeomConvert.hxx says above the declaration: "Raises DomainError if the curve C is infinite".
    Geom_Line is infinite. There is no parameter range to convert over, so there is nothing correct
    to add there, and adding one would have produced poles at ±1e100.

The defect is in the caller. LocOpe_SplitDrafts::Perform sweeps the plane/plane intersection
(always a Geom_Line) along new Geom_Line(NormalFg), and hands both, untrimmed, to
GeomFill_Pipe, whose GeomFill_UniformSection constructor calls CurveToBSplineCurve. It makes
the same mistake a second time further down Perform(), on a wire edge's basis curve it has just
deliberately stripped of its Geom_TrimmedCurve wrapper. The patch adds a file-local
TrimInfinite() that leaves a finite curve alone and trims an infinite one, from its own origin,
to the diagonal of the shape's bounding box enlarged by that origin, and applies it at all three
sites. No new toolkit dependency: TKTopAlgo is already in TKFeat's EXTERNLIB.cmake.

Closes #1393

CHANGELOG entry

Shape.splitDrafts works, on a kernel carrying the new patch 0034 (#1393)

Shape.splitDrafts returned nil for every input on OCCT 8.0.1, so the operation had never
succeeded once since it was wrapped. LocOpe_SplitDrafts::Perform sweeps the intersection of the
two drafted planes (always an infinite Geom_Line) along the face normal (another infinite
Geom_Line), and GeomFill_Pipe converts both through GeomConvert::CurveToBSplineCurve, which
refuses an infinite curve by documented design. Every valid call therefore threw
Standard_DomainError("No such curve"), which the bridge's catch (...) turned into nil.

Scripts/patches/0034
trims both lines, and a third at Perform()'s second pipe site, to the shape's own bounding-box
extent before the pipe sees them. On a 10 mm box, drafting half the top face by 10 degrees:

let box = Shape.box(width: 10, height: 10, depth: 10)!
let wire = Wire.line(from: SIMD3(0, -5, 5), to: SIMD3(0, 5, 5))!
let drafted = box.splitDrafts(
    faceIndex: topFaceIndex, wire: wire,
    direction: SIMD3(1, 0, 0),
    planeOrigin: SIMD3(0, 0, 0),
    planeNormal: SIMD3(1, 0, 0),
    angle: 10.0 * .pi / 180.0)!

drafted.faces().count   // 7, against the box's 6
drafted.volume!         // 1022.04, against the box's 1000: exactly the wedge the draft adds

The patch is not in Package.swift's pinned kernel asset, so Shape.splitDrafts still returns
nil for consumers of the released package until a repin. It is also the one carried patch that
will never be retired by a repin: OCCT master deleted LocOpe_SplitDrafts outright in
OCCT#1442 as dead code, so there is no
upstream fix to wait for, and the first kernel bump past that tag removes the class and this
wrapper with it.

SemVer impact

PATCH. No API surface changes. A consumer sees Shape.splitDrafts return a drafted solid instead
of nil, but only against a kernel built from Scripts/patches/; against the pinned asset the
released behaviour is unchanged. No migration.

Checklist

  • New or changed behavior is covered by a unit test in the same PR (not just manual
    verification), see SecondMouseAU/OCCTReconstruct#397
    for the ecosystem-wide test-coverage standard this is piloting.
  • Every new test and every new --self-test case was run once with its subject broken, and the
    failure is reported here, see okf/policies/prove-the-test-fails.md.
  • 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.
    It is assessed at release on main, not per PR.

Notes for the reviewer

How CI will behave, deliberately

Tests/OCCTModelingTests/Issue1393SplitDraftsTests.swift now holds one test per kernel, gated
on OCCTSWIFT_LOCAL, which is the convention 0027's test set (PR #915 review, finding 1):

  • planarRequestIsRefusedOnPinnedKernel, enabled when OCCTSWIFT_LOCAL != "1", asserts the
    refusal. This is what ci.yml's build-and-test runs, and it passes, because the pinned asset
    has no 0034. CI is green, not red.
  • planarRequestDraftsTheFace, enabled when OCCTSWIFT_LOCAL == "1", asserts the drafted result.
    kernel-integration.yml (triggered by Scripts/patches/**, builds V8_0_1 plus every carried
    patch from source, runs OCCTSWIFT_LOCAL=1 swift test) is the job that exercises it.

The refusal assertion is not deleted with nothing measuring the new behaviour, and neither kernel
is left untested. When a repin eventually carries 0034, the refusal test fails loudly on the
pinned side, which is the signal to delete it.

Verification, and proving both tests fail

No full kernel rebuild. Override-link for the C++ level, and an ar splice of the single patched
translation unit into the local libOCCT-macos.a for the Swift level (restored byte-identical
afterwards, cmp clean).

C++, Scripts/repro/1393-splitdrafts/probe.mm. Pristine translation unit
(git show HEAD:<path>), same link line:

extract +X, 10 deg     THREW No such curve
extract -X, 10 deg     THREW No such curve
extract +Z, 10 deg     THREW No such curve
extract +X,  5 deg     THREW No such curve

Patched translation unit:

extract +X, 10 deg     IsDone=1  faces=7 (box has 6)
extract -X, 10 deg     IsDone=1  faces=7 (box has 6)
extract +Z, 10 deg     IsDone=1  faces=7 (box has 6)
extract +X,  5 deg     IsDone=1  faces=7 (box has 6)

The result is geometrically right, not merely non-throwing. Exactly one face is tilted, its normal
(0.174, 0.000, 0.985), which is 10.000 degrees off vertical, and the volume goes 1000 to 1022.04
against the 22.04 the drafted wedge adds (0.5 * 5 * 5*tan(10 deg) * 10), agreeing to five figures.

Swift, both directions (okf/policies/prove-the-test-fails.md):

Kernel OCCTSWIFT_LOCAL Result
patched TU spliced in 1 planarRequestDraftsTheFace passed, refusal test skipped
archive restored 1 planarRequestDraftsTheFace failed: Expectation failed: maybeResult → nil
archive restored unset planarRequestIsRefusedOnPinnedKernel passed, behavioural test skipped

The middle row is the proof the behavioural test measures the fix rather than the absence of a
throw.

No upstream PR, and why not

okf/policies/upstream-occt-patch-process.md says go straight to a PR with a GTest. There is
nothing to open one against.
OCCT master deleted LocOpe_SplitDrafts on 2026-08-07 in
OCCT#1442, "Coding, Modeling Algo - Clean up
dead headers", 236 files and 24858 deletions, by maintainer dpasukhi. git grep SplitDrafts on
upstream/master returns nothing; the class had no caller anywhere in the OCCT tree, not even a
DRAW command, which is both why it was removed and the most likely reason a defect this total
reached 8.0.1 unnoticed. Opening a PR to restore and fix a class a maintainer just deleted as dead
would be arguing with a decision already taken.

The GTest was written and proven both ways all the same, and is kept rather than discarded, at
Scripts/repro/1393-splitdrafts/upstream/LocOpe_SplitDrafts_Test.cxx. It would have been TKFeat's
first (its GTests/FILES.cmake is empty upstream). It fails with the exact
Standard_DomainError: "No such curve" against the unpatched TU and passes against the patched one.

Follow-on

Shape.splitDrafts wraps a class OCCT has removed. That is a decision for a separate issue,
not this PR: the next kernel bump past OCCT#1442 makes Scripts/patches/0034 unapplicable and the
bridge function uncompilable, so both the wrapper and the patch have to go at that point. Recorded
in Scripts/patches/README.md, okf/references/carried-occt-patches.md,
okf/references/known-occt-bugs.md, docs/reference/Shape-Measurement.md and Package.swift's
manifest comment, all of which say "retire by deletion, not by a repin".

Records updated

ls Scripts/patches/*.patch | wc -l now answers 23, and the three places that state it were
moved together: CLAUDE.md ("twenty-three on disk, seventeen pinned"),
okf/references/carried-occt-patches.md (count, its "Pinned against carried" table row) and
Package.swift's manifest comment (count, the enumeration of the unpinned set, and the two
paragraphs that said "the five"). 0034 is the only one of the six unpinned patches with a
Swift-reachable assertion, so the manifest's "it cannot prove any of the five fixes works" sentence
is now correct rather than approximately correct.

Scripts/repro/1393-splitdrafts/README.md carries an explicit "Correction: the missing
Geom_Line case is not the bug" section rather than a silent rewrite, since its first reading is
what the brief for this work inherited.

Gates: all eight plus the four censuses clean.

…_Pipe

Shape.splitDrafts could not succeed for any input. LocOpe_SplitDrafts::Perform
sweeps the plane/plane intersection (always an infinite Geom_Line) along the
face normal (another one), and GeomFill_Pipe converts both through
GeomConvert::CurveToBSplineCurve, which refuses an infinite curve by documented
design, so every valid call threw Standard_DomainError("No such curve").

The missing untrimmed Geom_Line case in CurveToBSplineCurve is not the bug: its
trimmed branch has always converted a line exactly, and the caller is what hands
an infinite one to an API that says it rejects them. Scripts/patches/0034 trims
both, and both again at the second pipe site, to the shape's bounding-box extent.

Not upstream-bound: OCCT master deleted LocOpe_SplitDrafts in OCCT#1442 as dead
code, so the patch retires by deletion at the next kernel bump past that tag.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gsdali

gsdali commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator Author

Closing unmerged. The decision went the other way, and this PR is the artefact of the option not taken: Shape.splitDrafts is being removed instead, with no patch carried.

The reasoning is upstream's rather than ours. LocOpe_SplitDrafts was deleted outright on 2026-08-07 in OCCT#1442, a dead-header cleanup, with no caller anywhere in the OCCT tree, not even a DRAW command. So patch 0034 would have been this project reviving a class its own maintainer had just removed, unable to be filed upstream, and deleted anyway at the first kernel bump past that commit. okf/policies/scope-boundary.md says stay faithful to OCCT, and wrapping what OCCT has deleted is the opposite of that.

None of the work here is wasted, and two parts of it are why the decision could be made at all. The investigation that reached the real mechanism, GeomFill_Pipe being handed infinite lines rather than GeomConvert lacking a Geom_Line case, corrected the brief it was given and is what established that the class cannot work rather than merely being awkward to call. That investigation, both probes and the corrected README, is preserved in Scripts/repro/1393-splitdrafts/ on the removal branch, with an "Outcome" section explaining why the wrapper went.

The GTest written for a PR that could never be filed is the one genuine loss. It is recorded in the reproducer directory rather than deleted.

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.

LocOpe_SplitDrafts (Shape.splitDrafts) has zero test coverage anywhere; ground-truth construction is non-trivial

1 participant