added combine="cut" option for 3D operations #954

Jojain · 2022-01-07T22:55:06Z

Aims to target #30 I added the possibility to use combine="cut" for 3D operations to remove material from the context solid.
It's essentially a shorcut of calling cut() later.

I didn't added it for extrude as it has already cutBlind that does the same thing, I didn't added it to text since it has a cut kwarg. I think it should be changed to be consistent with all the other methods but since it will probably not be agreed on I didn't changed it.

As I already said in #832, I still think everything should be made consistent and that we should remove/deprecate things that aren't consistent. I know it would require breaking change but not doing it will likely confuse more and more new users.

added the option to combine="cut" for revolve and loft, added tests removed old arg of loft that was actually unused

added combine cut for twistextrude sweep. Didn't added it for extrude as it already as cutBlind and the option to extrude until a given face is combine by default and would need a little bit more change to handle combine=cut (but its still possible)

codecov · 2022-01-07T23:49:57Z

Codecov Report

Merging #954 (67e057d) into master (4fcada6) will increase coverage by 0.08%.
The diff coverage is 98.64%.

❗ Current head 67e057d differs from pull request most recent head 9217b3f. Consider uploading reports for the commit 9217b3f to get more accurate results

@@            Coverage Diff             @@
##           master     #954      +/-   ##
==========================================
+ Coverage   96.08%   96.17%   +0.08%     
==========================================
  Files          40       40              
  Lines        9257     9286      +29     
  Branches     1112     1109       -3     
==========================================
+ Hits         8895     8931      +36     
+ Misses        209      208       -1     
+ Partials      153      147       -6

