Assembly export: do not add leaf component when shapes is empty (#993)

[lorenzncode]

No description provided.

[codecov]

Codecov Report

Merging #1157 (040bcfc) into master (4568e45) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1157      +/-   ##
==========================================
+ Coverage   94.07%   94.09%   +0.01%     
==========================================
  Files          26       26              
  Lines        5421     5432      +11     
  Branches      919      919              
==========================================
+ Hits         5100     5111      +11     
  Misses        190      190              
  Partials      131      131              

Impacted Files	Coverage Δ
cadquery/occ_impl/assembly.py	98.60% <100.00%> (+0.15%)	⬆️
cadquery/occ_impl/exporters/assembly.py	100.00% <100.00%> (ø)
cadquery/occ_impl/shapes.py	92.65% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jmwright]

@lorenzncode When this is ready to test, please let me know. I can try running it against the KiCAD 3D generators. There's discussion on that GitLab repo on how nesting is not working correctly in assembly export and leads to messy file structures, especially in VRML.

[lorenzncode]

@jmwright Initially this PR attempted to resolve the #993 GLTF export issue. #993 is now already resolved with the OCCT/OCP 7.6 update. I think there remains an issue with empty leaf node in the STEP export so I've opened a new issue #1170. This PR is ready to review if there is agreement on #1170.

Regarding VRML export, there may be a separate issue because this change is for the export formats that call toCAF. toVTK already skips children with empty .shapes.

[adam-urbanczyk]

I think I'm -1 on this (see comment in #1170).

Maybe SetShape call can be conditional?

On a second thought: maybe what you are proposing is better. Let me sleep on this.

[adam-urbanczyk]

@lorenzncode do colors work for you when importing into FreeCAD (question not related strictly to this PR)?

I think that part of the color handling logic needs to be moved outside of the if v.shapes otherwise I think looks fine.

[lorenzncode]

@adam-urbanczyk No, colors are not working for me when importing into FreeCAD. Here using master:

a = cq.Workplane().box(1, 1, 1)
b = cq.Workplane().box(1, 1, 1)

assy = cq.Assembly()
assy.add(a, name="box0", color=cq.Color("red"))
assy.add(a, name="box1", color=cq.Color("green"), loc=cq.Location((2, 0, 0)))
assy.add(a, name="box2", color=cq.Color("blue"), loc=cq.Location((4, 0, 0)))
assy.add(b, name="box3", color=cq.Color("orange"), loc=cq.Location((6, 0, 0)))
assy.add(b, name="box4", color=cq.Color("black"), loc=cq.Location((8, 0, 0)))
assy.add(b, name="box5", color=cq.Color("yellow"), loc=cq.Location((10, 0, 0)))

I will take a look and try to debug.

[adam-urbanczyk]

Di you manage to find something @lorenzncode ? Worst case we could have two versions of the function.

[lorenzncode]

@adam-urbanczyk Yes, I think I've made some progress with the STEP color issue!

The case I appended before where a different color is applied to each box instance is working for me now.

Here is another case where color is not working in STEP.

r_wheel = 25
w_wheel = 10
l_axle = 80
l_chassis = 100

wheel_shape = (
    cq.Workplane("YZ").circle(r_wheel).extrude(w_wheel, both=True)
)
axle_shape = cq.Workplane("YZ").circle(r_wheel / 10).extrude(l_axle / 2, both=True)

wheel_axle = cq.Assembly(name="wheel-axle")
wheel_axle.add(
    wheel_shape,
    name="wheel-left",
    color=cq.Color("red"),
    loc=cq.Location((-l_axle / 2 - w_wheel, 0, 0)),
)
wheel_axle.add(
    wheel_shape,
    name="wheel-right",
    color=cq.Color("red"),
    loc=cq.Location((l_axle / 2 + w_wheel, 0, 0)),
)
wheel_axle.add(axle_shape, name="axle", color=cq.Color("green"))

