gumyr / build123d

A python CAD programming library
Apache License 2.0
402 stars 75 forks source link

OCP.Geom.Geom_BSplineSurface being created instead of PLANE #80

Closed gumyr closed 1 year ago

gumyr commented 1 year ago

The Mirror operation changes the geomtype from PLANE to BSPLINE. Rectangle on the construction_face isn't making a square. Extrude isn't creating a new Solid.

from build123d import *

ring_r, ring_t = 9, 2
wheel_r, wheel_t = 10, 6
hex_r, hex_len = 1, 5
axle_r, axle_depth = 1, 4

with BuildPart() as p:
    with BuildSketch(Plane.XZ) as side:
        Trapezoid(wheel_r, wheel_t / 2, 90, 45, centered=(False, False))
        with Locations((ring_r, ring_t / 2)):
            Circle(ring_t / 2, centered=(True, True), mode=Mode.SUBTRACT)
        with Locations((wheel_r, ring_t / 2)):
            Rectangle(2, 2, centered=(True, True), mode=Mode.SUBTRACT)
    Revolve(axis=Axis.Z)
    # Mirror(about=Plane.XY)
    construction_face = p.faces().sort_by(Axis.Z)[0]
    print(f"{type(construction_face)=}")
    print(f"{construction_face.geom_type()=}")
    with BuildSketch(construction_face):
        Circle(1)
        # Rectangle(1, 1)
    Extrude(amount=10, mode=Mode.REPLACE)

show_object(p.part.wrapped, name="part", options={"alpha": 0.8})
show_object(construction_face, name="construction_face")

image (with Mirror, without building on the construction_face)

bernhard-42 commented 1 year ago

Currently Mirror uses transform_geometry for which the comment says: "WARNING: transform_geometry will sometimes convert lines and circles to splines, ..."

A simple solution for mirror changing geom_type could be (similar to CQ):

    def __init__(
        self,
        *objects: Union[Edge, Wire, Face, Compound],
        about: Plane = Plane.XZ,
        mode: Mode = Mode.ADD,
    ):
        context: Builder = Builder._get_context(self)
        context.validate_inputs(self, objects)

        if not objects:
            objects = [context._obj]

        self.objects = objects
        self.about = about
        self.mode = mode

        # scale_matrix = Matrix(
        #     [
        #         [1.0, 0.0, 00.0, 0.0],
        #         [0.0, 1.0, 00.0, 0.0],
        #         [0.0, 0.0, -1.0, 0.0],
        #         [0.0, 0.0, 00.0, 1.0],
        #     ]
        # )
        # localized = [about.to_local_coords(o) for o in objects]
        # local_mirrored = [o.transform_geometry(scale_matrix) for o in localized]
        # mirrored = [about.from_local_coords(o) for o in local_mirrored]

        mirrored = [copy(o).mirror(about) for o in objects]

        context._add_to_context(*mirrored, mode=mode)
        super().__init__(Compound.make_compound(mirrored).wrapped)

But I guess you had a good reason for selecting a different approach.

gumyr commented 1 year ago

The Shape.mirror() method (as follows) also transforms the geometry:

    def mirror(self, mirror_plane: Plane = None) -> Shape:
        if not mirror_plane:
            mirror_plane = Plane.XY

        transformation = gp_Trsf()
        transformation.SetMirror(
            gp_Ax2(mirror_plane.origin.to_pnt(), mirror_plane.z_dir.to_dir())
        )

        return self._apply_transform(transformation)

I haven't tried it but I wouldn't expect there would be a difference in behaviour. Is there something I'm missing?

bernhard-42 commented 1 year ago

I gave it a try with the above change and the examples returns a shape where both the top and the bottom face are of geom_type "PLANE"

The docs for gp_Trsf (https://dev.opencascade.org/doc/refman/html/classgp___trsf.html) say:

This transformation never change the nature of the objects.

On the other side, if we look at transform_geometry (which is currently used in class Mirror), it uses BRepBuilderAPI_GTransform

The transformation to be applied is defined as a gp_GTrsf transformation

and gp_GTrsf says:

Be careful if you apply such a transformation to all points of a geometric object, as this can change the nature of the object and thus render it incoherent! Typically, a circle is transformed into an ellipse by an affinity transformation. To avoid modifying the nature of an object, use a gp_Trsf transformation instead, as objects of this class respect the nature of geometric objects.

And Shape.mirror uses BRepBuilderAPI_Transform which uses gp_Trsf and as such my expectation is that it keeps planes as planes.

That is at least how I understand the OCCT docs. But maybe I miss something ...

gumyr commented 1 year ago

Thanks @bernhard-42, I followed your advise and used:

        mirrored = [copy.deepcopy(o).mirror(about) for o in objects]

(and hence avoided gp_GTrsf) to do the mirror operation.

(Note that copy() is depreciated)