Add STEP Import for Assemblies #1779
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1779 +/- ##
==========================================
+ Coverage 95.66% 95.98% +0.31%
==========================================
Files 28 30 +2
Lines 7431 9233 +1802
Branches 1122 1499 +377
==========================================
+ Hits 7109 8862 +1753
- Misses 193 228 +35
- Partials 129 143 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@adam-urbanczyk In 4045c58 you set the We need to decide on the proper way to fix this. |
|
Hey, I do not understand the issue. You can always do this: Do you mean that you need to modify |
|
Correct, mypy complains. I can make that change to make |
…king through indirect lookup
|
I am still getting a ValueError with a STEP from the tests. |
|
@jmwright any updates on this PR? |
|
@adam-urbanczyk Not yet. I got busy with some other things. |
|
@adam-urbanczyk I'm getting mypy errors now that I did not before. Most of them are probably my fault, but there is this one about the OpenCASCADE code. Does this have anything to do with the stubs being published as part of OCP 7.9.x? |
|
Nothing should be published, but let me check |
|
I'm having trouble with mypy. It is giving me different errors locally than I get in CI even though I have the same version. I'll create a new environment from scratch tomorrow and try to match the CI environment to see if I can get rid of the discrepancies. |
|
@adam-urbanczyk @lorenzncode Please have another look when you get a chance. This PR has been significantly reworked, and a test or two added. |
|
Something is off with top level location handling. Before importing: After importing: Code: #%% imports
from cadquery import Assembly, Location, Color, importers
from cadquery.func import box, rect, sphere
from cadquery.vis import show
#%% prepare the model
def make_model(name: str, COPY: bool):
b = box(1,1,1)
assy_top = Assembly(name='test_assy_top', loc=Location((0,1,1)))
assy_top.add(sphere(1))
assy = Assembly(name='test_assy', loc=Location((5,5,1)))
assy.add(box(1,2,5), color=Color('green'))
for v in rect(10,10).vertices():
assy.add(
b.copy() if COPY else b,
loc=Location(v.Center()),
color=Color('red')
)
# show(assy)
assy_top.add(assy)
assy_top.export(name)
return assy_top
assy = make_model("test_assy.step", False)
show(assy, Location(), scale=5)
#%% import the assy without copies - this throws
assy_i = Assembly.importStep("test_assy.step")
wp = importers.importStep("test_assy.step")
show(assy_i, Location(),scale=5) |
|
@adam-urbanczyk Thanks, I fixed that bug and modified one of the tests so that I could add an assert to verify. |
There was a problem hiding this comment.
Thanks for the big effort @jmwright! The error I had before is resolved now when I try importing from the pytest generated step file.
I find import/export round trip changes the assembly as follows. Is this expected?
from cadquery import Assembly, Location
from cadquery.func import box
def round_trip_test(assy, n=3):
for i in range(n):
print(f"\n{i=}")
for name in assy._flatten().keys():
print(name)
assy.export(f"out{i}.step")
assy = Assembly.importStep(f"out{i}.step")
b = box(1, 1, 1)
assy = Assembly(name="assy")
assy.add(b, name="box0")
assy.add(b, name="box1", loc=Location((1, 0, 0)))
round_trip_test(assy, 5)i=0
assy/box0
assy/box1
assy
i=1
assy/box0/box0_part
assy/box0
assy/box1/box0_part
assy/box1
assy
i=2
assy/box0/box0_part/box0_part_part
assy/box0/box0_part
assy/box0
assy/box1/box0_part/box0_part_part
assy/box1/box0_part
assy/box1
assy
i=3
assy/box0/box0_part/box0_part_part/box0_part_part_part
assy/box0/box0_part/box0_part_part
assy/box0/box0_part
assy/box0
assy/box1/box0_part/box0_part_part/box0_part_part_part
assy/box1/box0_part/box0_part_part
assy/box1/box0_part
assy/box1
assy
i=4
assy/box0/box0_part/box0_part_part/box0_part_part_part/box0_part_part_part_part
assy/box0/box0_part/box0_part_part/box0_part_part_part
assy/box0/box0_part/box0_part_part
assy/box0/box0_part
assy/box0
assy/box1/box0_part/box0_part_part/box0_part_part_part/box0_part_part_part_part
assy/box1/box0_part/box0_part_part/box0_part_part_part
assy/box1/box0_part/box0_part_part
assy/box1/box0_part
assy/box1
assy
| assy = subshape_assy() | ||
|
|
||
| # Use a temporary directory | ||
| tmpdir = tmp_path_factory.mktemp("out") |
There was a problem hiding this comment.
FYI, you could use the tmpdir fixture (line 42) to reduce boilerplate.
| tmpdir = tmp_path_factory.mktemp("out") | ||
| plain_step_path = os.path.join(tmpdir, "plain_assembly_step.step") | ||
|
|
||
| # Simple cubes |
There was a problem hiding this comment.
There are existing assembly fixtures that could be used here (and for other tests) instead. OK if you would rather explicitly redefine here.
That is definitely not OK. |
| else: | ||
| cq_color = None | ||
|
|
||
| new_assy.add( |
There was a problem hiding this comment.
So probably some logic should be added here to handle the name / name_part pairs. E.g. something like:
if ref_name.endswith('_part') and ref_name.startswith(parent_name):
new_assy[parent_name].obj = cq_shape
else:
...
CC @lorenzncode
There was a problem hiding this comment.
@lorenzncode @jmwright I added this draft PR #1884, it needs some work still.
- Added logic to handle
_partshapes - Fixed
_copy()to include metadata
NB: I think that at least one test is actually wrong. Subshapes should be a property of the (sub)assy that owns the shape. Not a top level one.
| assert assy.children[0].name == "cube_1" | ||
| assert assy.children[1].children[0].name == "cylinder_1" | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
Probably a test that asserts idempotence of export/import is also needed.
|
@adam-urbanczyk I've pulled in your changes from #1884 and have things working except for a little bit of code coverage. You had a |
The goal is to make it possible to round-trip assemblies to and from STEP without loss of data. This data can include: