Skip to content
This repository has been archived by the owner on Feb 9, 2021. It is now read-only.

Add radius arc #254

Merged
merged 4 commits into from
Apr 25, 2018
Merged

Add radius arc #254

merged 4 commits into from
Apr 25, 2018

Conversation

gntech
Copy link
Contributor

@gntech gntech commented Apr 19, 2018

Add an function that draws an arc that is determined by startPoint, endPoint and radius.

@coveralls
Copy link

coveralls commented Apr 19, 2018

Coverage Status

Coverage decreased (-0.1%) to 92.203% when pulling d75ed82 on gntech:add-radiusArc into 86f3b7c on dcowden:master.

@AppVeyorBot
Copy link

Build cadquery 1.0.21 completed (commit f6c83eb355 by @gntech)

@codecov-io
Copy link

codecov-io commented Apr 19, 2018

Codecov Report

Merging #254 into master will decrease coverage by 0.1%.
The diff coverage is 85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   92.85%   92.74%   -0.11%     
==========================================
  Files          10       10              
  Lines        2196     2206      +10     
==========================================
+ Hits         2039     2046       +7     
- Misses        157      160       +3
Impacted Files Coverage Δ
cadquery/cq.py 94.12% <85%> (-0.35%) ⬇️

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 86f3b7c...d75ed82. Read the comment docs.

@dcowden
Copy link
Owner

dcowden commented Apr 19, 2018

@gntech thanks for another contribution! this looks great!
Is it ready to merge in from your view?

@gntech
Copy link
Contributor Author

gntech commented Apr 20, 2018

Actually, I was thinking about some things.

  1. For example: If the linear distance between startPoint and endPoint is more than 2 * radius the arc will fail since it it geometrically impossible. If this is the case I would like to raise some kind of Error and
    present it to the user in a logical way. Do you already do this kind of sanity checking of the parameters for other functions? Can you point me to some example?
if endPoint.sub(startPoint).Length > 2 * radius:
    raise ValueError
  1. The tests I added for the radiusArc and sagittaArc functions are basically just a test that the arc functions dont throw any error. They don't actually test if the result is correct. Do you have any suggestion how I can improve the test cases for the arc functions?

@jmwright
Copy link
Collaborator

Do you already do this kind of sanity checking of the parameters for other functions? Can you point me to some example?

I think that rasing a ValueError is how we do it elsewhere. See here for a case that's inside the CQ core code, and here for a case that's in an example.

Do you have any suggestion how I can improve the test cases for the arc functions?

Not sure if this will do exactly what you're looking for, but you can check the length of edges. The last line in this sample code is the important one.

import cadquery as cq

(L, H, W, t) = (100.0, 20.0, 20.0, 1.0)

# Define the points that the polyline will be drawn to/thru
pts = [
    (W/2.0, H/2.0),
    (W/2.0, (H/2.0 - t)),
    (t/2.0, (H/2.0-t)),
    (t/2.0, (t - H/2.0)),
    (W/2.0, (t - H/2.0)),
    (W/2.0, H/-2.0),
    (0, H/-2.0)
]

result = cq.Workplane("front").moveTo(0, H/2.0) \
                              .polyline(pts)

# Do check on edge lengths here. May have to get the radius indirectly
print(result.edges('>Y').first().val().Length())

Notice the .val() call to get the underlying shape object. The higher level Workplane object doesn't have functions like Length() attached to it.

@jmwright
Copy link
Collaborator

@gntech Also, after looking at the website you have in your profile link, you might want to check out issue #173 @RustyVermeer has been working on DXF import, which may help you a lot, especially if the 2D profiles in your designs get complex.

@fragmuffin
Copy link
Contributor

I would create a series of arcs and assert:

  • start, end, and midpoint locations
  • wire type (edge.geomType() == 'CIRCLE')

This apply_spline() function iterates along an arc or a spline getting vertices at different lengths along the wire.
It shows how to get the length of an arc.

Copy link
Contributor

@fragmuffin fragmuffin left a comment

Choose a reason for hiding this comment

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

Suggestion for ValueError implementation

cadquery/cq.py Outdated

# Calculate the sagitta from the radius
length = endPoint.sub(startPoint).Length / 2.0
sag = abs(radius) - math.sqrt(radius**2 - length**2)
Copy link
Contributor

Choose a reason for hiding this comment

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

As you mentioned, if the radius < length then the arc is not possible.
I think the best way to capture this is with a try block over the sag calculation

try:
    sag = abs(radius) - math.sqrt(radius**2 - length**2)
except ValueError:
    raise ValueError("Arc radius is not large enough to reach the given end-point")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jmwright and @fragmuffin: Thanks! Good suggestions, I will try to get this implemented later this weekend.

Since both radiusArc and sagittaArc boils down to a threePointArc in the end I want to be able to just return a threePointArc.
This increases code reuse. However, the function toWorldCoords seems to do something strange to a vector that already is in World coordinates.
This commit checks if the point already is an Vector before trying to make it an vector.

I also refined the testcases by checking edge length and edge type.
@gntech
Copy link
Contributor Author

gntech commented Apr 22, 2018

See my latest commit, I discovered something strange with the toWorldCoords function. When runned in sequence it changes the coordinates. I dont think that this is the intent?

@AppVeyorBot
Copy link

Build cadquery 1.0.22 completed (commit 3778358d0e by @gntech)

@gntech
Copy link
Contributor Author

gntech commented Apr 24, 2018

I realize that I made some wrong assumptions about what toWorldCoords do.

But this is my intention: sagittaArc, radiusArc and threePointArc all uses startPoint and endPoint but determines the arc between them in different ways: sag, radius or midPoint.

My idea is that sagittaArc and radiusArc should just calculate the corresponding midPoint and then hand that midpoint over to threePointArc for the actual arc creation.

Does this sound reasonable or is it prefferred to have separate functions that create the arcs by themself?

I will try to write some code later tonight to better explain.

@jmwright
Copy link
Collaborator

Does this sound reasonable or is it prefferred to have separate functions that create the arcs by themself?

I don't know what @dcowden thinks, but I think your approach is fine. I try to reuse existing building blocks whenever possible, which it sounds like you're doing.

@dcowden
Copy link
Owner

dcowden commented Apr 25, 2018

@gntech i like your approach.

@AppVeyorBot
Copy link

Build cadquery 1.0.23 completed (commit e6768412b8 by @gntech)

@dcowden
Copy link
Owner

dcowden commented Apr 25, 2018

@gntech this looks good, +1 from me. Are you ready to merge?

@jmwright does this get +1 from you?

@gntech
Copy link
Contributor Author

gntech commented Apr 25, 2018

Yes, I consider this ready for merge

@jmwright
Copy link
Collaborator

+1 from me

@dcowden dcowden merged commit c4df65c into dcowden:master Apr 25, 2018
@dcowden
Copy link
Owner

dcowden commented Apr 25, 2018

@gntech thanks for this contribution! You rock!

@gntech gntech deleted the add-radiusArc branch April 25, 2018 20:17
@gntech
Copy link
Contributor Author

gntech commented Apr 25, 2018

Thank you! And thanks for your work on cadquery which is awesome!

@dcowden
Copy link
Owner

dcowden commented Apr 25, 2018

@gntech absolutely! thanks for the kind words.

I think we can do really amazing things with cq once we get an OCC layer implemented, and we get proper feature trees and such.

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

Successfully merging this pull request may close these issues.

None yet

7 participants