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

Implement locations() for Wires #475

Merged
merged 17 commits into from
Oct 30, 2020
Merged

Implement locations() for Wires #475

merged 17 commits into from
Oct 30, 2020

Conversation

adam-urbanczyk
Copy link
Member

Implements locations() and locationAt() for both Wires and Edges.

@trayracing
Copy link

Giving locations on wires would be a great help on placing objects around the perimeter of a face. Two comments:

  1. There are three potentially interesting orientations to return for the points: tangential, locally perpendicular to the enclosed area, and perpendicular to the path, but locally in plane. (see cadquery google group for examples.)
  2. In the PR, the orientation is not consistent around the wire, and instead appears to be per edge. This makes it harder to use. [Example image] from cadquery group. (https://groups.google.com/group/cadquery/attach/2f33032517546/Auto%20Generated%20Inline%20Image%201?part=0.1&view=1&authuser=0)

@trayracing
Copy link

In the PR, the orientation is not consistent ...
As Adam pointed out in the google cadquery group, using the frame="corrected" parameter corrects this flipping. Apologies for the distraction.

@codecov
Copy link

codecov bot commented Oct 18, 2020

Codecov Report

Merging #475 into master will increase coverage by 0.59%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #475      +/-   ##
==========================================
+ Coverage   93.63%   94.22%   +0.59%     
==========================================
  Files          30       30              
  Lines        5859     7848    +1989     
  Branches      624      942     +318     
==========================================
+ Hits         5486     7395    +1909     
- Misses        234      289      +55     
- Partials      139      164      +25     
Impacted Files Coverage Δ
cadquery/occ_impl/shapes.py 91.67% <ø> (+0.78%) ⬆️
tests/test_cadquery.py 98.97% <ø> (-0.02%) ⬇️
cadquery/occ_impl/geom.py 89.50% <0.00%> (+0.56%) ⬆️
cadquery/cq.py 90.49% <0.00%> (+2.21%) ⬆️

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 7fd45e0...596ffab. Read the comment docs.

@marcus7070
Copy link
Member

positionAt looks great, I'm going to have a lot of use for that, thanks @adam-urbanczyk.

@adam-urbanczyk
Copy link
Member Author

@jmwright if everything passes I'd say it is ready for merging.

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.

@adam-urbanczyk Thanks for doing this. I just have a few minor comments.

cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved
cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved
tests/test_cadquery.py Show resolved Hide resolved
@marcus7070
Copy link
Member

Currently Mixin1DProtocol is not autosummarized in the HTML docs. It also looks like sphinx is set up to not autosummarize inherited methods. So while the methods of the Edge class appear in there, these new ones on Mixin1DProtocol do not.

Perhaps that's intentional, but I thought I'd point it out incase it's an oversight.

@adam-urbanczyk
Copy link
Member Author

Perhaps that's intentional, but I thought I'd point it out incase it's an oversight.

Good catch, it was not the intention. I'll add Mixin1D and other mixins to the docs.

@adam-urbanczyk
Copy link
Member Author

I tried to add Mixin1D and Mixin3D to the docs, but it does not show up (due to my poor sphinx-fu). Does anyone know how to do it properly?

@jmwright
Copy link
Member

@Peque Do you have any ideas on the Spinx part of this (see previous comment from @adam-urbanczyk )? You've done quite a bit of work with Sphinx.

@Peque
Copy link
Contributor

Peque commented Oct 30, 2020

@jmwright No idea, sorry. 😅 I may be able to have a look at it if you are still blocked in 1-2 weeks. Sounds like a good question for StackOverflow though. 😜

@adam-urbanczyk
Copy link
Member Author

It works now, but could be prettier. I propose to merge and maybe investigate autodoc alternatives in the future. @jmwright would you agree?

@jmwright
Copy link
Member

+1 to merge

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.

5 participants