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

rework Workplane.split #752

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

rework Workplane.split #752

wants to merge 3 commits into from

Conversation

marcus7070
Copy link
Member

Now also accepts Plane as an argument. Workplane split now goes through Solid.split instead of Solid.cut.

Still to do:

  • tests
  • docs

Related to #751.

Now also accepts Plane as an argument. Workplane split now goes through
Solid.split instead of Solid.cut.
@codecov
Copy link

codecov bot commented May 1, 2021

Codecov Report

Merging #752 (1b3a129) into master (2b7f39b) will decrease coverage by 0.06%.
The diff coverage is 78.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
- Coverage   94.64%   94.58%   -0.07%     
==========================================
  Files          32       32              
  Lines        7249     7256       +7     
  Branches      789      792       +3     
==========================================
+ Hits         6861     6863       +2     
- Misses        255      258       +3     
- Partials      133      135       +2     
Impacted Files Coverage Δ
cadquery/cq.py 90.90% <78.26%> (-0.43%) ⬇️

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 2b7f39b...1b3a129. Read the comment docs.

@marcus7070
Copy link
Member Author

marcus7070 commented May 2, 2021

I think I'll work on #464 (comment) before I continue this PR. I think it makes more sense to expand the functionality of Workplane.workplane. Then we can keep the current method of splitting on a workplane instead of introducing new code to do almost the same thing (splitting on a Plane).

If there is any use case that is not covered with an improved Workplane.workplane then we can have a look at .split(Plane(...)).

@fedorkotov
Copy link
Contributor

If there is any use case that is not covered with an improved Workplane.workplane then we can have a look at .split(Plane(...)).

Such a use case is splitting with a plane specified in world coordinate system (not related to any face or other feature of the model).

@adam-urbanczyk
Copy link
Member

adam-urbanczyk commented May 2, 2021

AFAIR the point of #464 would be to allow that too (i.e. specify origin).

@fedorkotov
Copy link
Contributor

@adam-urbanczyk I thought origin was specified in local coordinate system of current face or other object because of this passage from #464 description

Just as a side note, IMHO the Workplane.workplane method is focussed around making workplanes relative to a face. I like it how it is and I don't think it should be overloaded/overcomplicated to do this job as well.

If it is specified in world coordinate system then indeed my case is covered too.

@lalebarde
Copy link

How to use it please ?

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.

4 participants