-
Notifications
You must be signed in to change notification settings - Fork 359
Add STEP Import for Assemblies #1779
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1779 +/- ##
==========================================
+ Coverage 95.66% 95.84% +0.17%
==========================================
Files 28 30 +2
Lines 7431 7865 +434
Branches 1122 1220 +98
==========================================
+ Hits 7109 7538 +429
+ Misses 193 192 -1
- Partials 129 135 +6 ☔ 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, 2nd pass
|
||
# Process the color for the shape, which could be of different types | ||
color = Quantity_Color() | ||
cq_color = cq.Color(0.50, 0.50, 0.50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the magic parameters?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are the RGB values that most of the CQ visualization methods seem to be using as a default with assemblies (gray/silver). The values are not standardized as cadquery.vis.show
seems to use a gold color IIRC. CQ-editor uses the gray color though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, default color should be None
. See:
class Assembly(object):
"""Nested assembly of Workplane and Shape objects defining their relative positions."""
loc: Location
name: str
color: Optional[Color]
metadata: Dict[str, Any]
obj: AssemblyObjects
parent: Optional["Assembly"]
children: List["Assembly"]
objects: Dict[str, "Assembly"]
constraints: List[Constraint]
loc=cq.Location(parent_location), | ||
) | ||
else: | ||
assy.add(cq.Shape.cast(shape), name=name, color=cq_color) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this covered. Is parent location not optional?
# Pass down the location or other context as needed | ||
# Add the parent location if it exists | ||
if parent_location is not None: | ||
loc = parent_location * loc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this suspicious. Assy applies locations in a relative sense. It shouldn't be needed to construct absolute locations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to check, but I think I added this because of nested sub-assemblies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed. If the parent location is not passed through the call stack, nested subassemblies will break. I added the test_nested_subassembly_step_import
test to illustrate this. If you disable parent location passing and run that test, it should fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, but that is actually suspicious. When you construct an assy you do not need combine (i.e. multiply) the locations explicitly.
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. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a test that asserts idempotence of export/import is also needed.
The goal is to make it possible to round-trip assemblies to and from STEP without loss of data. This data can include: