feat(geometry): Adding Geometry Objects

[chriswmackey]

You can see that this is a fairly large PR with ~5k lines of pythonic new code and docstrings. It gives us more than half of the things originally on the checklist for this library as you can see on the updated Readme page on my fork

If you want to get an initial sense of the capabilities, I recommend browsing the Surface3D class since many of the other classes in the library are in service of this one and this is also the one that I have been imaging would describe honeybee surfaces, essentially allowing us to move/transform them, generate glazing based on a ratio from them, generate analysis grids from them, etc.

The main reason why this is a draft PR is that I have been doing most of the testing of the library with the help of Rhino since it is much easier to generate a lot of cases and visually evaluate them there. So, while I am confident that 80% of the code works well from these tests, this means that I don't have any pure Python tests for CI and there are probably some bugs in the parts that have not been tested with Rhino. So, the next step for me is writing these tests.

In the meantime, I could definitely use feedback on the code and especially the class names and organization of the library. The organization seems very clear to me but I have a feeling this is because I have been in the zone working on it the last week.

[mostaphaRoudsari]

Thanks @chriswmackey! This is a long one. Let's see what will be the best strategy to review it. A number of quick notes after checking a number of files.

1. Is Surface3D also a planar surface? If that is the case should we call it a Face?
2. I would say it's better to remove Ladybug from __repr__ methods. We can just call then Surface, Mesh and so on instead of Ladybug Surface, ...
3. Since the geometry instances will easily be used tens of thousands of time I think we should use __slots__ for all of them.
4. As you are writing tests it would be great if we can also use them as sample codes to show case how this library should be used.

[chriswmackey]

@mostaphaRoudsari ,
Thank you for the quick comments and I think we definitely want to take our time on reviewing this one. Given our busy schedule this month, I am not planning on being ready to merge this in until May but I think that I can work on writing tests as you review the code.

To respond to your comments:

1. Are you not concerned about the confusion that this could create by calling both mesh faces and objects representing polygonal surfaces by the name Faces? I guess I should have asked why you decided to call this object within the pollination API a Face instead of a Surface and what you think each of these terms means. Actually, after reading through the Wikipedia definiteion of surface, I think I get what you mean. Are you thinking that surfaces can be non-planar while faces are always planar? If so, I can get behind this name change. I am also realizing that there is not much risk of confusion with mesh faces here since there really isn't a mesh face class in this library (I just use tuples to store the indices of each face).

2. I originally had it the way that you suggested but then I started doing tests in Rhino, where I was working with both Rhino geometries and Ladybug geometries, and I realized how easy it is to get these two confused with one another. So I figured, if I have moments of mistaking a Rhino Point3D from a Ladybug Point3D, other people working with Ladybug API in Grasshopper are definitely going to have moments like this. Then it crossed my mind that, if we ever have a situation where the Ladybug geometry is output from a component (perhaps for speed), it would create a lot of confusion for users to see an output just called Mesh (vertices 4, faces 1) and then not be able to preview the mesh in the Rhino scene or treat it like a Rhino Mesh. So I added in the Ladybug in front of the __repr__ names for all of the geometries to help me (and presumably others) more easily distinguish between them and the Rhino objects. So my question to you would be, if not adding Ladybug in front of the __repr__ name, what should we do to help users distinguish these objects from their counterparts within CAD SDKs?

3. I am already using __slots__ on all of the small objects that get called thousands of times (points, vectors, line segments, rays) and I have already witnessed what a difference it makes in my tests. The reason why __slots__ is not being used on the Polygon or Surface class is that it is set up such that most of the properties are simply None upon creation of the object in order to help save memory and calculation time (things like area, perimeter, is_convex, triangulated_mesh are all None to start off with). If these properties are needed, they are calculated upon request and then cached on the object such that they don't need to be recalculated in future calls to the property. The thing is that this memory and computation saving feature conflicts with how __slots__ works because I end up re-defining the properties from None to the actual property when they are requested. There is a way around this in that I could go back to using __slots__ and simply not define the properties as None (using hasattr on the properties instead of asking if the property is None). But this type of call would be made hundreds of times for some objects and I thought calling the hasattr on the properties would take longer than asking if the property is None (admittedly, I am not sure if this is true). Also, using hasattr would prevent me from using some tricks I added in like, for mesh grids, I store the face area as a single value to save memory (since all mesh faces have the same area).

4. Will definitely do!

[chriswmackey]

@mostaphaRoudsari ,
I am just letting you know that, after our discussions yesterday, I have addressed all of the initial comments that you brought up.

[mostaphaRoudsari]

Added a number of comments on the first couple of files that I get to check. Will add more comments once I get a chance.

[mostaphaRoudsari]

It looks good! Thank you for making all the changes. As discussed let's merge this in and we will know more about the performance as we integrate the library into other ladybug tools libraries.

[mostaphaRoudsari]

PS: I suggest to squash and merge them as a single commit to end up with a cleaner git history and also make @AntoineDao happy! ;)

[AntoineDao]

Realistically what would make me happy is smaller PRs merged more often rather than one spanning from end of March... But of well, you can't have it all I guess 😛

[AntoineDao]

With regards to commit messages it's mostly for the sake of:

1. Commit history review
2. Semantic release version upgrades
3. Changelog of semantic releases being meaningful

semantic police away!!!

[chriswmackey]

Definitely going to Squash and merge this one. The only reason why I didn't do my semantic duty is because this is the first PR that is actually establishing the repo (all future PRs will be much more diligent). We definitely got carried away leaving this PR for 2 months and this should also not happen in the future.