chassis = cq.Assembly(name="chassis")
chassis.add(wheel_axle, name="wheel-axle-front", loc=cq.Location((0, l_chassis / 2, 0)))
chassis.add(wheel_axle, name="wheel-axle-rear", loc=cq.Location((0, -l_chassis / 2, 0)))

STEP exported with CQ master, imported in FreeCAD:

STEP exported with local changes, imported in FreeCAD:

I'll try to push an update soon after more testing.

[lorenzncode]

I'd like to investigate whether a similar makeCompound logic would make sense applied to the other export formats. Perhaps this logic could be moved out of toCAF eventually (with follow up in a separate issue). For now I'd propose to keep the changes isolated to toCAF to fix the color issue. Please make/suggest any other changes that may be required.

[jmwright]

Thanks @lorenzncode !

[jmwright]

@adam-urbanczyk This PR and #1210 are ready to merge from my perspective.

[adam-urbanczyk]

I'd like to still review.

[adam-urbanczyk]

@lorenzncode thanks for the hard work! AFAICT you made the toCAF function significantly more convoluted. To my understanding the root cause of the color issue was not making copies of the underlying shape objects. Would something like this be enough?

def toCAF(
    assy: AssemblyProtocol, coloredSTEP: bool = False
) -> Tuple[TDF_Label, TDocStd_Document]:

    # prepare a doc
    app = XCAFApp_Application.GetApplication_s()

    doc = TDocStd_Document(TCollection_ExtendedString("XmlOcaf"))
    app.InitDocument(doc)

    tool = XCAFDoc_DocumentTool.ShapeTool_s(doc.Main())
    tool.SetAutoNaming_s(False)
    ctool = XCAFDoc_DocumentTool.ColorTool_s(doc.Main())

    # add root
    top = tool.NewShape()
    TDataStd_Name.Set_s(top, TCollection_ExtendedString("CQ assembly"))

    def _toCAF(el, ancestor, color):

        # create a subassy
        subassy = tool.NewShape()
        setName(subassy, el.name, tool)

        # add a leaf with the actual part if needed
        if el.obj:
            lab = tool.NewShape()
            tool.SetShape(lab, Compound.makeCompound(el.shapes).copy().wrapped)

            setName(lab, f"{el.name}_part", tool)

            tool.AddComponent(subassy, lab, TopLoc_Location())

            # handle colors when exporting to STEP
            if coloredSTEP:
                if el.color:
                    setColor(lab, el.color, ctool)
                elif color:
                    setColor(lab, color, ctool)

        # handle colors when *not* exporting to STEP
        if not coloredSTEP:
            if el.color:
                setColor(subassy, el.color, ctool)
            elif color:
                setColor(subassy, color, ctool)

        # add children recursively
        for child in el.children:
            _toCAF(child, subassy, el.color if el.color else color)

        # add the current subassy to the higher level assy
        tool.AddComponent(ancestor, subassy, el.loc.wrapped)

    # process the whole assy recursively
    _toCAF(assy, top, None)

    tool.UpdateAssemblies()

    return top, doc

[lorenzncode]

@adam-urbanczyk Short answer: Yes, the simplified version is enough to fix the STEP color handling issue so that colors are displayed correctly.

Long answer: The simplified version is not equivalent to the convoluted PR implementation in terms of the resulting XDE/OCAF structure and STEP output.

In my opinion, the end result of the PR implementation is better for the following reasons:

1. The document structure is closer to the ideal from a "Bill of Materials" perspective.

   For example, see the chassis0_assy example from the tests.

   STEP export with PR implementation results in (considering only the wheels):
   4 x wheel_part

   STEP export with the simplified version results in:
   2 x wheel:left_part
   2 x wheel:right_part

   The simplified version contains additional referred shapes compared to the PR version.

   Since all wheels share the same shape I think the PR version is closer to the expected result.
   

   This also has implications for performance and STEP file size.

