Skip to content

Implement locations() for Wires#475

Merged
adam-urbanczyk merged 17 commits intomasterfrom
locations-wire
Oct 30, 2020
Merged

Implement locations() for Wires#475
adam-urbanczyk merged 17 commits intomasterfrom
locations-wire

Conversation

@adam-urbanczyk
Copy link
Copy Markdown
Member

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

@trayracing
Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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
Copy Markdown
Member

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

@adam-urbanczyk
Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
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.

@marcus7070
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

+1 to merge

@adam-urbanczyk adam-urbanczyk merged commit 65f9608 into master Oct 30, 2020
@jmwright jmwright deleted the locations-wire branch October 28, 2025 20:14
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