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

Add cubic and quadratic Bézier curves to Workplane and Sketch #1529

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

dov
Copy link
Contributor

@dov dov commented Mar 4, 2024

This change adds quadratic and cubic Bézier curves to the Workplane and the Sketch classes

@neri-engineering
Copy link

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).

  • In cq.py, that quadraticCurveTo() and cubicCurveTo() accept 2D coordinates is consistent with lineTo() and moveTo().
  • In 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.
  • In 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.

@adam-urbanczyk
Copy link
Member

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.

@dov
Copy link
Contributor Author

dov commented Mar 10, 2024

@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?

@adam-urbanczyk
Copy link
Member

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.

@dov
Copy link
Contributor Author

dov commented Mar 10, 2024

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"?

@adam-urbanczyk
Copy link
Member

allPoints = self._toVectors(listOfXYTuple, includeCurrent)

@dov
Copy link
Contributor Author

dov commented Mar 10, 2024

allPoints = self._toVectors(listOfXYTuple, includeCurrent)

Great! Thanks. That solved the problem.

@dov
Copy link
Contributor Author

dov commented Mar 10, 2024

@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()
  )

@adam-urbanczyk
Copy link
Member

Sorry, but that does not mesh with the current sketch API.

@dov
Copy link
Contributor Author

dov commented Mar 10, 2024

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.

@dov dov force-pushed the bezier branch 2 times, most recently from c3c2a24 to e4562ff Compare March 11, 2024 06:46
Copy link

codecov bot commented Mar 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.50%. Comparing base (153ed3f) to head (db26c0b).
Report is 1 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

@adam-urbanczyk
Copy link
Member

@dov are you going to fix the coverage of your PR?

@dov
Copy link
Contributor Author

dov commented Mar 11, 2024

Ok, I'll do it. Since it wasn't 100% before, I wasn't sure how stringent you were about it.

@adam-urbanczyk
Copy link
Member

Preferably 100% patch coverage

@jmwright
Copy link
Member

@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.

Install black

Use black

@dov dov force-pushed the bezier branch 2 times, most recently from 230c10b to 8d6f001 Compare March 12, 2024 18:38
@dov
Copy link
Contributor Author

dov commented Mar 12, 2024

@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.

Install black

Use black

Thanks @jmwright . I'm on to it one iteration at a time.

@jmwright
Copy link
Member

@adam-urbanczyk All green now.

Copy link
Member

@adam-urbanczyk adam-urbanczyk left a 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?

tests/test_workplanes.py Show resolved Hide resolved
@dov dov force-pushed the bezier branch 3 times, most recently from a2e5fa4 to 8c77a02 Compare March 17, 2024 09:00
"""
Create a cubic Bézier Curve from the points.

:param listOfVector: a list of Vectors that represent the points.
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Thanks!

)

bbBox = r.findSolid().BoundingBox()
print(bbBox)
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Thanks!

Copy link
Member

@jmwright jmwright left a 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?

@dov
Copy link
Contributor Author

dov commented Mar 20, 2024

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.

@adam-urbanczyk adam-urbanczyk merged commit 76fbff7 into CadQuery:master Mar 23, 2024
5 checks passed
@adam-urbanczyk
Copy link
Member

Thanks @dov !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants