Update assembledEdges in shapes.py #237

bragostin · 2019-12-13T07:30:19Z

BRepBuilderAPI_MakeWire::Add(const TopTools_ListOfShape & L) offers the option to accept a list of shapes directly as argument:
Adds the edges of <L> to the current wire. The edges are not to be consecutive. But they are to be all connected geometrically or topologically. If some of them are not connected the Status give DisconnectedWire but the "Maker" is Done() and you can get the partial result. (ie connected to the first edgeof the list <L>)
Following this I modified the assembledEdges in shapes.py to be able to provide a list of unordered edges to BRepBuilderAPI_MakeWire. This way, when the list of edges is generated by another function, there is not need to make them consecutive.

BRepBuilderAPI_MakeWire::Add(const TopTools_ListOfShape & L) offers the option to accept a list of shapes directly as argument: "Adds the edges of <L> to the current wire. The edges are not to be consecutive. But they are to be all connected geometrically or topologically. If some of them are not connected the Status give DisconnectedWire but the "Maker" is Done() and you can get the partial result. (ie connected to the first edgeof the list <L>)" Following this I modified the assembledEdges in shapes.py to be able to provide a list of unordered edges to BRepBuilderAPI_MakeWire. This way, when the list of edges is generated by another function, there is not need to make them consecutive.

Update assembledEdges in shapes.py

codecov · 2019-12-13T07:36:15Z

Codecov Report

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

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   94.86%   94.91%   +0.04%     
==========================================
  Files          18       18              
  Lines        4190     4226      +36     
==========================================
+ Hits         3975     4011      +36     
  Misses        215      215

