gumyr/build123d

Sweep wire gives wrong result with multipart path

Opened this issue · 5 comments

l = JernArc((0,0), (-1,0), 1, 180) + JernArc((0,0), (1,0), 2, -90)
s = Circle(0.5)
p = sweep((l^0)*s, l)
1
p = sweep((l^0)*s.wire(), l)
2

Sigh... everything looks good up until BRepOffsetAPI_MakePipeShell is called where the result is just for the first Edge :(

@gumyr I hit this bug recently and I did some investigation. I think everything goes well, but at the end of Face.sweep it's assumed that the result is a single face, but it turns out not to be the case.

In fact, if I change the code to return all the .faces() I get what I expect.

image

I can make a PR with this change, my only doubt is that I'd have to change the signature of Face.sweep which would be a breaking change in theory, but in practice I do not know how much Face.sweep is used directly rather than sweep() by clients.

Let me know what you think.

gumyr commented

Interesting, it wants to return a Shell. If I change the Face.sweep return to:

        return Shape.cast(builder.Shape())

it consistently returns a Shell even if this Shell only contains one Face.

As you say, this is going to be a breaking change at the direct API level - one way or another - but maybe this should be a Shell class method and not a Face class method. What do you think?

I guess Face.sweep makes sense only if the profile is an Edge because, I think, the Shell returned by OCCT must have a single face for construction in this case. I'm not sure if it's worth having and/or if it doesn't cause confusion with the other sweep methods.

The following functions make sense to me

class Edge:
    @classmethod
    def sweep(
        section: Union[Edge, Wire],
        path: Edge, transition=Transition.TRANSFORMED,  # not quite sure why this is RIGHT on master right now
    ) -> Face

class Shell:
    @classmethod
    def sweep(
        sections: Union[list[Face], list[Wire]],
        path: Union[Wire, Edge],
        is_frenet: bool = False,
        mode: Union[Vector, Wire, Edge, None] = None,
        transition: Transition = Transition.TRANSFORMED,
    ) -> Shell

class Solid:
    @classmethod
    def sweep(
        sections: Union[list[Face], list[Wire]],
        path: Union[Wire, Edge],
        is_frenet: bool = False,
        mode: Union[Vector, Wire, Edge, None] = None,
        transition: Transition = Transition.TRANSFORMED,
    ) -> Solid

Note that Solid.sweep and Shell.sweep have the same interface, only the return type is different. Moreover, I removed the inner_wires and make_solid parameters because make_solid does not make much sense anymore as one can choose the proper method. Wrt inner_wires I think it would make more sense to look into each Face.inner_wires() so that inner_wires would be supported even for multi_section profiles (which are automatically enabled if the list of sections contains more than one section).

Just my 2 cents.

@danieledapo I additionally propose that the deprecated decorator is added to the existing Face.sweep method with a short explanation of what to use instead. We can keep it around for a few more releases to give people time to switch.