Added AreaNthSelector by fedorkotov · Pull Request #688 · CadQuery/cadquery

fedorkotov · 2021-03-14T18:58:24Z

Added AreaNthSelector that is useful for nested features selection. Especially to select one of coplanar nested wires for subsequent extrusion, cutting or filleting.

This selector was developed as a result of discussion in #684

Added AreaNthSelector that is useful for nested features selection. Especially to select one of coplanar nested wires for subsequent extrusion, cutting or filleting.

fedorkotov · 2021-03-14T19:04:05Z

Unit tests for Solids and Shells are a bit contrived. Is there a way to get more than one Solid or Shell to select from while using Workplane? Or maybe using this selector for Solids and Shells makes no sense and support for them should be removed?

codecov · 2021-03-14T19:36:44Z

Codecov Report

Merging #688 (93fceff) into master (a54de64) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #688      +/-   ##
==========================================
+ Coverage   94.33%   94.41%   +0.08%     
==========================================
  Files          31       31              
  Lines        6810     6909      +99     
  Branches      732      740       +8     
==========================================
+ Hits         6424     6523      +99     
  Misses        252      252              
  Partials      134      134

Impacted Files	Coverage Δ
cadquery/selectors.py	`98.14% <100.00%> (+0.12%)`	⬆️
tests/__init__.py	`88.23% <100.00%> (ø)`
tests/test_selectors.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 a54de64...93fceff. Read the comment docs.

fedorkotov · 2021-03-14T20:29:53Z

Black is complaining about formatting. IMHO it makes the code less readable by merging long sequences of fluent API calls that I format as one call per line into long lines with too many calls. But if that is the accepted formatting style of the project I will implement black's recommendations

marcus7070 · 2021-03-15T00:19:31Z

Black is complaining about formatting. IMHO it makes the code less readable by merging long sequences of fluent API calls that I format as one call per line into long lines with too many calls. But if that is the accepted formatting style of the project I will implement black's recommendations

Yes please @fedorkotov. I know black isn't perfect, but it's much easier to read one consistent style.

fedorkotov · 2021-03-15T09:26:08Z

Two questions

AreaNthSelector is very similar to LengthNthSelector proposed in Length selector #646. Maybe I should add length selector in this pull request?
Maybe I should add A (area) and L (length) to string selectors grammar with allowed usage like this
1. >A - objects with the largest area
2. <L[-2] - objects with second smallest length

Or is it better to keep things separated and both of these additional features should have their separate pull requests?

marcus7070 · 2021-03-15T10:42:27Z

AreaNthSelector is very similar to LengthNthSelector proposed in Length selector #646.

I was thinking the same thing so I actually did #646 earlier today, thinking that the process would help me review this PR. I just put up the code as #690.

Maybe I should add A (area) and L (length) to string selectors grammar with allowed usage like this

>A - objects with the largest area

<L[-2] - objects with second smallest length

Or is it better to keep things separated and both of these additional features should have their separate pull requests?

In this case I think we should do another PR, because we can roll area, length and radius selectors into one review.

fedorkotov · 2021-03-15T11:23:23Z

I was thinking the same thing so I actually did #646 earlier today, thinking that the process would help me review this PR. I just put up the code as #690.

Ok. I will include LengthNthSelector into my pull request.
But first one more question. I see in your LengthNthSelector you use isinstance(obj, (...)) to determine what kind of Shape was passed into _NthSelector.key(..) implementation. I used Shape.ShapeType() in my AreaNthSelector for the same purpose. I assumed that comparing strings would be faster than type information manipulation but did not measure that. Should I change to isinstance? Or are these variants roughly equivalent?

In this case I think we should do another PR, because we can roll area, length and radius selectors into one review.

Ok.

marcus7070

You can include the LengthNthSelector code from #690 in here if you want, or we can work on #690 after this gets merged (which would be less work for you), I don't mind.

marcus7070 · 2021-03-15T11:47:40Z

+    .rect(20, 20)
+    .extrude(10)  # solid 20x20x10 box


Usually I would insist these lines be changed to .box(20, 20, 10, centered=(True, True, False)) but this hangs the kernel in the final cutBlind step, and I can't work around it either! So it will have to stay as this rect version until that bug is fixed. No change required here, this is just a comment.

marcus7070 · 2021-03-15T12:25:42Z

Unit tests for Solids and Shells are a bit contrived. Is there a way to get more than one Solid or Shell to select from while using Workplane?

Something like

shell = (
    cq.Workplane()
    .box(1, 1, 1)
    .faces(">Z")
    .shell(0.1)
    .val()
)
workplane_shells = (
    cq.Workplane()
    .rarray(2, 2, 3, 4)
    .eachpoint(lambda loc: shell.located(loc))
)

edit: the above code produces a Workplane with 12 solids, not shells, which was a confusing example for me to choose, sorry.

You can also union different sized shells/solids, which would give you more variety to test an area selector on.

Or maybe using this selector for Solids and Shells makes no sense and support for them should be removed?

Solids and shells have clearly defined areas, so I support this selector working on them.

marcus7070 · 2021-03-15T12:32:30Z

If you check https://raw.githubusercontent.com/CadQuery/cadquery/master/doc/classreference.rst you'll see where we need to add AreaNthSelector to make it appear in the list of selectors at https://cadquery--688.org.readthedocs.build/en/688/classreference.html#selector-classes

