-
Notifications
You must be signed in to change notification settings - Fork 300
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
Conversation
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.
Hey GitHub, I said this will close #531 in my commit message, pay attention! |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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. |
23c39f2
to
8871497
Compare
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]".
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 import cadquery as cq
part = (
cq
.Workplane()
.box(10, 10, 10)
.faces(">X")
.workplane()
.hole(1)
)
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. |
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. |
@marcus7070 do you want to add the |
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
This is all I've done regarding compatibility, I haven't had time to read the cq-kit code and check manually. |
Will do, I'll try to get to this tomorrow. |
Thanks for checking @marcus7070 . @michaelgale do you have anything against this PR? |
+ refactored CenterNthSelector to match DirectionNthSelector
OK I added the implementation and tests for |
@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. |
@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.:
They are almost all type errors--is it the |
I cannot reproduce it locally @michaelgale , maybe you can open a new issue with more detailed info? |
I added a #559 fix to this PR |
@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. |
@jmwright are you going to take a look? I think it can be merged now. |
I'll try to take a look here in a couple of hours and then merge. |
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.
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.
Just a couple of minor comments. Thanks for all the work you've put into this @marcus7070 !
Doc strings fixed, merging. Thanks @marcus7070 ! |
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.
After a242473:
While looking at the docs I noticed the tables were overflowing and it was hard to read.
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:
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.