Modeling Algorithms - Select parts of the tool, not the cut result, in BRepFeat_MakeCylindricalHole

[gsdali]

PerformThruNext, PerformUntilEnd, the ranged Perform(Radius, PFrom, PTo), and PerformBlind all choose which part of the drilling tool to keep by driving BRepFeat_Builder with the wrong operation:

SetOperation(Fuse);        // -> BOPAlgo_CUT
BOPAlgo_BOP::Perform();    // myShape := object CUT tool
PartsOfTool(parts);        // explores myShape for solids

BRepFeat_Builder::PartsOfTool() collects the solids of myShape, which only holds the tool split by the object after a BOPAlgo_COMMON operation. After a BOPAlgo_CUT, myShape is the finished workpiece, so the selection loop compares barycentres of the cut result and registers pieces of it as kept parts of the tool. PerformResult() then takes the kept-parts path with a keep set containing no tool part at all, returning the object with the cylinder's faces imprinted on it and no material removed, while reporting BRepFeat_NoError throughout.

BRepFeat_Form (BRepFeat_Form.cxx:806) and BRepFeat_RibSlot (BRepFeat_RibSlot.cxx:224), the other two users of the same builder, both call the two-argument SetOperation(myFuse, bFlag) with bFlag true before Perform(), selecting BOPAlgo_COMMON so that PartsOfTool() means what its name says. PerformResult() resets myOperation from myFuse before building, so the operation finally built is still a CUT either way. BRepFeat_MakeCylindricalHole calls the one-argument overload at all four selecting sites. Perform(Radius), the infinite-cylinder through-all mode, selects no parts and is unaffected.

The defect is hidden whenever the cut result has one solid, the case every existing test covers. It shows up as soon as it has two: a drill axis crossing two bodies of a compound, or, with no compound at all, a single bar the bore severs in half.

Measured

Two 50 x 50 x 20 plates stacked on the drill axis, drilled at r = 5 from (0, 0, 15) along -Z (one bore removes 1570.7963):

call	status	before	after
Perform(R)	NoError	3141.5927	3141.5927 (unaffected)
PerformUntilEnd	NoError	0.0000	3141.5927
PerformThruNext	NoError	1570.7963	1570.7963 (unaffected)
PerformBlind(20)	NoError	0.0000	1178.0972
Perform(R, 0, 70)	NoError	0.0000	3141.5927
Perform(R, 0, 30)	NoError	1570.7963	1570.7963
Perform(R, 30, 70)	NoError	1570.7963	1570.7963

PerformBlind(20)'s length is measured from the axis origin (0, 0, 15), which sits 5 units above plate A's entry face at axis parameter 5, not from that face. A blind depth of 20 therefore reaches axis parameter 20, boring only 15 units of the plate's 20-unit thickness: 1178.0972, matching pi * r^2 * 15. That is the mode's own contract, not a discrepancy against the other rows.

A single 8mm bar an r = 5 bore severs, where the CUT result is one workpiece split into two pieces rather than two separate bodies:

call	status	before	after
PerformUntilEnd	NoError	0.0000	1407.2952
PerformThruNext	NoError	0.0000	1407.2952
Perform(R, 0, 30)	NoError	0.0000	1407.2952

A single plate (one solid before and after the CUT) is byte-identical before and after; nbparts never reaches 2 there.

Two behaviour changes worth flagging

A status change for an oversized radius. A radius large enough to swallow the whole workpiece used to report BRepFeat_InvalidPlacement for PerformUntilEnd and PerformThruNext, because the CUT emptied myShape and nbparts was 0. Under BOPAlgo_COMMON the tool meets the whole workpiece, nbparts is 1, and both modes now return the same result Perform(Radius) already returns for the same input (measured directly against this change: both go from InvalidPlacement to NoError, matching Perform(Radius)'s existing NoError).

A second, separate defect in the same heuristic, reported here but not fixed. PerformThruNext's closest-interval fallback nests its // parbar > Last branch inside if (parbar < First), as the else of the distance comparison, so the "beyond Last" case is unreachable as written:

if (parbar < First)
{
  if (First - parbar < dmin)
  {
    dmin   = First - parbar;
    tokeep = its.Value();
  }
  else
  { // parbar > Last
    ...
  }
}

PerformBlind's equivalent fallback has no such structure. It compares std::abs(First - parbar) < dmin uniformly. The fallback only runs when no tool part's barycentre lies in [First, Last], which none of the geometries measured here reach, so it is left alone: changing it without a case that exercises it would be a guess.

Validation

• git apply --check against current master (this PR's base): applies with no fuzz.
• git clang-format against the repository's own .clang-format, restricted to the changed lines: no reformatting.
• Compiled BRepFeat_MakeCylindricalHole.cxx from current master, once unmodified and once with this change, as override translation units linked ahead of a prebuilt OCCT 8.0.1 archive (-DNo_Exception, matching a release build), and ran a probe that drives all four modes plus the unaffected Perform(Radius) over six geometries (a single plate, two- and three-plate compounds, a one-solid channel with two spans on the axis, a bar the bore severs, and a hollow box). The unmodified build reproduces every "before" figure above; the changed build reproduces every "after" figure, and the two non-regression geometries (channel, hollow box) are byte-identical between the two builds.
• Separately verified the oversized-radius status change (InvalidPlacement to NoError) the same way.
• Read BRepFeat_Builder::SetOperation/PerformResult on current master directly to confirm the two-argument overload's semantics and that PerformResult() re-derives the final operation from myFuse regardless of the earlier call.

Found while measuring hole-drilling extents for the OCCTSwift wrapper, which currently works around this bridge-side by driving BRepAlgoAPI_Cut directly (Shape.drilled) rather than this class for a stack. Full writeup, reproducer and before/after transcripts: https://github.com/SecondMouseAU/OCCTSwift/tree/main/Scripts/repro/532-cylindrical-hole-part-selection

[dpasukhi]

Dear @gsdali please add GTests into the commit, where you tested your code.
It would simplify integration for me.

[dpasukhi]

Dear @gsdali please prepare GTEst your changes.
Also,

// split the tool, so PartsOfTool() below has parts to select

Comment have not benefits as a part of code, please remove these comments

[gsdali]

I've seen the comment, we've just worked through our release against 8.0.1 and can now take a look at this.

[dpasukhi]

I've seen the comment, we've just worked through our release against 8.0.1 and can now take a look at this.

Thank you! It is not urgent ;) Our next release is far. Take your time ;)

Many thanks for your contribution.

[gsdali]

Thanks — pushed an update.

GTest added: BRepFeat_MakeCylindricalHole_Test.cxx in src/ModelingAlgorithms/TKFeat/GTests/ (first test in this toolkit), covering the two-plate stack from the table above for PerformUntilEnd/PerformBlind/the ranged Perform, plus Perform(Radius) and a single-plate case as controls. Verified both ways: linked with an unmodified translation unit ahead of the archive, the three affected modes remove ~0 material, matching the table's "before" column; linked with the patched one, all five tests pass with the measured volumes.

Comment removed: dropped // split the tool, so PartsOfTool() below has parts to select from all four SetOperation(Fuse, true) call sites — matches the two sibling callers (BRepFeat_Form.cxx:806, BRepFeat_RibSlot.cxx:224), neither of which comments its own call.