Impacted Files	Coverage Δ
cadquery/cq.py	`92.28% <96.55%> (+0.53%)`	⬆️
cadquery/occ_impl/shapes.py	`92.54% <100.00%> (ø)`
cadquery/occ_impl/sketch_solver.py	`91.14% <100.00%> (ø)`
tests/test_cadquery.py	`99.29% <100.00%> (+0.01%)`	⬆️
tests/test_sketch.py	`100.00% <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 4fcada6...9217b3f. Read the comment docs.

Jojain · 2022-01-10T21:47:40Z

I've added test to target the lines marked as non tested but it keeps failing

jmwright · 2022-01-11T14:05:00Z

I've added test to target the lines marked as non tested but it keeps failing

I think it's fine. We have to sometimes ignore codecov when it complains about minor things.

cadquery/cq.py

jmwright · 2022-01-11T14:23:24Z

cadquery/cq.py

@@ -2993,7 +2993,9 @@ def twistExtrude(

        r = Compound.makeCompound(shapes).fuse()

-        if combine:
+        if isinstance(combine, str) and combine == "cut":


It seems like this section of code ends up being repeated quite a bit, but I'm not sure that there's an quick/efficient way to avoid the code duplication. Probably best just to leave it for now unless someone has an idea.

I'd propose to refactor and move it to _combineWithBase

I'm not sure to understand what we are talking about. The plan would be to move what to _combineWithBase ?

There is a lot of repetitive code like:

if isinstance(combine, str) and combine == "cut": newS = self._cutFromBase(r) elif isinstance(combine, bool) and combine: newS = self._combineWithBase(r) else: newS = self.newObject([r])

it could be refactored to:

return self._combineWithBase(r, combine)

(ofc you'd need to update _combineWithBase accordingly).

Ok I think I've refactored _combineWithBase like you thought about it.
I'm proposing to add a breaking change to text that has both cut and combine kwarg. In my opinion we should get rid of the cut kwarg and keep the combine consistent with other methods. It would be fairly easy for user to make the change but would result in errors in model that still use it

Let me know if there is an agreement on this or not so I can make the change (or not) @adam-urbanczyk @jmwright @marcus7070

Long term it is fine, but please use this for now:

cadquery/cadquery/utils.py

Line 8 in ea66ded

class deprecate_kwarg:

.

jmwright

Thanks @Jojain !

Jojain · 2022-01-11T14:42:27Z

I've added test to target the lines marked as non tested but it keeps failing

I think it's fine. We have to sometimes ignore codecov when it complains about minor things.

Well ok, just let me know if there is something I missed, I'm not really used to add tests so I'm not always sure if it's done right or not

Co-authored-by: Jeremy Wright <wrightjmf@gmail.com>

Jojain · 2022-01-12T22:25:37Z

As discussed on discord I'm not sure it make sense to add the option for each for me it would result in wrong use of the method. This target the issue #754

I've not added it to extrude since there is already cutBlind and the extrude method is already very complex, it would however make sense to still add it (but requires a bit more work) and deprecate cutBlind, what are your thoughts about it ?

Finally github says there is conflicts but there isnt, I cannot solve it so a lead dev will have to resolve the so called conflicts

jmwright · 2022-01-12T22:41:50Z

I've not added it to extrude since there is already cutBlind and the extrude method is already very complex, it would however make sense to still add it (but requires a bit more work) and deprecate cutBlind, what are your thoughts about it ?

My vote would probably be to keep cutBlind(), but re-implement it to use the new combine/cut functionality. That way we don't have to break existing scripts, but we're leveraging the latest best practices within the codebase. Thoughts?

Jojain · 2022-01-12T23:14:35Z

My vote would probably be to keep cutBlind(), but re-implement it to use the new combine/cut functionality. That way we don't have to break existing scripts, but we're leveraging the latest best practices within the codebase. Thoughts?

Well I don't think it makes sense to have combine/cut with cutBlind since what it does is explicitly in the name, we can maybe make extrude call cutBlind internaly if it's set with combine="cut" and keep it cutBlind as it is, just telling user that best practice are to use extrude with combine="cut" instead of cutBlind. That a good compromise IMHO

adam-urbanczyk · 2022-01-13T06:46:00Z

To make everything self-consistent (make the code shorter and get rid of special cases) it would make sense to add the option to every method that allows to use combine. For now forget about cutBlind etc.

Jojain · 2022-01-14T19:14:49Z

Hopefully this is done.

In the end I didn't added combine option to each because I don't think it fits the method well, and actually having it only in eachpoint may lead users to better choose which method they actually need for what they want to do.

I still don't know about the merging issues I let you check that, all test are green on my side.

jmwright · 2022-01-17T12:15:40Z

I see that there were commits after the last comment that this was ready. Is it ready for a final review now?

Jojain · 2022-01-17T12:52:42Z

@jmwright yes it is

fedorkotov · 2022-01-17T14:43:10Z

Please don't forget to update changes.md file. It's very helpful when trying to get up to speed with CadQuery after a long pause.

* _combineWithBase allows to clean too * added missing clean params

Needed for backward compatibility (one test failing).

adam-urbanczyk · 2022-01-19T18:37:17Z

cadquery/cq.py

        else:
-            newS = self.newObject([r])
+            # do not combine branch
+            newS = self.newObject(obj if not isinstance(obj, Shape) else [obj])


I'm not sure that that is the optimal way. We might want to rewrite the failing test

adam-urbanczyk · 2022-01-19T18:38:12Z

cadquery/cq.py

@@ -61,6 +62,7 @@

 CQObject = Union[Vector, Location, Shape, Sketch]
 VectorLike = Union[Tuple[float, float], Tuple[float, float, float], Vector]
+CombineMode = Union[bool, Literal["cut", "a", "s"]]  # a : additive, s: subtractive


I added also modes used in Sketch. Note that it is better to use Literal for typing.

Co-authored-by: Jeremy Wright <wrightjmf@gmail.com>

adam-urbanczyk · 2022-01-19T18:39:44Z

Thanks for the contribution @Jojain ! I did some rework, let me know what you think.

Jojain

My comments aside I'm good with what you came up with. I think the code simplification is great.
It would be useful to document the changes in the docs as they introduce a "new concept" which is that every protusion method can also be used to remove material.
I think we should lead newcomers in the docs to use the combine argument so hopefully CQ scripts would be more unified

Jojain · 2022-01-19T22:19:30Z

cadquery/cq.py


-                r = self._extrude(None, both=both, taper=taper, upToFace=faceIndex)
+        # Handle `until` multiple values
+        if isinstance(until, str) and until in ("next", "last") and combine:


If combine is "cut" the conditional test will be True but the executed code won't be what's expected.
That's why I added an explicit condition to check if combine=="cut" and if this True we call the cutBlind method.
I think we should keep my first check

Thanks, missed that. I think it is now fixed.

cadquery/cq.py

Jojain · 2022-01-19T22:24:37Z

cadquery/cq.py

@@ -2392,7 +2393,7 @@ def each(
        self: T,
        callback: Callable[[CQObject], Shape],
        useLocalCoordinates: bool = False,
-        combine: Union[bool, str] = True,
+        combine: CombineMode = True,


I already discussed my opinion on adding combine arg for this method so I won't talk about it again but in any case if we decide to add it we should probably write somewhere that the new default behaviour is to combine with the base which wasnt the case before. Same remark goes for eachpoint

Good point. Do you think it would make sense to set it to False? I'm kind of hesitating about it. What do you think @jmwright?

Setting it to False would make things transparent but we would loose consistency with other methods.
I don't think each and eachpoint are that used so a breaking change here would probably not be a big deal.

OK, I added a note in changes.md

adam-urbanczyk · 2022-01-23T20:06:32Z

@jmwright @Jojain I'm done with my adjustments. Please take a final look.

jmwright · 2022-01-25T14:26:56Z

@Jojain This looks good to me. Do you think it is ready to merge?

Jojain · 2022-01-25T19:12:59Z

@jmwright @adam-urbanczyk It's all good for me

jmwright · 2022-01-25T20:06:05Z

Thanks!

Jojain added 4 commits January 7, 2022 23:10

added cut for revolve and loft

5fb8823

added the option to combine="cut" for revolve and loft, added tests removed old arg of loft that was actually unused

added combine cut for all 3D ops

0dab2be

added combine cut for twistextrude sweep. Didn't added it for extrude as it already as cutBlind and the option to extrude until a given face is combine by default and would need a little bit more change to handle combine=cut (but its still possible)

mypy correct

1460545

Update cq.py

0329d21

Jojain added 6 commits January 10, 2022 20:22

added tests

973dfda

Update test_cadquery.py

4bf22a9

added test

f9b5b07

Update test_cadquery.py

db070a9

Update test_cadquery.py

699ffa1

Trigger CI

b8c0063

jmwright reviewed Jan 11, 2022

View reviewed changes

cadquery/cq.py Outdated Show resolved Hide resolved

jmwright reviewed Jan 11, 2022

View reviewed changes

cadquery/cq.py Outdated Show resolved Hide resolved

jmwright reviewed Jan 11, 2022

View reviewed changes

jmwright approved these changes Jan 11, 2022

View reviewed changes

jmwright requested a review from adam-urbanczyk January 11, 2022 14:26

Jojain and others added 7 commits January 11, 2022 18:01

Update cadquery/cq.py

0cf408a

Co-authored-by: Jeremy Wright <wrightjmf@gmail.com>

refactor _combineWithBase

daa994b

mypy correction

3631e07

updated text and added deprecation warning

502dc3a

Update cq.py

59df1c0

resolved conflicts

5a7c254

Update utils.py

42e144b

Jojain added 2 commits January 14, 2022 20:03

Update utils.py

e2108d2

black fix

ee96ca7

adam-urbanczyk and others added 4 commits January 14, 2022 22:02

Merge branch 'master' into cut_kw_arg

4ff5f1a

black on test_cadquery.py

6ee6843

mypy fixes

b917810

remove unused import

8c63fc4

jmwright self-requested a review January 15, 2022 19:36

adam-urbanczyk added 2 commits January 19, 2022 19:13

Changed annotations and simplified the code

f95227c

* _combineWithBase allows to clean too * added missing clean params

Different clean handling

1b4fb27

Needed for backward compatibility (one test failing).

adam-urbanczyk approved these changes Jan 19, 2022

View reviewed changes

Typo fix

b4623a7

Co-authored-by: Jeremy Wright <wrightjmf@gmail.com>

Jojain commented Jan 19, 2022

View reviewed changes

adam-urbanczyk added 4 commits January 22, 2022 21:10

Update docstring

4f5bd96

Fix extrude in subtractive mode

92e1488

Fix Shape.__eq__

1a9e40d

Update changes.md

82b7f58

jmwright approved these changes Jan 25, 2022

View reviewed changes

docstring update

9217b3f

jmwright merged commit e0b4c94 into CadQuery:master Jan 25, 2022

Jojain deleted the cut_kw_arg branch January 25, 2022 20:49

adam-urbanczyk mentioned this pull request Jul 13, 2023

Implement subtractive features #30

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added combine="cut" option for 3D operations #954

added combine="cut" option for 3D operations #954

Jojain commented Jan 7, 2022

codecov bot commented Jan 7, 2022 •

edited

Loading

Jojain commented Jan 10, 2022

jmwright commented Jan 11, 2022

jmwright Jan 11, 2022

adam-urbanczyk Jan 11, 2022

Jojain Jan 11, 2022

adam-urbanczyk Jan 12, 2022

Jojain Jan 12, 2022

adam-urbanczyk Jan 12, 2022

jmwright left a comment

Jojain commented Jan 11, 2022

Jojain commented Jan 12, 2022 •

edited

Loading

jmwright commented Jan 12, 2022

Jojain commented Jan 12, 2022 •

edited

Loading

adam-urbanczyk commented Jan 13, 2022

Jojain commented Jan 14, 2022

jmwright commented Jan 17, 2022

Jojain commented Jan 17, 2022

fedorkotov commented Jan 17, 2022

adam-urbanczyk Jan 19, 2022

adam-urbanczyk Jan 19, 2022

adam-urbanczyk commented Jan 19, 2022

Jojain left a comment •

edited

Loading

Jojain Jan 19, 2022 •

edited

Loading

adam-urbanczyk Jan 22, 2022

Jojain Jan 19, 2022

adam-urbanczyk Jan 22, 2022 •

edited

Loading

Jojain Jan 22, 2022

adam-urbanczyk Jan 23, 2022

adam-urbanczyk commented Jan 23, 2022

jmwright commented Jan 25, 2022

Jojain commented Jan 25, 2022

jmwright commented Jan 25, 2022

added combine="cut" option for 3D operations #954

added combine="cut" option for 3D operations #954

Conversation

Jojain commented Jan 7, 2022

codecov bot commented Jan 7, 2022 • edited Loading

Codecov Report

Jojain commented Jan 10, 2022

jmwright commented Jan 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmwright left a comment

Choose a reason for hiding this comment

Jojain commented Jan 11, 2022

Jojain commented Jan 12, 2022 • edited Loading

jmwright commented Jan 12, 2022

Jojain commented Jan 12, 2022 • edited Loading

adam-urbanczyk commented Jan 13, 2022

Jojain commented Jan 14, 2022

jmwright commented Jan 17, 2022

Jojain commented Jan 17, 2022

fedorkotov commented Jan 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-urbanczyk commented Jan 19, 2022

Jojain left a comment • edited Loading

Choose a reason for hiding this comment

Jojain Jan 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-urbanczyk Jan 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-urbanczyk commented Jan 23, 2022

jmwright commented Jan 25, 2022

Jojain commented Jan 25, 2022

jmwright commented Jan 25, 2022

codecov bot commented Jan 7, 2022 •

edited

Loading

Jojain commented Jan 12, 2022 •

edited

Loading

Jojain commented Jan 12, 2022 •

edited

Loading

Jojain left a comment •

edited

Loading

Jojain Jan 19, 2022 •

edited

Loading

adam-urbanczyk Jan 22, 2022 •

edited

Loading