search

CadQuery / cadquery

A python parametric CAD scripting framework based on OCCT
https://cadquery.readthedocs.io
Other
3.23k stars 293 forks source link

mirrorY/mirrorX deliver unexpected edges back #125

Open bernhard-42 opened 5 years ago

bernhard-42 commented 5 years ago

I am trying to visualize interims steps when working with cadquery. Doing so, I ran into some issues with mirrorYet. al. (btw. it does not seem to be a problem when using extrude(), only when you try to visualize the wires):

1) Method close() works fine

import cadquery as cq
from jupyter_cadquery.cadquery import Assembly, Part, Edges, Faces, Wires, show

yz = cq.Workplane("YZ")
xy = cq.Workplane("YX")

diam = 5.2
base_thickness = 2
base_width = 38
base_length = 64
gap = 6

holder_thickness = 1
holder_length = 22

holder_width = diam + 2 * holder_thickness
holder_height = diam + holder_thickness

side_margin = base_width/2 - holder_width
bottom_margin = 15

base = (xy
    .vLineTo(-base_length/2 + gap/2, forConstruction=True)
    .hLineTo(-gap/2)
    .vLineTo(-base_length/2)
    .hLineTo(-base_width/2)
    .vLineTo(-base_length/2 + 10)
    .hLineTo(-base_width/2 + gap/2)
    .vLineTo(holder_length / 2 - 2)
    .hLineTo(-base_width/2)
    .vLineTo(holder_length / 2)
    .threePointArc((-base_width*3/8, base_length*3/8), (-gap/2, base_length/2))
    .vLineTo((base_length-gap)/2)
    .hLineTo(0)
)
Wires(base.close())

image

2) Method mirrorY() returns unexpected edges

base = (xy
    .vLineTo(-base_length/2 + gap/2, forConstruction=True)
    .hLineTo(-gap/2)
    .vLineTo(-base_length/2)
    .hLineTo(-base_width/2)
    .vLineTo(-base_length/2 + 10)
    .hLineTo(-base_width/2 + gap/2)
    .vLineTo(holder_length / 2 - 2)
    .hLineTo(-base_width/2)
    .vLineTo(holder_length / 2)
    .threePointArc((-base_width*3/8, base_length*3/8), (-gap/2, base_length/2))
    .vLineTo((base_length-gap)/2)
    .hLineTo(0)

)

Wires(base.mirrorY())

image

3) Own mirrorY without wire consolidation returns what I would expect

def my_mirrorY(workplane):
    n = workplane.wire(forConstruction=False)
    mirroredWires = workplane.plane.mirrorInPlane(n.wires().vals(), 'Y')
    for w in mirroredWires:
        n.objects.append(w)
        n._addPendingWire(w)
    return n

base = (xy
    .vLineTo(-base_length/2 + gap/2, forConstruction=True)
    .hLineTo(-gap/2)
    .vLineTo(-base_length/2)
    .hLineTo(-base_width/2)
    .vLineTo(-base_length/2 + 10)
    .hLineTo(-base_width/2 + gap/2)
    .vLineTo(holder_length / 2 - 2)
    .hLineTo(-base_width/2)
    .vLineTo(holder_length / 2)
    .threePointArc((-base_width*3/8, base_length*3/8), (-gap/2, base_length/2))
    .vLineTo((base_length-gap)/2)
    .hLineTo(0)
)
Wires(my_mirrorY(base))

image

Note, Wires() uses OCC.Extend.TopologyUtils.discretize_wire to extract drawable edges

jmwright commented 5 years ago

I wonder if this has a connection to this very old issue.

adam-urbanczyk commented 5 years ago

I am having troubles reproducing the issue, am I missing something?

image

yz = cq.Workplane("YZ")
xy = cq.Workplane("YX")

diam = 5.2
base_thickness = 2
base_width = 38
base_length = 64
gap = 6

holder_thickness = 1
holder_length = 22

holder_width = diam + 2 * holder_thickness
holder_height = diam + holder_thickness

side_margin = base_width/2 - holder_width
bottom_margin = 15

