Plane instantiation from planar Geom_BoundedSurface faces Issue #756 #764
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #764 +/- ##
==========================================
+ Coverage 96.05% 96.09% +0.03%
==========================================
Files 25 25
Lines 8387 8447 +60
==========================================
+ Hits 8056 8117 +61
+ Misses 331 330 -1 ☔ View full report in Codecov by Sentry. |
|
There is no issue or description of what problem this change is attempting to fix. Please describe the problem first. |
|
@gumyr apologies, I assumed the link to the issue in the commit message would be sufficient for provding the context. This addresses additional issues with loft() creating planar faces of different type than Geom_Plane (or of GeomType.Plane), as described in #756 . I've provided description of these related issues after you've closed the issue. I thought it better to reopen the closed issue, but I can open a new one if this is the prefered way. |
| BRep_Tool.Surface_s(arg_face.wrapped).Position().XDirection() | ||
| ) | ||
| ) | ||
| if isinstance(surface, Geom_BoundedSurface): |
There was a problem hiding this comment.
Why is the special case of Geom_BoundedSurface handled here?
There was a problem hiding this comment.
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.
| tangent_v = gp_Vec() | ||
| surface.D1(0.5, 0.5, point, face_x_dir, tangent_v) | ||
| else: | ||
| face_x_dir = surface.Position().XDirection() |
There was a problem hiding this comment.
The important ability to force the x direction has been removed and needs to be returned.
There was a problem hiding this comment.
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.
tests/test_direct_api.py
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Tests must make assertions to validate the results and not just run to completion.
There was a problem hiding this comment.
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.
|
I've updated the Plane instantiation code to use the new is_planar property of the Face class and added missing tests. |
|
My mistake about forcing the x direction. Thanks for the fix! |
This PR addresses additional problems with #756, specifically instantiating a Plane from planar faces that are not of type Geom_Plane.