Ellipse #265

bernhard-42 · 2020-01-03T11:33:53Z

I have implemented ellipses following the implementation of circles, however, having some more flexibility. Compared to circle the following parameters are avaiable:

x_radius, y_radius: While OCC needs major_radius > minor_radius, the implementation allows for arbitrary x and y radiuses. If x_radius < y_radius, the implementation swaps both and rotates the resulting ellipse by +90°. This was done to simplify building arbitrary ellipses in cadquery
angle1 (default=360), angle2 (default=360): allow to return an arc between angle1and angle2. This actually just makes GC_MakeArcOfEllipseavailable
closed (default=False): If an arc, return a closed wire if True
rotation_angle (default=0.0): rotate the resulting ellipse or arc around its center if rotation_angle != 0.0

codecov · 2020-01-03T11:42:13Z

Codecov Report

Merging #265 into master will increase coverage by 0.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #265      +/-   ##
==========================================
+ Coverage   94.98%   95.16%   +0.17%     
==========================================
  Files          18       18              
  Lines        4411     4567     +156     
==========================================
+ Hits         4190     4346     +156     
  Misses        221      221

Impacted Files	Coverage Δ
cadquery/cq.py	`93.42% <100%> (+0.16%)`	⬆️
cadquery/occ_impl/shapes.py	`90.63% <100%> (+0.39%)`	⬆️
tests/__init__.py	`93.75% <100%> (ø)`	⬆️
tests/test_cad_objects.py	`99.45% <100%> (+0.12%)`	⬆️
tests/test_cadquery.py	`99.16% <100%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b9d3a1c...e3edecd. Read the comment docs.

bernhard-42 · 2020-01-03T12:16:00Z

Adresses issue #261

jmwright · 2020-01-03T12:56:41Z

@bernhard-42 Thanks for putting the work into this. Is this ready for review?

bernhard-42 · 2020-01-03T13:00:12Z

@jmwright yes, it is ready for review.

The only unresolved issue I know of is the OCCT bug about center of mass of an ellipse - which I cannot do anything. Impact: concentric ellipses need a center(0,0), see doc string of ellipse method

cadquery/occ_impl/shapes.py

adam-urbanczyk · 2020-01-07T19:53:47Z

cadquery/cq.py

        return self.eachpoint(makeCircleWire, useLocalCoordinates=True)

+    # ellipse from current point
+    def ellipse(self, x_radius, y_radius, angle1=360, angle2=360, rotation_angle=0.0,


Wouldn't angle1=0 be more logical?

absolutely, however, makeCircle (and hence makeEllipse) uses 360 and I decided to keep that up to the top level. And from the later logic perspective, angle1 needs to be equal angle2 to get a full circle.

... or full ellipse

I see that circle does not expose those options at all - @jmwright what do you think?

I just looked at Workplane.spline which follows a different approach than Workplane.circle:
It does only have Edge.makeSpline and then applies the following logic

cadquery/cadquery/cq.py

Lines 1441 to 1450 in 74573fc

e = Edge.makeSpline(allPoints, tangents=tangents, periodic=periodic)

if makeWire:

rv = Wire.assembleEdges([e])

if not forConstruction:

self._addPendingWire(rv)

else:

rv = e

if not forConstruction:

self._addPendingEdge(e)

This allows splines to be used when building 2D paths.

If we expose angle1 and angle2 with ellipse shouldn't we change it like spline to allow ellipse arcs being used in building 2D paths? That was my original intention with exposing both angles, but I missed the point that circle doesn't add itself to pendingWires/pendingEdges (and hence my ellipse code doesn't either.

I wouldn't assume that the code in circle is the correct way to do it. I don't think that code has been touched since the early days of CQ, and may not represent our current understanding of best practices.

cadquery/cq.py

cadquery/occ_impl/shapes.py

bernhard-42 · 2020-01-10T19:55:09Z

The more I look into the cadquery source code the more I understand from the overall approach.

After analysing Workplane.spline and e.g. Workplane.sagittaArc I would keep Workplane.ellipse as is, but additionally add Workplane.ellipseArc as a 2d construction function:

def ellipseArc(self, x_radius, y_radius, angle1=360, angle2=360, rotation_angle=0.0, sense=1,
               forConstruction=False, startAtCurrent=True, makeWire=False):

    # start building the ellipse with the current point as center ...
    center = self._findFromPoint(useLocalCoords=True)
    e = Edge.makeEllipse(x_radius, y_radius, center, Vector(0, 0, 1), angle1, angle2, sense==1)

    # rotate if necessary ...
    if rotation_angle != 0.0:
        e = e.rotate(center, center.add(Vector(0,0,1)), rotation_angle)
    
    # and move the start point of the ellipse onto the last current point
    if startAtCurrent:
        startPoint = e.startPoint()
        e = e.translate(center.sub(startPoint))

    if makeWire:
        rv = Wire.assembleEdges([e])
        if not forConstruction:
            self._addPendingWire(rv)
    else:
        rv = e
        if not forConstruction:
            self._addPendingEdge(e)
    print(rv)
    return self.newObject([rv])

This would be analogous to circle:

Workplane.ellipse adds a wire object like Workplane.circle (but additionally exposes the angles)
Workplane.allipseArc works for 2d construction like Workplane.sagittaArc

Note, I had to add a sense parameter to control whether the arc goes clockwise or not.
This would now allow to write

x = (cq.Workplane("XY")
     .lineTo(0, 1)
     .ellipseArc(5, 4, -10, 190, 45, sense=-1)
)

ep = x.val().endPoint()

x = (x
     .hLineTo(ep.x + 2)
     .ellipseArc(5, 4, -10, 190, -45, sense=-1)
     .vLineTo(0)
     .mirrorX()
)

Would that be an enhancement you would like to add to cadquery?

jmwright · 2020-01-10T22:37:16Z

Workplane.sagittaArc is a relatively new addition to CQ (within probably the last year), just FYI.

I think the approach you're suggesting makes sense. If you want to put the work in, I'm all for having the increased functionality.

adam-urbanczyk · 2020-01-11T14:50:02Z

@bernhard-42 I also think that keeping ellipse similar to circle is the best approach.

bernhard-42 · 2020-01-18T13:51:13Z

I have built ellipse back to resemble what circle does. I only kept rotation_angle as a convenience parameter (which would not makes sense for circle)

I then added a ellipseArc method that allows to use arcs from ellipses in 2d construction. It supports a sense parameter for clockwise or counter clockwise arcs.

As a side note, since no point is included in the construction, I had to find the last point with useLocalCoords=False to get the correct result for starting with center() or eg. moveTo. I wanted in both cases the center of the new ellipse be either the around the new center or around the point moved to. Is this the correct way?

adam-urbanczyk · 2020-01-23T18:05:53Z

cadquery/cq.py

+        # Start building the ellipse with the current point as center
+        # to support strating as the result of center(x,y) or moveTo(x,y) do not use local coordinates
+        center = self._findFromPoint(useLocalCoords=False)
+        e = Edge.makeEllipse(x_radius, y_radius, center, Vector(0, 0, 1), angle1, angle2, sense==1)


if center is in global coords the Vector(0,0,1) is NOK I think. You might want to try: self.plane.zDir

yes, it is wrong. I will change and add a test case for a transformed plane

adam-urbanczyk · 2020-01-23T18:06:15Z

cadquery/cq.py

+
+        # Rotate if necessary
+        if rotation_angle != 0.0:
+            e = e.rotate(center, center.add(Vector(0,0,1)), rotation_angle)


Same potential issue here

adam-urbanczyk · 2020-01-23T18:08:50Z

cadquery/cq.py

        return self.spline(allPoints,includeCurrent=False,makeWire=True)

+    def ellipseArc(self, x_radius, y_radius, angle1=360, angle2=360, rotation_angle=0.0, sense=1,
+                forConstruction=False, startAtCurrent=True, makeWire=False):


Open question: do we have a usecase for startAtCurrent=False?

Good question. I found a similar construct with spline (includeCurrent) where I would also not see an immediate use case. So having seen this pattern in cq, I decided to add the same approach to ellipseArc and let others find out use cases.
But I am also happy to remove if someone has arguments against it.

bernhard-42 · 2020-02-08T12:52:57Z

I tried self.plane.zDir however, got slightly lost:

For this example:

def sample(angle):
    return(
        cq.Workplane("XY")
          .transformed(offset=(20, 5, 10), rotate=(angle, 0, 0))
          .rect(10,10)
          .workplane()
          .ellipseArc(20, 10, 90, 180, sense=-1, startAtCurrent=False))

calling sample(45) results in

while calling sample(46) results in

Why does changing the rotation angle from 45° to 46° change the orientation in which OCC creates ellipse arcs?
It will be in the same orientation from 46° until 135°

and then flip again 136°

bernhard-42 · 2020-02-08T18:07:53Z

I've opened an issue in pythonocc tpaviot/pythonocc-core#773

bernhard-42 · 2020-02-11T19:18:44Z

I went for the gp_Ax2(pnt, zDir, xDir) approach as mentioned in tpaviot/pythonocc-core#773 to ensure the orientation of ellipse arcs. If I now plot ellipseArcs onto a sphere (Rx and Ryrotate (0,0,20) around the x-axis or y-axis):

ellipses1 = [cq.Workplane("XY").transformed(offset=Rx(i, (0,0,20)), rotate=(i,0,0)).ellipseArc(3, 5, 0, 180, startAtCurrent=False).close().extrude(1) for i in range(0, 360, 30)]
ellipses2 = [cq.Workplane("XY").transformed(offset=Ry(i, (0,0,20)), rotate=(0,i,0)).ellipseArc(3, 5, 0, 180, startAtCurrent=False).close().extrude(1) for i in range(0, 360, 30)]

I get arcs of ellipses nicely aligned with the sphere surface:

jmwright · 2020-03-11T12:36:00Z

@bernhard-42 @adam-urbanczyk Where are we at with this pull request? Is pythonocc-core issue #773 a blocker for merging this?

adam-urbanczyk

Minor docstring remarks

cadquery/occ_impl/shapes.py

adam-urbanczyk · 2020-03-11T18:19:38Z

@bernhard-42 @jmwright two minor comments on the docstrings. Otherwise I think it can be merged. Using zDir and xDir sounds OK to me.

Co-Authored-By: Adam Urbańczyk <adam-urbanczyk@users.noreply.github.com>

bernhard-42 · 2020-03-14T17:35:24Z

@adam-urbanczyk Thought committing your proposals would be sufficient, however there is now a conflict. Shall I rebase my branch and push or can you resolve that?

adam-urbanczyk · 2020-03-14T17:54:03Z

I will resolve the conflict.

adam-urbanczyk · 2020-03-15T13:04:23Z

Alright, merging. Thanks for the contribution @bernhard-42 !

jmwright requested review from adam-urbanczyk and jmwright January 4, 2020 00:17

jmwright approved these changes Jan 5, 2020

View reviewed changes

cadquery/occ_impl/shapes.py Show resolved Hide resolved

adam-urbanczyk reviewed Jan 7, 2020

View reviewed changes

adam-urbanczyk reviewed Jan 23, 2020

View reviewed changes

bernhardBV added 7 commits February 10, 2020 20:26

added ellipse

475b2b0

removed unused math imports

6c72780

added method ellipseArc and adapted method ellipse to circle

6e9116c

introduced sense for ellipse building

282eddf

adapted ellipse test cases

55899a5

exclude vscode config folder

69d1e10

use gp_Ax2(p, zdir, xdir) for ellipse building

41c2343

ran black against the changes

914f75a

adam-urbanczyk requested changes Mar 11, 2020

View reviewed changes

cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved

cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved

bernhard-42 and others added 2 commits March 14, 2020 18:29

Fix docstring of makeEllipse

c1a8e9a

Co-Authored-By: Adam Urbańczyk <adam-urbanczyk@users.noreply.github.com>

Fix return value in docstring of makeEllips

1cb58b8

Co-Authored-By: Adam Urbańczyk <adam-urbanczyk@users.noreply.github.com>

adam-urbanczyk approved these changes Mar 14, 2020

View reviewed changes

adam-urbanczyk added 7 commits March 14, 2020 18:55

Merge branch 'master' into ellipse

3560689

Formatting fix

e1729c5

Increase test coverage

0ce5e5e

Formatting fixes

f7d873f

Add test for makeEllipse

47580f6

Test fix

a24b31f

Formatting + typo fix

e3edecd

adam-urbanczyk merged commit a499a4b into CadQuery:master Mar 15, 2020

adam-urbanczyk mentioned this pull request Apr 2, 2020

Implement ellipse shape #261

Closed

jmwright mentioned this pull request Dec 31, 2020

2020 CadQuery Community Update #558

Closed

	e = Edge.makeSpline(allPoints, tangents=tangents, periodic=periodic)

	if makeWire:
	rv = Wire.assembleEdges([e])
	if not forConstruction:
	self._addPendingWire(rv)
	else:
	rv = e
	if not forConstruction:
	self._addPendingEdge(e)

Ellipse #265

Ellipse #265

Uh oh!

Conversation

bernhard-42 commented Jan 3, 2020

Uh oh!

codecov bot commented Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bernhard-42 commented Jan 3, 2020

Uh oh!

jmwright commented Jan 3, 2020

Uh oh!

bernhard-42 commented Jan 3, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bernhard-42 commented Jan 10, 2020

Uh oh!

jmwright commented Jan 10, 2020

Uh oh!

adam-urbanczyk commented Jan 11, 2020

Uh oh!

bernhard-42 commented Jan 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernhard-42 commented Feb 8, 2020

Uh oh!

bernhard-42 commented Feb 8, 2020

Uh oh!

bernhard-42 commented Feb 11, 2020

Uh oh!

jmwright commented Mar 11, 2020

Uh oh!

adam-urbanczyk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adam-urbanczyk commented Mar 11, 2020

Uh oh!

bernhard-42 commented Mar 14, 2020

Uh oh!

adam-urbanczyk commented Mar 14, 2020

Uh oh!

adam-urbanczyk commented Mar 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

codecov bot commented Jan 3, 2020 •

edited

Loading

bernhard-42 commented Jan 18, 2020 •

edited

Loading