base = (xy
    .vLineTo(-base_length/2 + gap/2, forConstruction=True)
    .hLineTo(-gap/2)
    .vLineTo(-base_length/2)
    .hLineTo(-base_width/2)
    .vLineTo(-base_length/2 + 10)
    .hLineTo(-base_width/2 + gap/2)
    .vLineTo(holder_length / 2 - 2)
    .hLineTo(-base_width/2)
    .vLineTo(holder_length / 2)
    .threePointArc((-base_width*3/8, base_length*3/8), (-gap/2, base_length/2))
    .vLineTo((base_length-gap)/2)
    .hLineTo(0).mirrorY()
)
bernhard-42 commented 5 years ago

Interesting, maybe a version issue. This is what I currently use:

cadquery                  0.5-SNAPSHOT             pypi_0    pypi
oce                       0.18.2               h1d359ef_0    cadquery
pythonocc-core            0.18.2           py36h781236e_0    cadquery

What is the command that will install the latest version of cadquery? Is it pip install git+https://github.com/CadQuery/cadquery ?

adam-urbanczyk commented 5 years ago

I think I am on the edge-face-fix branch, I don't expect much impact though. Your pip command is correct (will get you the latest master).

Anyhow, could you verify the number of edges according to CQ len(base.edges().objects))? For me it is 22.

bernhard-42 commented 5 years ago

yes, it is 22

I need to look into it tomorrow. Maybe I use discretize_wire in a wrong way ...

bernhard-42 commented 5 years ago

Let's take the two approaches:

import cadquery as cq

base_width = 38
base_length = 64
gap = 6
holder_length = 22

xy = cq.Workplane("YX")

def my_mirrorY(workplane):
    n = workplane.wire(forConstruction=False)
    mirroredWires = workplane.plane.mirrorInPlane(n.wires().vals(), 'Y')
    for w in mirroredWires:
        n.objects.append(w)
        n._addPendingWire(w)
    return n
base1 = (xy
    .vLineTo(-base_length/2 + gap/2, forConstruction=True)
    .hLineTo(-gap/2)
    .vLineTo(-base_length/2)
    .hLineTo(-base_width/2)
    .vLineTo(-base_length/2 + 10)
    .hLineTo(-base_width/2 + gap/2)
    .vLineTo(holder_length / 2 - 2)
    .hLineTo(-base_width/2)
    .vLineTo(holder_length / 2)
    .threePointArc((-base_width*3/8, base_length*3/8), (-gap/2, base_length/2))
    .vLineTo((base_length-gap)/2)
    .hLineTo(0)
    .mirrorY()
)
base2 = my_mirrorY(xy
    .vLineTo(-base_length/2 + gap/2, forConstruction=True)
    .hLineTo(-gap/2)
    .vLineTo(-base_length/2)
    .hLineTo(-base_width/2)
    .vLineTo(-base_length/2 + 10)
    .hLineTo(-base_width/2 + gap/2)
    .vLineTo(holder_length / 2 - 2)
    .hLineTo(-base_width/2)
    .vLineTo(holder_length / 2)
    .threePointArc((-base_width*3/8, base_length*3/8), (-gap/2, base_length/2))
    .vLineTo((base_length-gap)/2)
    .hLineTo(0)
)

and compare them from an edges perspective:

