Add Cylinder3D #84
Add Cylinder3D #84
Conversation
Initial class properties and methods
|
Thanks @santiagogaray . I will review this a soon as I get the chance. You'll see that I opened an issue about adding a Also @mostaphaRoudsari brought up a good point to me the other day, which is that, for any object that cannot have a 2D counterpart, we can probably drop the |
|
Sounds good @chriswmackey, makes a lot of sense to drop '3D' in these cases, will start working on it. |
Remove '3D' from class name. Remove '3d' from sphere intersect methods name Remove '3D' from sphere_tests
Remove '3D' from class name Remove '3D' from cone_tests
Remove '3D' from class name
There was a problem hiding this comment.
This generally looks good @santiagogaray . Thanks for putting it together. There's just one bug to fix, one small feature request, and you'll need to add some tests for the cylinder. Then, we can merge this in.
| * diameter | ||
| * height | ||
| * area | ||
| * volume |
There was a problem hiding this comment.
I like how you have set up the init method of this object but I think we will need easy access to the center_end point in order to use this for certain applications. This should be easy to add as a property since it's effectively the following:
self.center + self.axis
Along these lines, it would be good to have a classmethod to initialize the cylinder from a start point, end point, and radius since I know that this is how cylinders are represented in the Radiance engine.
+ Fix minor bugs
Add 'from_start_end' class method Include new constructor in tests
|
Thank you @chriswmackey for checking the code. I made the requested changes and included the cylinder tests for your review. |
There was a problem hiding this comment.
This looks great @santiagogaray . Thank you for making the edits and adding the tests.
Let's merge this!
|
🎉 This PR is included in version 1.11.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Hi @chriswmackey,
I added a initial class for the cylinder for your review. This object is inspired in the cone in terms of its definition. Please let me know if that makes sense,
In regards to naming, I am not sure if the '_center' name is the most correct one here. It is actually the bottom base center of the cylinder but I'm trying to keep it simple if that makes sense.
Please let me know also if I should add any additional methods/properties to it.