-
Notifications
You must be signed in to change notification settings - Fork 299
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
Implement makeSplineApprox for edges and faces #694
Conversation
Codecov Report
@@ Coverage Diff @@
## master #694 +/- ##
==========================================
+ Coverage 94.41% 94.52% +0.11%
==========================================
Files 31 32 +1
Lines 6909 7071 +162
Branches 740 766 +26
==========================================
+ Hits 6523 6684 +161
- Misses 252 254 +2
+ Partials 134 133 -1
Continue to review full report at Codecov.
|
Will I have needed to do that before, when making a ruled surface between a Wire with 4 edges and a spline with one edge. I needed to add 3 more vertices at certain locations on the spline. It was awkward and being able to split wires would be great improvement! |
I did a quick check and it seems to work. |
I pushed two more tests. Feel free to revert if you don't want them, it was just easier to write the code than suggest them in comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've still got more to look at here, I'll try to get back to it later today.
@adam-urbanczyk pushed some 500 lines of code without needing the "black fix" commit, I push like 15 LOC and mess up the streak. 🤦 |
I don't think we test splitting with multiple tool Shapes? edit: referring to the
edit: Added a test for this case below. |
Co-authored-by: Marcus Boyd <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @adam-urbanczyk this was a lot of work. I just had a few minor comments.
OK, I think this is ready to be merged. |
@adam-urbanczyk Thanks again! |
Will resolve #682
Edge.makeSplineApprox
Face.makeSplineApprox
parametricCurve
parametricSurface
Edge.close
BRepAlgoAPI_Splitter
and rework Workplane.splitAlso related to #675