Sweep wire gives wrong result with multipart path
Opened this issue · 5 comments
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.
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.
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.