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

All modified and coverable lines are covered by tests ✅

Project coverage is 94.86%. Comparing base (f29f2d6) to head (2f07d38).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1593   +/-   ##
=======================================
  Coverage   94.86%   94.86%           
=======================================
  Files          28       28           
  Lines        6226     6230    +4     
  Branches     1261     1262    +1     
=======================================
+ Hits         5906     5910    +4     
  Misses        193      193           
  Partials      127      127           

☔ 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.