Fix workplane cylinder center when generated using a custom direction

[kmarchais]

Fixes #1591.

• This PR fixes the following bug when a cylinder generated from a workplane with a direction different than the default one (0, 0, 1) was not centered at the right position.

>>> cq.Shape.centerOfMass(cq.Workplane("XY").cylinder(radius=1, height=10, direct=cq.Vector(1, 0, 0)).val()) 
Vector: (5.0, 0.0, -5.0)
>>> cq.Shape.centerOfMass(cq.Workplane("XY").cylinder(radius=1, height=10, direct=cq.Vector(0, 1, 0)).val()) 
Vector: (0.0, 5.0, -5.0)
>>> cq.Shape.centerOfMass(cq.Workplane("XY").cylinder(radius=1, height=10, direct=cq.Vector(0, 0, 1)).val()) 
Vector: (0.0, 0.0, 0.0)

• This PR also brings the possibility to center the cylinder when it is generated using any custom direction.

A test has been added to check these two points. It fails before the fix and passes after the fix.

The case where centered != (True, True, True) with a direction different than one along the x, y, or z axes is not managed. What would we like to do in this case?

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.33%. Comparing base (f29f2d6) to head (0c2b482).
Report is 24 commits behind head on master.

Files with missing lines	Patch %	Lines
cadquery/cq.py	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1593      +/-   ##
==========================================
+ Coverage   94.86%   95.33%   +0.47%     
==========================================
  Files          28       27       -1     
  Lines        6226     6774     +548     
  Branches     1261     1010     -251     
==========================================
+ Hits         5906     6458     +552     
  Misses        193      193              
+ Partials      127      123       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lorenzncode]

The case where centered != (True, True, True) with a direction different than one along the x, y, or z axes is not managed. What would we like to do in this case?

Something like the following should handle it.

        
        if isinstance(centered, bool):
            centered = (centered, centered, centered)

        offset = Vector()
        plane = Plane((0, 0, 0), normal=direct)
        if not centered[0]:
            offset += plane.toWorldCoords((radius, 0, 0))
        if not centered[1]:
            offset += plane.toWorldCoords((0, radius, 0))
        if centered[2]:
            offset += plane.toWorldCoords((0, 0, -height / 2))

        s = Solid.makeCylinder(radius, height, offset, direct, angle)

        # We want a cylinder for each point on the workplane
        return self.eachpoint(lambda loc: s.moved(loc), True, combine, clean)

[kmarchais]

Thanks, I prefer this than what I did but then it requires to be careful about the axis to set for centering.

To center a cylinder along its height but directed along the y axis, it requires to put the third component of centered to True as the following example show.

c1 = cq.Shape.centerOfMass(
    cq.Workplane("XY").cylinder(
        radius=1,
        height=10,
        direct=cq.Vector(0, 1, 0),
        centered=(False, False, True),
    ).val()
) 
print(c1)
c2 = cq.Shape.centerOfMass(
    cq.Workplane("XY").cylinder(
        radius=1,
        height=10,
        direct=cq.Vector(0, 1, 0),
        centered=(False, True, False),
    ).val()
)
print(c2)

Results in

Vector: (1.0, 0.0, 1.0)
Vector: (0.0, 5.0, 1.0)

Is it the expected behavior?

[adam-urbanczyk]

res = cq.Workplane("XY").cylinder(
        radius=1,
        height=10,
        direct=cq.Vector(0, 1, 0),
        centered=(False, False, True),
).val()

results in this:

res2 = cq.Workplane("XY").cylinder(
        radius=1,
        height=10,
        direct=cq.Vector(0, 1, 0),
        centered=(False, True, False),
).val()

and

in this:

@kmarchais how did you actually generate the reference data for the tests? Visually the result is as expected AFAICT.

[kmarchais]

The test was pretty straight forward in the previous implementation, testing all the possible combinations of centering and directions.
With this implementation, I will need to take time to find how to manage it correctly.

Also I want to make sure, is the following the expected result?

>>> cq.Shape.centerOfMass(cq.Workplane("XY").cylinder(radius=10, height=40, direct=cq.Vector(1, 0, 0), centered=False).val())
Vector: (20, -10, 10)

[adam-urbanczyk]

Looks OK, but the local coordinate system of the cylinder is ill-defined with only Z vector. Is that the point you are trying to make?

[kmarchais]

I am just not used to this radius shift and wanted to make sure that this -10 in the y direction is fine.
I will try to update the test soon.

[adam-urbanczyk]

Hey @kmarchais any updates on this? Do you intend to finalize it?

[kmarchais]

@adam-urbanczyk Thanks for the remainder. I'm on it.

[kmarchais]

I could not find a simple way to test every combination of centered and direct parameters. I made it so that it checks different configurations:

• Main axis directions (±X, ±Y, ±Z)
• Centering configurations (all centered, none centered, mixed centering)
• Verifies correct cylinder center position in each case

[lorenzncode]

Another idea is update the typing of the direct param

    direct: Union[Tuple[float, float, float], Vector] = Vector(0, 0, 1),

or = (0, 0, 1)
See Plane normal:

cadquery/cadquery/occ_impl/geom.py

Line 553 in 0c472ef

normal: Union[Tuple[float, float, float], Vector] = (0, 0, 1),

[adam-urbanczyk]

@lorenzncode it seems that your suggestions were applied. Are you OK with merging?

[lorenzncode]

@lorenzncode it seems that your suggestions were applied. Are you OK with merging?

I came across a couple of cases I'm not certain are correct.

1. Compare box and cylinder for the following case. It seems the result is inconsistent. Why is the cylinder in negative Y? Shouldn't the bounding box of the cylinder be identical to box1? That is, both the box and cylinder in +Y.

import cadquery as cq

centered = (False, False, False)

r = 1
h = 10

cyl1 = (
    cq.Workplane()
    .cylinder(h, r, (1, 0, 0), centered=centered)
)

box1 = (
    cq.Workplane()
    .box(h, r * 2, r * 2, centered=centered)
)

2. Here, I expected the second cylinder to be a YZ mirror of the first (bound box of cylinder to be in +Z).

import cadquery as cq

r = 1
h = 10
centered = (False, False, False)

cyl1 = (
    cq.Workplane()
    .cylinder(h, r, (1, 0, 0), centered=centered)
)

cyl2 = (
    cq.Workplane()
    .cylinder(h, r, (-1, 0, 0), centered=centered)
)

[lorenzncode]

I tested the snippet again here and get the same result. I suppose this is fine as is.

If I were to use this method I think I would choose only one of these centering options:

centered = (True, True, True)
# or 
centered = (True, True, False)

Anyway the default is (True, True, True).

[adam-urbanczyk]

Alright, merging. Thanks @kmarchais !