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

Refactor selectors and add CenterNthSelector #549

Merged
merged 22 commits into from
Jan 5, 2021

Conversation

marcus7070
Copy link
Member

This PR will close #517 and add a CenterNthSelector to resolve #530. I touched a few other things as well, sorry for lumping them all in one PR but I've at least rebased all the commits into neat chunks.


Commit 6d2f3ba adds _NthSelector. It is an abstract class, it should only be inherited from, not instantiated.

Previously DirectionNthSelector lumped objects at a similar distance together by rounding that distance to a certain number of decimal places. This would mean that with a tolerance of 0.1, 0.249999999 and 0.250000001 would be put into seperate bins. So I've changed it to a simple clustering method. It's not perfect, but it's simple and I think it's an improvement over the previous one.

These two commits should not change or add any behaviour to CQ (except to remove that rounding issue). All our previous tests pass with these two new commits.


344651d added CenterNthSelector, to select the Nth object/objects along a direction. Similar to DirectionNthSelector, but without running them through ParallelDirSelector first.

Now DirectionMinMaxSelector can become a simple subclass of CenterNthSelector with the index set to -1.


Previously "Combining Selectors" appeared as a top level item on the side bar menu.
screenshot2020-12-18-111717

After a242473:
screenshot2020-12-18-154652


While looking at the docs I noticed the tables were overflowing and it was hard to read.

screenshot2020-12-18-154812

4486381 adds some extra CSS from here to change this behaviour, now tables (such as the Class Summary) line wrap. I've quickly checked all the pages in the HTML docs and I can't see any problems, I think it's only improved things. Now:

screenshot2020-12-18-154905


I've tried to add mypy type hints throughout all this, but I'm going to have to ask for help from the mypy gurus for one bit. I can't see why mypy errors on RadiusNthSelector.filter. I'm leaving this as a draft PR until I get the mypy errors sorted.

Added NthSelector abstract class and refactored RadiusNthSelector,
DirectionMinMaxSelector and DirectionNthSelector. Closes CadQuery#517.
Correct title underlining means the sidebar no longer shows "Combining
Selectors" as a top level item, instead it's a sub item under "String
Selectors Reference". Updated some other parts and expanded some
explanations. Resolves CadQuery#531.
@marcus7070
Copy link
Member Author

Hey GitHub, I said this will close #531 in my commit message, pay attention!

@adam-urbanczyk adam-urbanczyk mentioned this pull request Dec 18, 2020
@codecov
Copy link

codecov bot commented Dec 19, 2020

Codecov Report

Merging #549 (1dc170a) into master (a3ebdb9) will increase coverage by 0.06%.
The diff coverage is 98.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #549      +/-   ##
==========================================
+ Coverage   94.19%   94.26%   +0.06%     
==========================================
  Files          29       29              
  Lines        6220     6348     +128     
  Branches      665      675      +10     
==========================================
+ Hits         5859     5984     +125     
- Misses        224      226       +2     
- Partials      137      138       +1     
Impacted Files Coverage Δ
cadquery/selectors.py 98.01% <96.29%> (-1.16%) ⬇️
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 a3ebdb9...1dc170a. Read the comment docs.

cadquery/selectors.py Outdated Show resolved Hide resolved
@marcus7070
Copy link
Member Author

Thanks for the mypy stuff @adam-urbanczyk.

codecov is right, there is some checks I've missed, I'm working on some more tests now. Once I've got them done I think this is ready for review and I'll change from draft to regular pull request.

In certain circumstances this will be breaking, most commonly when using
DirectionNthSelector and there was previously a non-planar face in the
list. In this case eg. ">X[2]" will have to become ">X[1]".
@marcus7070
Copy link
Member Author

Commit e19a05d is breaking under certain circumstances.

I think it was always the intent for ParallelDirSelector to filter out any non-planar surface, but there was a typo where the check was performed on Edges and any Face was let through as long as it's .tangentAt(None) was correct. For instance:

import cadquery as cq

part = (
    cq
    .Workplane()
    .box(10, 10, 10)
    .faces(">X")
    .workplane()
    .hole(1)
)

Before selection:
screenshot2020-12-08-162315

.faces(">X[1]") and ">Z[1]" select the side of the cube, as you expect. .faces(">Y[1]") catches the cylinder, because it's normal happens to be in right direction.

screenshot2020-12-08-162330

So after e19a05d, if someone had made a model like above, used DirectionMinMaxSelector to select a planar face and caught a non-planar edge with the correct normal, their ">Y[2]" would now have to become "Y[1]".


This is ready for review now.

@marcus7070 marcus7070 marked this pull request as ready for review December 25, 2020 04:44
@adam-urbanczyk adam-urbanczyk self-requested a review December 26, 2020 21:10
@jmwright jmwright self-requested a review December 29, 2020 16:14
@adam-urbanczyk
Copy link
Member

