Modeling Algorithms - Fix Defeaturing failing to remove a corner round

[GabrielJMS]

Pre-Submission Checks

• I checked existing issues, pull requests, discussions, and forum topics for related work.
• I followed the contribution guidance in .github/CONTRIBUTING.md.
• I used a PR title in the Group - Summary format.

Fixes #1521.

Problem / Motivation

BRepAlgoAPI_Defeaturing cannot remove a convex cylindrical corner round, while the concave fillets of the very same part are removed without trouble. The operation reports BOPAlgo_AlertUnableToRemoveTheFeature and returns the shape unmodified.

FillGap::ExtendAdjacentFaces sizes the extension of the adjacent faces from the bounding box of the feature:

Bnd_Box aFeatureBox;
BRepBndLib::Add(myFeature, aFeatureBox);
const double anExtLength = sqrt(aFeatureBox.SquareExtent());

That length is unrelated to how far the adjacent faces actually have to travel to meet each other. For the part attached to #1521 the extension is 10.7278, and the extended neighbour plane reaches x = -58.44 where it needs x ~ -56.85 to intersect the extended neighbour cylinder - it misses by ~1.6 mm.

With no closing edge from that intersection, every split of the extended face still carries a free boundary edge of the extension, the ext-edge filter discards all of them, aLFTrimmed ends up empty and FillGap::TrimFace falls into its last-resort branch:

else if (aLFTrimmed.IsEmpty())
{
  // Use all splits, including those having the bounds of extended face

The untrimmed extensions then leak into the reconstruction handed to BOPAlgo_MakerVolume - one adjacent face keeps 2% of its area, another contributes pieces sticking 10.7 mm above and below the solid, a third contributes its whole extended plane. MakerVolume builds 0 solids (no errors, one BOPAlgo_AlertFaceBuilderUnusedEdges warning) and RemoveFeature takes the !anExpS.More() branch.

On that part, reaching the fallback branch correlates perfectly with failure: 20 of the 48 faces reach it and all 20 fail, while all 9 faces removable today reach it zero times.

Proposed Solution

Treat the degenerate trim as the retry signal it already is:

• FillGap::TrimFace raises a new myTrimDegenerated flag when it takes the "use all splits" branch.
• FillGap::Perform repeats extension + trim with a doubled length, up to three attempts, stopping at the first attempt that trims cleanly.
• If no attempt trims cleanly, the result of the first one is restored, so every case that passes through the fallback today produces exactly the result it produces now.
• ExtendAdjacentFaces takes the length as a parameter instead of recomputing it.

The retry only runs for features that currently reach the degenerate branch, so unaffected cases pay nothing.

Validation

Original

Defeatured

Built and tested on Linux x64, GCC 13.3, on this branch's base (IR @ c7ea5bf865, 8.1.0.dev1).

testgrid boolean removefeatures, same build with the patch toggled:

	result
without the patch	1 FAILED, 27 OK, 31 SKIPPED
with the patch	28 OK, 31 SKIPPED

The single failing case is the new test, and the per-case diff of the two runs is exactly one line:

< CASE boolean removefeatures G5: FAILED (error)
---
> CASE boolean removefeatures G5: OK

None of the 27 existing cases changes state.

Checks performed:

• Relevant tests were added or updated when applicable.
• Relevant local tests were run.

CLA Confirmation

• I confirm that I have read the contribution requirements, and that I have signed and submitted the CLA or I am covered by an approved company CLA.

CLA ID / submission reference: Pending

Review Notes

This PR needs a data file added to the test dataset. tests/boolean/removefeatures/G5 loads bug1521_corner_round.step, which is attached to #1521 (https://gist.github.com/GabrielJMS/6c722dc8e675c42211af777e342aeb25) and is not yet in opencascade-dataset. The test will report SKIPPED until it is added - happy to submit it wherever it should go.

31 of the 58 removefeatures cases were SKIPPED locally because their data files are absent from the public opencascade-dataset-7.9.0 archive (bug28840_*.brep, bug29481_L3.step, bug29481_window_slots.brep, bug30094.brep, bug26689_nist_ctc_01_asme1_ap242.stp), so I could not exercise those - CI covers them.

The number of attempts (3) and the doubling factor are the conservative values that fix the reported part; happy to change either if you would rather scale the extension differently, for instance from the bounding box of the adjacent faces instead of retrying.

[GabrielJMS]

@dpasukhi Sorry, I deleted my previous comments because I think it’s actually a completely different issue, unrelated to this bug.

I’m now trying to defeature the fillet chain {113, 114, 115, 116, 117, 134, 137} on the attached shape, but I haven’t been able to figure out what the problem is with this one.

So, from my point of view, I think the fix is safe to go ahead with. Concerning this other case, I haven’t given up, I’ll keep investigating and propose a new PR if I figure it out.

piston.zip

[GabrielJMS]

@dpasukhi Sorry, I deleted my previous comments because I think it’s actually a completely different issue, unrelated to this bug.

I’m now trying to defeature the fillet chain {113, 114, 115, 116, 117, 134, 137} on the attached shape, but I haven’t been able to figure out what the problem is with this one.

So, from my point of view, I think the fix is safe to go ahead with. Concerning this other case, I haven’t given up, I’ll keep investigating and propose a new PR if I figure it out.

piston.zip

Found a solution inspired by Woo's cell-based decomposition: #1524

[dpasukhi]

There is no delays from your side ;)

At the moment, I have no ability to validate your PR :( probably it will be opened up to next weekends or maybe I will have some time on Sunday.

Thank you for your contribution

[dpasukhi]

Also if possible, it Is better to create "synthetic" Google test (s) which can be unit or integration in some way. If you want screenshots, then draw still accepted, but better with no external files

We are no longer takes draw tests with external files (to simplify future life). If it is possible somehow to "isolate" the case to some syntactic geometry/topology generated by c++ as a GTest it would be amazing.

Your brep/step files will be used during review, but they will be not be merged :( im sorry

[GabrielJMS]

Also if possible, it Is better to create "synthetic" Google test (s) which can be unit or integration in some way. If you want screenshots, then draw still accepted, but better with no external files

We are no longer takes draw tests with external files (to simplify future life). If it is possible somehow to "isolate" the case to some syntactic geometry/topology generated by c++ as a GTest it would be amazing.

Your brep/step files will be used during review, but they will be not be merged :( im sorry

Ok. Thank you! Fixed!

[BarionLP]

using this branch i was able to remove multiple features that I previously had to do by hand, thanks

[BarionLP]

i think there could be a bug:
If the intersection of extended faces fails outright on a retry (BOPAlgo_AlertBuilderFailed), TrimExtendedFaces returns early with myFaces empty and myTrimDegenerated == false. The loop takes that as success with zero reconstructed faces,
throwing away the (partial) result of attempt 0.

[GabrielJMS]

@BarionLP Thank you for the feedback! I will investigate it!

Also, have you tried it with this branch? #1525

I’ve broken down the fixes into a series of chained PRs to address the different bugs I found and make the review easier.