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

Expose makeWire to parametricCurve #555

Merged
merged 1 commit into from
Dec 29, 2020

Conversation

just-georgeb
Copy link
Contributor

The use case - creating a complex wire made up of multiple parametricCurves or a combination of parametricCurves and other 2d operations. I think the only way currently would be to create each parametricCurve individually (creating separate spline wires) and then using Wire.combine() at the end, which seems messy.

@adam-urbanczyk
Copy link
Member

You can already achieve this by calling .wire(), why this PR?

@just-georgeb
Copy link
Contributor Author

Seems odd to me that the Spline() function has makeWire as an option, but parametricCurve() (which calls spline under the hood) does not.

For me I am trying to make gear geometry - so I'm using parametricCurve() for the tooth flanks, with radiusArc() in between. I actually did this a little while ago so it's not as fresh in memory as I'd like - but from memory the issue was that with makeWire=True, the current position on the workplane doesn't change, so you then need to use moveTo() as well as combining the wires at the end to make a continuous profile.

If not wanted then that's fine though - it was simple to add and helped me out so thought I'd add it as a PR

@adam-urbanczyk
Copy link
Member

Thanks @just-georgeb , making things consistent with spline sounds like a good rationale. If all tests pass we'll take a look and merge it.

BTW: if you manage to make a nice gear with CQ please share the code if possible.

@codecov
Copy link

codecov bot commented Dec 27, 2020

Codecov Report

Merging #555 (e6feeb9) into master (3fa9753) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #555   +/-   ##
=======================================
  Coverage   94.19%   94.19%           
=======================================
  Files          29       29           
  Lines        6220     6220           
  Branches      665      665           
=======================================
  Hits         5859     5859           
  Misses        224      224           
  Partials      137      137           
Impacted Files Coverage Δ
cadquery/cq.py 89.65% <100.00%> (ø)

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 3fa9753...e6feeb9. Read the comment docs.

@adam-urbanczyk adam-urbanczyk self-requested a review December 27, 2020 22:00
@just-georgeb
Copy link
Contributor Author

Definitely will do! It's working at the moment but the code is a real mess and a bit bandit so needs some work.

Also sorry have messed this commit up - needs another change before you review so it actually uses the makeWire value!

@just-georgeb
Copy link
Contributor Author

Should be good to go now by the way (not sure how I managed to make such a mess of 3 lines!)

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.

Looks good, thanks @just-georgeb

@jmwright jmwright merged commit 84acb70 into CadQuery:master Dec 29, 2020
@just-georgeb just-georgeb deleted the parametricCurveFix branch January 2, 2021 00:30
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.

3 participants