Skip to content

Avoid duplicate boolean clean passes#1308

Merged
gumyr merged 1 commit into
gumyr:devfrom
sammekekko:batch-fuse-performance
May 13, 2026
Merged

Avoid duplicate boolean clean passes#1308
gumyr merged 1 commit into
gumyr:devfrom
sammekekko:batch-fuse-performance

Conversation

@sammekekko
Copy link
Copy Markdown
Contributor

@sammekekko sammekekko commented May 8, 2026

Avoid running a second clean pass after boolean operations that already clean their result.

The shared boolean helper already applies ShapeUpgrade_UnifySameDomain when SkipClean.clean is enabled. This removes the caller-level clean from direct Shape and Compound additions. Builder context integration now skips the caller-level clean for add, subtract, and intersect paths that call the shared boolean helper, while retaining cleanup for non-boolean integrations that do not go through that helper.

To keep the scope tight, this does not change logging behavior and does not enable OCCT glue.

  • Remove redundant clean work in direct Shape and Compound additions.
  • Skip duplicate clean work when builder context integration uses add, subtract, or intersect.
  • Keep builder cleanup for non-boolean context integration paths.
  • Add regression tests for empty-object batch additions with overlapping boxes, mixed curved solids, and rotated solids.

Validation:

  • uv run --extra development pytest (1988 passed, 2 skipped)
  • uv run --extra development pytest tests/test_benchmarks.py::test_ppp_0106 tests/test_algebra.py tests/test_build_common.py tests/test_build_part.py tests/test_direct_api/test_compound.py tests/test_direct_api/test_shape.py (292 passed)
  • uv run --extra development pytest tests/test_benchmarks.py::test_ppp_0106 tests/test_examples.py -k ppp0106 (1 passed)
  • uv run --extra development pylint --rcfile=.pylintrc --fail-under=9.5 src/build123d

@sammekekko sammekekko marked this pull request as ready for review May 8, 2026 09:49
@gumyr
Copy link
Copy Markdown
Owner

gumyr commented May 8, 2026

The logging changes - if logger.isEnabledFor(logging.INFO): - might make processing a little faster but outside of the very special case of fusing many, many boxes (6 planar sided objects) the extra checks would slow processing down. What is the application that requires merging large numbers of boxes? Couldn't this be done more efficiently in the 2D domain?

@sammekekko
Copy link
Copy Markdown
Contributor Author

Fair point. I’ll trim this PR down so it’s focused on the batch fuse behavior and remove the logging/context micro-optimizations.

The case I was looking at was generated geometry where a lot of simple box-like solids get combined, for example grid/fixture/block-style models. But I agree that when the model is really a set of 2D footprints with a common height, doing the union in 2D and extruding afterward is probably the better approach.

I’ll update the PR so the scope is clearer and less speculative.

@sammekekko sammekekko force-pushed the batch-fuse-performance branch from e9c2155 to a8889e4 Compare May 11, 2026 15:01
@sammekekko sammekekko changed the title Improve batch fuse performance Avoid duplicate boolean clean passes May 11, 2026
@sammekekko
Copy link
Copy Markdown
Contributor Author

I tightened the PR further and force-pushed the reduced version.

This no longer changes logging, location context handling, edge parallelization, or OCCT glue. I also dropped the box-specific optimized path after checking it more closely, since glue is not the right general answer for overlapping boxes.

The PR now only avoids a duplicate clean pass after boolean operations that already clean inside the shared boolean helper. The existing boolean path is unchanged, and I added regression tests for overlapping box batches, mixed curved additions, and rotated additions. Full test suite is green, and the updated PR description has the reduced benchmark numbers.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.17%. Comparing base (40a886d) to head (9d346dc).
⚠️ Report is 30 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1308      +/-   ##
==========================================
+ Coverage   95.98%   96.17%   +0.18%     
==========================================
  Files          35       36       +1     
  Lines       13096    13551     +455     
==========================================
+ Hits        12570    13032     +462     
+ Misses        526      519       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sammekekko sammekekko force-pushed the batch-fuse-performance branch 2 times, most recently from 53f01dd to a8889e4 Compare May 12, 2026 12:32
Copy link
Copy Markdown
Owner

@gumyr gumyr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for identifying this, what appears to be, redundant call to clean.

Comment thread src/build123d/topology/composite.py Outdated
clean_already_done = SkipClean.clean

if SkipClean.clean:
if SkipClean.clean and not clean_already_done:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems as though this clean operation should be removed entirely. Either a single object is in the Compound (should that be cleaned?) or the fuse will have already cleaned the result.

Comment thread src/build123d/topology/shape_core.py Outdated
clean_already_done = SkipClean.clean

if SkipClean.clean and not isinstance(sum_shape, list):
if (
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment, it seems that this clean is redundant. Either a single object is present or the results were already cleaned.

Comment thread src/build123d/build_common.py Outdated
# )

if self._obj is not None and clean:
if self._obj is not None and clean and not clean_already_done:
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here, I think clean is redundant here. I'm guessing clean was added to _bool_op after this code was written and the duplicate clean never identified/removed..

@sammekekko sammekekko force-pushed the batch-fuse-performance branch from a8889e4 to 9d346dc Compare May 13, 2026 15:35
@sammekekko
Copy link
Copy Markdown
Contributor Author

I pushed an update for these review comments.

The direct Shape and Compound addition paths no longer do a caller-level clean after the boolean operation. For builder context integration, add, subtract, and intersect now also skip the caller-level clean after _bool_op() has already handled cleanup.

I kept cleanup for the non-boolean builder integration paths because removing it entirely broke ttt-ppp0106.py: make_face() adds a single face without going through _bool_op(), and the following fillet depends on that cleanup. So the builder path now only cleans when the integration did not run through the shared boolean helper.

Validation is updated in the PR body. Full test suite and pylint both pass locally.

@gumyr gumyr merged commit 5800485 into gumyr:dev May 13, 2026
19 checks passed
@gumyr
Copy link
Copy Markdown
Owner

gumyr commented May 13, 2026

Thank you!

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.

2 participants