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 - second version #257

Merged
merged 1 commit into from
Apr 29, 2018
Merged

Add logic to the close() command - second version #257

merged 1 commit into from
Apr 29, 2018

Conversation

gntech
Copy link
Contributor

@gntech gntech commented Apr 27, 2018

This is a squashed version of pull request #255

commit b281a4c
Author: Gustav Näslund gustav@gntech.se
Date: Fri Apr 27 07:17:39 2018 +0200

Fix converting to global coords in threePointArc

Add more elaborate testcases with workplane origin moved and comparing
the result of volumes created by threePointArc and sagittaArc.

commit 679eb21
Author: Gustav Näslund gustav@gntech.se
Date: Thu Apr 26 22:21:06 2018 +0200

Use local coordinates and not global coordinates in close()

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

It does matter when the origin of the workplane is moved.

commit d458dcb
Author: Gustav Näslund gustav@gntech.se
Date: Wed Apr 25 17:47:02 2018 +0200

Add logic to the close command

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.

@AppVeyorBot
Copy link

Build cadquery 1.0.30 failed (commit a4d2e65b67 by @gntech)

@coveralls
Copy link

coveralls commented Apr 27, 2018

Coverage Status

Coverage increased (+0.05%) to 92.252% when pulling e23ccdc on gntech:improve-close-2 into 7a9317a on dcowden:master.

@gntech
Copy link
Contributor Author

gntech commented Apr 28, 2018

@dcowden and @fragmuffin this pull request is ready for review or merge. It is the same code as in the previous pull request but all in a single commit.

@dcowden
Copy link
Owner

dcowden commented Apr 28, 2018

@gntech this looks ok to me, but we need to get the builds passing before we can merge.

The python3 build is failing and the appveyor (windows) build is as well

@fragmuffin
Copy link
Contributor

@gntech I ran the 3 tests from #238...

  1. Manually closing yields no solid
    yields the same fault, no solid is created
  2. Closing on coincidental point
    pass 👍
  3. Closing on coincidental point (arcs)
    pass 👍

But I guess this raises the question:

should the close() call be mandatory if you're expecting to extrude a solid?
(and the same for a loft, revolve, sweep, and so on)

I'm thinking it probably should be, in which case test 1 is irrelevant.
What do you think?

@gntech
Copy link
Contributor Author

gntech commented Apr 28, 2018

@fragmuffin Good question, I agree that it should be mandatory to call the close() function before extruding.

As a comparison; In other CAD software it is possible to extrude a non-closed profile and then do what is called a "thin extrude". I do not know it this is even possible with opencascade but in order to allow for it we should not auto close wires.

@fragmuffin
Copy link
Contributor

Excellent @gntech , in which case I think #238 can be closed when this PR is merged.
Nice work 👍

@dcowden
Copy link
Owner

dcowden commented Apr 29, 2018

It appears this is ready to go, once Adam gets the miniconda issue sorted out. That should fix the appveyor and python 3 builds

@adam-urbanczyk
Copy link
Contributor

@dcowden please merge #259 first, it solves the conda issue.

@AppVeyorBot
Copy link

Build cadquery 1.0.33 completed (commit 9ad0295e1c by @gntech)

@jmwright
Copy link
Collaborator

@gntech Could you please pull master into this branch? AppVeyor is fixed, and hopefully a pull will fix the Travis build.

commit b281a4c
Author: Gustav Näslund <gustav@gntech.se>
Date:   Fri Apr 27 07:17:39 2018 +0200

    Fix converting to global coords in threePointArc

    Add more elaborate testcases with workplane origin moved and comparing
    the result of volumes created by threePointArc and sagittaArc.

commit 679eb21
Author: Gustav Näslund <gustav@gntech.se>
Date:   Thu Apr 26 22:21:06 2018 +0200

    Use local coordinates and not global coordinates in close()

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

    It does matter when the origin of the workplane is moved.

commit d458dcb
Author: Gustav Näslund <gustav@gntech.se>
Date:   Wed Apr 25 17:47:02 2018 +0200

    Add logic to the close command

    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.
@gntech
Copy link
Contributor Author

gntech commented Apr 29, 2018

@jmwright Ok, now I have rebased this branch on latest master

@AppVeyorBot
Copy link

Build cadquery 1.0.34 completed (commit 81353ee9e3 by @gntech)

@codecov-io
Copy link

Codecov Report

Merging #257 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   92.74%   92.79%   +0.04%     
==========================================
  Files          10       10              
  Lines        2206     2207       +1     
==========================================
+ Hits         2046     2048       +2     
+ Misses        160      159       -1
Impacted Files Coverage Δ
cadquery/cq.py 94.27% <100%> (+0.14%) ⬆️

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 7a9317a...e23ccdc. Read the comment docs.

@jmwright
Copy link
Collaborator

@gntech Thanks for all your work on this contribution, and thanks to everyone else who participated.

@jmwright jmwright merged commit 491ec9f into dcowden:master Apr 29, 2018
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.

None yet

8 participants