Part: Fix mirror() regression with non-identity Placement (#26963)
* Part: Fix mirror() regression with non-identity Placement The makeElementMirror() function incorrectly extracted and pre-multiplied the shape's Location with the mirror transform. Since BRepBuilderAPI_Transform already handles shapes with Location correctly, this resulted in the placement being applied twice, producing incorrect results for shapes with non-identity Placement. Fixes #20834 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Part: Add regression test for mirror() with Placement Adds testTopoShapeMirrorWithPlacement to verify that mirror() produces identical results regardless of whether the shape is positioned via direct coordinates or via Placement. This test would have caught the bug fixed in the previous commit where shapes with non-identity Placement produced incorrect mirror results. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * Fix regression test for mirror with Placement The test was incorrectly trying to create equivalent boxes using different methods that don't actually produce the same geometry. Part.makeBox with a direction vector is not equivalent to setting a Placement with rotation. Fixed to use the correct approach that demonstrates the actual bug: - Method 1: Box with geometry at (0,30,0), identity Placement - Method 2: Box with geometry at origin, moved via Placement Both should produce identical mirror results, which they now do with the makeElementMirror() fix. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Timothy Miller <theosib@zeromark.com> Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -3990,9 +3990,10 @@ TopoShape& TopoShape::makeElementMirror(const TopoShape& shape, const gp_Ax2& ax
|
||||
}
|
||||
gp_Trsf mat;
|
||||
mat.SetMirror(ax2);
|
||||
TopLoc_Location loc = shape.getShape().Location();
|
||||
gp_Trsf placement = loc.Transformation();
|
||||
mat = placement * mat;
|
||||
// Note: Do NOT extract and pre-multiply the shape's Location/Placement here.
|
||||
// BRepBuilderAPI_Transform correctly handles shapes with Location already.
|
||||
// Pre-multiplying would double-apply the placement, causing incorrect results
|
||||
// for shapes with non-identity Placement. See GitHub issue #20834.
|
||||
BRepBuilderAPI_Transform mkTrf(shape.getShape(), mat);
|
||||
return makeElementShape(mkTrf, shape, op);
|
||||
}
|
||||
|
||||
@@ -607,6 +607,76 @@ class TopoShapeTest(unittest.TestCase, TopoShapeAssertions):
|
||||
if mirror.ElementMapVersion != "": # Should be '4' as of Mar 2023.
|
||||
self.assertEqual(mirror.ElementMapSize, 26)
|
||||
|
||||
def testTopoShapeMirrorWithPlacement(self):
|
||||
"""Test that mirror() produces identical results regardless of how the
|
||||
shape is positioned - via direct coordinates or via Placement.
|
||||
Regression test for GitHub issue #20834.
|
||||
|
||||
The bug was: when a shape has a non-identity Location (Placement),
|
||||
the mirror result was incorrect because the placement was being
|
||||
double-applied in makeElementMirror().
|
||||
"""
|
||||
# Create two identical boxes at the same visual location using different methods:
|
||||
# Method 1: Box geometry positioned directly at (0, 30, 0), identity Placement
|
||||
box_direct = Part.makeBox(10, 20, 30, App.Vector(0, 30, 0))
|
||||
|
||||
# Method 2: Box geometry at origin, then moved via Placement
|
||||
box_placed = Part.makeBox(10, 20, 30)
|
||||
box_placed.Placement = App.Placement(App.Vector(0, 30, 0), App.Rotation())
|
||||
|
||||
# Verify both boxes appear at the same location
|
||||
self.assertAlmostEqual(box_direct.BoundBox.XMin, box_placed.BoundBox.XMin, places=5)
|
||||
self.assertAlmostEqual(box_direct.BoundBox.YMin, box_placed.BoundBox.YMin, places=5)
|
||||
self.assertAlmostEqual(box_direct.BoundBox.ZMin, box_placed.BoundBox.ZMin, places=5)
|
||||
|
||||
# Mirror both across the XZ plane (Y=0)
|
||||
# A point (x, y, z) mirrors to (x, -y, z)
|
||||
# So box at Y=30..50 should mirror to Y=-50..-30
|
||||
mirror_direct = box_direct.mirror(App.Vector(), App.Vector(0, 1, 0))
|
||||
mirror_placed = box_placed.mirror(App.Vector(), App.Vector(0, 1, 0))
|
||||
|
||||
# The mirrored shapes should have identical bounding boxes
|
||||
self.assertAlmostEqual(
|
||||
mirror_direct.BoundBox.XMin,
|
||||
mirror_placed.BoundBox.XMin,
|
||||
places=5,
|
||||
msg="Mirror with Placement produced different XMin",
|
||||
)
|
||||
self.assertAlmostEqual(
|
||||
mirror_direct.BoundBox.YMin,
|
||||
mirror_placed.BoundBox.YMin,
|
||||
places=5,
|
||||
msg="Mirror with Placement produced different YMin",
|
||||
)
|
||||
self.assertAlmostEqual(
|
||||
mirror_direct.BoundBox.ZMin,
|
||||
mirror_placed.BoundBox.ZMin,
|
||||
places=5,
|
||||
msg="Mirror with Placement produced different ZMin",
|
||||
)
|
||||
self.assertAlmostEqual(
|
||||
mirror_direct.BoundBox.XMax,
|
||||
mirror_placed.BoundBox.XMax,
|
||||
places=5,
|
||||
msg="Mirror with Placement produced different XMax",
|
||||
)
|
||||
self.assertAlmostEqual(
|
||||
mirror_direct.BoundBox.YMax,
|
||||
mirror_placed.BoundBox.YMax,
|
||||
places=5,
|
||||
msg="Mirror with Placement produced different YMax",
|
||||
)
|
||||
self.assertAlmostEqual(
|
||||
mirror_direct.BoundBox.ZMax,
|
||||
mirror_placed.BoundBox.ZMax,
|
||||
places=5,
|
||||
msg="Mirror with Placement produced different ZMax",
|
||||
)
|
||||
|
||||
# Verify the expected mirror result: Y=30..50 mirrors to Y=-50..-30
|
||||
self.assertAlmostEqual(mirror_direct.BoundBox.YMin, -50.0, places=5)
|
||||
self.assertAlmostEqual(mirror_direct.BoundBox.YMax, -30.0, places=5)
|
||||
|
||||
def testTopoShapeScale(self):
|
||||
# Act
|
||||
scale = self.doc.Box1.Shape.scaled(2)
|
||||
|
||||
Reference in New Issue
Block a user