Conversation
…s that are subclassed from Shape
…ect/touch, add new tests and fix xpass and xfail
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## dev #1209 +/- ##
==========================================
+ Coverage 95.49% 95.70% +0.20%
==========================================
Files 34 34
Lines 11549 11647 +98
==========================================
+ Hits 11029 11147 +118
+ Misses 520 500 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Looks like a great performance improvement! I don't have many notes on the methodology change since its a complete departure from my contribution, so as long as the existing and new tests have adequate coverage I'm happy. I will say, this introduces a large number of conditionals in I like the addition
Contrived
|
|
Thanks @jwagenet for looking into it. And sorry that I completely changed to approach, but I did not find a way to improve the performance of the
Very good example, I need to think about it. The actual thought was if And yes, the OTHER check with a bunch of sample points might be a replacement if I can't get
Didn't want to make it a first class citizen right now, since I wasn't too sure it works and that I have thought about all the cases (and I haven't, see your example). And I haven't looked in faces ...
Good point, I wrote it differently before I realized that there is |
I view the geometric equivalency for intersection results as as a practical check and IMO from a naive, practical standpoint I haven't seen a case where edge direction is a critical quality to preserve (face normals seem more important, for example). However I am sure there is a use case I am not familiar with. |
The issue is that Vector(c1.Location()) == Vector(c2.Location())
Vector(c1.Axis().Direction()) == Vector(c2.Axis().Direction())to the comparison of the conic sections. Note: This is cheap, as it only adds Vector comparisons, e.g. 100x100 Vector comparisons take on my M1 Apple 2.9 ms total
And added your example to the test cases. The geometric check attributes are now:
With this implementation, your example and similar are covered. |
|
@jwagenet Currently reversed curves are not seen as equal In [2]: c = CenterArc((1,1,1), 2, 30, 50)
In [3]: c2 = c.reversed()
In [4]: geom_equal(c, c2)
Out[4]: False |
|
And I've removed |
…low passing Edge(axis)
|
@bernhard-42 if you need a way to generate GeomType.OTHER (which I think should help code coverage) here is something that you could adapt for tests (please feel free to use it if it helps): m1 = Spline((0,0,1),(2,0,1),(2,2,2))
rect = Rectangle(10,10).face()
proj = m1.project(rect,direction=(0,0,-1)) # GeomType.OTHER |
Most notable changes:
Final list of new/changed methods
|
It is a wire and wires have geom_type OTHER. But the edges still have "proper" geom_types. According to my analysis, it doesn't seem to be possible to build an edge with GeomAbs_OtherCurve in Python. |
|
I've built a complex example to test the algorithms: It has intersections, touching faces, edges and vertices. I realized that the result wasn't fully correct with this PR, so I streamlined the touch logic while fixing some missing touch edge cases. It now produces the correct result: 
Example code: sl1 = Pos(0, 0.1, 0) * (
Box(2, 2, 2)
+ Pos(0.5, 0, 1) * Box(1, 1, 1)
+ Pos(1, 0, 1) * Box(2, 0.1, 1)
+ Pos(1, -1, 0.5) * Box(0.5, 0.1, 0.5, align=Align.MAX)
+ Pos(1, -1.1, 1) * Box(0.5, 0.1, 1, align=Align.MAX)
+ Pos(0.8, 0.8, 1.1) * Sphere(0.2)
+ Pos(0.8, -0.8, 1.1) * Rot(0, 0, 45) * Sphere(0.2)
+ Location((1 - math.sqrt(0.08), 0.2, 1.55), (0, 45, 0))
* Box(0.2, 0.2, 0.2, align=(Align.MIN, Align.CENTER, Align.MIN))
+ Location((0.9, -0.2, 1.6), (0, 90, 0)) * Cone(0.2, 0, 0.2)
+ Pos(2, -1, -0.3) * Box(2, 0.3, 0.5, align=Align.MAX)
)
sl1 = lc(sl1, "sl1", "purple")
slx = (
Pos(1, 0, -1) * Box(4, 2, 1)
+ Pos(2.5) * Box(1, 2, 3)
+ Pos(2, 0, 1.5) * Box(2, 2, 1)
)
slx = lc(slx, "slx", "lightblue")
intersect = sl1.intersect(slx)
touch = sl1.touch(slx) |
|
I just swapped the algorithm priority then keep whichever succeeds and that fixed it for me, although I didn't test to see if it's robust. I think having the option to filter out shared boundary geometry is a great addition. |
gumyr
left a comment
gumyr
left a comment
There was a problem hiding this comment.
Thank you for the huge amount of effort that went into creating this PR.
I've marked a few comments that are mostly for myself as I'll be merging this PR as is and going back over a few things in the future. I'm planning to substantially use the code from geom_equal as the basis for a set of is_equal methods for each of the Shape derived classes (currently is_equal is quite weak and not used with the build123d code or any of the user projects I could find on GitHub). After considering the pros and cons I'm planning on leaving the __eq__ method as is which is based on is_same behaviour as this matches how hash works and enables reliable set creation.
| raise ValueError("Can't find adaptor for empty edge") | ||
| return BRepAdaptor_Curve(self.wrapped) | ||
|
|
||
| def geom_equal( |
There was a problem hiding this comment.
There are a few things to consider here:
- When comparing Vector a tolerance of TOLERANCE is used not the value provided by the user. To be consistent should those comparisons be changed to
(v1-v2).length < tol? Or should a tol be added toVector.__eq__or maybeVector.is_equal(self, other: Vector, tol:float=TOLERANCE)? - Should edge orientation be considered here? Given that build123d attempts to hide orientation from the user shouldn't this comparison do that too? If so the end point comparison would need to change.
- For periodic curves (Bezier, BSpline) the order of the knots/poles seem like they could be different for the same curve.
- For Ellipse could two curves be considered the same is the major/minor/axis are flipped?
|
|
||
| def test_different_knot_values(self): | ||
| """BSplines with different internal knot positions have different shapes.""" | ||
| from OCP.Geom import Geom_BSplineCurve |
There was a problem hiding this comment.
Imports should be consolidated at the top of the module.
|
|
||
| def test_different_multiplicities(self): | ||
| """BSplines with same poles/knots but different multiplicities have different shapes.""" | ||
| from OCP.Geom import Geom_BSplineCurve |
There was a problem hiding this comment.
Imports should be consolidated at the top of the module.
|
|
||
| def test_rational_bspline_different_weights(self): | ||
| """Rational BSplines with different weights.""" | ||
| from OCP.Geom import Geom_BSplineCurve |
There was a problem hiding this comment.
Imports should be consolidated at the top of the module.
|
|
||
| def test_expand_with_vector(self): | ||
| """ShapeList containing Vector objects.""" | ||
| from build123d import Vector, ShapeList |
There was a problem hiding this comment.
Imports should be consolidated at the top of the module.
|
|
||
| def test_expand_mixed(self): | ||
| """ShapeList with mixed types.""" | ||
| from build123d import Vector |
There was a problem hiding this comment.
Imports should be consolidated at the top of the module.
| - NOT on any edge of the shell (self) | ||
| - NOT on any edge of the face (other) | ||
| """ | ||
| import math |
There was a problem hiding this comment.
Imports should be consolidated at the top of the module.
|
|
||
| Same as above but with arguments swapped to test the 'other is Shell' path. | ||
| """ | ||
| import math |
There was a problem hiding this comment.
Imports should be consolidated at the top of the module.
|
|
||
| Early return when compound has no elements. | ||
| """ | ||
| from OCP.TopoDS import TopoDS_Compound |
There was a problem hiding this comment.
Imports should be consolidated at the top of the module.
|
|
||
| def test_empty_compound_intersect_with_face(self): | ||
| """Empty Compound.intersect(Face) returns None.""" | ||
| from OCP.TopoDS import TopoDS_Compound |
There was a problem hiding this comment.
Imports should be consolidated at the top of the module.
5 participants
I've now finalized the improved implementation of intersect-all.
Intersection Refactoring Summary
Current Implementation
The current implementation uses the (simplified) pattern
Unfortunately, filtering of found objects up to the highest order is not trivial in OCCT and can take a significant time per comparision, especially when solids with curved surfaces are involved.
And given the apporach, n x m comparisions are needed in the filter function (performance details see see #1147).
Goal of the new apporach
include_touchedthat add touch results to the intersect resultsIntersect vs Touch
The distinction between
intersectandtouchis based on result dimension:Touch filtering: At each contact location, only the highest-dimensional shape is returned. Lower-dimensional shapes that are boundaries of higher-dimensional contacts are filtered out. Note that this can get more expensive than the intersect implementation.
Examples:
touch→[Face](not the 4 edges and 4 vertices of that face)touch→[Edge](not the 2 endpoint vertices)touch→[Vertex]intersect→[Face, Edge]Multi-object and Compound handling
* A compound as tool shall not have overlapping solids according to OCCT docs
Key:
Tangent Contact Validation
For tangent contacts (surfaces touching at a point), the
touch()method validates:Edge boundary check: Points near edges of both faces (within
tolerance) are filtered out as edge-edge intersections, not vertex touches. Users should increase tolerance if BRepExtrema returns inaccurate points near edges.Normal direction check: For points in the interior of both faces, normals must be parallel (dot ≈ 1) or anti-parallel (dot ≈ -1), meaning surfaces are tangent. This filters out false positives where surfaces cross at an angle.
Crossing vertices: Points on an edge of one face meeting the interior of another (perpendicular normals) are valid crossing vertices.
Call Flow
Legend:
other._intersect(self, ...)elem._intersect(...)t:include_touchedpassed throughintersect() Call Flow
_intersect(Vertex, Vertex, )_intersect(Vertex, *, t)_intersect(Edge, Edge, )_intersect(Edge, Wire, )_intersect(Edge, Vertex, )_intersect(Edge, *, t)other._intersect(Edge, t)_intersect(Wire, ..., )_intersect(Face, Face, )_intersect(Face, Shell, )_intersect(Face, Edge, )_intersect(Face, Wire, )_intersect(Face, Vertex, )_intersect(Face, *, t)other._intersect(Face, t)_intersect(Shell, ..., )include_touched==True:self.touch(other)_intersect(Solid, Solid, )_intersect(Solid, Face, )_intersect(Solid, Shell, )_intersect(Solid, Edge, )_intersect(Solid, Wire, )_intersect(Solid, Vertex, )_intersect(Solid, *, t)other._intersect(Solid, t)include_touched==True:self.touch(other)_intersect(Compound, Compound, t)_intersect(Compound, *, t)Delegation chains (examples):
Edge._intersect(Solid, t)→Solid._intersect(Edge, t)→ handleVertex._intersect(Face, t)→Face._intersect(Vertex, t)→ handleFace._intersect(Solid, t)→Solid._intersect(Face, t)→ handleEdge._intersect(Compound, t)→Compound._intersect(Edge, t)→ distributetouch() Call Flow
touch(Shape, *)ShapeList()(base impl.)touch(Face, Face)touch(Face, Shell)touch(Face, *)other.touch(self)(delegate)touch(Solid, Solid)<self face>.touch(<other face>)for tangent contactstouch(Solid, Face)touch(Solid, Edge)touch(Solid, Vertex)touch(Solid, *)other.touch(self)(delegate)touch(Compound, *)Code reuse:
Mixin3D.touch()callsMixin2D.touch()(via<self face>.touch(<other face>)) for Solid+Solid tangent vertex detection, ensuring consistent edge boundary and normal direction validation.Comparison Optimizations with non-optimal Bounding Boxes
1. Early Exit with Bounding Box Overlap
In
touch()and_intersect(), we compare many shape pairs (faces×faces, edges×edges). Before callingBRepAlgoAPI_Commonor other expensive methods, we want to early detect pairs that don't need to be checked (early exit)This can be done with
distance_to()calls (which useBRepExtrema_DistShapeShape), or checking bounding boxes overlap:BoundBox.overlaps()uses OCCT'sBnd_Box.Distance()method. Option 2 (bbox) is less accurate but significantly faster, see below.2. Non-Optimal Bounding Boxes
Shape.bounding_box(optimal=True)computes precise bounds but is slow for curved geometry. For early-exit filtering, we useoptimal=False:Accuracy trade-off (non-optimal bbox expansion):
Larger bboxes cause more false-positive overlaps → extra
BRepExtremachecks, but the 100-800x speedup will most of the time outweigh this cost.3. Pre-calculate and Cache Bounding Boxes
Without caching, nested loops recalculate bboxes n×m times:
4. Performance Comparison
Face×face pair comparisons using ttt-ppp01* examples:
Edge×edge pair comparisons using ttt-ppp01* examples:
The bbox approach is in any case significantly faster, making it essential for n×m pair operations in
touch()and_intersect().Geometric Edge Comparisons
Rationale
Edge deduplication in
touch()requires geometric equality comparison, not topological identity. The built-inShape.__eq__checks if two shapes are the same OCC object (topological identity), but we need to know if two independently created shapes represent the same geometry.Example: Two edges from
Edge.make_line((0,0,0), (1,1,1))calls are geometrically equal but topologically distinct—==returnsFalse.Implementation:
geom_equal()in helpers.pyThe
geom_equal()function compares geometric objects for equality within a tolerance:Type handling:
==(uses 1e-6 tolerance)==(quaternion-based comparison)Edge comparison by GeomType:
num_interpolation_pointsalong the curveUsage in touch()
The
is_duplicate()helper inMixin3D.touch()usesgeom_equal()for edge deduplication:This ensures duplicate edges from Common/Section operations are properly filtered, avoiding issues like "duplicate edges in result" that occurred with simpler heuristics.
Edit 21.01. Removed the runtime import
## Typing Workaround### Problem: Circular Dependenciesshape_core.pydefines base classes, but intersection logic needs to check types (isinstance(x, Wire)), call methods (shape.faces()), etc. Direct imports would cause circular import errors.### Solution: helpers.py as a Leaf Modulehelpers.py imports everything at module level (it's a leaf - no one imports from it at module level):Other modules do runtime imports from helpers:Runtime imports happen after all modules are loaded, breaking the cycle.Tests
Test Case Counts
* Parametrized tests include symmetry swaps (A×B also tested as B×A) where applicable
Breakdown by matrix:
Changes Summary
Infrastructure changes:
include_touched: bool = FalsetoCasedataclassrun_testto passinclude_touchedtoShape.intersect(geometry objects don't have it)make_paramsto includeinclude_touchedin test parameters; symmetry swaps disabled forinclude_touchedtests@pytest.mark.parametrizedecoratorsNew test objects:
sh7,sh8: Half-sphere shells for tangent touch testingfc10: Tangent face for sphere tangent contactNew test case categories:
None, withinclude_touched→[Vertex])include_touched: tests for boundary contacts through compoundsBehavioral: Solid boundary contacts (intersect vs touch separation)
[Vertex]None[Vertex][Edge]None[Edge][Vertex]None[Vertex][Edge]None[Edge][Vertex]None[Vertex]None[Face]Behavioral: Face/Shell boundary contacts (intersect vs touch separation)
[Vertex]None[Vertex]None[Vertex]None[Vertex]Two non-coplanar faces that cross at a single point (due to finite extent) now return the vertex via
touch()rather thanintersect(). AddedMixin2D.touch()method.These represent the semantic change: boundary contacts are not interior intersections, so
intersect()returnsNone. Useinclude_touched=Trueto get them.Bug fixes / xfail removals
[Edge]with xfail "duplicate edges"[Edge]passing[Edge, Edge]with xfail[Edge, Edge, Edge, Edge]passingPerformance tests
Summary
Note: Changed test_ppp_0109 to use
extrude(UNITL)instead ofextrudeas indevbranch and this PRCC: @jdegenstein @jwagenet