Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added cone 3D primitive to Workplane. #859

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions cadquery/cq.py
Original file line number Diff line number Diff line change
Expand Up @@ -3800,6 +3800,62 @@ def sphere(
else:
return self.union(spheres, clean=clean)

def cone(
self: T,
height: float,
radius1: float = None,
radius2: float = None,
radius: float = 0,
direct: Vector = Vector(0, 0, 1),
angle: float = 360,
combine: bool = True,
clean: bool = True,
) -> T:
"""
Returns a cone with the specified radius and height for each point on the stack
A truncated cone can be created by specifying parameters radius1 and radius2 instead of radius.
:param height: The height of the cone
:type height: float > 0
:param radius1: The radius of the bottom of the cone
:type radius1: float > 0
:param radius2: The radius of the top of the cone
:type radius2: float > 0
:param radius: The radius of the cone
:type radius: float > 0
:param direct: The direction axis for the creation of the cone
:type direct: A three-tuple
:param angle: The angle to sweep the cone arc through
:type angle: float > 0
:param combine: Whether the results should be combined with other solids on the stack
(and each other)
:type combine: true to combine shapes, false otherwise
:param clean: call :py:meth:`clean` afterwards to have a clean shape
:return: A cone object for each point on the stack

One cone is created for each item on the current stack. If no items are on the stack, one
cone is created using the current workplane center.

If combine is true, the result will be a single object on the stack. If a solid was found
in the chain, the result is that solid with all cones produced fused onto it otherwise,
the result is the combination of all the produced cones.

If combine is false, the result will be a list of the cones produced.
"""

r1 = radius if radius1 is None or radius1 == 0 else radius1
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
r1 = radius if radius1 is None or radius1 == 0 else radius1
r1 = radius1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a reason for this, it to allow the user to specify cone(height=40, radius=10) for a simple cone and cone(height=40, radius1=10, radius2=5) for a truncated cone. This is akin to how OpenSCAD handles things.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is only convenient if argument names are always specified. But what about cone(40,10) for simple cone and cone(40,10,5) for truncated cone? cone(40,10,5) will give surprising results with three radius arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I could solve that problem by ordering the aguments: cone(height, radius1, radius2, radius). Then

cone(40,10)
cone(40,10,5)
cone(height=40,radius=10)
cone(height=40,radius1=10,radius2=5)

All behave as expected.

Copy link
Member

Choose a reason for hiding this comment

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

The API of OpenSCAD is not really relevant here (different language; no intended relation with CQ). What is very relevant on the other hand is consistence with existing functions. Taking this into account, I'd propose:

def cone(
        self: T,
        radius1: float,
        height: float,
        radius2: float = 0.0,
       ...

The same holds for the recently merged cylinder (radius and height should be interchanged to be similar to e.g. the hole signature). It would be really great, if you could amend it in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API of OpenSCAD is not really relevant here (different language; no intended relation with CQ).

While I agree that there is no need to be the same as OpenSCAD, I don't believe other languages are not relevant - taking cues from how other established systems work can often suggest a pattern to use.

What is very relevant on the other hand is consistence with existing functions

Agreed. The reason for consistency is not for its own sake, it is because consistency helps avoid errors. The parameter ordering cone(radius1, height, radius2) is horrible and error-inducing, thus defeating the reason for being consistent. And if we really want to follow other functions, eg cboreHole and cskHole then we should use diameter rather than radius. But we already have sphere, fillet, ellipse and circle which use radius. So orthogonality is already broken.

Having said all that, I suggest:

def cone(
    self: T,
    height: float,
    radius1: float,
    radius2: float = 0.0,
...

This:

  1. Uses radius and so is consistent with the other geometric primitives.
  2. Allows a natural parameter ordering for truncated cone.
  3. Does not have the contentious radius parameter.

r2 = 0 if radius2 is None else radius2
offset = Vector()
s = Solid.makeCone(r1, r2, height, offset, direct, angle)

# We want a cone for each point on the workplane
cones = self.eachpoint(lambda loc: s.moved(loc), True)

# If we don't need to combine everything, just return the created cones
if not combine:
return cones
else:
return self.union(cones, clean=clean)

def cylinder(
self: T,
height: float,
Expand Down
1 change: 1 addition & 0 deletions doc/apireference.rst
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ Some 3D operations also require an active 2D workplane, but some do not.
Workplane.box
Workplane.sphere
Workplane.cylinder
Workplane.cone
Workplane.union
Workplane.combine
Workplane.intersect
Expand Down
1 change: 0 additions & 1 deletion doc/roadmap.rst
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ rotation/transform that return a copy

primitive creation
Need primitive creation for:
* cone
* torus
* wedge

Expand Down
22 changes: 22 additions & 0 deletions tests/test_cadquery.py
Original file line number Diff line number Diff line change
Expand Up @@ -2476,6 +2476,28 @@ def testCylinderCentering(self):
s0.val().Center().toTuple(), s1.val().Center().toTuple(), 3
)

def testConeDefaults(self):
s = Workplane("XY").cone(40, 10)
self.assertEqual(1, s.size())
self.assertEqual(1, s.solids().size())
self.assertEqual(2, s.faces().size())
self.assertEqual(2, s.vertices().size())
s1 = Workplane("XY").cone(40, 10, 5)
self.assertEqual(1, s1.size())
self.assertEqual(1, s1.solids().size())
self.assertEqual(3, s1.faces().size())
self.assertEqual(2, s1.vertices().size())
s2 = Workplane("XY").cone(40, radius1=10, radius2=5)
self.assertEqual(1, s2.size())
self.assertEqual(1, s2.solids().size())
self.assertEqual(3, s2.faces().size())
self.assertEqual(2, s2.vertices().size())
s3 = Workplane("XY").cone(40, radius1=10, radius2=5, combine=False)
self.assertEqual(1, s3.size())
self.assertEqual(1, s3.solids().size())
self.assertEqual(3, s3.faces().size())
self.assertEqual(2, s3.vertices().size())

def testWedgeDefaults(self):
s = Workplane("XY").wedge(10, 10, 10, 5, 5, 5, 5)
self.saveModel(s)
Expand Down