ladybug-tools / honeybee-legacy

:bee: Honeybee is a free and open source plugin to connect Grasshopper3D to EnergyPlus, Radiance, Daysim and OpenStudio for building energy and daylighting simulation
http://ladybug.tools
Other
125 stars 145 forks source link

Split Up "SplitBldgMass" Component in Two and Build Up a Library of Core/Perimeter Functions #497

Closed chriswmackey closed 6 years ago

chriswmackey commented 8 years ago

as referenced in this discussion: http://www.grasshopper3d.com/group/ladybug/forum/topic/show?id=2985220%3ATopic%3A1493543&xg_source=msg

This really seems to be the best way around the issue. The code just became too complex trying to do everything in one component and an organized strategy is needed to make this component great.

-Chris

saeranv commented 7 years ago

Hey @chriswmackey, how's progress on this issue going?

I'm willing to take a stab at addressing the issues here (currently a huge bottleneck in my own workflow). If I am reading your thread comments correctly, it seems that there's two issues here: 1) Separating the perimeter/core zoning into a different components. (Shouldn't be too difficult). 2) Resolving some of the inadequacies of the perimeter/core split. This is a more complex problem, and the current solution is working well, but every now and then will fail for simple convex shapes, and definitely has issues with convex shapes. Eventually, it'd be nice to have something like this (page 9): http://www.ibpsa.org/proceedings/eSimPapers/2014/5B.2.pdf Do you have any thoughts on how to tackle this?

Let me know how I can help.

chriswmackey commented 7 years ago

@saeranv ,

I have not made much progress on this and please feel free to take a stab at it. Definitely the first step is separating the code into two components for everyone's sanity.

Improving the perimeter/core split is also a mostly reductive process in my mind. I think that I just got overly-ambitious about trying to handle curved geometry in the core/perimeter zoning. In retrospect, I probably should have taken an approach close to Timur (I remember sitting next to him when he was working on that paper) in which he was only concerned with getting it to work for planar geometry. In the end, it created something that's more reliable.

I won't be able to get to this for a bit so please feel free to go ahead and let me know if you have questions.

-Chirs

saeranv commented 7 years ago

@chriswmackey

Two components! https://drive.google.com/open?id=0B1aL4rZ85UMSZk0zWDRNZGxTSWc

https://github.com/saeranv/Honeybee/blob/master/src/Honeybee_SplitBuildingMass.py https://github.com/saeranv/Honeybee/blob/master/src/Honeybee_SplitZone.py

It's working, however there are redundancies that I need to eliminate, i.e. the same getFloorCrvs function is in both components right now. I tested it out, and it seems to be matching the functionality of the existing component. Figuring out the perimeter/core split is the next step...

s

saeranv commented 7 years ago

Making note of progress and issues that remain to be addressed: http://www.grasshopper3d.com/group/ladybug/forum/topics/split-building-mass-component-error-again

saeranv commented 7 years ago

@chriswmackey Sorry it's taking a while to finish this! But I am making progress. I have a question about something you wrote in the original LB post regarding this component: You mentioned that you wanted to: "...build up a library of functions that can divide floors into core/perimeter, which is the main function that is failing here."

What do you mean by library of functions exactly? Are you suggesting there should be other ways of automatically dividing masses, or assigning materials?

S

chriswmackey commented 7 years ago

@saeranv ,

Thanks for asking and no worries if this is taking some time. I would rather take time and get it right now rather than produce a bunch of messy code like I did on this originally.

What I meant by "a library of functions" was just that we have a set of organized methods for subdividing the core and perimeter, with each method as its own function. The subdivision of solids is something that Rhino is not particularly good at and it would be helpful to have several ways of performing this subdivision in case one of the methods fails. Some examples of possible methods include:

1) Offset both floor and ceiling curves. Loft them together (this is how the current component works but this fails any time the offset function causes the offset curve to intersect itself).

2) Simplifying the entire problem into a 2D one using just the floor surface. Instead of using Rhino's offset, we do a "manual" one, figure of intersections between the moved curve segments, and solve locations where the curve would intersect itself. I know that Timur Dogan published an interesting paper on this method and I will try to find it at some point. The only downside of this method is that it's really just meant for zones that are extruded 2D curves and it might not be as elegant a solution for zones with sloped walls.

3) I don't know if this idea is particularly good but we could also try using Rhino's Offset Surface method instead of offsetting curves in cases where all walls are vertical. Then, we could try boolean differencing the resulting solids with one another.

Just starting with one of these methods is perfectly fine and we can always add more functions to the "library" if the first function is organized well.

saeranv commented 7 years ago

@chriswmackey

Yes, I have lost many nights of sleep trying to fight Rhino's buggy solid subdivision methods :) I like your idea of a library of functions, the key I think will be to test as many different use cases as possible to identify possible failures where we'll need a different geometric approach.

My current approach is to implement Felkel and Obdrzalek's algorithm for a 'straight skeleton' or 'angular bisector networks' computation, and then use an adjacency matrix to keep track of interior/exterior walls to extrude and boolean difference cores from perimeters. The advantage of this approach is that we avoid a lot of Rhino's geometry methods upfront and rely more on simple vector/line manipulations (which will also allow us to port more of this functionality into the core HB+ library as well, when it comes to that).

There are three parts to the algorithm to handle convex, concave and shapes with holes in them respectively. So far the results look pretty promising. Here's a comparison of different shapes for the new version and the old component:

persp_view_perimeter_split2

Link to GH example file: https://drive.google.com/open?id=0B1aL4rZ85UMSVW9zUnhJbV92UWs

As you can see, it seems to be able to handle any kind of wonky convex shape I throw at it, and it executes a lot more smoothly then the existing one. It should take me less time to develop the other two parts of the algorithm.

S

chriswmackey commented 7 years ago

@saeranv ,

I feel your pain but this is looking great! Felkel and Obdrzalek's algorithm for a 'straight skeleton' definitely seems t be the way to go for the vast majority of cases. Any way that we can simplify the issue into a 2D one is going to give us a lot more reliability.

Feel free to send a pull request of the components whenever you like and I will merge them in. If you still feel like there is a lot of work to be done, you can always send them to the WIP section for the time being. I feel we should at least have the components as they are for the next stable release, which should be around mid-summer.

-Chris

saeranv commented 6 years ago

@chriswmackey, @devngc - sorry it took so long for me to post this - I'm in the process of finishing up a major deadline at work and don't have much free time at the moment...

Anyway, to review, as we discussed in this post there are two things I'll be working on:

  1. Add a concave checking function to the SplitFloor2ThermalZones component, so that it warns users before they use the new component that SplitFloor2ThermalZones can't handle it.
  2. Swap old/new components.

In terms of 1, one method that I've been using is here, I think this might be similar to the stackoverflow post @chriswmackey posted, but I have to take a closer look at it.

Brief overview of method:

I'm interested in seeing what you're implementing @devngc, and if you think any improvements to this method could be made?

devang-chauhan commented 6 years ago

Hi @saeranv,

Thanks for posting this. The StackOverflow thread that Chris referred to, is great. Taking cross product is certainly a way to go about this and I am doing the same. However, I have experienced challenges with this method.

It works fine when a surface is untouched (No geometry operations have been performed in rhino). But if a surface is touched by some geometry operations such as splitting in rhino, then occasionally it changes the order to vertices, and adds new vertices as well. The ordering of vertices is easy enough to solve and I have done the same. However, this cross-product approach misses such surfaces in my observations. Please find results of some of the test cases.

These are, as you can see, untouched geometries. So, no issues here. 01 02 03 04 This is not untouched geometry. Take a look at the vertices. Now intuitively, we know this is a convex polygon but the cross product method fails to recognize that. 05 I have gone a step further, I have managed to sum direction vectors for cases like this when two consecutive vectors are parallel to each other. This gives me a single vector for 5-0-1-2. So I use only four vectors to do the cross-product. Unfortunately, this still does not solve it.

So that is my report on progress so far. I am still at it. I have another idea that I have yet to test. I am travelling for a conference this week. Please let me know if you still wish to look at the GH file. I will have to make it tidy to share it.