v1 = [[(v.X, v.Y, v.Z) for v in edge.Vertices()] for edge in base1.edges().objects]
v2 = [[(v.X, v.Y, v.Z) for v in edge.Vertices()] for edge in base2.edges().objects]
print(str(v1) == str(v2))
v1
# result:
True
[[(-29.0, 0.0, 0.0), (-29.0, -3.0, 0.0)],
 [(-29.0, -3.0, 0.0), (-32.0, -3.0, 0.0)],
 [(-32.0, -3.0, 0.0), (-32.0, -19.0, 0.0)],
 [(-32.0, -19.0, 0.0), (-22.0, -19.0, 0.0)],
 [(-22.0, -19.0, 0.0), (-22.0, -16.0, 0.0)],
 [(-22.0, -16.0, 0.0), (9.0, -16.0, 0.0)],
 [(9.0, -16.0, 0.0), (9.0, -19.0, 0.0)],
 [(9.0, -19.0, 0.0), (11.0, -19.0, 0.0)],
 [(11.0, -19.0, 0.0), (32.0, -3.0, 0.0)],
 [(32.0, -3.0, 0.0), (29.0, -3.0, 0.0)],
 [(29.0, -3.0, 0.0), (29.0, 0.0, 0.0)],
 [(-29.0, 0.0, 0.0), (-29.0, 3.0, 0.0)],
 [(-29.0, 3.0, 0.0), (-32.0, 3.0, 0.0)],
 [(-32.0, 3.0, 0.0), (-32.0, 19.0, 0.0)],
 [(-32.0, 19.0, 0.0), (-22.0, 19.0, 0.0)],
 [(-22.0, 19.0, 0.0), (-22.0, 16.0, 0.0)],
 [(-22.0, 16.0, 0.0), (9.0, 16.0, 0.0)],
 [(9.0, 16.0, 0.0), (9.0, 19.0, 0.0)],
 [(9.0, 19.0, 0.0), (11.0, 19.0, 0.0)],
 [(11.0, 19.0, 0.0), (32.0, 3.0, 0.0)],
 [(32.0, 3.0, 0.0), (29.0, 3.0, 0.0)],
 [(29.0, 3.0, 0.0), (29.0, 0.0, 0.0)]]

So all fine, even when we compare with the actual image (ignoring that the arc is a line; grid tick size is 5): image

Now lets move over to OCC:

w1 = [obj.wrapped for obj in base1.objects]
w1
# result
[class<'TopoDS_Wire'; id:899793089>]
w2 = [obj.wrapped for obj in base2.objects]
w2
# result
[class<'TopoDS_Wire'; id:897496145>, class<'TopoDS_Wire'; id:1562179796>]

So the builtin mirrorY returns one TopoDS_Wire (due to consolidation) and my non-consolidated version returns two TopoDS_Wires.

It gets interesting when we discretize the wire into vertices:

from OCC.Extend.TopologyUtils import discretize_wire
dw1 = [discretize_wire(w) for w in w1]
dw2 = [discretize_wire(w) for w in w2]

for i, ((x1,y1,z1),(x2,y2,y3)) in enumerate(zip(dw1[0], dw2[0]+dw2[1])):
    if i == 222:
        print("= = = = ")
    print("% 3d:" % i, x1, y1, " - ", x2, y2)

# result
  0: -29.0 0.0  -  -29.0 0.0
  1: -29.0 -0.5  -  -29.0 -0.5
  2: -29.0 -1.0  -  -29.0 -1.0
  3: -29.0 -1.5  -  -29.0 -1.5
  4: -29.0 -2.0  -  -29.0 -2.0
  5: -29.0 -2.5  -  -29.0 -2.5
  6: -29.0 -3.0  -  -29.0 -3.0
  7: -29.0 -3.0  -  -29.0 -3.0
  8: -29.5 -3.0  -  -29.5 -3.0
  9: -30.0 -3.0  -  -30.0 -3.0
 10: -30.5 -3.0  -  -30.5 -3.0
 11: -31.0 -3.0  -  -31.0 -3.0
 12: -31.5 -3.0  -  -31.5 -3.0
 13: -32.0 -3.0  -  -32.0 -3.0
 14: -32.0 -3.0  -  -32.0 -3.0
 15: -32.0 -3.5  -  -32.0 -3.5
 16: -32.0 -4.0  -  -32.0 -4.0
 17: -32.0 -4.5  -  -32.0 -4.5
 18: -32.0 -5.0  -  -32.0 -5.0
 19: -32.0 -5.5  -  -32.0 -5.5
 20: -32.0 -6.0  -  -32.0 -6.0

...

 211: 30.5 -3.0  -  30.5 -3.0
 212: 30.0 -3.0  -  30.0 -3.0
 213: 29.5 -3.0  -  29.5 -3.0
 214: 29.0 -3.0  -  29.0 -3.0
 215: 29.0 -3.0  -  29.0 -3.0
 216: 29.0 -2.5  -  29.0 -2.5
 217: 29.0 -2.0  -  29.0 -2.0
 218: 29.0 -1.5  -  29.0 -1.5
 219: 29.0 -1.0  -  29.0 -1.0
 220: 29.0 -0.5  -  29.0 -0.5
 221: 29.0 0.0  -  29.0 0.0
= = = = 
 222: 29.0 3.0  -  -29.0 0.0
 223: 29.0 2.5  -  -29.0 0.5
 224: 29.0 2.0  -  -29.0 1.0
 225: 29.0 1.5  -  -29.0 1.5
 226: 29.0 1.0  -  -29.0 2.0
 227: 29.0 0.5  -  -29.0 2.5
 228: 29.0 0.0  -  -29.0 3.0
 229: 32.0 3.0  -  -29.0 3.0
 230: 31.5 3.0  -  -29.5 3.0
 231: 31.0 3.0  -  -30.0 3.0
 232: 30.5 3.0  -  -30.5 3.0

...

 433: -30.5 3.0  -  30.5 3.0
 434: -31.0 3.0  -  30.0 3.0
 435: -31.5 3.0  -  29.5 3.0
 436: -32.0 3.0  -  29.0 3.0
 437: -29.0 0.0  -  29.0 3.0
 438: -29.0 0.5  -  29.0 2.5
 439: -29.0 1.0  -  29.0 2.0
 440: -29.0 1.5  -  29.0 1.5
 441: -29.0 2.0  -  29.0 1.0
 442: -29.0 2.5  -  29.0 0.5
 443: -29.0 3.0  -  29.0 0.0

After line 221 (when the original line was discretized, the order of the vertices of the mirrorY version get wrong. and the wrong order is reflected in the visualization:

image

Actually what it looks like is that every straight line (edge) gets discretized in reverse direction. And if you then try to plot the lines, the "jumps" appear. So maybe something in the consolidation during mirrorYis broken?

Or did I do something wrong in the above analysis?

bernhard-42 commented 5 years ago

In a new attempt, I kept the first call to consolidateWires in mirrorY:

def my_mirrorY2(workplane):
    n = workplane.wire(forConstruction=False)
    consolidated = n.consolidateWires()  # now kept
    mirroredWires = workplane.plane.mirrorInPlane(consolidated.wires().vals(), 'Y')
    for w in mirroredWires:
        consolidated.objects.append(w)
        consolidated._addPendingWire(w)
    return consolidated  # consolidation still omitted

If I plot the figure for base3 being calculated with my_mirrorY2 all fine. If I plot base3.consolidateWires() instead, the plot looks broken again.

adam-urbanczyk commented 5 years ago

@bernhard-42 the intention of mirror is to get one wire. I checked with isValid() method and it seems to be OK. Are you sure this is a cq issue? Given the fact that built-in rendering is fine, I'd say it is equally likely that the discretization code you are using has some problems.

To be sure, you can try rendering the following code in your viewer. I call ShapeFix_Wire.FixReorder() to resolve potential ordering issue in the wire. We could add it permanently if it improves the situation


from OCC.Core.ShapeFix import ShapeFix_Wire

yz = cq.Workplane("YZ")
xy = cq.Workplane("YX")

diam = 5.2
base_thickness = 2
base_width = 38
base_length = 64
gap = 6

holder_thickness = 1
holder_length = 22

holder_width = diam + 2 * holder_thickness
holder_height = diam + holder_thickness

side_margin = base_width/2 - holder_width
bottom_margin = 15

base = (xy
    .vLineTo(-base_length/2 + gap/2, forConstruction=True)
    .hLineTo(-gap/2)
    .vLineTo(-base_length/2)
    .hLineTo(-base_width/2)
    .vLineTo(-base_length/2 + 10)
    .hLineTo(-base_width/2 + gap/2)
    .vLineTo(holder_length / 2 - 2)
    .hLineTo(-base_width/2)
    .vLineTo(holder_length / 2)
    .threePointArc((-base_width*3/8, base_length*3/8), (-gap/2, base_length/2))
    .vLineTo((base_length-gap)/2)
    .hLineTo(0).mirrorY()
)

w = base.val()
f = cq.Face.makeFromWires(w)

sf = ShapeFix_Wire(w.wrapped,f.wrapped,1e-6)
sf.FixReorder()
base2 = cq.Shape(sf.Wire())
bernhard-42 commented 5 years ago

@adam-urbanczyk Unfortunately it does not help

To simplify things, if I remove all of my viewing code and use just cq and OCC:

from OCC.Extend.TopologyUtils import discretize_wire

base = (xy
    .vLineTo(-base_length/2 + gap/2, forConstruction=True)
    .hLineTo(-gap/2)
    .vLineTo(-base_length/2)
    .hLineTo(-base_width/2)
    .vLineTo(-base_length/2 + 10)
    .hLineTo(-base_width/2 + gap/2)
    .vLineTo(holder_length / 2 - 2)
    .hLineTo(-base_width/2)
    .vLineTo(holder_length / 2)
    .threePointArc((-base_width*3/8, base_length*3/8), (-gap/2, base_length/2))
    .vLineTo((base_length-gap)/2)
    .hLineTo(0).mirrorY()
)
w = discretize_wire(base.objects[0].wrapped)

for i,(x,y,z) in enumerate(w[210:234]):
    print("%03d %6.4f  %6.4f  %6.4f" % (i+210, x, y, z))

# result
210 31.0000  -3.0000  0.0000
211 30.5000  -3.0000  0.0000
212 30.0000  -3.0000  0.0000
213 29.5000  -3.0000  0.0000
214 29.0000  -3.0000  0.0000
215 29.0000  -3.0000  0.0000
216 29.0000  -2.5000  0.0000
217 29.0000  -2.0000  0.0000
218 29.0000  -1.5000  0.0000
219 29.0000  -1.0000  0.0000
220 29.0000  -0.5000  0.0000
221 29.0000  0.0000  0.0000
222 29.0000  3.0000  0.0000
223 29.0000  2.5000  0.0000
224 29.0000  2.0000  0.0000
225 29.0000  1.5000  0.0000
226 29.0000  1.0000  0.0000
227 29.0000  0.5000  0.0000
228 29.0000  0.0000  0.0000
229 32.0000  3.0000  0.0000
230 31.5000  3.0000  0.0000
231 31.0000  3.0000  0.0000
232 30.5000  3.0000  0.0000
233 30.0000  3.0000  0.0000

I still see the jump from 221 to 222 and from 228 to 299 and so on.

So not sure what I can improve here.

However, I will change to rendering single edges (and maybe this is was the internal viewer does), because then the direction of each single edge is irrelevant. Only if I try to "follow the wire", I create jumps. And I guess, after discretize_wire nothing can be repaired any more.

bernhard-42 commented 5 years ago

btw., when I group the discretized vertices in the mirrored part according to the edges they belong to and then reverse the order of these groups/edges, it looks like all could be well (from viewing the coordinates, haven‘t tried). Could it be that consolidateWires reverses the order of the second wire when it adds it to the first?

adam-urbanczyk commented 5 years ago

Interesting @bernhard-42 , will need to look into this. For the time being, could you try if the following code yields a reasonable discretization?

yz = cq.Workplane("YZ")
xy = cq.Workplane("YX")

diam = 5.2
base_thickness = 2
base_width = 38
base_length = 64
gap = 6

holder_thickness = 1
holder_length = 22

holder_width = diam + 2 * holder_thickness
holder_height = diam + holder_thickness

side_margin = base_width/2 - holder_width
bottom_margin = 15

base = (xy
    .vLineTo(-base_length/2 + gap/2, forConstruction=True)
    .hLineTo(-gap/2)
    .vLineTo(-base_length/2)
    .hLineTo(-base_width/2)
    .vLineTo(-base_length/2 + 10)
    .hLineTo(-base_width/2 + gap/2)
    .vLineTo(holder_length / 2 - 2)
    .hLineTo(-base_width/2)
    .vLineTo(holder_length / 2)
    .threePointArc((-base_width*3/8, base_length*3/8), (-gap/2, base_length/2))
    .vLineTo((base_length-gap)/2)
    .hLineTo(0).mirrorY()
)

w = base.val()
f = cq.Face.makeFromWires(w)
discretize_me = f.Wires()[0]
bernhard-42 commented 5 years ago

Same results with "jumps". I think I'll have some time tomorrow evening to look a little bit deeper into it.

bernhard-42 commented 5 years ago

@adam-urbanczyk Assume I have a wire along the following edges (I omit z for simplicity):

(x1, 0.0) / (x2, y2)
(x2, y2) / (x3, y3)
(x3, y3) / (x4, y4)
(x4, y4) / (x5, 0.0)

This means that the wire starts touching the y axis and stops touching the y-axis.

Then mirrorInPlane should deliver

(x1, 0) / (x2, -y2)
(x2, -y2) / (x3, -y3)
(x3, -y3) / (x4, -y4)
(x4, -y4) / (x5, 0)

Both wires are directed in the same direction. If I now want to combine them, wouldn't I need to reverse the second one (i.e. reverse order of edges and reverse order of vertices of each edge)?

(x5, 0) / (x4, -y4)
(x4, -y4) / (x3, -y3)
(x3, -y3) / (x2, -y2)
(x2, -y2) / (x1, 0)

Because now, the combination would be:

(x1, 0) / (x2, y2)
(x2, y2) / (x3, y3)
(x3, y3) / (x4, y4)
(x4, y4) / (x5, 0)
(x5, 0) / (x4, -y4)
(x4, -y4) / (x3, -y3)
(x3, -y3) / (x2, -y2)
(x2, -y2) / (x1, 0)

the nice and expected mirrored closed wire.

I tried to implement it for test purposes. However I did not manage to convince OCC to Reverse() or Reversed() a wire, sigh ...

bernhard-42 commented 5 years ago

I don't really understand the programming model of OCC. Reverse and Reversed change the Orientation of an edge, but discretize_wire ignores this flag.

So for test purposes I came up with "poor man's" reverse:

# don't do it, you'll migrate every curve to a straight line
def reverseEdge(edge):
    brt = BRep_Tool()
    vertices = _entities(edge, "Vertex")
    pnts = [brt.Pnt(topods_Vertex(v)) for v in vertices]

    return BRepBuilderAPI_MakeEdge(pnts[1], pnts[0]).Edge()

def reverseWire(wire):
    wire_builder = BRepBuilderAPI_MakeWire()
    we = WireExplorer(wire.wrapped)
    edges = reversed(list(we.ordered_edges()))
    for edge in edges:
        wire_builder.Add(reverseEdge(edge))
    result = wire_builder.Wire()
    return wire.__class__(result)

def mirrorY(wire):
    # convert edges to a wire, if there are pending edges
    n = wire.wire(forConstruction=False)

    # attempt to consolidate wires together.
    consolidated = n.consolidateWires()

    mirroredWires = wire.plane.mirrorInPlane(consolidated.wires().vals(), 'Y')
    for w in mirroredWires:
        reversed_wire = reverseWire(w)
        consolidated.objects.append(reversed_wire)
        consolidated._addPendingWire(reversed_wire)

    # attempt again to consolidate all of the wires
    return consolidated.consolidateWires()

the order is now correct, but of course the curve is replaced by a straight line in the mirror:

image

I give up now, OCC is too tough for me ;-) I will convert wires to a list of edges and plot them.

adam-urbanczyk commented 5 years ago

@bernhard-42 what is the status of this issue? Should we investigate this further?

bernhard-42 commented 5 years ago

As mentioned, I gave up on it since I wasn't able to reverse non linear curves in OCC.

Logically, I think mirrorX/mirrorY is broken, since in order to get a well ordered wire from mirroring, one needs to reverse order of edges and direction of each edge of the mirror. For plotting you can overcome the issue by ignoring wires and plotting each separate edge (for this approach wrong order and direction doesn't matter). So it doesn't block me currently.

But the issue might come back as soon as a well defined wire is necessary for an existing or new algorithm. So I'm tempted to recommend further investigation to avoid future issues. Unfortunately I got stuck with OCC here ...