Note to self: check if this is compatible with downstream applications, e.g.

https://github.com/michaelgale/cq-kit/blob/master/cqkit/cq_selectors.py

@marcus7070
Copy link
Member Author

Note to self: check if this is compatible with downstream applications, e.g.

https://github.com/michaelgale/cq-kit/blob/master/cqkit/cq_selectors.py

Great point, I did not check this.

@adam-urbanczyk
Copy link
Member

adam-urbanczyk commented Jan 2, 2021

@marcus7070 do you want to add the CenterNthSelector to the string syntax? Maybe like so: >>X[0]?

@marcus7070
Copy link
Member Author

marcus7070 commented Jan 3, 2021

Note to self: check if this is compatible with downstream applications, e.g.

https://github.com/michaelgale/cq-kit/blob/master/cqkit/cq_selectors.py

I just got the cq-kit tests to run with this branch. I get failures in cq-kit's file exporters related to a missing defaultdict, but all the selector tests pass.

collected 36 items                                                                                                                     

test_discrete.py ...                                                                                                             [  8%]
test_files.py .FFFF..                                                                                                            [ 27%]
test_geometry.py ...............                                                                                                 [ 69%]
test_pprint.py .                                                                                                                 [ 72%]
test_selectors.py ..........                                                                                                     [100%]

========================== short test summary info ==========================
FAILED test_files.py::test_step_export_simple - NameError: name 'defaultdict' is not defined
FAILED test_files.py::test_step_export - NameError: name 'defaultdict' is not defined
FAILED test_files.py::test_export_function - NameError: name 'defaultdict' is not defined
FAILED test_files.py::test_step_import - NameError: name 'defaultdict' is not defined
========================= 4 failed, 32 passed in 0.93s =========================

This is all I've done regarding compatibility, I haven't had time to read the cq-kit code and check manually.

@marcus7070
Copy link
Member Author

@marcus7070 do you want to add the CenterNthSelector to the string syntax? Maybe like so: >>X[0]?

Will do, I'll try to get to this tomorrow.

@adam-urbanczyk
Copy link
Member

Thanks for checking @marcus7070 . @michaelgale do you have anything against this PR?

+ refactored CenterNthSelector to match DirectionNthSelector
@adam-urbanczyk
Copy link
Member

OK I added the implementation and tests for >> and <<. NB: >>X[-1] is the same as >>X and >X.

@michaelgale
Copy link

@adam-urbanczyk This PR looks fine to me! Since I do not make use of the indexed Nth selectors, the cq-kit selectors are not affected. I've added an import for defaultdict to clear the errors noted above.

@michaelgale
Copy link

@adam-urbanczyk Incidentally, I noticed cq-kit now fails many CI tests. It looks like the new CQ2.x that gets pulled into the test environment breaks a lot of the my direct access into OCP. e.g.:

>           writer.Transfer(self.shape.val().wrapped, STEPControl_AsIs)
1050E           TypeError: in method 'STEPControl_Writer_Transfer', argument 2 of type 'TopoDS_Shape const &'
1051
1052../../../../miniconda/envs/cadquery/lib/python3.7/site-packages/cqkit-0.2.0-py3.7.egg/cqkit/cq_files.py:324: TypeError

They are almost all type errors--is it the const reference?

@adam-urbanczyk
Copy link
Member

I cannot reproduce it locally @michaelgale , maybe you can open a new issue with more detailed info?

@adam-urbanczyk
Copy link
Member

I added a #559 fix to this PR

@michaelgale
Copy link

@adam-urbanczyk The CI issues with CQ-Kit are resolved--no more errors. My travis environment was outdated and was pulling in pythonocc-core and oce, rather than the latest OCP based OCCT + cadquery.

@adam-urbanczyk
Copy link
Member

@jmwright are you going to take a look? I think it can be merged now.

@jmwright
Copy link
Member

jmwright commented Jan 4, 2021

I'll try to take a look here in a couple of hours and then merge.

@marcus7070
Copy link
Member Author

OK I added the implementation and tests for >> and <<. NB: >>X[-1] is the same as >>X and >X.

Thanks for taking care of that @adam-urbanczyk. I have never used pyparsing before and I wasn't looking forward to learning it.

I've just got one line in the docs that needs fixing.

Warning changed to reflect the new >> string selector.

Changed note about some types of filters not applying to edges, since
now every selector works with edges.
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.

Just a couple of minor comments. Thanks for all the work you've put into this @marcus7070 !

cadquery/selectors.py Outdated Show resolved Hide resolved
doc/selectors.rst Outdated Show resolved Hide resolved
@jmwright
Copy link
Member

jmwright commented Jan 5, 2021

Doc strings fixed, merging. Thanks @marcus7070 !

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.

Cannot select a hole in a chamfered object Mixin NthSelector
4 participants