@marcus7070

- Switched Shape.ShapeType() to isinstance to determine what kind of Shape was passed into _NthSelector.key(..) implementation in AreaNthSelector for consistency with other selectors based on _NthSelector - Added LengthNthSelector developed by @marcus7070 in CadQuery#690 with somewhat modified unit tests - Added new selector references to documentation - Black compatible code examples in AreaNthSelector docstring - Explicitly marked _NthSelector class and _NthSelector.key(..) method as abstract using standard abc package

fedorkotov · 2021-03-15T19:51:52Z

Switched Shape.ShapeType() to isinstance to determine what kind of Shape was passed into _NthSelector.key(..) implementation in AreaNthSelector for consistency with other selectors based on _NthSelector
Added LengthNthSelector developed by @marcus7070 in Length selector #690 with somewhat modified unit tests
Added new selector references to documentation
Black compatible code examples in AreaNthSelector docstring
Explicitly marked _NthSelector class and _NthSelector.key(..) method as abstract using standard abc package

Can't figure out what caused Windows pipelines to fail. Don't have access to Windows machine right now.

The log says

DLL load failed: The specified module could not be found

But which module and which dll? And why?
Have you experienced something like that before?

marcus7070

I think all our Windows CI are failing, it's not due to your code. Once we get that fixed and can see all the tests pass I think this looks good to merge.

Thanks for doing this @fedorkotov, there is a lot of work here especially the tests.

AreaNthSelector.key raises ValueError if temporary face creation fails for a wire for any reason (non-planar, non-closed). That causes _NthSelector that it inherits to ignore such wires. Done so for consistency with RadiusNthSelector that ignores anything it can not get radius from.

fedorkotov · 2021-03-16T10:46:43Z

Forgot to check for non-planar or non-closed wires. Fixed.
For that I catch all exceptions that Face.makeFromWires(..) might throw. In general catching everything is a code smell. Should I catch something more specific?

adam-urbanczyk · 2021-03-16T18:38:08Z

@fedorkotov I propose to leave the catch as-is for now. makeFromWires needs some refactoring anyhow.

Thanks for all the hard work!

marcus7070

Good catch with that non-planar wire test.

marcus7070 · 2021-03-17T03:45:08Z

Thanks @fedorkotov!

Added AreaNthSelector

51932b1

Added AreaNthSelector that is useful for nested features selection. Especially to select one of coplanar nested wires for subsequent extrusion, cutting or filleting.

Fixed error: AreaNthSelector not imported in example 26

109c07d

marcus7070 self-requested a review March 14, 2021 19:43

Using cast to appease mypy type checker

fe58219

adam-urbanczyk self-requested a review March 14, 2021 20:55

fedorkotov added 2 commits March 15, 2021 09:34

Sacrificing some readability to Black Formatter

bb7a465

AreaNthSelector test docstrings, minor formatting

cf0286d

marcus7070 requested changes Mar 15, 2021

View reviewed changes

marcus7070 reviewed Mar 15, 2021

View reviewed changes

Comment thread cadquery/selectors.py Outdated

removed unused import

0fe9abd

marcus7070 approved these changes Mar 15, 2021

View reviewed changes

fedorkotov mentioned this pull request Mar 16, 2021

Importing cadquery in python 3.8 on Windows fails #692

Closed

fedorkotov requested a review from marcus7070 March 16, 2021 10:47

adam-urbanczyk approved these changes Mar 16, 2021

View reviewed changes

marcus7070 approved these changes Mar 17, 2021

View reviewed changes

marcus7070 merged commit d5ce132 into CadQuery:master Mar 17, 2021

This was referenced Mar 17, 2021

Length selector #690

Closed

Refactor Face.makeFromWires #693

Open

Length selector #646

Closed

adam-urbanczyk mentioned this pull request Mar 29, 2021

How to cut a lip on a case seam? #684

Closed

Uh oh!

Uh oh!

Conversation

fedorkotov commented Mar 14, 2021

Uh oh!

fedorkotov commented Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

fedorkotov commented Mar 14, 2021

Uh oh!

marcus7070 commented Mar 15, 2021

Uh oh!

fedorkotov commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcus7070 commented Mar 15, 2021

Uh oh!

fedorkotov commented Mar 15, 2021

Uh oh!

marcus7070 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcus7070 Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

marcus7070 commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

marcus7070 commented Mar 15, 2021

Uh oh!

fedorkotov commented Mar 15, 2021

Uh oh!

marcus7070 left a comment

Choose a reason for hiding this comment

Uh oh!

fedorkotov commented Mar 16, 2021

Uh oh!

adam-urbanczyk commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcus7070 left a comment

Choose a reason for hiding this comment

Uh oh!

marcus7070 commented Mar 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fedorkotov commented Mar 14, 2021 •

edited

Loading

codecov Bot commented Mar 14, 2021 •

edited

Loading

fedorkotov commented Mar 15, 2021 •

edited

Loading

marcus7070 left a comment •

edited

Loading

marcus7070 commented Mar 15, 2021 •

edited

Loading

adam-urbanczyk commented Mar 16, 2021 •

edited

Loading