Added cone 3D primitive to Workplane. #859
Conversation
|
Tests now only failing on Which was a pre-existing error. |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
cadquery/cq.py
Outdated
| def cone( | ||
| self: T, | ||
| height: float, | ||
| radius: float = 0, |
There was a problem hiding this comment.
Please remove the redundancy (at most two radius params needed) and defaulting of radius/radius1 :
radius1: float,
radius2: Optional[float] =None
| 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 |
There was a problem hiding this comment.
| r1 = radius if radius1 is None or radius1 == 0 else radius1 | |
| r1 = radius1 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
- Uses
radiusand so is consistent with the other geometric primitives. - Allows a natural parameter ordering for truncated cone.
- Does not have the contentious
radiusparameter.
|
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
<#859 (comment)>:
> + (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
<#859 (comment)>, 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>.
|
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.) |
|
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
<#859 (comment)>,
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>.
|
|
@adam-urbanczyk please reconsider Of course CadQuery does not have to follow traditions and conventions of other products but look at cadquery/cadquery/occ_impl/shapes.py Line 2582 in 4c45eb2 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. |
|
@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. |
|
Another option would be to wait for sketch branch being merged (and implicitly multimethods) and implement two overloads |
I think I'd rather wait for the implementation of multimethods than add the legacy of a method with the parameter ordering of |
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.rstThis follows the OpenSCAD style, so if a single radius is specified a normal cone is created, whereas if
radius1andradius2is specified then a truncated cone is created.So
cq.Workplane("XY").cone(40, 10)creates a cone with height 40 and radius 10whereas
cq.Workplane("XY").cone(40, radius1=10, radius2=5)creates a truncated cone with bottom radius 10 and top radius 5.