-
Notifications
You must be signed in to change notification settings - Fork 289
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
Add cubic and quadratic Bézier curves to Workplane and Sketch #1529
Conversation
I had a quick look through the changes and these look to be thorough changes. Unfortunately I cannot vouch for nor can I condemn these changes b/c I have not mastered the art of Git or the art of running CQ from source code (it's on my TODO list after I finish with elliptical B-splines). But here are my thoughts upon seeing the changes (besides the test code).
I just hope that the handing of 2D and 3D works. The datatypes used support various formats, they use the |
Hey, thanks. I'm personally not convinced that this is needed, but OK. If you want to get it merged, I'll ask you to change the API so that it follows what spline does. Also please support any bezier, not just cubic/quadratic. The API should look approximately like this: cq.Shape.makeBezier
cq.Workplane.bezier(
self: T,
listOfXYTuple: Iterable[VectorLike],
forConstruction: bool = False,
includeCurrent: bool = False,
makeWire: bool = False,
)
cq.Sketch.bezier(
self: T,
pts: Iterable[Point],
tag: Optional[str] = None,
forConstruction: bool = False,
) Rationale: generality, consistency with spline and 3D support. |
@adam-urbanczyk Thanks. A question, with the above change and if someone wants to draw a 2D bezier curve e.g. in the XZ plane, do the vector coordinates need to provided with x,y and z coordinates, with the ycoordinate==0? |
I'm not sure what you exactly mean. You can provide 2D coordinates in the local coordinate system, see: https://cadquery.readthedocs.io/en/latest/examples.html#defining-an-edge-with-a-spline. |
Ok. Thanks. That's what I thought. But for whatever reason, after I tried implementing your suggestions, I could only get a solid after extrusion if doing chosing an XY Workplane. If choosing an XZ WorkPlane, I got a degenerate solid: import sys
sys.path.insert(0,'/path/to/dev/cadquery/')
import cadquery as cq
for plane in ('XY','XZ'):
res = (cq
.Workplane(plane)
.bezier([(0,0), (1, 2), (5, 0)])
.bezier([(5,0), (1, -2), (0, 0)])
.close()
.extrude(1)
)
print(f'{plane=} Volume={res.findSolid().Volume()}') outputs:
The implementation in
and in def bezier(
self: T,
listOfXYTuple: Iterable[VectorLike],
forConstruction: bool = False,
includeCurrent: bool = False,
makeWire: bool = False
) -> T:
if includeCurrent:
listOfXYTuple = [self._findFromPoint(False)] [v for v in listOfXYTuple]
e = Edge.makeBezier(listOfXYTuple)
if makeWire:
rv_w = Wire.assembleEdges([e])
if not forConstruction:
self._addPendingWire(rv_w)
elif not forConstruction:
self._addPendingEdge(e)
return self.newObject([rv_w if makeWire else e]) Do you have any idea, why the local coordinate system isn't "catching"? |
Line 1887 in 153ed3f
|
Great! Thanks. That solved the problem. |
@adam-urbanczyk One thing that is bothering me with the suggested API for s1 = (
cq.Sketch()
.segment((0,0), (0,0.5))
.bezier(((0, 0.5), (-1, 2), (1, 0.5), (5, 0)))
.bezier(((5,0), (1, -0.5), (-1, -2), (0, -0.5)))
.close() I can write: s1 = (
cq.Sketch()
.segment((0,0), (0,0.5))
.bezier(((-1, 2), (1, 0.5), (5, 0)), includeCurrent=True)
.bezier(((1, -0.5), (-1, -2), (0, -0.5)), includeCurrent=True)
.close()
) |
Sorry, but that does not mesh with the current sketch API. |
Ok. As you wish. But in the future perhaps you can add some place holder like s1 = (
cq.Sketch()
.segment((0,0), (0,0.5))
.bezier((cq.CP, (0, 0.5), (-1, 2), (1, 0.5), (5, 0)))
.bezier((cq.CP, (5,0), (1, -0.5), (-1, -2), (0, -0.5)))
.close() or even: _ = cq.CP
s1 = (
cq.Sketch()
.segment((0,0), (0,0.5))
.bezier((_, (-1, 2), (1, 0.5), (5, 0)))
.bezier((_, (1, -0.5), (-1, -2), (0, -0.5)))
.close() In any case, that is outside the scope of this pull request. |
c3c2a24
to
e4562ff
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1529 /- ##
==========================================
Coverage 94.48% 94.50% 0.02%
==========================================
Files 28 28
Lines 5780 5804 24
Branches 1071 1076 5
==========================================
Hits 5461 5485 24
Misses 193 193
Partials 126 126 ☔ View full report in Codecov by Sentry. |
@dov are you going to fix the coverage of your PR? |
Ok, I'll do it. Since it wasn't 100% before, I wasn't sure how stringent you were about it. |
Preferably 100% patch coverage |
@dov It looks like the black formatting check is falling in AppVeyor. Two links are provided below in case you haven't used black before. |
230c10b
to
8d6f001
Compare
@adam-urbanczyk All green now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I removed one unused variable. Could you clarify or remove the comment you made in tests?
a2e5fa4
to
8c77a02
Compare
cadquery/occ_impl/shapes.py
Outdated
""" | ||
Create a cubic Bézier Curve from the points. | ||
|
||
:param listOfVector: a list of Vectors that represent the points. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docstring param name mismatch.
The docs list both points, and listOfVector:
https://cadquery--1529.org.readthedocs.build/en/1529/classreference.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! Thanks!
tests/test_workplanes.py
Outdated
) | ||
|
||
bbBox = r.findSolid().BoundingBox() | ||
print(bbBox) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a stray print statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the one comment from @lorenzncode and the other from me, it looks good. Thanks @dov . Have you updated your gist to work with this latest changes in this PR yet?
I assume you mean the svg path to cadquery gist. I just updated it. Thanks for reminding me. |
Thanks @dov ! |
This change adds quadratic and cubic Bézier curves to the Workplane and the Sketch classes