Added cone 3D primitive to Workplane.

[martinbudden]

I've added a cone 3D primitive to the Workplane, as per item on the roadmap, https://github.com/CadQuery/cadquery/blob/master/doc/roadmap.rst

This follows the OpenSCAD style, so if a single radius is specified a normal cone is created, whereas if radius1 and radius2 is specified then a truncated cone is created.

So cq.Workplane("XY").cone(40, 10) creates a cone with height 40 and radius 10 whereas cq.Workplane("XY").cone(40, radius1=10, radius2=5) creates a truncated cone with bottom radius 10 and top radius 5.

[martinbudden]

Tests now only failing on

cadquery\occ_impl\exporters\dxf.py:86: error: Module has no attribute "BSplineClosed"

Which was a pre-existing error.

[codecov[bot]]

Codecov Report

Merging #859 (724af40) into master (ba1dfe4) will increase coverage by 0.01%. The diff coverage is 100.00%.

:exclamation: Current head 724af40 differs from pull request most recent head 57f0809. Consider uploading reports for the commit 57f0809 to get more accurate results

@@            Coverage Diff             @@
##           master     #859      +/-   ##
==========================================
+ Coverage   95.88%   95.90%   +0.01%     
==========================================
  Files          32       32              
  Lines        7663     7693      +30     
  Branches      815      817       +2     
==========================================
+ Hits         7348     7378      +30     
  Misses        186      186              
  Partials      129      129              

Impacted Files	Coverage Δ
cadquery/cq.py	91.42% <100.00%> (+0.07%)	:arrow_up:
tests/test_cadquery.py	99.22% <100.00%> (+<0.01%)	:arrow_up:

---

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 ba1dfe4...57f0809. Read the comment docs.

[adam-urbanczyk]

It is not perfect so let's make it even worse with regard to consistent param ordering? Interesting reasoning but I don't agree. -1 on merging with your proposal.

On Wed, Sep 8, 2021, 11:12 PM Martin Budden @.***> wrote:

@.**** commented on this pull request.

In cadquery/cq.py https://github.com/CadQuery/cadquery/pull/859#discussion_r704778931:

•        (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
  

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.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/CadQuery/cadquery/pull/859#discussion_r704778931, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKVOYUK5WWPCQQHF3X3CODUA7GVRANCNFSM5CZU2W4A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[martinbudden]

It is not perfect so let's make it even worse with regard to consistent param ordering?

No, that's not what I am saying at all.

What I am saying is that there are a number of factors that determine function signature, consistency is one of them and usability is another. Consistency is in fact a subset of usability, and so making something consistent but less usable makes no sense. (I also had a side argument that defining what is consistent is somewhat subjective when the set is already inconsistent.)

[adam-urbanczyk]

Our understanding of usability is very different.

On Thu, Sep 9, 2021, 1:29 AM Martin Budden @.***> wrote:

It is not perfect so let's make it even worse with regard to consistent param ordering?

No, that's not what I am saying at all.

What I am saying is that there are a number of factors that determine function signature, consistency is one of them and usability is another. Consistency is in fact a subset of usability, and so making something consistent but less usable makes no sense. (I also had a side argument that defining what is consistent is somewhat subjective when the set is already inconsistent.)

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/CadQuery/cadquery/pull/859#issuecomment-915640341, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKVOYW57NTV7M5CZV7LLELUA7WVLANCNFSM5CZU2W4A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[fedorkotov]

@adam-urbanczyk please reconsider radius1, height, radius2 argument order. IMHO it is really non-obvious and surprising. Look at GUIs and APIs of other somewhat similar software https://docs.blender.org/api/current/bpy.ops.mesh.html#bpy.ops.mesh.primitive_cone_add https://docs.salome-platform.org/latest/gui/GEOM/create_cone_page.html

Of course CadQuery does not have to follow traditions and conventions of other products but look at https://github.com/CadQuery/cadquery/blob/4c45eb20e3576a2cff74f30c76bc96bf0f58561a/cadquery/occ_impl/shapes.py#L2582 and BRepPrimAPI_MakeCone https://github.com/CadQuery/OCP/blob/e1df3469a39228ce2553aacc3a0238e334d4dd9a/opencascade/BRepPrimAPI_MakeCone.hxx#L49 Both of them have radii close together not and not separated by height.

[adam-urbanczyk]

Radii are separated due to the 2nd radius being keyword arg with 0 default value.

On Thu, Sep 9, 2021, 8:31 AM Fedor Kotov @.***> wrote:

@adam-urbanczyk https://github.com/adam-urbanczyk please reconsider radius1, height, radius2 argument order. IMHO it is really non-obvious and surprising. Look at GUIs and APIs of other somewhat similar software

https://docs.blender.org/api/current/bpy.ops.mesh.html#bpy.ops.mesh.primitive_cone_add https://docs.salome-platform.org/latest/gui/GEOM/create_cone_page.html

Of course CadQuery does not have to follow traditions and conventions of other products but look at

https://github.com/CadQuery/cadquery/blob/4c45eb20e3576a2cff74f30c76bc96bf0f58561a/cadquery/occ_impl/shapes.py#L2582 and BRepPrimAPI_MakeCone

https://github.com/CadQuery/OCP/blob/e1df3469a39228ce2553aacc3a0238e334d4dd9a/opencascade/BRepPrimAPI_MakeCone.hxx#L49 Both of them have radii close together not and not separated by height.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CadQuery/cadquery/pull/859#issuecomment-915802659, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADKVOYRLE7OEII24WDB67HTUBBIDVANCNFSM5CZU2W4A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

[fedorkotov]

@martinbudden I think that having a cone primitive creation method in Workplane is better than not having one. And so suggest you follow @adam-urbanczyk recommendations even if you do not agree so that this can be merged sooner rather than later (or never). If you have very strong opinion on this matter, there is also https://github.com/CadQuery/cadquery-plugins repo. Plugins don't have to conform to the same standards as "built in" features of CadQuery.

[adam-urbanczyk]

Another option would be to wait for sketch branch being merged (and implicitly multimethods) and implement two overloads r,h and r1,r2,h. It will take some time though.

[martinbudden]

Another option would be to wait for sketch branch being merged (and implicitly multimethods) and implement two overloads r,h and r1,r2,h. It will take some time though.

I think I'd rather wait for the implementation of multimethods than add the legacy of a method with the parameter ordering of r1, h, r2.

[Konubinix]

Hi. I'm not fluent in cadQuery and just wanted to make a cone the same way I make other primitives, with Workpkane().cone(...). IIUC, this PR enables this, right ?

[adam-urbanczyk]

Indeed, it is waiting to be reworked so that it provides two overloads cone(r, h) and cone(r1, r2, h). Seems though that the OP lost interest in the meantime, so if you want to pick this up you are more than welcome.

[Konubinix]

AU @.***> writes:

Indeed, it is waiting to be reworked so that it provides two overloads cone(r, h) and cone(r1, r2, h).

Thank you for the answer. I could manage with ~cq.Workplane("XY", obj=cq.Solid.makeCone(coneinnerdiameter/2, coneouterdiameter/2, coneheight))~

cadquery still looks a bit like magic to me, but the documentation is pretty nice and the examples help a lot. I'm always amazed to find out that my (very simple) experiments can be done with this tool. Kudo for the great work.

-- Konubinix GPG Key : 7439106A Fingerprint: 5993 BE7A DA65 E2D9 06CE 5C36 75D2 3CED 7439 106A