2. Due to the differences mentioned in (1.), the resulting STEP file size also varies depending on implementation.
   (see following example with 10K box instances)

3. The two implementations may also differ in performance. (see following example)

10K boxes example

import cadquery as cq
import timeit

box = cq.Workplane().box(1, 1, 1)

assy = cq.Assembly()
num = 100
for i in range(1, num+1, 1):
    for j in range(1, num+1, 1):
        assy.add(
            box,
            name=f"box{i}_{j}",
            color=cq.Color("red"),
            loc=cq.Location((1.1 * i, 1.1 * j, 0)),
        )

print(
    timeit.timeit(
        'assy.save("test.step")', globals=globals(), number=1
    )
)

Implementation	STEP File Size	STEP Export Time
current PR	23M	~5s
Simple	197M	~32s

Thanks for providing the simplified version for comparison. Do you think any of these points are important? If not, or they are out of scope for the color fix then perhaps the simpler implementation is good enough for now. The pytests can still be reused to validate the new color handling.

[adam-urbanczyk]

Thanks, motivation is clear now. If I understand your intent correctly, the following code will do the same. Would you be willing to refactor?

def toCAF(
    assy: AssemblyProtocol, coloredSTEP: bool = False
) -> Tuple[TDF_Label, TDocStd_Document]:

    # prepare a doc
    app = XCAFApp_Application.GetApplication_s()

    doc = TDocStd_Document(TCollection_ExtendedString("XmlOcaf"))
    app.InitDocument(doc)

    tool = XCAFDoc_DocumentTool.ShapeTool_s(doc.Main())
    tool.SetAutoNaming_s(False)
    ctool = XCAFDoc_DocumentTool.ColorTool_s(doc.Main())

    # add root
    top = tool.NewShape()
    TDataStd_Name.Set_s(top, TCollection_ExtendedString("CQ assembly"))

    # used to store unique part-color combinations
    unique_objs = {}

    def _toCAF(el, ancestor, color):

        # create a subassy
        subassy = tool.NewShape()
        setName(subassy, el.name, tool)

        # define the current color
        current_color = el.color if el.color else color

        # add a leaf with the actual part if needed
        if el.obj:
            # get/register unique parts referenced in the assy
            key = (current_color.toTuple(), el.obj)

            if key in unique_objs:
                lab = unique_objs[key]
            else:
                lab = tool.NewShape()
                tool.SetShape(lab, Compound.makeCompound(el.shapes).wrapped)
                setName(lab, f"{el.name}_part", tool)
                unique_objs[key] = lab

                # handle colors when exporting to STEP
                if coloredSTEP and current_color:
                    setColor(lab, current_color, ctool)

            tool.AddComponent(subassy, lab, TopLoc_Location())

        # handle colors when *not* exporting to STEP
        if not coloredSTEP and current_color:
            setColor(subassy, current_color, ctool)

        # add children recursively
        for child in el.children:
            _toCAF(child, subassy, current_color)

        # add the current subassy to the higher level assy
        tool.AddComponent(ancestor, subassy, el.loc.wrapped)

    # process the whole assy recursively
    _toCAF(assy, top, None)

    tool.UpdateAssemblies()

    return top, doc

[lorenzncode]

Thanks @adam-urbanczyk! Yes, definitely willing to refactor. Let me do some testing with this version.

[lorenzncode]

The suggested code works after minor changes.

For now, I duplicated the type definition AssemblyObjects. Attempting to import from the existing definition resulted in a circular import. Perhaps it should go to types.py?

[adam-urbanczyk]

Typing looks good @lorenzncode , let me take a second look and I think it is good to go.

[adam-urbanczyk]

Thanks @lorenzncode !

[adam-urbanczyk]

Shall I merge @lorenzncode , @jmwright ?

[lorenzncode]

@adam-urbanczyk Yes merging OK with me.

[jmwright]

@adam-urbanczyk Yes, please do.