Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loft produces unexpected faces of GeomType.BSPLINE or filter_by(Plane) should work with GeomType.BSPLINE shapes #756

Closed
dalibor-frivaldsky opened this issue Nov 4, 2024 · 4 comments
Labels
enhancement New feature or request

Comments

@dalibor-frivaldsky
Copy link
Contributor

The following code creates the shape in the picture below:

with BuildPart() as mount:
    with BuildSketch():
        Rectangle((1+16+4)*mm, 20*mm, align=(Align.MIN, Align.CENTER))
    with BuildSketch(Pos(1*mm, 0, 4*mm)):
        Rectangle(16*mm, 20*mm, align=(Align.MIN, Align.CENTER))
    loft()
Screenshot 2024-11-04 at 14 49 17

I would like to filter out the trapezoidal faces with mount.faces().filter_by(Plane.XZ), however, this returns an empty list.

I have narrowed down the issue to the implemenation of the Shape.filter_by method, specifically the plane_parallel_predicate. The shape being filtered needs to be both an instance of Face and of a GeomType.PLANE. The loft creates the trapezoidal faces as GeomType.BSPLINE, which fails the second part of the check and results in the empty list after applying the filter.

I don't know whether the issue is with the loft creating the faces as b-splines (which maybe seems unnecessary?) or whether the filter_by function should be able to work with (plannar?) b-spline faces. What do you think?

@gumyr gumyr added the enhancement New feature or request label Nov 4, 2024
@gumyr gumyr added this to the Not Gating Release 1.0.0 milestone Nov 4, 2024
@gumyr
Copy link
Owner

gumyr commented Nov 4, 2024

Unfortunately, the OCCT CAD kernel returns these BSPLINE faces where a PLANAR face would be more appropriate. The check for planar needs to be change from just looking at the geom_type property to something like this:

from build123d import *
from OCP.BRep import BRep_Tool
from OCP.GeomLib import GeomLib_IsPlanarSurface
from ocp_vscode import show_all


def is_face_planar(face: Face) -> bool:
    surface = BRep_Tool.Surface_s(face.wrapped)
    is_planar = GeomLib_IsPlanarSurface(surface, 1e-6)
    return is_planar.IsPlanar()


with BuildPart() as mount:
    with BuildSketch():
        Rectangle((1 + 16 + 4) * MM, 20 * MM, align=(Align.MIN, Align.CENTER))
    with BuildSketch(Pos(1 * MM, 0, 4 * MM)):
        Rectangle(16 * MM, 20 * MM, align=(Align.MIN, Align.CENTER))
    loft()

for f in mount.faces():
    print(f"{f.geom_type} is planar: {is_face_planar(f)}")
GeomType.PLANE is planar: True
GeomType.BSPLINE is planar: True
GeomType.PLANE is planar: True
GeomType.BSPLINE is planar: True
GeomType.PLANE is planar: True
GeomType.PLANE is planar: True

I'm thinking a new property should be added to the Face class, is_planar : bool that does what is shown here.

@dalibor-frivaldsky
Copy link
Contributor Author

I assume with the is_planar property in place, the code in plane_parallel_predicate:

if isinstance(shape, Face) and shape.geom_type == GeomType.PLANE:

would change to:

if isinstance(shape, Face) and shape.is_planar:

That looks good to me.

gumyr added a commit that referenced this issue Nov 4, 2024
@gumyr
Copy link
Owner

gumyr commented Nov 4, 2024

Now:

non_trap = mount.faces().filter_by(Plane.XZ, reverse=True)

results in:
image

@gumyr gumyr closed this as completed Nov 4, 2024
@dalibor-frivaldsky
Copy link
Contributor Author

I have found one more place where the behavior of loft() causes issues:

with BuildPart() as mount:
    with BuildSketch():
        Rectangle((1+16+4)*mm, 20*mm, align=(Align.MIN, Align.CENTER))
    with BuildSketch(Pos(1*mm, 0, 4*mm)):
        Rectangle(16*mm, 20*mm, align=(Align.MIN, Align.CENTER))
    loft()

    with BuildSketch(mount.faces()[1]):
        Rectangle(4*mm, 4*mm)
    extrude(amount=4*mm)

This fails with "ValueError: Planes can only be created from planar faces" on BuildSketch(mount.faces()[1]) when creating a Plane object from the provided trapezoid face, which is of GeomType.BSPLINE. The place to fix this seems to be here

geometry.py

elif arg_face:
    # Determine if face is planar
    surface = BRep_Tool.Surface_s(arg_face.wrapped)
    if not isinstance(surface, Geom_Plane):
        raise ValueError("Planes can only be created from planar faces")

I can give this a try and fix it. I've already created a dev environment for build123d in the meantime and implemented partial fix for the original issue.

In general, the underlying loft() kernel implementation is behaving fairly unexpectedly in multiple ways. As another example, creating a Plane from the top and bottom faces (parallel with Plane.XY) sets their x_dir vector to a seemengly arbitrary value (shown below). But I don't think build123d is the place to try to fix this.

with BuildPart() as mount:
    with BuildSketch():
        Rectangle((1+16+4)*mm, 20*mm, align=(Align.MIN, Align.CENTER))
    with BuildSketch(Pos(1*mm, 0, 4*mm)):
        Rectangle(16*mm, 20*mm, align=(Align.MIN, Align.CENTER))
    loft()

with BuildSketch(mount.faces() | Plane.XY):
        Rectangle(4*mm, 4*mm)
    extrude(amount=4*mm)
image

dalibor-frivaldsky added a commit to dalibor-frivaldsky/build123d that referenced this issue Nov 5, 2024
dalibor-frivaldsky added a commit to dalibor-frivaldsky/build123d that referenced this issue Nov 7, 2024
dalibor-frivaldsky added a commit to dalibor-frivaldsky/build123d that referenced this issue Nov 7, 2024
dalibor-frivaldsky added a commit to dalibor-frivaldsky/build123d that referenced this issue Nov 8, 2024
gumyr added a commit that referenced this issue Nov 8, 2024
…e-from-planar-faces

Plane instantiation from planar Geom_BoundedSurface faces Issue #756
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants