Skip to content
This repository has been archived by the owner on Feb 9, 2021. It is now read-only.

Add logic to the close command #255

Closed
wants to merge 11 commits into from
Closed

Add logic to the close command #255

wants to merge 11 commits into from

Conversation

gntech
Copy link
Contributor

@gntech gntech commented Apr 25, 2018

If start and end point of a set of 2d edges coincide; create a wire directly.
If there is a distance between start and end point; add a line segment before creating the wire.

I propose this should close #238

Since both radiusArc and sagittaArc boils down to a threePointArc in the end I want to be able to just return a threePointArc.
This increases code reuse. However, the function toWorldCoords seems to do something strange to a vector that already is in World coordinates.
This commit checks if the point already is an Vector before trying to make it an vector.

I also refined the testcases by checking edge length and edge type.
If start and end point of a set of 2d edges coincide; create a wire directly.
If there is a distance between start and end point; add a line segment before creating the wire.
@coveralls
Copy link

coveralls commented Apr 25, 2018

Coverage Status

Coverage decreased (-0.05%) to 92.252% when pulling 52d67eb on gntech:improve-close into 86f3b7c on dcowden:master.

@AppVeyorBot
Copy link

Build cadquery 1.0.24 completed (commit 06a7a7d32f by @gntech)

@codecov-io
Copy link

Codecov Report

Merging #255 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #255      +/-   ##
==========================================
+ Coverage   92.85%   92.86%   +<.01%     
==========================================
  Files          10       10              
  Lines        2196     2199       +3     
==========================================
+ Hits         2039     2042       +3     
  Misses        157      157
Impacted Files Coverage Δ
cadquery/cq.py 94.49% <100%> (+0.02%) ⬆️

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 86f3b7c...d458dcb. Read the comment docs.

@gntech thanks for this contribution! You rock!
@gntech
Copy link
Contributor Author

gntech commented Apr 26, 2018

The testcase I added is creating the previously impossible geometry mentioned in #238

@dcowden
Copy link
Owner

dcowden commented Apr 26, 2018

@gntech you are on 🔥!
This looks great. +1 as far as I'm concerned.

@fragmuffin does this satisfy you as far as #238 goes? If so I'll merge

@fragmuffin
Copy link
Contributor

Nice work @gntech !

The Good: This does satisfy the linear cases outlined in #238
The Bad: However, it fails when joining 2 arcs.

I've added a 3rd error case to #238 to demonstrate.

@gntech
Copy link
Contributor Author

gntech commented Apr 26, 2018

@fragmuffin Nice catch! After some investigation I found that the problem was triggered by that the origin for workplane was moved.

If I set origin(0, 0, 0) in your example it works. Any other origin yields the same error as you describe.

The solution was that when getting endPoint in close() ; use local coordinates and not global coordinates.

I have also added testcase 3 to test suite

When getting the endPoint in close() ; use local coordinates and not global coordinates.

It does matter when the origin of the workplane is moved.
@fragmuffin
Copy link
Contributor

@gntech Nice fix!

I can confirm, that's working beautifully

If start and end point of a set of 2d edges coincide; create a wire directly.
If there is a distance between start and end point; add a line segment before creating the wire.
When getting the endPoint in close() ; use local coordinates and not global coordinates.

It does matter when the origin of the workplane is moved.
Add more elaborate testcases with workplane origin moved and comparing
the result of volumes created by threePointArc and sagittaArc.
@AppVeyorBot
Copy link

Build cadquery 1.0.29 failed (commit ba9ad634ff by @gntech)

@gntech
Copy link
Contributor Author

gntech commented Apr 27, 2018

When rebasing on master I discovered a problem with global coordinates that introduced in radiusArc that is fixed now as well as adding more elaborate test cases for the close()

I however managed to create redundant commits when rebasing on master and pushing. Is that okay or should I squash all commits in the pull request?

@gntech
Copy link
Contributor Author

gntech commented Apr 27, 2018

I will clean up the commit history for this pull request later today.

@gntech
Copy link
Contributor Author

gntech commented Apr 27, 2018

I will open a new pull request with cleaner history

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2d planar wire extrusion on coincident start & finish
6 participants