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.

Sign up for GitHub

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

Jump to bottom

Plane instantiation from planar Geom_BoundedSurface faces Issue #756 #764

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Plane instantiation from planar Geom_BoundedSurface faces Issue #756 #764

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3fdefd
Plane instantiation from planar Geom_BoundedSurface faces Issue #756
dalibor-frivaldsky Nov 5, 2024
e588107
Extended test coverage for Issue #756
dalibor-frivaldsky Nov 7, 2024
89c29ab
Test code simplificatoin for Issue #756
dalibor-frivaldsky Nov 7, 2024
a797e59
Plane instantiation from Face uses the new is_planar property Issue #756
dalibor-frivaldsky Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions src/build123d/geometry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@
from OCP.BRepGProp import BRepGProp, BRepGProp_Face # used for mass calculation
from OCP.BRepMesh import BRepMesh_IncrementalMesh
from OCP.BRepTools import BRepTools
from OCP.Geom import Geom_Line, Geom_Plane
from OCP.Geom import Geom_BoundedSurface, Geom_Line, Geom_Plane
from OCP.GeomAPI import GeomAPI_ProjectPointOnSurf, GeomAPI_IntCS, GeomAPI_IntSS
from OCP.GeomLib import GeomLib_IsPlanarSurface
from OCP.gp import (
gp_Ax1,
gp_Ax2,
Expand Down Expand Up @@ -2092,18 +2093,20 @@ def optarg(kwargs, name, args, index, default):
elif arg_face:
# Determine if face is planar
surface = BRep_Tool.Surface_s(arg_face.wrapped)
if not isinstance(surface, Geom_Plane):
is_surface_planar = GeomLib_IsPlanarSurface(surface, TOLERANCE).IsPlanar()
if not is_surface_planar:
raise ValueError("Planes can only be created from planar faces")
properties = GProp_GProps()
BRepGProp.SurfaceProperties_s(arg_face.wrapped, properties)
self._origin = Vector(properties.CentreOfMass())
self.x_dir = (
Vector(arg_x_dir)
if arg_x_dir
else Vector(
BRep_Tool.Surface_s(arg_face.wrapped).Position().XDirection()
)
)
if isinstance(surface, Geom_BoundedSurface):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the special case of Geom_BoundedSurface handled here?

Copy link
Contributor Author

@dalibor-frivaldsky dalibor-frivaldsky Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The added test to this PR fails otherwise when trying to instantiate a Plane from the trapezoid faces created by the loft(), since the underlying kernel creates them as Geom_BSplineSurface and not Geom_Plane. Initially I thought of making the special case for Geom_BSPlineSurface specifically, but there are other surface types, like Geom_BezierSurface. These can be planar as well and it should be possible to create a Plane object from those too. The Geom_BoundedSurface is a base class of these and makes the implementation more general, that's why I opted for it.

point = gp_Pnt()
face_x_dir = gp_Vec()
tangent_v = gp_Vec()
surface.D1(0.5, 0.5, point, face_x_dir, tangent_v)
else:
face_x_dir = surface.Position().XDirection()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important ability to force the x direction has been removed and needs to be returned.

Copy link
Contributor Author

@dalibor-frivaldsky dalibor-frivaldsky Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is the case, the line 2109 retains the ability to force the x direction from the argument to the Plane constructor. All the tests pass so I suppose such a regression would have been caught by them, unless I overlooked something.

self.x_dir = Vector(arg_x_dir) if arg_x_dir else Vector(face_x_dir)
self.x_dir = Vector(round(i, 14) for i in self.x_dir)
self.z_dir = Plane.get_topods_face_normal(arg_face.wrapped)
self.z_dir = Vector(round(i, 14) for i in self.z_dir)
Expand Down
11 changes: 11 additions & 0 deletions tests/test_direct_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2556,6 +2556,17 @@ def test_plane_init(self):
with self.assertRaises(TypeError):
Plane(Edge.make_line((0, 0), (0, 1)))

# can be instantiated from planar faces of different surface types
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests must make assertions to validate the results and not just run to completion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've extended the test to also verify properties of the two trapezoid faces, which are created as Geom_BSplineSurface by loft() and are the focus of the introduced changes.

# this loft creates new faces of types Geom_Plane and Geom_BSplineSurface
lofted_solid = Solid.make_loft(
[
Rectangle(3, 1).wire(),
Pos(0, 0, 1) * Rectangle(1, 1).wire(),
]
)
for f in lofted_solid.faces():
Plane(f)

def test_plane_neg(self):
p = Plane(
origin=(1, 2, 3),
Expand Down