Impacted Files	Coverage Δ
tests/__init__.py	`93.75% <ø> (ø)`	⬆️
cadquery/occ_impl/shapes.py	`89.6% <100%> (+0.13%)`	⬆️
tests/test_cadquery.py	`99.01% <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 e69b2f8...42c35b7. Read the comment docs.

jmwright · 2019-12-13T11:53:36Z

@bragostin @adam-urbanczyk What would you guys think about printing a warning when the status is DisconnectedWire? Is it practical to do that?

If some of them are not connected the Status give DisconnectedWire but the "Maker" is Done()

Users often have trouble when they're using a set of points and a joint isn't connected or the wire isn't closed. It would be nice to have some indication that they need to look at their points again unless they meant for things to be disconnected.

jmwright · 2019-12-13T16:28:45Z

I'm not saying it needs to happen in this PR, but looking ahead, implementing warnings like that would be nice for users.

jmwright · 2019-12-13T16:29:00Z

I'm not saying it needs to happen in this PR, but looking ahead, implementing warnings like that would be nice for users.

bragostin · 2019-12-13T18:16:43Z

@jmwright @adam-urbanczyk would a simple print('BRepBuilderAPI_MakeWire() Status = ', wire_builder.IsDone(), wire_builder.Error()) be enough?
As I understand the documentation, if the wires are not connected it will not throw an exception but return a partial result.

jmwright · 2019-12-13T22:07:58Z

@bragostin That sounds good to me. It's just something to keep the user from having to guess what's going on if things don't work as expected.

adam-urbanczyk · 2019-12-14T08:32:31Z

@bragostin looks good but I'd propose to use warnings.warn i.s.o. print

bragostin · 2019-12-14T15:51:22Z

@jmwright @adam-urbanczyk I am just discovering the module warnings.warn. Would something like

if not wire_builder.IsDone():
    w1 = 'BRepBuilderAPI_MakeWire::IsDone(): returns the construction status. BRepBuilderAPI_WireDone if the wire is built, or another value of the BRepBuilderAPI_WireError enumeration indicating why the construction failed = '+ str(wire_builder.IsDone())
    w2 = 'BRepBuilderAPI_MakeWire::Error(): returns true if this algorithm contains a valid wire. IsDone returns false if: there are no edges in the wire, or the last edge which you tried to add was not connectable = ' + str(wire_builder.Error())
    warnings.warn(w1)
    warnings.warn(w2)

be ok?

jmwright · 2019-12-14T20:04:54Z

@bragostin I think that looks fine.

bragostin · 2019-12-15T08:49:45Z

I am not familiar with using github yet and I am obviously doing something wrong here, any idea what it is?

adam-urbanczyk · 2019-12-15T10:05:52Z

What do you mean @bragostin ?

bragostin · 2019-12-15T10:43:53Z

Well, 2 checks were not successful:
FAILED: codecov/patch — 60% of diff hit (target 94.86%)
FAILED: codecov/project — 94.76% (-0.11%) compared to acf363d
PASSED: continuous-integration/appveyor/pr — AppVeyor build succeeded
PASSED: continuous-integration/travis-ci/pr — The Travis CI build passed

adam-urbanczyk · 2019-12-15T14:54:57Z

Yes, indeed: codecov is complaining about test coverage decrease. If you feel like you could add a test that triggers the newly implemented warning.

bragostin · 2019-12-15T17:47:39Z

@adam-urbanczyk I just did add a test that triggers the warning and the 4 checks are ok now.

bragostin · 2019-12-16T07:50:35Z

I have improved the test TestAssembleEdgesWarnings.py so that it returns only a warning and no error.

jmwright · 2019-12-18T13:26:00Z

@adam-urbanczyk Do you want me to go ahead and merge this?

bragostin · 2019-12-21T15:34:47Z

Sorry I did a mess, I forgot to create a new branch for BRepOffsetAPI_MakeFilling, I put things back in order.

cadquery/occ_impl/shapes.py

jmwright · 2019-12-23T21:37:48Z

@bragostin The reason you're getting a conflict is because runtest.py was deleted recently as part of the upgrade to pytest. #236

update

bragostin · 2019-12-24T12:03:20Z

@jmwright In the test_cadquery.py file I have added a test_assembleEdges function with 4 tests including consecutive and non consecutive edges. All checks have passed.

jmwright · 2019-12-24T12:59:10Z

Thanks. Sorry you didn't get a heads-up about the test changes. Changes have been happening quickly and it's hard to keep track.

cadquery/occ_impl/shapes.py

adam-urbanczyk · 2019-12-24T13:49:06Z

@bragostin I think it looks good. Just take a look a the single comment I had.

jmwright · 2019-12-24T19:38:00Z

@bragostin It looks like you addressed the comment from @adam-urbanczyk . Do you feel this PR is ready to merge? I looked it over again and I can't think of anything else that we're missing.

bragostin · 2019-12-25T08:37:42Z

@jmwright I have been using this new assembleEdges function for 2 weeks now because it is a pre-requisite for the BRepOffsetAPI_MakeFilling implementation and I have not noticed any issue so far, and older models are still running correctly. So from my side it looks ok.

jmwright · 2019-12-25T12:16:59Z

@bragostin Thanks for all your work on this. Looks like it's time to merge it.

When I get a chance I'll take a look at some of the other edge assembly related issues here in GitHub to see if this fixes them.

adam-urbanczyk · 2019-12-25T14:53:49Z

Thanks for the contribution @bragostin !

bragostin added 2 commits December 13, 2019 08:22

Merge pull request #1 from bragostin/bragostin-assembledEdges

3b210fe

Update assembledEdges in shapes.py

bragostin added 3 commits December 15, 2019 09:12

Update shapes.py

46c2375

Update shapes.py

23f2ba6

Update shapes.py

b19b8ff

bragostin added 2 commits December 15, 2019 18:30

Add files via upload

a7ca48e

Update shapes.py

fd1e959

Update TestAssembleEdgesWarnings.py

80b93da

bragostin added 4 commits December 21, 2019 16:29

Add files via upload

fc4e921

Add files via upload

5765e24

Delete TestInterpPlate.py

ef49b6f

Delete Ex101_InterpPlate.py

d9893eb

jmwright reviewed Dec 23, 2019

View reviewed changes

cadquery/occ_impl/shapes.py Show resolved Hide resolved

bragostin added 4 commits December 23, 2019 15:18

Update TestAssembleEdges.py

9b2c8da

Update TestAssembleEdges.py

57c4c99

Add files via upload

84775d4

Add files via upload

b0c9f1b

bragostin added 12 commits December 24, 2019 10:45

Delete runtests.py

e64ac5d

Merge pull request #2 from CadQuery/master

a1df7a4

update

Update test_cadquery.py

10a7299

Delete TestAssembleEdges.py

cdb1658

Update test_cadquery.py

38c3d27

Update test_cadquery.py

be3ad42

Update test_cadquery.py

81b3f57

Update test_cadquery.py

707dac5

Update test_cadquery.py

3ab5b51

Update test_cadquery.py

a26766a

Update test_cadquery.py

428a8d5

Update test_cadquery.py

d2c029c

adam-urbanczyk reviewed Dec 24, 2019

View reviewed changes

cadquery/occ_impl/shapes.py Outdated Show resolved Hide resolved

bragostin added 2 commits December 24, 2019 16:31

Update shapes.py

6196729

Add files via upload

1d7b11b

Update test_cadquery.py

42c35b7

jmwright approved these changes Dec 25, 2019

View reviewed changes

jmwright merged commit 3cd466a into CadQuery:master Dec 25, 2019

Update assembledEdges in shapes.py #237

Update assembledEdges in shapes.py #237

Uh oh!

Conversation

bragostin commented Dec 13, 2019

Uh oh!

codecov bot commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jmwright commented Dec 13, 2019

Uh oh!

jmwright commented Dec 13, 2019

Uh oh!

jmwright commented Dec 13, 2019

Uh oh!

bragostin commented Dec 13, 2019

Uh oh!

jmwright commented Dec 13, 2019

Uh oh!

adam-urbanczyk commented Dec 14, 2019

Uh oh!

bragostin commented Dec 14, 2019

Uh oh!

jmwright commented Dec 14, 2019

Uh oh!

bragostin commented Dec 15, 2019

Uh oh!

adam-urbanczyk commented Dec 15, 2019

Uh oh!

bragostin commented Dec 15, 2019

Uh oh!

adam-urbanczyk commented Dec 15, 2019

Uh oh!

bragostin commented Dec 15, 2019

Uh oh!

bragostin commented Dec 16, 2019

Uh oh!

jmwright commented Dec 18, 2019

Uh oh!

bragostin commented Dec 21, 2019

Uh oh!

Uh oh!

jmwright commented Dec 23, 2019

Uh oh!

bragostin commented Dec 24, 2019

Uh oh!

jmwright commented Dec 24, 2019

Uh oh!

Uh oh!

adam-urbanczyk commented Dec 24, 2019

Uh oh!

jmwright commented Dec 24, 2019

Uh oh!

bragostin commented Dec 25, 2019

Uh oh!

jmwright commented Dec 25, 2019

Uh oh!

adam-urbanczyk commented Dec 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Dec 13, 2019 •

edited

Loading