Skip to content

Conversation

@jmwright
Copy link
Member

@jmwright jmwright commented Feb 3, 2023

This PR adds two new modes for STEP export.

1. Simplified STEP - Keeps the hierarchy to one level but will have multiple shapes, uses the names set for the assembly components as the shape names, and preserves colors.
2. Fused STEP - Will only have one shape when the solids in the assembly can be fused, otherwise will have a one-level hierarchy. Preserves colors by adding the faces of the assembly's solids as subshapes to the STEP document.

@jmwright
Copy link
Member Author

jmwright commented Feb 3, 2023

@adam-urbanczyk @lorenzncode I'm still writing the docs, but feel free to critique the code. I'm planning to do a full test run of the fused export method with the KiCAD generator before taking this out of draft.

@adam-urbanczyk
Copy link
Member

adam-urbanczyk commented Feb 3, 2023

TBH I think that the fused method should not use cut. You don't even need to fuse to be able to add colors to faces.

@kliment
Copy link

kliment commented Feb 3, 2023

Is there a way to implement color-preserving fuse without the cut?

@adam-urbanczyk
Copy link
Member

@kliment the first question is: why do you think you need a fuse to add colors to subshapes?

@kliment
Copy link

kliment commented Feb 3, 2023

The requirement that Jeremy's work is based on was for turning an assembly of different-colored parts into a single compound which preserves face colors. I believe a fuse is required for this. Obviously it's much easier to add colors without fusing, but this doesn't meet the single compound requirement.

@kliment
Copy link

kliment commented Feb 3, 2023

For reference, this requirement is coming from the KiCad library team, as we're looking for a replacement for the export method we use in FreeCad, which uses FreeCad's color-preserving fuse operation.

@adam-urbanczyk
Copy link
Member

adam-urbanczyk commented Feb 3, 2023

As I wrote above, you can add colors to a compound and fusing is not needed to have a single compound... E.g. I can get this without fusing:

afbeelding

@jmwright
Copy link
Member Author

jmwright commented Feb 3, 2023

If someone can present a fuse-with-colors method that does not need the cut and also works reliably, I'm interested in it. I tried several things and this was the only method that would do what was needed. I may be missing something though as I have trouble following everything that FreeCAD is doing with their export.

@jmwright
Copy link
Member Author

jmwright commented Feb 3, 2023

@adam-urbanczyk Can you post the code you used to get the result in the screenshot above?

@adam-urbanczyk
Copy link
Member

There you go:

from OCP.Quantity import Quantity_Color, Quantity_TOC_RGB
from OCP.TopoDS import TopoDS_Compound
from OCP.STEPControl import STEPControl_Writer, STEPControl_AsIs
from OCP.BRep import BRep_Tool, BRep_Builder
from OCP.TCollection import TCollection_ExtendedString
from OCP.TDocStd import TDocStd_Application, TDocStd_Document
from OCP.XCAFDoc import XCAFDoc_DocumentTool, XCAFDoc_ColorType, XCAFDoc_ColorGen
from OCP.STEPCAFControl import STEPCAFControl_Writer
from OCP.XSControl import XSControl_WorkSession
from OCP.TDataStd import TDataStd_Name
import cadquery as cq

# Build a simple assembly
assy = cq.Assembly()
box1 = cq.Workplane().box(10, 10, 10)
assy.add(box1, color=cq.Color(1, 0, 0))
box2 = cq.Workplane().center(0, 10).box(10, 10, 10)
assy.add(box2, color=cq.Color(0, 1, 0))

##############################
## EXPORT ####################
##############################
# The document
app = TDocStd_Application()
doc = TDocStd_Document(TCollection_ExtendedString("BinXCAF"))
app.InitDocument(doc)

# The shape and color tool
shape_tool = XCAFDoc_DocumentTool.ShapeTool_s(doc.Main())
color_tool = XCAFDoc_DocumentTool.ColorTool_s(doc.Main())


# Set up the compound
comp = cq.Compound.makeCompound(
    assy.toCompound().Solids()
)

# Compound assembly type
top_lbl = shape_tool.AddShape(comp.wrapped, False, True)

# NB I just assign random colors here, this should be based on a mapping to the original assy
for f in comp.Solids()[0].Faces():
    cur_lbl = shape_tool.AddSubShape(top_lbl, f.wrapped)
    color_tool.SetColor(cur_lbl, assy.children[1].color.wrapped, XCAFDoc_ColorGen)

