Closed dov closed 3 months ago
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).
cq.py
, that quadraticCurveTo()
and cubicCurveTo()
accept 2D coordinates is consistent with lineTo()
and moveTo()
.shapes.py
, the methods added take VectorLike
objects, which can either be 2D or 3D. So from the API perspective there is support for specifying 3D coordinates. Great.sketch.py
, Point
datatype includes Vector
, which has ability to specify in 3D.I just hope that the handing of 2D and 3D works. The datatypes used support various formats, they use the Union
construct typically. I can't say anything more intelligent because I'm not yet an expert w.r.t. CQ and Python.
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:
plane='XY' Volume=6.666666666666666
plane='XZ' Volume=0.0
The implementation in Shape.py
is quite straight forward:
@classmethod
def makeBezier(
cls,
points: List[Vector]
) -> "Edge":
"""
Create a cubic Bézier Curve from the points.
:param listOfVector: a list of Vectors that represent the points.
The edge will pass through the first and the last point,
and the inner points are Bézier control points.
:return: An edge
"""
# Convert to a TColgp_Array1OfPnt
arr = TColgp_Array1OfPnt(1, len(points))
for i, v in enumerate(points):
arr.SetValue(i + 1, Vector(v).toPnt())
bez = Geom_BezierCurve(arr)
return cls(BRepBuilderAPI_MakeEdge(bez).Edge())
and in cq.py
:
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"?
Great! Thanks. That solved the problem.
@adam-urbanczyk One thing that is bothering me with the suggested API for Sketch
is that there is no support for starting from the current point. Would it be ok if i add a includeCurrent
flag for the Sketch API as well so that instead of writing:
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 cq.CP
, cq.current_pos
, or cq.cont
as a shortcut for the current point? That would benifit your spline()
method as well.
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.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 94.50%. Comparing base (
153ed3f
) to head (db26c0b
). Report is 1 commits behind head on master.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@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.
@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.
Thanks @jmwright . I'm on to it one iteration at a time.
@adam-urbanczyk All green now.
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