My goal is to, eventually, Take a list of Breps and spit out a list of surfaces that are non-convex.

Thank you for your time! -Devang

saeranv commented 6 years ago

@devngc, Very interesting! I'm glad I asked for your input, I've never encountered this before.

For that last image, one thing to check, which perhaps you've already thought of: it looks like the surface you're checking in the triangular geometry is a vertical surface. For the 3d cross product, the sign of the cross vector depends on only the x,y coordinates of both input vectors.

i.e cross = a x b cross.x = a.y b.z - a.z b.y cross.y = a.z b.x - a.x b.z cross.z = a.x b.y - a.x a.y * b.x

So perhaps the issue here is that the edge vectors lie in the xz or yz plane? Have you tried flipping the surface down back to the xy plane and seeing if cross product works there?

Are you using Rhino's cross product or manually calculating it yourself? I think if we manually calculate it using the above formula we should be able to figure out whats wrong.

And yes, please send me the gh files,

devang-chauhan commented 6 years ago

Hi @saeranv,

I took you suggestion and rotated the surface to be in XY plane. Unfortunately, my implementation still failed to recognize it as a convex surface.

Since I started working on this problem, I have tried to use the cross-product method in various ways. However, I could not come up with a way to pass all the test geometries that I currently have. So I started from scratch.

I thought of using split operation. What if we draw lines between all the vertices and try to split the baseBrepSurface with those lines? If it's a non-convex baseBrepSurface, at least one of the line will not split the baseBrepSurface. Here, I could not find a method in rhinocommon to split a baseBrepSurface with a line. So I had to do something else.

Now, I thought of intersections. I created a brepSurface from all those lines (Lines that we can have connecting any two vertices of the brepSurface) and tried intersection of brepSurfaces with the baseBrepSurface. This worked (At least on test cases I have in mind) and following are the results.

01 02 03 04 05

Sometimes people add Breps with faulty geometry [In yellow]. I am catching these as well. 06

Sample

Thank you!

devang-chauhan commented 6 years ago

non-convex checking is implemented in #660

saeranv commented 6 years ago

Great work @devngc ! Glad you figured it out. And here I am falling behind on pushing out my part of the code... I'll see if I can spend the weekend doing so, and will include your the non-convex function as part of it.

One thing to note is that, while I believe checking to see if a surface can be split between all vertices is a really clever solution given the problems we're having with Rhino geometries - I believe the algorithmic performance (in terms of time complexity) is not as good as checking the cross product of adjacent vertices.

Checking the cross products for a n-edge polygon will take 2n steps, and checking to see if a line between vertices will cross the surface should take n^2 steps (every vertice must check all other vertices once). So if we assume users typically are using a surfaces with 4 - 6 edges, that will be 8 - 12, and 16 - 36 steps respectively for each method.

All of which is to say, if one of us does figure out what was wrong with the cross-product method you were trying earlier, it's worthwhile to refactor the non-convex checking component to use that method!

ETA: Also, if its not too much trouble, do you mind sending me the gh script/code where you were trting the cross product method and it was failing on breps that had rhino geometric operations performaned on them? I'm very curious about why such a thing would fail...

devang-chauhan commented 6 years ago

@saeranv , Thank you! I totally agree, that in terms of algorithm complexity, the cross-product approach does much better. I would look myself why I failed to implement the cross-product method. Another advantage of doing this the cross-product way will be that, we will be able to port it to Dynamo easily. Currently, we rely on Rhino API methods and the same methods may not exist on Dynamo side. Which will be limiting. So all in all, sure, the work continues!

devang-chauhan commented 6 years ago

@saeranv , You can check this.

saeranv commented 6 years ago

Square geometries causing a bug, from here: http://discourse.ladybug.tools/t/strange-results-from-split2zone-component/2300

I'll see if I can tackle this later this week.

S

chriswmackey commented 6 years ago

Since that PR was merged, this seems to all be fixed. so I'm going to close out this issue

Thank you @saeranv and @devngc for all of the help with bringing this to fruition. I look forward to posting about this in the upcoming release notes!