# Export to STEP
session = XSControl_WorkSession()
writer = STEPCAFControl_Writer(session, False)
writer.Transfer(doc, STEPControl_AsIs)
status = writer.Write("xde_step_export_test.step")

@jmwright
Copy link
Member Author

jmwright commented Feb 4, 2023

@adam-urbanczyk I can only get your result when I check Enable STEP Compound merge within FreeCAD. Edit->Preferences->Import/Export->STEP (Tab). Otherwise I get this:

Screenshot from 2023-02-04 07-57-27

I don't think this is what we want because FreeCAD is artificially merging the compound and not showing the hierarchy that is actually in the file.

I think the only way to do this without a cut is to walk through all the faces of the compound and check if they are within the bounds of a face from the original assembly. I'm guessing OCCT has a way to check that, but I'm not sure what it is yet.

For reference, here is an explanation of why this fused method is important to KiCAD users. And here is a follow-up comment to that one.

@adam-urbanczyk
Copy link
Member

Fair enough, not using fuse is not an option, but it has nothing to do with colors.

On avoiding cut: If you want to map faces of the arguments of an OCCT operation to faces of the result, you can use the Modified method (see here).

@jmwright
Copy link
Member Author

jmwright commented Feb 6, 2023

@adam-urbanczyk @kliment There is still some clean-up to do, but the toFused() method now uses the history rather than the cut operation to handle the modified faces.

@kliment
Copy link

kliment commented Feb 11, 2023

Looks like the appveyor builds are failing because of a code style issue

@jmwright
Copy link
Member Author

@kliment Yeah, I'm still doing full runs of the KiCAD generator and tweaking toFused() as things come up. It's close though, and I'll do some clean up when it's ready.

Copy link
Member

@adam-urbanczyk adam-urbanczyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass

@adam-urbanczyk
Copy link
Member

@lorenzncode what is the status here? Still adding some tests? AFAICT they are quite extensive as-is.

@lorenzncode
Copy link
Member

Agreed, the tests are probably sufficient to catch most regressions. I wanted to get a better idea of the performance. Experimenting with fuse op SetRunParallel(True) I did not find significant speedup for the case of intersecting shapes. Setting glue=True was much faster - consider enabling glue by default?

@adam-urbanczyk
Copy link
Member

Agreed, the tests are probably sufficient to catch most regressions. I wanted to get a better idea of the performance. Experimenting with fuse op SetRunParallel(True) I did not find significant speedup for the case of intersecting shapes. Setting glue=True was much faster - consider enabling glue by default?

I'd say no, it should be generic by default.

@lorenzncode
Copy link
Member

lorenzncode commented Apr 23, 2023

What is the preferred argument name for the "export mode" of method exportAssembly? Currently the name is "exportMode", where the doc examples use the name "export_mode".

edit: That is - change the docs or change the argument name? Note that the other arg names of this method use underscore, e.g. "fuzzy_tol".

* Correct the export mode keyword arg name in example
* Include example of exporters.export opt dict
@lorenzncode
Copy link
Member

For now, I've updated the doc examples to correct the export mode argument name (with regards to the current code).

@jmwright
Copy link
Member Author

I think exportMode matches the other parameters. I have been trying to only use the underscore naming convention within kwargs.

@adam-urbanczyk
Copy link
Member

I think exportMode matches the other parameters. I have been trying to only use the underscore naming convention within kwargs.

Indeed, but in this particular case I'd propose to use just mode.

@jmwright
Copy link
Member Author

I made that change, and even though the tests still pass, it broke the fuse mode so that the setting seems to be ignored now. I may just revert the change and let you guys set it to what you think is best.

@adam-urbanczyk
Copy link
Member

Hm, AFAIR the tests check the number of resulting solids. How did you conclude that your change is not working?

@jmwright
Copy link
Member Author

The problem ended up being that I didn't get the parameter name changed everywhere in the KiCAD generator.

@adam-urbanczyk
Copy link
Member

OK, so are we good to merge this?

@jmwright
Copy link
Member Author

+1 to merge

@lorenzncode
Copy link
Member

I like the mode rename. I did not find any other issues.

@adam-urbanczyk adam-urbanczyk merged commit 33bf83e into master Apr 27, 2023
@adam-urbanczyk
Copy link
Member

Merged, thanks for all the hard work @jmwright and @lorenzncode !

@jmwright
Copy link
Member Author

Thanks for all the feedback @adam-urbanczyk and @lorenzncode

@jmwright jmwright deleted the step branch April 27, 2023 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants