Assembly hierarchy

[bernhard-42]

As discussed in #522, here is a pull request that:

• Introduces hierarchical assembly names delimited by "/"
• splits _query into _query and _query_workplane (as needed for my mate approach)
• a little change to simplify subclassing (without overriding add method)

My hexapod example with pure cadquery Assemblies

leg = (cq.Assembly(upper_leg)
    .add(lower_leg, name="lower")
)

hexapod = (cq.Assembly(base, name="bottom")
    .add(base, name="top")
    .add(stand, name="front_stand")
    .add(stand, name="back_stand")
)
for i, name in enumerate(leg_names):
    hexapod.add(leg, name=name)

now has the following hexapod.objects:

{'bottom': <cadquery.assembly.Assembly at 0x7efdcf51fc10>,
 'bottom/top': <cadquery.assembly.Assembly at 0x7efdcf51feb0>,
 'bottom/front_stand': <cadquery.assembly.Assembly at 0x7efdcf51ffa0>,
 'bottom/back_stand': <cadquery.assembly.Assembly at 0x7efdcf51f7c0>,
 'bottom/right_back': <cadquery.assembly.Assembly at 0x7efdcf51fd60>,
 'bottom/right_back/lower': <cadquery.assembly.Assembly at 0x7efdcf52e3a0>,
 'bottom/right_middle': <cadquery.assembly.Assembly at 0x7efdcf51ff70>,
 'bottom/right_middle/lower': <cadquery.assembly.Assembly at 0x7efdcf52e700>,
 'bottom/right_front': <cadquery.assembly.Assembly at 0x7efdcf52e1f0>,
 'bottom/right_front/lower': <cadquery.assembly.Assembly at 0x7efdcf52ea00>,
 'bottom/left_back': <cadquery.assembly.Assembly at 0x7efdcf52e730>,
 'bottom/left_back/lower': <cadquery.assembly.Assembly at 0x7efdcf52e9a0>,
 'bottom/left_middle': <cadquery.assembly.Assembly at 0x7efdcf52ea30>,
 'bottom/left_middle/lower': <cadquery.assembly.Assembly at 0x7efdcf52e7f0>,
 'bottom/left_front': <cadquery.assembly.Assembly at 0x7efdcf52e550>,
 'bottom/left_front/lower': <cadquery.assembly.Assembly at 0x7efdcf52eca0>}

[codecov]

Codecov Report

Merging #534 (9d1e335) into assembly-tags (20eb996) will increase coverage by 0.45%.
The diff coverage is 98.09%.

@@                Coverage Diff                @@
##           assembly-tags     #534      +/-   ##
=================================================
+ Coverage          94.08%   94.53%   +0.45%     
=================================================
  Files                 30       29       -1     
  Lines               6794     6994     +200     
  Branches             765      873     +108     
=================================================
+ Hits                6392     6612     +220     
+ Misses               257      230      -27     
- Partials             145      152       +7     

Impacted Files	Coverage Δ
cadquery/occ_impl/shapes.py	91.91% <78.94%> (-1.04%)	⬇️
tests/test_workplanes.py	99.30% <98.48%> (-0.70%)	⬇️
cadquery/assembly.py	90.50% <100.00%> (+0.50%)	⬆️
cadquery/cq.py	91.72% <100.00%> (+3.04%)	⬆️
cadquery/selectors.py	99.17% <100.00%> (-0.22%)	⬇️
tests/__init__.py	88.23% <100.00%> (+0.73%)	⬆️
tests/test_assembly.py	100.00% <100.00%> (ø)
tests/test_cad_objects.py	99.10% <100.00%> (+0.87%)	⬆️
tests/test_cadquery.py	99.11% <100.00%> (+0.07%)	⬆️
tests/test_selectors.py	100.00% <100.00%> (ø)
... and 7 more

---

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 20eb996...f19895e. Read the comment docs.

[adam-urbanczyk]

Any clue why travis is not triggered?

[jmwright]

Any clue why travis is not triggered?

That's strange. Travis ran fine yesterday for the ProjectOrigin PR. I wonder if the GitHub webhook is having trouble right now.

[adam-urbanczyk]

this PR is not to master.

[bernhard-42]

@adam-urbanczyk Final comment on the nested assemblies: I think the assembly tutorial in the docs would need a little update to reflect the nested ids now.

[bernhard-42]

@adam-urbanczyk Looks like the commit "More strict grammar" breaks _query_workplane()

For line 285, I think, instead of

name: str = query.name

it should now be

name: str = PATH_DELIM.join(query.name)

[adam-urbanczyk]

@bernhard-42 I want to get rid of the root in the names and thus keep the original convention for non-nested assemblies. Are you OK with that?

Also I'd like to revert the copy changes to not alter the ids of the objects and in the end only keep that paths in the objects dict.

[bernhard-42]

@adam-urbanczyk

Also I'd like to revert the copy changes to not alter the ids of the objects and in the end only keep that paths in the objects dict.

That should be possible and - the more I think about it - I would also prefer to do so, but I need to look into it.

I want to get rid of the root in the names and thus keep the original convention for non-nested assemblies. Are you OK with that?

I also wasn't too much of a fan of it (especially due to uuids in Assembly() case), but saw some issues in doing so. Let's discuss some details:

1 Remove the top level

Given my example above, assume that there would only be bottom, front_stand, back_stand and top, then it would look like:

{'bottom': <cadquery.assembly.Assembly at 0x7efdcf51fc10>,
 'top': <cadquery.assembly.Assembly at 0x7efdcf51feb0>,
 'front_stand': <cadquery.assembly.Assembly at 0x7efdcf51ffa0>,
 'back_stand': <cadquery.assembly.Assembly at 0x7efdcf51f7c0>
}

Pros:

• Easier addressing

Cons:

• What I don't like about the idea is that it adds an inherent semantic (especially for the manual mating): The first element's loc element changes the location of all elements while the loc elements of the other three elements changes only their location relative to the first element. To me this doesn't feel intuitive.
• A child cannot have the same name as the parent. Assembly(obj1, name="abc").add(obj2, name="abc) would lead to one abc element only instead of the more correct abc and abc/abc. Agreed, corner case, but wanted to mention.

2 Consistency with nested assemblies

If I now add a leg, the naming should not suddenly change for the 2., 3. and 4. element just because it is now a nested assembly. So I'd say the root element then should always be omitted, leading to:

{'bottom': <cadquery.assembly.Assembly at 0x7efdcf51fc10>,
 'top': <cadquery.assembly.Assembly at 0x7efdcf51feb0>,
 'front_stand': <cadquery.assembly.Assembly at 0x7efdcf51ffa0>,
 'back_stand': <cadquery.assembly.Assembly at 0x7efdcf51f7c0>,
 'right_back': <cadquery.assembly.Assembly at 0x7efdcf51fd60>,
 'right_back/lower': <cadquery.assembly.Assembly at 0x7efdcf52e3a0>,
 'right_middle': <cadquery.assembly.Assembly at 0x7efdcf51ff70>,
 'right_middle/lower': <cadquery.assembly.Assembly at 0x7efdcf52e700>
}

Pros:

• Easier addressing
• Same naming when an assembly gets a nested one by adding another assembly

Cons

• To me it gets even more inconsistent from a loc element perspective: right_back.loc changes the location of two elements as indicated by the naming hierarchy. bottom still changes all without being visible from the naming.

3 Alternative: Keep the hierarchy, but make root optional in addressing

To correctly reflect internal dependencies (hierarchy) with the naming convention, keep the root element in the name, i.e.

{'bottom': <cadquery.assembly.Assembly at 0x7efdcf51fc10>,
 'bottom/top': <cadquery.assembly.Assembly at 0x7efdcf51feb0>,
  ...
}

however, allow addressing with omission of the top level assembly root element (not possible for nested assemblies of course):

_query("top")
_query("right_middle/lower")

would be shortcuts for the also valid

_query("bottom/top")
_query("bottom/right_middle/lower")

This would also help avoiding uuids in empty root:

a = Assembly().add(obj1, name='obj1').add(obj2, name='obj2')

would internally still be

{'7b7e6636-3d0d-11eb-bdae-3497f65b2717': <cadquery.assembly.Assembly at 0x7f544ce1b820>,
 '7b7e6636-3d0d-11eb-bdae-3497f65b2717/obj1': <cadquery.assembly.Assembly at 0x7f544cea67f0>,
 '7b7e6636-3d0d-11eb-bdae-3497f65b2717/obj2': <cadquery.assembly.Assembly at 0x7f544d2faf10>}

allowing to change the location of obj1 and obj2 at once by changing the root location. But the elements can easily be addressed by

_query("obj1")
_query("obj2")

Pros

• Easier addressing for all case while keeping the hierarchy info visible
• Would also solve the corner case in 1) since a direct addressing of abc/abc is possible.

Cons

• Slightly more work for _query. After parsing it needs to check for name and root/name in the objects dict.

4 Mandatory name for assemblies (optional point)

This has nothing to do with 1) - 3), more of a sidetrack: I personally would make the name element mandatory and get rid of uuids. They are a great invention, but not very human friendly. Forcing users to write

a = Assembly(name="assy").add(obj1, name='obj1').add(obj2, name='obj2')

is not much of a change, but would lead to something more readable/addressable.

Especially for cases like

a = Assembly().add(obj1, name='obj1').add(obj2, name='obj2')
b = Assembly().add(obj2, name="obj3").add(a)

since

{'2d59d6d0-3d16-11eb-bdae-3497f65b2717': <cadquery.assembly.Assembly at 0x7f544cedfca0>,
 '2d59d6d0-3d16-11eb-bdae-3497f65b2717/obj3': <cadquery.assembly.Assembly at 0x7f544ce2eeb0>,
 '2d59d6d0-3d16-11eb-bdae-3497f65b2717/2d59cc12-3d16-11eb-bdae-3497f65b2717': <cadquery.assembly.Assembly at 0x7f544ce2ed30>,
 '2d59d6d0-3d16-11eb-bdae-3497f65b2717/2d59cc12-3d16-11eb-bdae-3497f65b2717/obj1': <cadquery.assembly.Assembly at 0x7f544ce2eee0>,
 '2d59d6d0-3d16-11eb-bdae-3497f65b2717/2d59cc12-3d16-11eb-bdae-3497f65b2717/obj2': <cadquery.assembly.Assembly at 0x7f544ce2ea60>}

is really ugly. You could argue that this is the developers choice. Right, however forcing for better style often isn't a bad idea ...

SUMMARY

• I would change the copy method as you want (if no issue turns up by doing that, but I don't expect it)
• I would go for option 3. To me it combines ease of addressing with a high level of consistency throughout the assembly system.
• And I would force names (couldn't resist)

What do you think?

[adam-urbanczyk]

In your nomenclature 2 is what I meant. I'll open another PR to show what I have in mind in terms of code.

The objects dict was not supposed to encode parent/child relations and reflect the loc structure in the original design. This is encoded in the children and parent fields.

[adam-urbanczyk]

Could you take a look if #545 works for your use case?

[bernhard-42]

@adam-urbanczyk

It works.

However, for my use case to work I had to split _query again into _query_workplane and _query (see 9affb15; I need the workplane info for my mates) and to change add to use assy = self.__class__(arg, **kwargs) instead of assy = Assembly(arg, **kwargs) (see 156e331; then my subclass MAssembly does not need to override and modify add to use MAssembly instead of Assembly),

[adam-urbanczyk]

OK, I added the call to __class__ , Why do you actually want use Workplane - it'll have no relation in general with the selected object (e.g. different normal).

[bernhard-42]

Thanks for the __class__ change.

For holes I would like to select the center of the hole (i.e. the center of the circle on one face) as the origin of a mate and the zDir and xDir of the face as the zDir and xDir of the mate. So actually, I am interested in the face. The circle is just used to move the mate from the center of the face to the center of the circle.
The easiest way was to tag each hole's circle on a face, then later select them as obj and get their Center and zDir, xDir from the workplane:

self.pnt = obj.val().Center()
self.x_dir = obj.workplane().plane.xDir
self.z_dir = obj.workplane().plane.zDir

It did work very well for all my test assemblies.

If I now start from obj.val(), as _query does, I lose the workplane of the face - but maybe my approach ist not correct and there is a better one?

[adam-urbanczyk]

How is this supposed to work with holes that have a different normal than your workplane? I think you want to get the actual normal using e.g. normal() on an Edge / Wire or normalAt() on a Face object.

[bernhard-42]

Understood, the normal would be the z direction.
And for the x direction I could follow _computeXdir(normal) from the method Workplane.workplane and compute the cross product of z-dir and (1,0,0), right?

To be more concrete, to get z-dir for a selected object that is a

• cadquery.occ_impl.shapes.Wire I'd use wire.normal()
• cadquery.occ_impl.shapes.Face I'd use face.normalAt(face.Center()), given the face center is where I want to add my mate

and for x-dir in both cases something like _computeXdir

If this approach is OK, then I think I don't need to something _query_workplane.

[adam-urbanczyk]

To be honest the x-direction has to be defined by the user in the most general case. For example given a hexagonal hole you cannot tell a priori which xdir is the desired one.

[bernhard-42]

I think I just need a defined x-dir, so cross product of normal and (1,0,0) might do it. You can add rotations when you define mates, so the user can finally control it => I will do some tests.
Thanks for your help

[bernhard-42]

btw. just to mention, that's why I thought about using the x-dir of the face the circle is on. Just to have a defined direction that makes sense in the context of the face.

[bernhard-42]

I now had time to try out some ideas, especially on objects which faces are not parallel to the world coord-system planes. Looks like the current _query method does not work for me:

Assume I want to place a mate at the center of a face with xDirand zDir being parallel to the ones of the face (no further complexity, no holes involved):

workplane = cq.Workplane().transformed(rotate=(30, 45, 60))   # rotate for the sake of the argument
box = workplane.box(5,10,15)
a = cq.Assembly(box, name="box")
face = box.faces(f">{workplane.plane.zDir.toTuple()}")

If I would get back the actual workplane object from a._query(f"box@faces@>{workplane.plane.zDir.toTuple()}") instead of the Face it would be as easy as using (obj.val().Center(), obj.plane.xDir, obj.plane.zDir) for the mate.
Since I get back a Face object, I now see no other chance than additionally provide the plane's xDir and zDir. Possible, but not very nice.

I fully buy into your point around normals of holes and planes, but even for the simple example above I still think that a split into _query and _query_workplane would be helpful, at least for me.
However, we can discuss again if you would be interested in the mate base assembly PR ....

[adam-urbanczyk]

Thanks @bernhard-42 for the feedback, you selected an example where the object is clearly aligned with the local coordinate system. How would the _query_workplane solution work for the object below? It is artificial, but is meant to illustrate the point that CQ models don't need to have a direct relation with the current workplane.

res = (
    cq.Workplane()
    .rect(2,1)
    .extrude(.5,taper=45)
    .rotateAboutCenter((0,0,1),45)
    .faces('>(1,1,1)')
)

[bernhard-42]

@adam-urbanczyk This is a great example to prove that my approach is wrong, thank you! I wasn't able to come up with one myself.

I think I need to base my Mate definitions on the normals of faces and circles returned by _query and then work with something like the _computeXdir function in your workplane method to compute the x direction.

But I need to think more about it. I don't think there is a "correct" way to select x for a given normal, but I want something that is easy to understand and work with in the context of the face or shape.

[bernhard-42]

@adam-urbanczyk To continue where we stopped some weeks ago, this is the current logic for creating Mates:

self.origin = val.Center()

if val.geomType() in ["CIRCLE", "ELLIPSE"]:
    self.z_dir = val.normal()

    vertices = val.Vertices()
    if len(vertices) == 1:  # full circle or ellipse
        # Use the vector defined by the circle's/ellipse's vertex and the origin as x direction
        self.x_dir = sub(vertices[0], self.origin)
    else:  # arc
        # Use the vector defined by start and end of the arc as x direction
        self.x_dir = sub(vertices[1], vertices[0])

elif isinstance(val, Wire):
    self.z_dir = val.normal()

    vertices = val.Vertices()
    if len(vertices) == 1:  # e.g. a single closed spline
        # Use the vector defined by the vertex and the origin as x direction
        self.x_dir = sub(vertices[0], self.origin)
    else:
        # Use the vector defined by the first two vertices as x direction
        self.x_dir = sub(vertices[1], vertices[0])

elif isinstance(val, Face):
    self.z_dir = val.normalAt(val.Center())

    # x_dir will be derived from the local coord system of the underlying plane
    xd = val._geomAdaptor().Position().XDirection()
    self.x_dir = Vector(xd.X(), xd.Y(), xd.Z())

else:
    raise ValueError("Needs a Face, Wire, Circle or an Ellipse")

I am not saying that these rules are the only ones or the correct ones, however, the idea is that for faces, circles, ellipses and wires the xaxis never appears random to the user but follow a clear set of rules.

Does this make sense for you?